PR libstdc++/91057 set locale::id::_M_index atomically
diff mbox series

Message ID 20191009155950.GA3735@redhat.com
State New
Headers show
Series
  • PR libstdc++/91057 set locale::id::_M_index atomically
Related show

Commit Message

Jonathan Wakely Oct. 9, 2019, 3:59 p.m. UTC
If two threads see _M_index==0 concurrently they will both try to set
it, potentially storing the facet at two different indices in the array.

Either set the _M_index data member using an atomic compare-exchange
operation or while holding a mutex.

Also move the LONG_DOUBLE_COMPAT code into a separate function to remove
the visual noise it creates.

	PR libstdc++/91057
	* src/c++98/locale.cc (locale::id::_M_id()) [__GTHREADS]: Use atomic
	compare-exchange or double-checked lock to ensure only one thread sets
	the _M_index variable.
	[_GLIBCXX_LONG_DOUBLE_COMPAT]: Call find_ldbl_sync_facet to detect
	facets that share another facet's ID.
	[_GLIBCXX_LONG_DOUBLE_COMPAT] (find_ldbl_sync_facet): New function.

Tested x86_64-linux, committed to trunk.
commit 9868e2cb77b226e7aeaeec4c04637a161d7f05fc
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Jul 5 14:03:30 2019 +0100

    PR libstdc++/91057 set locale::id::_M_index atomically
    
    If two threads see _M_index==0 concurrently they will both try to set
    it, potentially storing the facet at two different indices in the array.
    
    Either set the _M_index data member using an atomic compare-exchange
    operation or while holding a mutex.
    
    Also move the LONG_DOUBLE_COMPAT code into a separate function to remove
    the visual noise it creates.
    
            PR libstdc++/91057
            * src/c++98/locale.cc (locale::id::_M_id()) [__GTHREADS]: Use atomic
            compare-exchange or double-checked lock to ensure only one thread sets
            the _M_index variable.
            [_GLIBCXX_LONG_DOUBLE_COMPAT]: Call find_ldbl_sync_facet to detect
            facets that share another facet's ID.
            [_GLIBCXX_LONG_DOUBLE_COMPAT] (find_ldbl_sync_facet): New function.

Comments

Jonathan Wakely Oct. 10, 2019, 4:16 p.m. UTC | #1
On 09/10/19 16:59 +0100, Jonathan Wakely wrote:
>If two threads see _M_index==0 concurrently they will both try to set
>it, potentially storing the facet at two different indices in the array.
>
>Either set the _M_index data member using an atomic compare-exchange
>operation or while holding a mutex.
>
>Also move the LONG_DOUBLE_COMPAT code into a separate function to remove
>the visual noise it creates.
>
>	PR libstdc++/91057
>	* src/c++98/locale.cc (locale::id::_M_id()) [__GTHREADS]: Use atomic
>	compare-exchange or double-checked lock to ensure only one thread sets
>	the _M_index variable.
>	[_GLIBCXX_LONG_DOUBLE_COMPAT]: Call find_ldbl_sync_facet to detect
>	facets that share another facet's ID.
>	[_GLIBCXX_LONG_DOUBLE_COMPAT] (find_ldbl_sync_facet): New function.
>
>Tested x86_64-linux, committed to trunk.

That patch was broken on powerpc and other targets with long double
compat symbols.

Tested powerpc64le-linux, committed to trunk.

Patch
diff mbox series

diff --git a/libstdc++-v3/src/c++98/locale.cc b/libstdc++-v3/src/c++98/locale.cc
index 8652f8559c2..1d00edc6f51 100644
--- a/libstdc++-v3/src/c++98/locale.cc
+++ b/libstdc++-v3/src/c++98/locale.cc
@@ -474,6 +474,30 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   // Definitions for static const data members of locale::id
   _Atomic_word locale::id::_S_refcount;  // init'd to 0 by linker
 
