mailing list of musl libc
 help / color / mirror / code / Atom feed
From: Jonathan Wakely <jwakely@redhat.com>
To: Rich Felker <dalias@libc.org>
Cc: "Florian Weimer" <fw@deneb.enyo.de>, Арсений <a@saur0n.science>,
	musl@lists.openwall.com
Subject: Re: [musl] Mutexes are not unlocking
Date: Mon, 23 Nov 2020 16:19:07 +0000	[thread overview]
Message-ID: <20201123161907.GT1312820@redhat.com> (raw)
In-Reply-To: <20201123145329.GU534@brightrain.aerifal.cx>

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.

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



  reply	other threads:[~2020-11-23 16:43 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-20  5:25 a
2020-11-20  5:58 ` Rich Felker
2020-11-21  6:46   ` Re[2]: " a
2020-11-21 15:51     ` Rich Felker
2020-11-22 18:43       ` Re[2]: " Арсений
2020-11-22 19:11         ` Rich Felker
2020-11-22 19:23           ` Florian Weimer
2020-11-22 19:28             ` Rich Felker
2020-11-22 19:45               ` Re[2]: " Арсений
2020-11-22 20:05               ` Арсений
2020-11-23 12:24                 ` Jonathan Wakely
2020-11-23 14:56                   ` Rich Felker
2020-11-23 16:58                     ` Jonathan Wakely
2020-11-23 11:41               ` Jonathan Wakely
2020-11-23 14:53                 ` Rich Felker
2020-11-23 16:19                   ` Jonathan Wakely [this message]
2020-11-23 16:51                     ` Rich Felker
2020-11-23 17:10                       ` Jonathan Wakely
2020-11-23 17:18                         ` Florian Weimer
2020-11-23 16:59                     ` Florian Weimer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201123161907.GT1312820@redhat.com \
    --to=jwakely@redhat.com \
    --cc=a@saur0n.science \
    --cc=dalias@libc.org \
    --cc=fw@deneb.enyo.de \
    --cc=musl@lists.openwall.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).