libstdc++: Hide TLS variables in `std::call_once`

Message ID 255071ac-f54a-4ac4-b158-74fb03d7143e@126.com
State New
Headers
Series libstdc++: Hide TLS variables in `std::call_once` |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm fail Test failed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 fail Test failed

Commit Message

LIU Hao Nov. 29, 2024, 2:50 p.m. UTC
  -- 
Best regards,
LIU Hao
From 78ae9cacdfea8bab4fcc8a18068ad30401eb588d Mon Sep 17 00:00:00 2001
From: LIU Hao <lh_mouse@126.com>
Date: Fri, 29 Nov 2024 17:17:01 +0800
Subject: [PATCH] libstdc++: Hide TLS variables in `std::call_once`

This is a transitional change for PR80881, because on Windows, thread-local
variables can't be exported from a DLL.

This commit hides `__once_callable` and `__once` beneath a wrapper function,
but also leaves them external for backward compatibility.

Another purpose for this wrapper function is that, if in the future we decide
to fix PR66146, the changes will be local to source files.

libstdc++-v3/ChangeLog:
	PR libstdc++/66146, target/80881
	* include/std/mutex (once_flag::_M_use_callable): New function
	(call_once): Duplicate from old one for TLS, and make it call
	`_M_use_callable` instead
	* src/c++11/mutex.cc (once_flag::_M_use_callable): New function
---
 libstdc++-v3/include/std/mutex  | 48 ++++++++++++++-------------------
 libstdc++-v3/src/c++11/mutex.cc | 15 +++++++++++
 2 files changed, 35 insertions(+), 28 deletions(-)
  

Comments

Jonathan Wakely Nov. 29, 2024, 3:08 p.m. UTC | #1
The change seems reasonable but this needs a change to
config/abi/pre/gnu.ver to export the new symbol in the GLIBCXX_3.4.34
version.

Please add full stops (periods) to the ChangeLog entry, to make
complete sentences.

Is "PR libstdc++/nnnn, target/nnnn" valid like that? I don't think it
is, it should be two separate lines:

<TAB>PR libstdc++/nnnn
<TAB>PR target/nnnn
  
LIU Hao Nov. 29, 2024, 3:30 p.m. UTC | #2
在 2024-11-29 23:08, Jonathan Wakely 写道:
> The change seems reasonable but this needs a change to
> config/abi/pre/gnu.ver to export the new symbol in the GLIBCXX_3.4.34
> version.

I have added it in the attached patch. However it exists only if `_GLIBCXX_HAVE_TLS` is defined, so 
seems to require an #if.. ?

> Please add full stops (periods) to the ChangeLog entry, to make
> complete sentences.
> 
> Is "PR libstdc++/nnnn, target/nnnn" valid like that? I don't think it
> is, it should be two separate lines:
> 
> <TAB>PR libstdc++/nnnn
> <TAB>PR target/nnnn

Fixed now.


-- 
Best regards,
LIU Hao
From d5cf40386b742a6c2327d2eb5b3685165272f63c Mon Sep 17 00:00:00 2001
From: LIU Hao <lh_mouse@126.com>
Date: Fri, 29 Nov 2024 17:17:01 +0800
Subject: [PATCH] libstdc++: Hide TLS variables in `std::call_once`

This is a transitional change for PR80881, because on Windows, thread-local
variables can't be exported from a DLL.

This commit hides `__once_callable` and `__once` beneath a wrapper function,
but also leaves them external for backward compatibility.

Another purpose for this wrapper function is that, if in the future we decide
to fix PR66146, the changes will be local to source files.

libstdc++-v3/ChangeLog:
	PR libstdc++/66146
	PR target/80881
	* include/std/mutex (once_flag::_M_use_callable): New function.
	(call_once): Duplicate from old one for TLS, and make it call
	`_M_use_callable` instead.
	* src/c++11/mutex.cc (once_flag::_M_use_callable): New function.

Signed-off-by: LIU Hao <lh_mouse@126.com>
---
 libstdc++-v3/config/abi/pre/gnu.ver |  6 ++++
 libstdc++-v3/include/std/mutex      | 48 ++++++++++++-----------------
 libstdc++-v3/src/c++11/mutex.cc     | 15 +++++++++
 3 files changed, 41 insertions(+), 28 deletions(-)

diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver
index 31449b5b87b8..fa7b2277df0e 100644
--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -2528,6 +2528,12 @@ GLIBCXX_3.4.33 {
     _ZNKSt12__basic_fileIcE13native_handleEv;
 } GLIBCXX_3.4.32;
 
+# GCC 15.1.0
+GLIBCXX_3.4.34 {
+    # std::once_flag::_M_use_callable(void (*)(void*), void*)
+    _ZNSt9once_flag15_M_use_callableEPFvPvES0_;
+} GLIBCXX_3.4.33;
+
 # Symbols in the support library (libsupc++) have their own tag.
 CXXABI_1.3 {
 
diff --git a/libstdc++-v3/include/std/mutex b/libstdc++-v3/include/std/mutex
index 8dd9b23191fd..bc1cc4bb006e 100644
--- a/libstdc++-v3/include/std/mutex
+++ b/libstdc++-v3/include/std/mutex
@@ -818,44 +818,37 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     // for most targets this doesn't work correctly for exceptional executions.
     __gthread_once_t _M_once = __GTHREAD_ONCE_INIT;
 
+# ifdef _GLIBCXX_HAVE_TLS
+    void
+    _M_use_callable(void (*__call)(void*), void* __callable);
+# else
     struct _Prepare_execution;
+# endif
 
     template<typename _Callable, typename... _Args>
       friend void
       call_once(once_flag& __once, _Callable&& __f, _Args&&... __args);
   };
 
-  /// @cond undocumented
 # ifdef _GLIBCXX_HAVE_TLS
-  // If TLS is available use thread-local state for the type-erased callable
-  // that is being run by std::call_once in the current thread.
-  extern __thread void* __once_callable;
-  extern __thread void (*__once_call)();
+  /// Invoke a callable and synchronize with other calls using the same flag
+  template<typename _Callable, typename... _Args>
+    void
+    call_once(once_flag& __once, _Callable&& __f, _Args&&... __args)
+    {
+      // Closure type that runs the function
+      auto __callable = [&] {
+	  std::__invoke(std::forward<_Callable>(__f),
+			std::forward<_Args>(__args)...);
+      };
 
-  // RAII type to set up state for pthread_once call.
-  struct once_flag::_Prepare_execution
-  {
-    template<typename _Callable>
-      explicit
-      _Prepare_execution(_Callable& __c)
-      {
-	// Store address in thread-local pointer:
-	__once_callable = std::__addressof(__c);
-	// Trampoline function to invoke the closure via thread-local pointer:
-	__once_call = [] { (*static_cast<_Callable*>(__once_callable))(); };
-      }
+      auto __call = [](void* __p) {
+	  (*static_cast<decltype(__callable)*>(__p))();
+      };
 
-    ~_Prepare_execution()
-    {
-      // PR libstdc++/82481
-      __once_callable = nullptr;
-      __once_call = nullptr;
+      __once._M_use_callable(__call, &__callable);
     }
 
-    _Prepare_execution(const _Prepare_execution&) = delete;
-    _Prepare_execution& operator=(const _Prepare_execution&) = delete;
-  };
-
 # else
   // Without TLS use a global std::mutex and store the callable in a
   // global std::function.
@@ -892,8 +885,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     _Prepare_execution(const _Prepare_execution&) = delete;
     _Prepare_execution& operator=(const _Prepare_execution&) = delete;
   };
-# endif
-  /// @endcond
 
   // This function is passed to pthread_once by std::call_once.
   // It runs __once_call() or __once_functor().
@@ -916,6 +907,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       if (int __e = __gthread_once(&__once._M_once, &__once_proxy))
 	__throw_system_error(__e);
     }
+# endif
 
 #else // _GLIBCXX_HAS_GTHREADS
 
diff --git a/libstdc++-v3/src/c++11/mutex.cc b/libstdc++-v3/src/c++11/mutex.cc
index bfeea538a7c3..5aea0e7b60e2 100644
--- a/libstdc++-v3/src/c++11/mutex.cc
+++ b/libstdc++-v3/src/c++11/mutex.cc
@@ -41,6 +41,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     __once_call();
   }
 
+  void
+  once_flag::_M_use_callable(void (*__call)(void*), void* __callable)
+  {
+    auto once_p = make_pair(__call, __callable);
+    __once_callable = &once_p;
+    __once_call = [] {
+	auto p = static_cast<decltype(once_p)*>(__once_callable);
+	(*p->first)(p->second);
+    };
+
+    // XXX pthread_once does not reset the flag if an exception is thrown.
+    if (int __e = __gthread_once(&_M_once, &__once_proxy))
+      __throw_system_error(__e);
+  }
+
 #else // ! TLS
 
   // Explicit instantiation due to -fno-implicit-instantiation.
  
