[committed] libstdc++: Use __libc_single_threaded for locale initialization

[committed] libstdc++: Use __libc_single_threaded for locale initialization

[Jonathan Wakely]

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

Most initialization of locales and facets happens before main() during
startup, when the program is likely to only have one thread. By using
the new __gnu_cxx::__is_single_threaded() function instead of checking
__gthread_active_p() we can avoid using pthread_once or atomics for the
common case.

That said, I'm not sure why we don't just use a local static variable
instead, as __cxa_guard_acquire() already optimizes for the
single-threaded case:

  static const bool init = (_S_initialize_once(), true);

I'll revisit that for GCC 12.

libstdc++-v3/ChangeLog:

	* src/c++98/locale.cc (locale::facet::_S_get_c_locale())
	(locale::id::_M_id() const): Use __is_single_threaded.
	* src/c++98/locale_init.cc (locale::_S_initialize()):
	Likewise.

Tested powerpc64le-linux. Committed to trunk.


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

commit e253d36214015ed10ffd335e3628ccaac22dd5c7
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Nov 24 12:29:30 2020

    libstdc++: Use __libc_single_threaded for locale initialization
    
    Most initialization of locales and facets happens before main() during
    startup, when the program is likely to only have one thread. By using
    the new __gnu_cxx::__is_single_threaded() function instead of checking
    __gthread_active_p() we can avoid using pthread_once or atomics for the
    common case.
    
    That said, I'm not sure why we don't just use a local static variable
    instead, as __cxa_guard_acquire() already optimizes for the
    single-threaded case:
    
      static const bool init = (_S_initialize_once(), true);
    
    I'll revisit that for GCC 12.
    
    libstdc++-v3/ChangeLog:
    
            * src/c++98/locale.cc (locale::facet::_S_get_c_locale())
            (locale::id::_M_id() const): Use __is_single_threaded.
            * src/c++98/locale_init.cc (locale::_S_initialize()):
            Likewise.

