Re: [committed] libstdc++: Rewrite std::call_once to use futexes [PR 66146]

[Jonathan Wakely <jwakely@redhat.com>]

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

On 03/11/20 18:45 +0000, Jonathan Wakely wrote:
>The current implementation of std::call_once uses pthread_once, which
>only meets the C++ requirements when compiled with support for
>exceptions. For most glibc targets and all non-glibc targets,
>pthread_once does not work correctly if the init_routine exits via an
>exception. The pthread_once_t object is left in the "active" state, and
>any later attempts to run another init_routine will block forever.
>
>This change makes std::call_once work correctly for Linux targets, by
>replacing the use of pthread_once with a futex, based on the code from
>__cxa_guard_acquire. For both glibc and musl, the Linux implementation
>of pthread_once is already based on futexes, and pthread_once_t is just
>a typedef for int, so this change does not alter the layout of
>std::once_flag. By choosing the values for the int appropriately, the
>new code is even ABI compatible. Code that calls the old implementation
>of std::call_once will use pthread_once to manipulate the int, while new
>code will use the new std::once_flag members to manipulate it, but they
>should interoperate correctly. In both cases, the int is initially zero,
>has the lowest bit set when there is an active execution, and equals 2
>after a successful returning execution. The difference with the new code
>is that exceptional exceptions are correctly detected and the int is
>reset to zero.
>
>The __cxa_guard_acquire code (and musl's pthread_once) use an additional
>state to say there are other threads waiting. This allows the futex wake
>syscall to be skipped if there is no contention. Glibc doesn't use a
>waiter bit, so we have to unconditionally issue the wake in order to be
>compatible with code calling the old std::call_once that uses Glibc's
>pthread_once. If we know that we're using musl (and musl's pthread_once
>doesn't change) it would be possible to set a waiting state and check
>for it in std::once_flag::_M_finish(bool), but this patch doesn't do
>that.
>
>This doesn't fix the bug for non-linux targets. A similar approach could
>be used for targets where we know the definition of pthread_once_t is a
>mutex and an integer. We could make once_flag._M_activate() use
>pthread_mutex_lock on the mutex member within the pthread_once_t, and
>then only set the integer if the execution finishes, and then unlock the
>mutex. That would require careful study of each target's pthread_once
>implementation and that work is left for a later date.

Something like the attached (completely untested) patch might work so
we could stop using pthread_once for the BSDs and Solaris (and so fix
the bug with exceptional executions).

It's a kluge though (and fragile for Solaris because it assumes the
pthread_once_t type will always have the same layout - in the public
header it's just got lots of 64-bit integers for padding).

I should probably test this some time.

In theory something similar could be done for AIX, but its
pthread_once_t is another struct full of padding integers, and unlike
Solaris we can't look at the code to see how the OS uses those bits.

I'm not sure if this can work for Darwin, as its pthread_once_t isn't
just an aggregate of a pthread_mutex_t and a flag, I think it contains
state used by GCD. That means we can't perform equivalent operations
directly just in terms of Pthread APIs.


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

diff --git a/libstdc++-v3/config/os/bsd/dragonfly/os_defines.h b/libstdc++-v3/config/os/bsd/dragonfly/os_defines.h
index c42c6e661630..0c63a0ee1fdd 100644
--- a/libstdc++-v3/config/os/bsd/dragonfly/os_defines.h
+++ b/libstdc++-v3/config/os/bsd/dragonfly/os_defines.h
@@ -38,4 +38,8 @@
 #define _GLIBCXX_USE_C99_LONG_LONG_CHECK 1
 #define _GLIBCXX_USE_C99_LONG_LONG_DYNAMIC (_GLIBCXX_USE_C99_DYNAMIC || !defined __LONG_LONG_SUPPORTED)
 
+// Make std::call_once access pthread_once_t members directly
+#define _GLIBCXX_COMPAT_PTHREAD_ONCE_T \
+  { int _M_done; pthread_mutex_t _M_mutex; }
+
 #endif
diff --git a/libstdc++-v3/config/os/bsd/freebsd/os_defines.h b/libstdc++-v3/config/os/bsd/freebsd/os_defines.h
index d896de4254f4..fbae94c0370d 100644
--- a/libstdc++-v3/config/os/bsd/freebsd/os_defines.h
+++ b/libstdc++-v3/config/os/bsd/freebsd/os_defines.h
@@ -40,4 +40,8 @@
 #define _GLIBCXX_USE_C99_FLOAT_TRANSCENDENTALS_CHECK 1
 #define _GLIBCXX_USE_C99_FLOAT_TRANSCENDENTALS_DYNAMIC defined _XOPEN_SOURCE
 
+// Make std::call_once access pthread_once_t members directly
+#define _GLIBCXX_COMPAT_PTHREAD_ONCE_T \
+  { int _M_done; pthread_mutex_t _M_mutex; }
+
 #endif
diff --git a/libstdc++-v3/config/os/bsd/netbsd/os_defines.h b/libstdc++-v3/config/os/bsd/netbsd/os_defines.h
index d15727e76e0a..0ab2e6528228 100644
--- a/libstdc++-v3/config/os/bsd/netbsd/os_defines.h
+++ b/libstdc++-v3/config/os/bsd/netbsd/os_defines.h
@@ -30,4 +30,8 @@
 
 #define __ssize_t ssize_t
 
+// Make std::call_once access pthread_once_t members directly
+#define _GLIBCXX_COMPAT_PTHREAD_ONCE_T \
+  { pthread_mutex_t _M_mutex; int _M_done; }
+
 #endif
diff --git a/libstdc++-v3/config/os/bsd/openbsd/os_defines.h b/libstdc++-v3/config/os/bsd/openbsd/os_defines.h
index 1761a10e3836..d248bd2cab29 100644
--- a/libstdc++-v3/config/os/bsd/openbsd/os_defines.h
+++ b/libstdc++-v3/config/os/bsd/openbsd/os_defines.h
@@ -38,4 +38,8 @@
 #define _GLIBCXX_USE_C99_FLOAT_TRANSCENDENTALS_DYNAMIC _GLIBCXX_USE_C99_DYNAMIC
 #define _GLIBCXX_USE_C99_FP_MACROS_DYNAMIC _GLIBCXX_USE_C99_DYNAMIC
 
+// Make std::call_once access pthread_once_t members directly
+#define _GLIBCXX_COMPAT_PTHREAD_ONCE_T \
+  { int _M_done; pthread_mutex_t _M_mutex; }
+
 #endif
diff --git a/libstdc++-v3/config/os/solaris/os_defines.h b/libstdc++-v3/config/os/solaris/os_defines.h
index 36e8cb786cbc..58b7a9f309d4 100644
--- a/libstdc++-v3/config/os/solaris/os_defines.h
+++ b/libstdc++-v3/config/os/solaris/os_defines.h
@@ -35,5 +35,9 @@
 #define __CORRECT_ISO_CPP_WCHAR_H_PROTO
 #endif
 
+// Make std::call_once access pthread_once_t members directly
+#define _GLIBCXX_COMPAT_PTHREAD_ONCE_T \
+  { pthread_mutex_t _M_mutex; unsigned _M_unused; unsigned _M_done; }
+
 #endif
 
diff --git a/libstdc++-v3/include/std/mutex b/libstdc++-v3/include/std/mutex
index 8c7664bff550..6d87834b3bfd 100644
--- a/libstdc++-v3/include/std/mutex
+++ b/libstdc++-v3/include/std/mutex
@@ -684,6 +684,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     enum _Bits : int { _Init = 0, _Active = 1, _Done = 2 };
 
     int _M_once = _Bits::_Init;
+#elif defined _GLIBCXX_COMPAT_PTHREAD_ONCE_T
+    struct _Once _GLIBCXX_COMPAT_PTHREAD_ONCE_T;
+    static_assert(sizeof(_Once) == sizeof(__gthread_once_t), "config error");
+    static_assert(alignof(_Once) == alignof(__gthread_once_t), "config error");
+    union
+    {
+      __gthread_once_t _M_native = __GTHREAD_ONCE_INIT;
+      _Once _M_once;
+    };
+#else
+    __gthread_once_t _M_once = __GTHREAD_ONCE_INIT;
+#endif // ! GTHREADS
 
     // Non-blocking query whether this is known to be a passive execution.
     bool
@@ -706,11 +718,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       once_flag& _M_flag;
       bool _M_returning = false;
     };
