Re: [musl] Mutexes are not unlocking

[Rich Felker <dalias@libc.org>]

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.

> 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.

Rich