diff --git a/libstdc++-v3/src/c++98/locale.cc b/libstdc++-v3/src/c++98/locale.cc
index 06422412039c..9b3fc3515152 100644
--- a/libstdc++-v3/src/c++98/locale.cc
+++ b/libstdc++-v3/src/c++98/locale.cc
@@ -214,7 +214,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   locale::facet::_S_get_c_locale()
   {
 #ifdef __GTHREADS
-    if (__gthread_active_p())
+    if (!__gnu_cxx::__is_single_threaded())
       __gthread_once(&_S_once, _S_initialize_once);
     else
 #endif
@@ -515,7 +515,7 @@ namespace {
 #endif
 
 #ifdef __GTHREADS
-	if (__gthread_active_p())
+	if (!__gnu_cxx::__is_single_threaded())
 	  {
 	    if (__atomic_always_lock_free(sizeof(_M_index), &_M_index))
 	      {
diff --git a/libstdc++-v3/src/c++98/locale_init.cc b/libstdc++-v3/src/c++98/locale_init.cc
index c3841ccbd3c9..fc8416ba01a6 100644
--- a/libstdc++-v3/src/c++98/locale_init.cc
+++ b/libstdc++-v3/src/c++98/locale_init.cc
@@ -320,7 +320,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   locale::_S_initialize()
   {
 #ifdef __GTHREADS
-    if (__gthread_active_p())
+    if (!__gnu_cxx::__is_single_threaded())
       __gthread_once(&_S_once, _S_initialize_once);
 #endif
     if (!_S_classic)

Re: [committed] libstdc++: Use __libc_single_threaded for locale initialization

[Stephan Bergmann]

On 24/11/2020 15:59, Jonathan Wakely via Gcc-patches wrote:
> Most initialization of locales and facets happens before main() during
> startup, when the program is likely to only have one thread. By using
> the new __gnu_cxx::__is_single_threaded() function instead of checking
> __gthread_active_p() we can avoid using pthread_once or atomics for the
> common case.
> 
> That said, I'm not sure why we don't just use a local static variable
> instead, as __cxa_guard_acquire() already optimizes for the
> single-threaded case:
> 
>    static const bool init = (_S_initialize_once(), true);
> 
> I'll revisit that for GCC 12.
> 
> libstdc++-v3/ChangeLog:
> 
> 	* src/c++98/locale.cc (locale::facet::_S_get_c_locale())
> 	(locale::id::_M_id() const): Use __is_single_threaded.
> 	* src/c++98/locale_init.cc (locale::_S_initialize()):
> 	Likewise.
> 
> Tested powerpc64le-linux. Committed to trunk.

I now started to get weird crashes when running LibreOffice test code at 
least with Clang -fsanitize=address and latest libstdc++, where the 
Clang ASan machinery SEGVs while it wants to report some malloc/free 
issue.  That goes away when reverting this commit, and I think the root 
cause is that locale::facet::_S_initialize_once() now gets called twice: 
  First during __cxx_global_var_init when the process is still single 
threaded (so

   if (!__gnu_cxx::__is_single_threaded())

in locale::facet::_S_get_c_locale, reading __libc_single_threaded, is 
false, whereas

   if (__gthread_active_p())

would have been true even if the process still only had a single 
thread).  And again after the process has spawned further threads via 
pthread_create (flipping __libc_single_threaded) and calls 
std::ostringstream() -> ... std::locale() -> ..., at which point

   if (!__gnu_cxx::__is_single_threaded())

in locale::facet::_S_get_c_locale is true now.

Re: [committed] libstdc++: Use __libc_single_threaded for locale initialization

[Jonathan Wakely]

On 27/11/20 09:43 +0100, Stephan Bergmann via Libstdc++ wrote:
>On 24/11/2020 15:59, Jonathan Wakely via Gcc-patches wrote:
>>Most initialization of locales and facets happens before main() during
>>startup, when the program is likely to only have one thread. By using
>>the new __gnu_cxx::__is_single_threaded() function instead of checking
>>__gthread_active_p() we can avoid using pthread_once or atomics for the
>>common case.
>>
>>That said, I'm not sure why we don't just use a local static variable
>>instead, as __cxa_guard_acquire() already optimizes for the
>>single-threaded case:
>>
>>   static const bool init = (_S_initialize_once(), true);
>>
>>I'll revisit that for GCC 12.
>>
>>libstdc++-v3/ChangeLog:
>>
>>	* src/c++98/locale.cc (locale::facet::_S_get_c_locale())
>>	(locale::id::_M_id() const): Use __is_single_threaded.
>>	* src/c++98/locale_init.cc (locale::_S_initialize()):
>>	Likewise.
>>
>>Tested powerpc64le-linux. Committed to trunk.
>
>I now started to get weird crashes when running LibreOffice test code 
>at least with Clang -fsanitize=address and latest libstdc++, where the 
>Clang ASan machinery SEGVs while it wants to report some malloc/free 
>issue.  That goes away when reverting this commit, and I think the 
>root cause is that locale::facet::_S_initialize_once() now gets called 
>twice:  First during __cxx_global_var_init when the process is still 
>single threaded (so
>
>  if (!__gnu_cxx::__is_single_threaded())
>
>in locale::facet::_S_get_c_locale, reading __libc_single_threaded, is 
>false, whereas
>
>  if (__gthread_active_p())
>
>would have been true even if the process still only had a single 
>thread).  And again after the process has spawned further threads via 
>pthread_create (flipping __libc_single_threaded) and calls 
>std::ostringstream() -> ... std::locale() -> ..., at which point
>
>  if (!__gnu_cxx::__is_single_threaded())
>
>in locale::facet::_S_get_c_locale is true now.

Gah, yes, that's broken. Sorry. I'm reverting that part and testing
it now.

I still think we should just use local static variables here and let
the runtime do the initialization and optimize it for single-threaded,
but I'll leave that for GCC 12. There might be some reason that wasn't
done originally, maybe so the library could be built with
-fno-threadsafe-statics (which I don't think we should support) or
maybe just because it predates the threadsafe-static code working
well.

Re: [committed] libstdc++: Use __libc_single_threaded for locale initialization

[Jonathan Wakely]

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

On 27/11/20 11:15 +0000, Jonathan Wakely wrote:
>On 27/11/20 09:43 +0100, Stephan Bergmann via Libstdc++ wrote:
>>On 24/11/2020 15:59, Jonathan Wakely via Gcc-patches wrote:
>>>Most initialization of locales and facets happens before main() during
>>>startup, when the program is likely to only have one thread. By using
>>>the new __gnu_cxx::__is_single_threaded() function instead of checking
>>>__gthread_active_p() we can avoid using pthread_once or atomics for the
>>>common case.
>>>
>>>That said, I'm not sure why we don't just use a local static variable
>>>instead, as __cxa_guard_acquire() already optimizes for the
>>>single-threaded case:
>>>
>>>  static const bool init = (_S_initialize_once(), true);
>>>
>>>I'll revisit that for GCC 12.
>>>
>>>libstdc++-v3/ChangeLog:
>>>
>>>	* src/c++98/locale.cc (locale::facet::_S_get_c_locale())
>>>	(locale::id::_M_id() const): Use __is_single_threaded.
>>>	* src/c++98/locale_init.cc (locale::_S_initialize()):
>>>	Likewise.
>>>
>>>Tested powerpc64le-linux. Committed to trunk.
>>
>>I now started to get weird crashes when running LibreOffice test 
>>code at least with Clang -fsanitize=address and latest libstdc++, 
>>where the Clang ASan machinery SEGVs while it wants to report some 
>>malloc/free issue.  That goes away when reverting this commit, and I 
>>think the root cause is that locale::facet::_S_initialize_once() now 
>>gets called twice:  First during __cxx_global_var_init when the 
>>process is still single threaded (so
>>
>> if (!__gnu_cxx::__is_single_threaded())
>>
>>in locale::facet::_S_get_c_locale, reading __libc_single_threaded, 
>>is false, whereas
>>
>> if (__gthread_active_p())
>>
>>would have been true even if the process still only had a single 
>>thread).  And again after the process has spawned further threads 
>>via pthread_create (flipping __libc_single_threaded) and calls 
>>std::ostringstream() -> ... std::locale() -> ..., at which point
>>
>> if (!__gnu_cxx::__is_single_threaded())
>>
>>in locale::facet::_S_get_c_locale is true now.
>
>Gah, yes, that's broken. Sorry. I'm reverting that part and testing
>it now.
>
>I still think we should just use local static variables here and let
>the runtime do the initialization and optimize it for single-threaded,
>but I'll leave that for GCC 12. There might be some reason that wasn't
>done originally, maybe so the library could be built with
>-fno-threadsafe-statics (which I don't think we should support) or
>maybe just because it predates the threadsafe-static code working
>well.

Fixed with this patch, tested powerp64le-linux (glibc 2.32).

Pushed to trunk.

Thanks for the testing and bug report, as always.



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

commit 0d7d69ca4a8c05d883e07ee42058c9c6b0c72370
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Nov 27 11:00:15 2020

    libstdc++: Partially revert r11-5314
    
    The changes in r11-5314 are broken, because it means we don't use
    __gthread_once for the first few initializations, but after the program
    becomes multi-threaded we will repeat the initialization, using
    __gthread_once once this time. This leads to memory errors.
    
    The use of __is_single_threaded() in locale::id::_M_id() is OK, because
    the side effects are the same either way.
    
    libstdc++-v3/ChangeLog:
    
            * src/c++98/locale.cc (locale::facet::_S_get_c_locale()):
            Revert change to use __is_single_threaded.
            * src/c++98/locale_init.cc (locale::_S_initialize()):
            Likewise.

diff --git a/libstdc++-v3/src/c++98/locale.cc b/libstdc++-v3/src/c++98/locale.cc
index 9b3fc3515152..4c1612cc5dca 100644
--- a/libstdc++-v3/src/c++98/locale.cc
+++ b/libstdc++-v3/src/c++98/locale.cc
@@ -214,7 +214,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   locale::facet::_S_get_c_locale()
   {
 #ifdef __GTHREADS
-    if (!__gnu_cxx::__is_single_threaded())
+    if (__gthread_active_p())
       __gthread_once(&_S_once, _S_initialize_once);
     else
 #endif
diff --git a/libstdc++-v3/src/c++98/locale_init.cc b/libstdc++-v3/src/c++98/locale_init.cc
index fc8416ba01a6..c3841ccbd3c9 100644
--- a/libstdc++-v3/src/c++98/locale_init.cc
+++ b/libstdc++-v3/src/c++98/locale_init.cc
@@ -320,7 +320,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   locale::_S_initialize()
   {
 #ifdef __GTHREADS
-    if (!__gnu_cxx::__is_single_threaded())
+    if (__gthread_active_p())
       __gthread_once(&_S_once, _S_initialize_once);
 #endif
     if (!_S_classic)