Jonathan Wakely Nov. 29, 2024, 3:49 p.m. UTC | #3
On Fri, 29 Nov 2024 at 15:31, LIU Hao <lh_mouse@126.com> wrote:
>
> 在 2024-11-29 23:08, Jonathan Wakely 写道:
> > The change seems reasonable but this needs a change to
> > config/abi/pre/gnu.ver to export the new symbol in the GLIBCXX_3.4.34
> > version.
>
> I have added it in the attached patch. However it exists only if `_GLIBCXX_HAVE_TLS` is defined, so
> seems to require an #if.. ?

It looks like your patch is against gcc-14 not trunk, the
GLIBCXX_15.1.0 version is already there.
  
Jonathan Wakely Nov. 29, 2024, 3:50 p.m. UTC | #4
On Fri, 29 Nov 2024 at 15:49, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On Fri, 29 Nov 2024 at 15:31, LIU Hao <lh_mouse@126.com> wrote:
> >
> > 在 2024-11-29 23:08, Jonathan Wakely 写道:
> > > The change seems reasonable but this needs a change to
> > > config/abi/pre/gnu.ver to export the new symbol in the GLIBCXX_3.4.34
> > > version.
> >
> > I have added it in the attached patch. However it exists only if `_GLIBCXX_HAVE_TLS` is defined, so
> > seems to require an #if.. ?
>
> It looks like your patch is against gcc-14 not trunk, the
> GLIBCXX_15.1.0 version is already there.

Sorry, I mean GLIBCXX_3.4.34 for 15.1.0
  
LIU Hao Nov. 29, 2024, 3:57 p.m. UTC | #5
在 2024-11-29 23:50, Jonathan Wakely 写道:
>> It looks like your patch is against gcc-14 not trunk, the
>> GLIBCXX_15.1.0 version is already there.
> 
> Sorry, I mean GLIBCXX_3.4.34 for 15.1.0

Oops that's what I used to test the patch. Reapplied to master now.


-- 
Best regards,
LIU Hao
From bafa1604e632d621a57b50ccd9d7a87b01822fd7 Mon Sep 17 00:00:00 2001
From: LIU Hao <lh_mouse@126.com>
Date: Fri, 29 Nov 2024 17:17:01 +0800
Subject: [PATCH] libstdc++: Hide TLS variables in `std::call_once`

This is a transitional change for PR80881, because on Windows, thread-local
variables can't be exported from a DLL.

This commit hides `__once_callable` and `__once` beneath a wrapper function,
but also leaves them external for backward compatibility.

Another purpose for this wrapper function is that, if in the future we decide
to fix PR66146, the changes will be local to source files.

libstdc++-v3/ChangeLog:
	PR libstdc++/66146
	PR target/80881
	* include/std/mutex (once_flag::_M_use_callable): New function.
	(call_once): Duplicate from old one for TLS, and make it call
	`_M_use_callable` instead.
	* src/c++11/mutex.cc (once_flag::_M_use_callable): New function.
---
 libstdc++-v3/config/abi/pre/gnu.ver |  2 ++
 libstdc++-v3/include/std/mutex      | 48 ++++++++++++-----------------
 libstdc++-v3/src/c++11/mutex.cc     | 15 +++++++++
 3 files changed, 37 insertions(+), 28 deletions(-)

diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver
index ae79b371d80d..ef7141a06b6a 100644
--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -2540,6 +2540,8 @@ GLIBCXX_3.4.34 {
     _ZNSt8__format25__locale_encoding_to_utf8ERKSt6localeSt17basic_string_viewIcSt11char_traitsIcEEPv;
     # __sso_string constructor and destructor
     _ZNSt12__sso_string[CD][12]Ev;
+    # std::once_flag::_M_use_callable(void (*)(void*), void*)
+    _ZNSt9once_flag15_M_use_callableEPFvPvES0_;
 } GLIBCXX_3.4.33;
 
 # Symbols in the support library (libsupc++) have their own tag.
