[committed] libstdc++: Use __libc_single_threaded to optimise atomics [PR 96817]
Jonathan Wakely
jwakely@redhat.com
Thu Oct 1 11:56:07 GMT 2020
On 01/10/20 08:50 +0100, Jonathan Wakely wrote:
>On 01/10/20 09:30 +0200, Christophe Lyon via Libstdc++ wrote:
>>On Wed, 30 Sep 2020 at 22:44, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>
>>>On 30/09/20 16:03 +0100, Jonathan Wakely wrote:
>>>>On 29/09/20 13:51 +0200, Christophe Lyon via Libstdc++ wrote:
>>>>>On Sat, 26 Sep 2020 at 21:42, Jonathan Wakely via Gcc-patches
>>>>><gcc-patches@gcc.gnu.org> wrote:
>>>>>>
>>>>>>Glibc 2.32 adds a global variable that says whether the process is
>>>>>>single-threaded. We can use this to decide whether to elide atomic
>>>>>>operations, as a more precise and reliable indicator than
>>>>>>__gthread_active_p.
>>>>>>
>>>>>>This means that guard variables for statics and reference counting in
>>>>>>shared_ptr can use less expensive, non-atomic ops even in processes that
>>>>>>are linked to libpthread, as long as no threads have been created yet.
>>>>>>It also means that we switch to using atomics if libpthread gets loaded
>>>>>>later via dlopen (this still isn't supported in general, for other
>>>>>>reasons).
>>>>>>
>>>>>>We can't use __libc_single_threaded to replace __gthread_active_p
>>>>>>everywhere. If we replaced the uses of __gthread_active_p in std::mutex
>>>>>>then we would elide the pthread_mutex_lock in the code below, but not
>>>>>>the pthread_mutex_unlock:
>>>>>>
>>>>>> std::mutex m;
>>>>>> m.lock(); // pthread_mutex_lock
>>>>>> std::thread t([]{}); // __libc_single_threaded = false
>>>>>> t.join();
>>>>>> m.unlock(); // pthread_mutex_unlock
>>>>>>
>>>>>>We need the lock and unlock to use the same "is threading enabled"
>>>>>>predicate, and similarly for init/destroy pairs for mutexes and
>>>>>>condition variables, so that we don't try to release resources that were
>>>>>>never acquired.
>>>>>>
>>>>>>There are other places that could use __libc_single_threaded, such as
>>>>>>_Sp_locker in src/c++11/shared_ptr.cc and locale init functions, but
>>>>>>they can be changed later.
>>>>>>
>>>>>>libstdc++-v3/ChangeLog:
>>>>>>
>>>>>> PR libstdc++/96817
>>>>>> * include/ext/atomicity.h (__gnu_cxx::__is_single_threaded()):
>>>>>> New function wrapping __libc_single_threaded if available.
>>>>>> (__exchange_and_add_dispatch, __atomic_add_dispatch): Use it.
>>>>>> * libsupc++/guard.cc (__cxa_guard_acquire, __cxa_guard_abort)
>>>>>> (__cxa_guard_release): Likewise.
>>>>>> * testsuite/18_support/96817.cc: New test.
>>>>>>
>>>>>>Tested powerpc64le-linux, with glibc 2.31 and 2.32. Committed to trunk.
>>>>>
>>>>>Hi,
>>>>>
>>>>>This patch introduced regressions on armeb-linux-gnueabhf:
>>>>>--target armeb-none-linux-gnueabihf --with-cpu cortex-a9
>>>>> g++.dg/compat/init/init-ref2 cp_compat_x_tst.o-cp_compat_y_tst.o execute
>>>>> g++.dg/cpp2a/decomp1.C -std=gnu++14 execution test
>>>>> g++.dg/cpp2a/decomp1.C -std=gnu++17 execution test
>>>>> g++.dg/cpp2a/decomp1.C -std=gnu++2a execution test
>>>>> g++.dg/init/init-ref2.C -std=c++14 execution test
>>>>> g++.dg/init/init-ref2.C -std=c++17 execution test
>>>>> g++.dg/init/init-ref2.C -std=c++2a execution test
>>>>> g++.dg/init/init-ref2.C -std=c++98 execution test
>>>>> g++.dg/init/ref15.C -std=c++14 execution test
>>>>> g++.dg/init/ref15.C -std=c++17 execution test
>>>>> g++.dg/init/ref15.C -std=c++2a execution test
>>>>> g++.dg/init/ref15.C -std=c++98 execution test
>>>>> g++.old-deja/g++.jason/pmf7.C -std=c++98 execution test
>>>>> g++.old-deja/g++.mike/leak1.C -std=c++14 execution test
>>>>> g++.old-deja/g++.mike/leak1.C -std=c++17 execution test
>>>>> g++.old-deja/g++.mike/leak1.C -std=c++2a execution test
>>>>> g++.old-deja/g++.mike/leak1.C -std=c++98 execution test
>>>>> g++.old-deja/g++.other/init19.C -std=c++14 execution test
>>>>> g++.old-deja/g++.other/init19.C -std=c++17 execution test
>>>>> g++.old-deja/g++.other/init19.C -std=c++2a execution test
>>>>> g++.old-deja/g++.other/init19.C -std=c++98 execution test
>>>>>
>>>>>and probably some (280) in libstdc++ tests: (I didn't bisect those):
>>>>> 19_diagnostics/error_category/generic_category.cc execution test
>>>>> 19_diagnostics/error_category/system_category.cc execution test
>>>>> 20_util/scoped_allocator/1.cc execution test
>>>>> 20_util/scoped_allocator/2.cc execution test
>>>>> 20_util/scoped_allocator/construct_pair_c++2a.cc execution test
>>>>> 20_util/to_address/debug.cc execution test
>>>>> 20_util/variant/run.cc execution test
>>>>
>>>>I think this is a latent bug in the static initialization code for
>>>>EABI that affects big endian. In libstdc++-v3/libsupc++/guard.cc we
>>>>have:
>>>>
>>>># ifndef _GLIBCXX_GUARD_TEST_AND_ACQUIRE
>>>>
>>>>// Test the guard variable with a memory load with
>>>>// acquire semantics.
>>>>
>>>>inline bool
>>>>__test_and_acquire (__cxxabiv1::__guard *g)
>>>>{
>>>> unsigned char __c;
>>>> unsigned char *__p = reinterpret_cast<unsigned char *>(g);
>>>> __atomic_load (__p, &__c, __ATOMIC_ACQUIRE);
>>>> (void) __p;
>>>> return _GLIBCXX_GUARD_TEST(&__c);
>>>>}
>>>># define _GLIBCXX_GUARD_TEST_AND_ACQUIRE(G) __test_and_acquire (G)
>>>># endif
>>>>
>>>>That inspects the first byte of the guard variable. But for EABI the
>>>>"is initialized" bit is the least significant bit of the guard
>>>>variable. For little endian that's fine, the least significant bit is
>>>>in the first byte. But for big endian, it's not in the first byte, so
>>>>we are looking in the wrong place. This means that the initial check
>>>>in __cxa_guard_acquire is wrong:
>>>>
>>>> extern "C"
>>>> int __cxa_guard_acquire (__guard *g)
>>>> {
>>>>#ifdef __GTHREADS
>>>> // If the target can reorder loads, we need to insert a read memory
>>>> // barrier so that accesses to the guarded variable happen after the
>>>> // guard test.
>>>> if (_GLIBCXX_GUARD_TEST_AND_ACQUIRE (g))
>>>> return 0;
>>>>
>>>>This will always be false for big endian EABI, which means we run the
>>>>rest of the function even when the variable is initialized. Previously
>>>>that still gave the right answer, but inefficiently. After my change
>>>>it gives the wrong answer, because the new code assumes that the check
>>>>right at the start of __cxa_guard_acquire ACTUALLY WORKS. Silly me.
>>>>
>>>>I think the attached should fix it. This should be backported to fix
>>>>inefficient code for static init on big endian EABI targets.
>>>
>>>
>>>Committed as r11-3572, but I haven't been able to test it on a big
>>>endian EABI system, only little endian EABI.
>>>
>>Hi Jonathan,
>>
>>unfortunately, this breaks the build even for little-endian for me:
>>9980570_1.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabi/gcc3/arm-none-linux-gnueabi/libstdc++-v3/include
>>-I/tmp/9980570_1.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++
>>-D_GLIBCXX_SHARED -fno-implicit-templates -Wall -Wextra
>>-Wwrite-strings -Wcast-qual -Wabi=2 -fdiagnostics-show-location=once
>>-ffunction-sections -fdata-sections -frandom-seed=guard.lo -g -O2
>>-D_GNU_SOURCE -c
>>/tmp/9980570_1.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/guard.cc
>>-fPIC -DPIC -D_GLIBCXX_SHARED -o guard.o
>>In file included from
>>/tmp/9980570_1.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/cxxabi.h:50,
>> from
>>/tmp/9980570_1.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/guard.cc:28:
>>/tmp/9980570_1.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/guard.cc:
>>In function 'int
>>__cxxabiv1::__cxa_guard_acquire(__cxxabiv1::__guard*)':
>>/tmp/9980570_1.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++/guard.cc:249:9:
>>error: invalid type argument of unary '*' (have 'int')
>> 249 | if (_GLIBCXX_GUARD_TEST_AND_ACQUIRE (g))
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>make[4]: *** [Makefile:763: guard.lo] Error 1
>>
>>How did you test it? Maybe I'm missing something?
>
>Apparently I didn't do a clean build and the guard.cc file didn't get
>recompiled with the new header.
>
>The attached works on armv7l-unknown-linux-gnueabihf, testing it now.
My testing was only partially successful because the machine I'm
testing on keeps locking up and needing a reboot half way through the
testsuite. But at least it compiles now, and I didn't see any
failures. Only tested on little endian EABI though.
Pushed to master as r11-3585.
>commit 30c59cc6ce174a948d2968ce8d4fef6b55af1bbc
>Author: Jonathan Wakely <jwakely@redhat.com>
>Date: Thu Oct 1 08:45:02 2020
>
> libstdc++: Fix test_and_acquire for EABI
>
> libstdc++-v3/ChangeLog:
>
> * config/cpu/arm/cxxabi_tweaks.h (_GLIBCXX_GUARD_TEST_AND_ACQUIRE):
> Do not try to dereference return value of __atomic_load_n.
>
>diff --git a/libstdc++-v3/config/cpu/arm/cxxabi_tweaks.h b/libstdc++-v3/config/cpu/arm/cxxabi_tweaks.h
>index 4fb34869f8a..a08afed7d21 100644
>--- a/libstdc++-v3/config/cpu/arm/cxxabi_tweaks.h
>+++ b/libstdc++-v3/config/cpu/arm/cxxabi_tweaks.h
>@@ -48,7 +48,7 @@ namespace __cxxabiv1
> typedef int __guard;
>
> #define _GLIBCXX_GUARD_TEST_AND_ACQUIRE(x) \
>- _GLIBCXX_GUARD_TEST(__atomic_load_n(x, __ATOMIC_ACQUIRE))
>+ ((__atomic_load_n(x, __ATOMIC_ACQUIRE) & 1) != 0)
> #define _GLIBCXX_GUARD_SET_AND_RELEASE(x) \
> __atomic_store_n(x, 1, __ATOMIC_RELEASE)
>
More information about the Libstdc++
mailing list