-#else
-    __gthread_once_t _M_once = __GTHREAD_ONCE_INIT;
 
     struct _Prepare_execution;
-#endif // ! GTHREADS
 
     template<typename _Callable, typename... _Args>
       friend void
@@ -753,7 +762,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   once_flag::_M_exceptional() noexcept
   { _M_once = _Bits::_Init; }
 
-#else // ! FUTEX && GTHREADS
+#elif defined _GLIBCXX_COMPAT_PTHREAD_ONCE_T
+
+  inline bool
+  once_flag::_M_passive() const noexcept
+  { return false; }
+
+#else
 
   /// @cond undocumented
 # ifdef _GLIBCXX_HAVE_TLS
@@ -814,7 +829,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     void
     call_once(once_flag& __once, _Callable&& __f, _Args&&... __args)
     {
-#if defined _GLIBCXX_HAVE_LINUX_FUTEX || ! defined _GLIBCXX_HAS_GTHREADS
+#if defined _GLIBCXX_HAVE_LINUX_FUTEX || ! defined _GLIBCXX_HAS_GTHREADS \
+      || _GLIBCXX_COMPAT_PTHREAD_ONCE_T
       if (__once._M_passive())
 	return;
       else if (__once._M_activate())
diff --git a/libstdc++-v3/src/c++11/mutex.cc b/libstdc++-v3/src/c++11/mutex.cc
index 286f77f9a454..e63b709749a6 100644
--- a/libstdc++-v3/src/c++11/mutex.cc
+++ b/libstdc++-v3/src/c++11/mutex.cc
@@ -82,6 +82,26 @@ std::once_flag::_M_finish(bool returning) noexcept
     }
 }
 
+#elif defined _GLIBCXX_COMPAT_PTHREAD_ONCE_T
+
+bool
+std::once_flag::_M_activate()
+{
+  pthread_mutex_lock(&_M_once._M_mutex);
+  if (_M_once._M_done == 0)
+    return true;
+  pthread_mutex_unlock(&_M_once._M_mutex);
+  return false;
+}
+
+void
+std::once_flag::_M_finish(bool returning) noexcept
+{
+  if (returning)
+    _M_once._M_done = 1;
+  pthread_mutex_unlock(&_M_once._M_mutex);
+}
+
 #endif // ! FUTEX
 
 #ifndef _GLIBCXX_HAVE_TLS