diff --git a/libstdc++-v3/include/std/mutex b/libstdc++-v3/include/std/mutex
index e0cedc4398a9..0fe624f075c5 100644
--- a/libstdc++-v3/include/std/mutex
+++ b/libstdc++-v3/include/std/mutex
@@ -820,44 +820,37 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     // for most targets this doesn't work correctly for exceptional executions.
     __gthread_once_t _M_once = __GTHREAD_ONCE_INIT;
 
+# ifdef _GLIBCXX_HAVE_TLS
+    void
+    _M_use_callable(void (*__call)(void*), void* __callable);
+# else
     struct _Prepare_execution;
+# endif
 
     template<typename _Callable, typename... _Args>
       friend void
       call_once(once_flag& __once, _Callable&& __f, _Args&&... __args);
   };
 
-  /// @cond undocumented
 # ifdef _GLIBCXX_HAVE_TLS
-  // If TLS is available use thread-local state for the type-erased callable
-  // that is being run by std::call_once in the current thread.
-  extern __thread void* __once_callable;
-  extern __thread void (*__once_call)();
+  /// Invoke a callable and synchronize with other calls using the same flag
+  template<typename _Callable, typename... _Args>
+    void
+    call_once(once_flag& __once, _Callable&& __f, _Args&&... __args)
+    {
+      // Closure type that runs the function
+      auto __callable = [&] {
+	  std::__invoke(std::forward<_Callable>(__f),
+			std::forward<_Args>(__args)...);
+      };
 
-  // RAII type to set up state for pthread_once call.
-  struct once_flag::_Prepare_execution
-  {
-    template<typename _Callable>
-      explicit
-      _Prepare_execution(_Callable& __c)
-      {
-	// Store address in thread-local pointer:
-	__once_callable = std::__addressof(__c);
-	// Trampoline function to invoke the closure via thread-local pointer:
-	__once_call = [] { (*static_cast<_Callable*>(__once_callable))(); };
-      }
+      auto __call = [](void* __p) {
+	  (*static_cast<decltype(__callable)*>(__p))();
+      };
 
-    ~_Prepare_execution()
-    {
-      // PR libstdc++/82481
-      __once_callable = nullptr;
-      __once_call = nullptr;
+      __once._M_use_callable(__call, &__callable);
     }
 
-    _Prepare_execution(const _Prepare_execution&) = delete;
-    _Prepare_execution& operator=(const _Prepare_execution&) = delete;
-  };
-
 # else
   // Without TLS use a global std::mutex and store the callable in a
   // global std::function.
@@ -894,8 +887,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     _Prepare_execution(const _Prepare_execution&) = delete;
     _Prepare_execution& operator=(const _Prepare_execution&) = delete;
   };
-# endif
-  /// @endcond
 
   // This function is passed to pthread_once by std::call_once.
   // It runs __once_call() or __once_functor().
@@ -918,6 +909,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       if (int __e = __gthread_once(&__once._M_once, &__once_proxy))
 	__throw_system_error(__e);
     }
+# endif
 
 #else // _GLIBCXX_HAS_GTHREADS
 
diff --git a/libstdc++-v3/src/c++11/mutex.cc b/libstdc++-v3/src/c++11/mutex.cc
index bfeea538a7c3..5aea0e7b60e2 100644
--- a/libstdc++-v3/src/c++11/mutex.cc
+++ b/libstdc++-v3/src/c++11/mutex.cc
@@ -41,6 +41,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     __once_call();
   }
 
+  void
+  once_flag::_M_use_callable(void (*__call)(void*), void* __callable)
+  {
+    auto once_p = make_pair(__call, __callable);
+    __once_callable = &once_p;
+    __once_call = [] {
+	auto p = static_cast<decltype(once_p)*>(__once_callable);
+	(*p->first)(p->second);
+    };
+
+    // XXX pthread_once does not reset the flag if an exception is thrown.
+    if (int __e = __gthread_once(&_M_once, &__once_proxy))
+      __throw_system_error(__e);
+  }
+
 #else // ! TLS
 
   // Explicit instantiation due to -fno-implicit-instantiation.
  
LIU Hao Nov. 30, 2024, 6:05 a.m. UTC | #6
在 2024-11-29 23:57, LIU Hao 写道:
> 在 2024-11-29 23:50, Jonathan Wakely 写道:
>>> It looks like your patch is against gcc-14 not trunk, the
>>> GLIBCXX_15.1.0 version is already there.
>>
>> Sorry, I mean GLIBCXX_3.4.34 for 15.1.0
> 
> Oops that's what I used to test the patch. Reapplied to master now.
> 
> 

