Bug 78017 - weak reference usage in gthr-posix.h (__gthread*) is broken with new enough glibc (GTHREAD_USE_WEAK can be defined to 0 now)
Summary: weak reference usage in gthr-posix.h (__gthread*) is broken with new enough g...
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: libgcc (show other bugs)
Version: 7.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
: 87189 109540 (view as bug list)
Depends on:
Blocks: 87189
  Show dependency treegraph
 
Reported: 2016-10-18 09:35 UTC by nsz
Modified: 2024-04-08 22:11 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2020-11-23 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description nsz 2016-10-18 09:35:08 UTC
there seem to be no open bug for the issue discussed in
https://gcc.gnu.org/ml/libstdc++/2014-11/msg00122.html

i think libstdc++, libgfortran and libgcov are affected.

libgcc/gthr-posix.h uses weak references for the pthread api to
1) detect single-threadedness
2) use pthread api without adding -lpthread dependency.

this does not work

a) with static linking:

because 1) fails on the target:

single threaded execution can be assumed if pthread_create and
thrd_create are not referenced (on targets without libpthread
dlopen support), but the gthr logic only checks pthread_create
(on bionic) or pthread_cancel (on some targets as a misguided
attempt to avoid detecting threads because of ldpreloaded pthread
wrappers which "seem unlikely" to define pthread_cancel).

symbols required by libstdc++ may be missing because of 2),
(causing hard to debug runtime crashes):

redhat puts all of libpthread into a single .o, others use
 -Wl,--whole-archive -lpthread -Wl,--no-whole-archive
linker flags as a workaround, but this should not be needed:
if libstdc++.a semantically requires strong references then
those references must be strong (the calls may be elided
using the detection above).

b) if there is dlopen support for libpthread

then single-threadedness can change at runtime, so any check
would break code like

  std::mutex m;
  m.lock();
  dlopen(.. something that starts threads and use m ..)
  m.unlock();


various targets explicitly opt out from the weak ref hacks,
(using gcc configure time hacks), i think instead the gthr
logic should be safe by default:

assume multi-threaded execution by default and only try
to optimize the single threaded case when it is guaranteed
to be safe.
Comment 1 Andrew Pinski 2016-10-18 14:02:27 UTC
IIRC this was declared a libc bug or an user error (not using the full archive). There is another thread on the glibc side if you want to read up on that.
Comment 2 nsz 2016-10-18 15:28:42 UTC
i see the glibc threads linked from
https://sourceware.org/bugzilla/show_bug.cgi?id=5784
but there are other libcs with static linking support, so even
if weakrefs worked on glibc (now they don't) this would be a
gcc bug on other targets..

when this came up with musl libc it required fair amount of
debugging and i see other cases where people spent time on this:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=33960
so i think it is worth changing the logic.
Comment 3 Andrew Pinski 2016-10-19 01:29:33 UTC
There are other reasons why using static libraries does not make sense for libpthread.
Comment 4 nsz 2016-10-19 08:32:37 UTC
(In reply to Andrew Pinski from comment #3)
> There are other reasons why using static libraries does not make sense for
> libpthread.

i can't immediately think of any, can you give a hint?
Comment 5 Ben Longbons 2020-06-08 01:36:24 UTC
I just hit this in my testsuite, and it's worse than described for the dynamic case:

if libpthread.so is dlopen'ed after libstdc++, __gthread_active_p() will *not* become true - weak symbols are not rebound.

Note that plugins that indirectly pull in pthreads are very common in the wild.

Further, LD_PRELOAD=libpthread.so.0 does NOT help.

Thus, all C++ locks will do nothing despite the presence of multiple threads. Correct code will experience race conditions.

Do note that, nowadays, all of the *other* symbols you're making weak are in libc.so (for GLIBC at least, which AFAIK is the only dlopen'able pthread implementation)

(I had falsely assumed the only danger was doing the dlopen while any lock is held, leading to mismatched lock/unlock pairs, which would be fairly easy to avoid)

Tested on Debian 10 (buster), with:

g++ (Debian 8.3.0-6) 8.3.0
GNU C Library (Debian GLIBC 2.28-10) stable release version 2.28.
GNU ld (GNU Binutils for Debian) 2.31.1



// link with -ldl
#include <gnu/lib-names.h>

#include <assert.h>
#include <dlfcn.h>

#include <bits/gthr.h>

int main(void)
{
    void *libpthread = dlopen(LIBPTHREAD_SO, RTLD_LAZY);
    assert (libpthread);
    assert (__gthread_active_p());
}
Comment 6 Jonathan Wakely 2020-11-23 13:37:47 UTC
(In reply to Ben Longbons from comment #5)
> I just hit this in my testsuite, and it's worse than described for the
> dynamic case:
> 
> if libpthread.so is dlopen'ed after libstdc++, __gthread_active_p() will
> *not* become true - weak symbols are not rebound.

The problem isn't that weak symbols are not rebound (they are), it's that __gthread_active_p stores the result of the first check in a static variable which is initialized once and then it's value doesn't change:

  static void *const __gthread_active_ptr
    = __extension__ (void *) &GTHR_ACTIVE_PROXY;

> Note that plugins that indirectly pull in pthreads are very common in the
> wild.

At the time of writing, the main executable needs to link to libpthread, not just the plugins. Otherwise libstdc++ won't work.

Glibc also has a history of bugs with libpthread only being loaded via dlopen.
 
> Further, LD_PRELOAD=libpthread.so.0 does NOT help.
> 
> Thus, all C++ locks will do nothing despite the presence of multiple
> threads. Correct code will experience race conditions.
> 
> Do note that, nowadays, all of the *other* symbols you're making weak are in
> libc.so (for GLIBC at least, which AFAIK is the only dlopen'able pthread
> implementation)

I don't think that's true at all. It's planned for a future version of glibc though, see https://sourceware.org/legacy-ml/libc-alpha/2019-10/msg00080.html

When that happens, __gthread_active_p will always be true for glibc (and the weak symbols won't be needed anyway).
Comment 7 Andrew Pinski 2024-04-08 21:37:44 UTC
*** Bug 87189 has been marked as a duplicate of this bug. ***
Comment 8 Andrew Pinski 2024-04-08 21:50:44 UTC
*** Bug 114646 has been marked as a duplicate of this bug. ***
Comment 9 Andrew Pinski 2024-04-08 22:11:34 UTC
*** Bug 109540 has been marked as a duplicate of this bug. ***