+  // XXX GLIBCXX_ABI Deprecated
+#ifdef _GLIBCXX_LONG_DOUBLE_COMPAT
+namespace {
+  inline locale::id*
+  find_ldbl_sync_facet(locale::id* __idp)
+  {
+# define _GLIBCXX_SYNC_ID(facet, mangled) \
+    if (__idp == &::mangled)		  \
+      return &facet::id
+
+    _GLIBCXX_SYNC_ID (num_get<char>, _ZNSt7num_getIcSt19istreambuf_iteratorIcSt11char_traitsIcEEE2idE);
+    _GLIBCXX_SYNC_ID (num_put<char>, _ZNSt7num_putIcSt19ostreambuf_iteratorIcSt11char_traitsIcEEE2idE);
+    _GLIBCXX_SYNC_ID (money_get<char>, _ZNSt9money_getIcSt19istreambuf_iteratorIcSt11char_traitsIcEEE2idE);
+    _GLIBCXX_SYNC_ID (money_put<char>, _ZNSt9money_putIcSt19ostreambuf_iteratorIcSt11char_traitsIcEEE2idE);
+# ifdef _GLIBCXX_USE_WCHAR_T
+    _GLIBCXX_SYNC_ID (num_get<wchar_t>, _ZNSt7num_getIwSt19istreambuf_iteratorIwSt11char_traitsIwEEE2idE);
+    _GLIBCXX_SYNC_ID (num_put<wchar_t>, _ZNSt7num_putIwSt19ostreambuf_iteratorIwSt11char_traitsIwEEE2idE);
+    _GLIBCXX_SYNC_ID (money_get<wchar_t>, _ZNSt9money_getIwSt19istreambuf_iteratorIwSt11char_traitsIwEEE2idE);
+    _GLIBCXX_SYNC_ID (money_put<wchar_t>, _ZNSt9money_putIwSt19ostreambuf_iteratorIwSt11char_traitsIwEEE2idE);
+# endif
+  }
+} // namespace
+#endif
+
   size_t
   locale::id::_M_id() const throw()
   {
@@ -481,26 +505,38 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
 	// XXX GLIBCXX_ABI Deprecated
 #ifdef _GLIBCXX_LONG_DOUBLE_COMPAT
-	locale::id *f = 0;
-# define _GLIBCXX_SYNC_ID(facet, mangled) \
-	if (this == &::mangled)				\
-	  f = &facet::id
-	_GLIBCXX_SYNC_ID (num_get<char>, _ZNSt7num_getIcSt19istreambuf_iteratorIcSt11char_traitsIcEEE2idE);
-	_GLIBCXX_SYNC_ID (num_put<char>, _ZNSt7num_putIcSt19ostreambuf_iteratorIcSt11char_traitsIcEEE2idE);
-	_GLIBCXX_SYNC_ID (money_get<char>, _ZNSt9money_getIcSt19istreambuf_iteratorIcSt11char_traitsIcEEE2idE);
-	_GLIBCXX_SYNC_ID (money_put<char>, _ZNSt9money_putIcSt19ostreambuf_iteratorIcSt11char_traitsIcEEE2idE);
-# ifdef _GLIBCXX_USE_WCHAR_T
-	_GLIBCXX_SYNC_ID (num_get<wchar_t>, _ZNSt7num_getIwSt19istreambuf_iteratorIwSt11char_traitsIwEEE2idE);
-	_GLIBCXX_SYNC_ID (num_put<wchar_t>, _ZNSt7num_putIwSt19ostreambuf_iteratorIwSt11char_traitsIwEEE2idE);
-	_GLIBCXX_SYNC_ID (money_get<wchar_t>, _ZNSt9money_getIwSt19istreambuf_iteratorIwSt11char_traitsIwEEE2idE);
-	_GLIBCXX_SYNC_ID (money_put<wchar_t>, _ZNSt9money_putIwSt19ostreambuf_iteratorIwSt11char_traitsIwEEE2idE);
-# endif
-	if (f)
-	  _M_index = 1 + f->_M_id();
+	if (locale::id* f = find_ldbl_sync_facet(this))
+	{
+	  const size_t sync_id = f->_M_id();
+	  _M_index = 1 + sync_id;
+	  return sync_id;
+	}
+#endif
+
+#ifdef __GTHREADS
+	if (__gthread_active_p())
+	  {
+	    if (__atomic_always_lock_free(sizeof(_M_index), &_M_index))
+	      {
+		const _Atomic_word next
+		  = 1 + __gnu_cxx::__exchange_and_add(&_S_refcount, 1);
+		size_t expected = 0;
+		__atomic_compare_exchange_n(&_M_index, &expected, next,
+					    /* weak = */ false,
+					    /* success = */ __ATOMIC_ACQ_REL,
+					    /* failure = */ __ATOMIC_ACQUIRE);
+	      }
+	    else
+	      {
+		static __gnu_cxx::__mutex m;
+		__gnu_cxx::__scoped_lock l(m);
+		if (!_M_index)
+		  _M_index = ++_S_refcount;
+	      }
+	  }
 	else
 #endif
-	  _M_index = 1 + __gnu_cxx::__exchange_and_add_dispatch(&_S_refcount,
-								1);
+	_M_index = ++_S_refcount; // single-threaded case
       }
     return _M_index - 1;
   }