Attached is test results on x86_64-pc-linux-gnu:

        === libstdc++ Summary ===

    # of expected passes    18895
    # of expected failures    131
    # of unsupported tests    741
    runtest completed at Sat Nov 30 14:01:31 2024





-- 
Best regards,
LIU Hao
  

Patch

diff --git a/libstdc++-v3/include/std/mutex b/libstdc++-v3/include/std/mutex
index 8dd9b23191fd..bc1cc4bb006e 100644
--- a/libstdc++-v3/include/std/mutex
+++ b/libstdc++-v3/include/std/mutex
@@ -818,44 +818,37 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     // for most targets this doesn't work correctly for exceptional executions.
     __gthread_once_t _M_once = __GTHREAD_ONCE_INIT;
 
+# ifdef _GLIBCXX_HAVE_TLS
+    void
+    _M_use_callable(void (*__call)(void*), void* __callable);
+# else
     struct _Prepare_execution;
+# endif
 
     template<typename _Callable, typename... _Args>
       friend void
       call_once(once_flag& __once, _Callable&& __f, _Args&&... __args);
   };
 
-  /// @cond undocumented
 # ifdef _GLIBCXX_HAVE_TLS
-  // If TLS is available use thread-local state for the type-erased callable
-  // that is being run by std::call_once in the current thread.
-  extern __thread void* __once_callable;
-  extern __thread void (*__once_call)();
+  /// Invoke a callable and synchronize with other calls using the same flag
+  template<typename _Callable, typename... _Args>
+    void
+    call_once(once_flag& __once, _Callable&& __f, _Args&&... __args)
+    {
+      // Closure type that runs the function
+      auto __callable = [&] {
+	  std::__invoke(std::forward<_Callable>(__f),
+			std::forward<_Args>(__args)...);
+      };
 
-  // RAII type to set up state for pthread_once call.
-  struct once_flag::_Prepare_execution
-  {
-    template<typename _Callable>
-      explicit
-      _Prepare_execution(_Callable& __c)
-      {
-	// Store address in thread-local pointer:
-	__once_callable = std::__addressof(__c);
-	// Trampoline function to invoke the closure via thread-local pointer:
-	__once_call = [] { (*static_cast<_Callable*>(__once_callable))(); };
-      }
+      auto __call = [](void* __p) {
+	  (*static_cast<decltype(__callable)*>(__p))();
+      };
 
-    ~_Prepare_execution()
-    {
-      // PR libstdc++/82481
-      __once_callable = nullptr;
-      __once_call = nullptr;
+      __once._M_use_callable(__call, &__callable);
     }
 
-    _Prepare_execution(const _Prepare_execution&) = delete;
-    _Prepare_execution& operator=(const _Prepare_execution&) = delete;
-  };
-
 # else
   // Without TLS use a global std::mutex and store the callable in a
   // global std::function.
@@ -892,8 +885,6 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     _Prepare_execution(const _Prepare_execution&) = delete;
     _Prepare_execution& operator=(const _Prepare_execution&) = delete;
   };
-# endif
-  /// @endcond
 
   // This function is passed to pthread_once by std::call_once.
   // It runs __once_call() or __once_functor().
@@ -916,6 +907,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       if (int __e = __gthread_once(&__once._M_once, &__once_proxy))
 	__throw_system_error(__e);
     }
+# endif
 
 #else // _GLIBCXX_HAS_GTHREADS
 
diff --git a/libstdc++-v3/src/c++11/mutex.cc b/libstdc++-v3/src/c++11/mutex.cc
index bfeea538a7c3..5aea0e7b60e2 100644
--- a/libstdc++-v3/src/c++11/mutex.cc
+++ b/libstdc++-v3/src/c++11/mutex.cc
@@ -41,6 +41,21 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     __once_call();
   }
 
+  void
+  once_flag::_M_use_callable(void (*__call)(void*), void* __callable)
+  {
+    auto once_p = make_pair(__call, __callable);
+    __once_callable = &once_p;
+    __once_call = [] {
+	auto p = static_cast<decltype(once_p)*>(__once_callable);
+	(*p->first)(p->second);
+    };
+
+    // XXX pthread_once does not reset the flag if an exception is thrown.
+    if (int __e = __gthread_once(&_M_once, &__once_proxy))
+      __throw_system_error(__e);
+  }
+
 #else // ! TLS
 
   // Explicit instantiation due to -fno-implicit-instantiation.