[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20201123165121.GW534@brightrain.aerifal.cx>
Date: Mon, 23 Nov 2020 11:51:22 -0500
From: Rich Felker <dalias@...c.org>
To: Jonathan Wakely <jwakely@...hat.com>
Cc: Florian Weimer <fw@...eb.enyo.de>,
Арсений <a@...r0n.science>,
musl@...ts.openwall.com
Subject: Re: Mutexes are not unlocking
On Mon, Nov 23, 2020 at 04:19:07PM +0000, Jonathan Wakely wrote:
> On 23/11/20 09:53 -0500, Rich Felker wrote:
> >On Mon, Nov 23, 2020 at 11:41:24AM +0000, Jonathan Wakely wrote:
> >>On 22/11/20 14:28 -0500, Rich Felker wrote:
> >>>On Sun, Nov 22, 2020 at 08:23:23PM +0100, Florian Weimer wrote:
> >>>>* Rich Felker:
> >>>>
> >>>>> On Sun, Nov 22, 2020 at 09:43:35PM +0300, Арсений wrote:
> >>>>>>
> >>>>>> Hello,
> >>>>>> The problem is that mutex is not got unlocked after the first unlock().
> >>>>>>
> >>>>>> libstdc++ uses a wrapper for pthread called gthreads. This wrapper
> >>>>>> checks for the state of the mutex system. For
> >>>>>> example, pthread_mutex_unlock() is called in a following way:
> >>>>>>
> >>>>>> static inline int
> >>>>>> __gthread_mutex_unlock (__gthread_mutex_t *__mutex)
> >>>>>> {
> >>>>>> if (__gthread_active_p ())
> >>>>>> return __gthrw_(pthread_mutex_unlock) (__mutex);
> >>>>>> else
> >>>>>> return 0;
> >>>>>> }
> >>>>>
> >>>>> Yes. This code is invalid (it misinterprets weak symbol information to
> >>>>> draw incorrect conclusions about whether threads may be in use) and
> >>>>> thus is disabled in builds of gcc/libstdc++ targeting musl-based
> >>>>> systems. GCC and glibc-based distro folks mostly don't care because
> >>>>> it only breaks static linking, but some of them actually hack gcc's
> >>>>> libpthread.a into one giant .o file to work around the problem rather
> >>>>> than fixing this in gcc...
> >>>>
> >>>>GCC 11 has a fix (if used along with glibc 2.32), but I wonder if it's
> >>
> >>The GCC 11 change isn't used for mutexes. I use __libc_single_threaded
> >>for deciding whether to use atomics or plain non-atomic ops for
> >>ref-count updates. But for actions like mutex locks I don't use it,
> >>because it would do the wrong thing for code like:
> >>
> >>int main() {
> >> std::mutex m;
> >> std::lock_guard<std::mutex> l(m);
> >> std::thread t( []{} };
> >> t.join();
> >>}
> >>
> >>In this program __libc_single_threaded is true when we construct the
> >>lock_guard, but false when it is destroyed at the end of the block. If
> >>we checked __libc_single_threaded for mutex locking/unlocking then we
> >>would be inconsistent here, we would elide the lock but try to unlock
> >>a mutex that was never locked, which would be UB.
> >>
> >>Whatever condition we check to decide whether to call the pthreads
> >>function needs to give the same result for the lock and unlock. That
> >>condition is currently __gthread_active_p, which checks if a symbol is
> >>weak.
> >>
> >>>>going to run into a similar issue because inlined code from older GCC
> >>>>versions uses a diverging version of the check.
> >>
> >>No, everything uses the same check. GCC 11 didn't change anything for
> >>mutexes.
> >>
> >>>>Jonathan, more context is here:
> >>>>
> >>>> <https://www.openwall.com/lists/musl/2020/11/22/1>
> >>>
> >>>Uhg, that's a really nasty problem. Is __gthread_active_p() also
> >>>inlined? Or can it be given a definition to mitigate the problem?
> >>
> >>It's inlined, and making it a non-inline PLT call would negate some of
> >>the point of using it (it's there mostly as an optimization).
> >>
> >>But I don't think inlining it is a problem, because its definition
> >>hasn't changed.
> >>
> >>I think the right thing to do is a new configuration which completely
> >>avoids using weak symbols in gthreads, and just calls the pthread
> >>symbols directly. A future version of glibc is going to move pthreads
> >>symbols into libc anyway:
> >>https://sourceware.org/legacy-ml/libc-alpha/2019-10/msg00080.html
> >>After that, the __gthread_active_p condition is just a wasted branch,
> >>because it will be unconditionally true:
> >>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89714
> >
> >The potential problem is that removing the condition makes an
> >inconsistency with binaries where the condition was already inlined.
>
> But if the result of the condition is always true, it doesn't matter.
> The code that doesn't check the condition assumes it's true, and the
> code that already inlined the condition does a runtime check which
> will be true. The end result is the same.
The condition is false if you static link and don't use
pthread_key_create in your program. I think the problem would only
arise if you have old object files with new libstdc++.a or vice versa
though, but this could reasonably happen with binaryware libraries.
> >>If the musl libstdc++ removes the use of __gthread_active_p, but code
> >>compiled against a glibc libstdc++ inlines the __gthread_active_p
> >>check, then it seems to me that musl (or the musl libstdc++) needs to
> >>ensure that the inlined __gthread_active_p will return true. It can do
> >>that by providing a non-weak symbol called __pthread_key_create (the
> >>symbol won't be called, it just has to exist).
> >
> >The target files for -musl tuples disable the hack entirely by adding
> >t-gthr-noweak and -DGTHREAD_USE_WEAK=0 where appropriate. So there's
> >no issue here. OP's problem was trying to make glibc-linked binaries
> >and use them with musl's very limited glibc-ABI-compat capabilities,
> >which is not recommended; it's only intended as an aid in running
> >binaryware (esp shared libraries) you can't build/get native versions
> >of.
>
> OK. If supporting that not recommended case does become desirable,
> defining a __pthread_key_create symbol somewhere should work.
>
> Or users might be able to solve it for themselves by creating a tiny
> shared library that contains nothing but a symbol called
> __pthread_key_create and LD_PRELOADing that, so that the checks in the
> binaryware looking for that symbol become true.
My understanding was that only static linking was broken by the weak
ref hack, so I'm not sure why it's happening to begin with. Is
libstdc++ actually looking for __pthread_key_create rather than
pthread_key_create? That could explain it since we don't provide the
former (and as responsibility for making this stuff work is shifted to
gcompat, gcompat could provide it).
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
