Re: [PATCH] libstdc++: Ensure __gthread_self doesn't call undefined weak symbol [PR 95989]

[Jonathan Wakely <jwakely@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1153 bytes --]

On 11/11/20 19:08 +0100, Jakub Jelinek via Libstdc++ wrote:
>On Wed, Nov 11, 2020 at 05:24:42PM +0000, Jonathan Wakely wrote:
>> --- a/libgcc/gthr-posix.h
>> +++ b/libgcc/gthr-posix.h
>> @@ -684,7 +684,14 @@ __gthread_equal (__gthread_t __t1, __gthread_t __t2)
>>  static inline __gthread_t
>>  __gthread_self (void)
>>  {
>> +#if __GLIBC_PREREQ(2, 27)
>
>What if it is a non-glibc system where __GLIBC_PREREQ macro isn't defined?
>I think you'd get then
>error: missing binary operator before token "("
>So I think you want
>#if defined __GLIBC__ && defined __GLIBC_PREREQ
>#if __GLIBC_PREREQ(2, 27)
>  return pthread_self ();
>#else
>  return __gthrw_(pthread_self) ();
>#else
>  return __gthrw_(pthread_self) ();
>#endif
>or similar.


Here's a fixed version of the patch.

I've moved the glibc-specific code in this_thread::get_id() into a new
macro defined in config/os/gnu-linux/os_defines.h (where we already
know we are dealing with glibc). That means we don't do the
__GLIBC_PREREQ check directly in <thread>, it's hidden away in a
target-specific header.

Tested powerpc64le-linux (glibc 2.17 and 2.32), sparc-solaris2.11 and
powerpc-aix.





[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 5416 bytes --]

commit 822914f1f1f4710ff252764ee634aa07ac565d53
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Nov 11 19:26:00 2020

    libstdc++: Ensure __gthread_self doesn't call undefined weak symbol [PR 95989]
    
    Since glibc 2.27 the pthread_self symbol has been defined in libc rather
    than libpthread. Because we only call pthread_self through a weak alias
    it's possible for statically linked executables to end up without a
    definition of pthread_self. This crashes when trying to call an
    undefined weak symbol.
    
    We can use the __GLIBC_PREREQ version check to detect the version of
    glibc where pthread_self is no longer in libpthread, and call it
    directly rather than through the weak reference.
    
    It would be better to check for pthread_self in libc during configure
    instead of hardcoding the __GLIBC_PREREQ check. That would be somewhat
    complicated by the fact that prior to glibc 2.27 only libc.so.6
    contained the pthread_self symbol. The configure checks would need to
    try to link both statically and dynamically, and the result would depend
    on whether the static libc.a happens to be installed during configure
    (which could vary between different systems using the same version of
    glibc). Doing it properly is left for a future date, as it will be
    needed anyway after glibc moves all pthread symbols from libpthread to
    libc. When that happens we should revisit the whole approach of using
    weak symbols for pthread symbols.
    
    For the purposes of std::this_thread::get_id() we create a fake non-zero
    thread ID ((__gthread_t)1) when using glibc but not linked to
    libpthread. When using glibc 2.27 or later pthread_self() never returns
    zero so we don't need to use (__gthread_t)1 for new glibc.
    
    An undesirable consequence of this change is that code compiled prior to
    the change might inline the old definition of this_thread::get_id()
    which always returns (__gthread_t)1 in a program that isn't linked to
    libpthread. Code compiled after the change will use pthread_self() and
    so get a real TID. That could result in the main thread having different
    thread::id values in different translation units. This seems acceptable,
    as there are not expected to be many uses of thread::id in programs
    that aren't linked to libpthread.
    
    libgcc/ChangeLog:
    
            PR libstdc++/95989
            * gthr-posix.h (__gthread_self) [__GLIBC_PREREQ(2, 27)]: Call
            pthread_self directly rather than using weak alias.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/95989
            * config/os/gnu-linux/os_defines.h (_GLIBCXX_NATIVE_THREAD_ID):
            Define new macro to get reliable thread ID.
            * include/std/thread (this_thread::get_id): Use new macro if
            it's defined.

diff --git a/libgcc/gthr-posix.h b/libgcc/gthr-posix.h
index 965247602acf..dc34645d1c52 100644
--- a/libgcc/gthr-posix.h
+++ b/libgcc/gthr-posix.h
@@ -684,7 +684,18 @@ __gthread_equal (__gthread_t __t1, __gthread_t __t2)
 static inline __gthread_t
 __gthread_self (void)
 {
+#if defined __GLIBC__ && defined __GLIBC_PREREQ
+# if __GLIBC_PREREQ(2, 27)
+  /* Since Glibc 2.27, pthread_self is defined in libc not libpthread.
+   * Call it directly so that we get a non-weak reference and won't call
+   * an undefined weak symbol when linked to the libc.a static lib.  */
+  return pthread_self ();
+# else
   return __gthrw_(pthread_self) ();
+# endif
+#else
+  return __gthrw_(pthread_self) ();
+#endif
 }
 
 static inline int
diff --git a/libstdc++-v3/config/os/gnu-linux/os_defines.h b/libstdc++-v3/config/os/gnu-linux/os_defines.h
index f821486ec8f5..ca61ecf60f62 100644
--- a/libstdc++-v3/config/os/gnu-linux/os_defines.h
+++ b/libstdc++-v3/config/os/gnu-linux/os_defines.h
@@ -49,4 +49,14 @@
 // version dynamically in case it has changed since libstdc++ was configured.
 #define _GLIBCXX_NO_OBSOLETE_ISINF_ISNAN_DYNAMIC __GLIBC_PREREQ(2,23)
 
+#if ! __GLIBC_PREREQ(2, 27)
+// Since glibc 2.27 pthread_self() is usable without linking to libpthread.
+// Before then it was in libc.so.6 but not libc.a, and always returns 0,
+// which breaks the invariant this_thread::get_id() != thread::id{}.
+// So only use it if we know the libpthread version is available.
+// Otherwise just use (__gthread_t)1 as the ID of the main (and only) thread.
+#define _GLIBCXX_NATIVE_THREAD_ID \
+  (__gthread_active_p() ? __gthread_self() : (__gthread_t)1)
+#endif
+
 #endif
diff --git a/libstdc++-v3/include/std/thread b/libstdc++-v3/include/std/thread
index 080036e26097..66d30901ac5f 100644
--- a/libstdc++-v3/include/std/thread
+++ b/libstdc++-v3/include/std/thread
@@ -350,15 +350,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     inline thread::id
     get_id() noexcept
     {
-#ifdef __GLIBC__
-      // For the GNU C library pthread_self() is usable without linking to
-      // libpthread.so but returns 0, so we cannot use it in single-threaded
-      // programs, because this_thread::get_id() != thread::id{} must be true.
-      // We know that pthread_t is an integral type in the GNU C library.
-      if (!__gthread_active_p())
-	return thread::id(1);
-#endif
+#ifdef _GLIBCXX_NATIVE_THREAD_ID
+      return thread::id(_GLIBCXX_NATIVE_THREAD_ID);
+#else
       return thread::id(__gthread_self());
+#endif
     }
 #endif // _GLIBCXX_HAS_GTHREADS