On 22/05/15 17:56, Torvald Riegel wrote: > On Fri, 2015-05-22 at 12:37 +0100, Ramana Radhakrishnan wrote: >> Hi, >> >> While writing atomic_word.h for the ARM backend to fix PR target/66200 >> I >> thought it would make more sense to write it all up with atomic >> primitives instead of providing various fragile bits of inline >> asssembler. Thus this patch came about. I intend to ask for a >> specialized version of this to be applied for the ARM backend in any >> case after testing completes. >> >> If this goes in as a cleanup I expect all the ports to be deleting >> their >> atomic_word.h implementation in the various ports directories and >> I'll >> follow up with patches asking port maintainers to test and apply for >> each of the individual cases if this is deemed to be good enough. > > Could you use C++11 atomics right away instead of adapting the wrappers? I spent some time trying to massage guard.cc into using C++11 atomics but it was just easier to write it with the builtins - I consider this to be a step improvement compared to where we are currently. Rewritten to use the builtins in guard.cc instead of std::atomic as that appears to be a bigger project than moving forward compared to where we are. I prefer small steps rather than big steps in these areas. Further I do not believe you can use the C++11 language features in the headers as they were not necessarily part of the standard for tr1 etc. and thus it's better to remain with the builtins, after all I am also continuing with existing practice in the headers. > > I think the more non-C++11 atomic operations/wrappers we can remove the > better. > >> diff --git a/libstdc++-v3/config/cpu/generic/atomic_word.h b/libstdc >> ++-v3/config/cpu/generic/atomic_word.h >> index 19038bb..bedce31 100644 >> --- a/libstdc++-v3/config/cpu/generic/atomic_word.h >> +++ b/libstdc++-v3/config/cpu/generic/atomic_word.h >> @@ -29,19 +29,46 @@ >> #ifndef _GLIBCXX_ATOMIC_WORD_H >> #define _GLIBCXX_ATOMIC_WORD_H 1 >> >> +#include >> + >> typedef int _Atomic_word; >> >> -// Define these two macros using the appropriate memory barrier for >> the target. >> -// The commented out versions below are the defaults. >> -// See ia64/atomic_word.h for an alternative approach. >> + >> +namespace __gnu_cxx _GLIBCXX_VISIBILITY(default) >> +{ >> + // Test the first byte of __g and ensure that no loads are hoisted >> across >> + // the test. > > That comment is not quite correct. I'd just say that this is a > memory_order_acquire load and a comparison. > Done. >> + inline bool >> + __test_and_acquire (__cxxabiv1::__guard *__g) >> + { >> + unsigned char __c; >> + unsigned char *__p = reinterpret_cast(__g); >> + __atomic_load (__p, &__c, __ATOMIC_ACQUIRE); >> + return __c != 0; >> + } >> + >> + // Set the first byte of __g to 1 and ensure that no stores are >> sunk >> + // across the store. > > Likewise; I'd just say this is a memory_order_release store with 1 as > value. Ok. I've now realized that this is superfluous and better to fix the one definition in guard.cc to do the right thing instead of something like this. > >> + inline void >> + __set_and_release (__cxxabiv1::__guard *__g) >> + { >> + unsigned char *__p = reinterpret_cast(__g); >> + unsigned char val = 1; >> + __atomic_store (__p, &val, __ATOMIC_RELEASE); >> + } >> + >> +#define _GLIBCXX_GUARD_TEST_AND_ACQUIRE(G) >> __gnu_cxx::__test_and_acquire (G) >> +#define _GLIBCXX_GUARD_SET_AND_RELEASE(G) >> __gnu_cxx::__set_and_release (G) >> >> // This one prevents loads from being hoisted across the barrier; >> // in other words, this is a Load-Load acquire barrier. > > Likewise; I'd just say that this is an mo_acquire fence. > >> -// This is necessary iff TARGET_RELAXED_ORDERING is defined in >> tm.h. >> -// #define _GLIBCXX_READ_MEM_BARRIER __asm __volatile ("":::"memory") >> +// This is necessary iff TARGET_RELAXED_ORDERING is defined in tm.h. >> +#define _GLIBCXX_READ_MEM_BARRIER __atomic_thread_fence >> (__ATOMIC_ACQUIRE) >> >> // This one prevents stores from being sunk across the barrier; in >> other >> // words, a Store-Store release barrier. > > Likewise; mo_release fence. > >> -// #define _GLIBCXX_WRITE_MEM_BARRIER __asm __volatile >> ("":::"memory") >> +#define _GLIBCXX_WRITE_MEM_BARRIER __atomic_thread_fence >> (__ATOMIC_RELEASE) >> + >> +} I don't think we can remove _GLIBCXX_READ_MEM_BARRIER and _GLIBCXX_WRITE_MEM_BARRIER from atomic_word.h even though they are superseded by the atomics as it is published in the documentation as available macros. It was obvious that alpha, powerpc, aix, ia64 could just fall back to the standard implementations. The cris and sparc implementations have different types for _Atomic_word from generic and the rest - my guess is sparc can remove the _GLIBCXX_READ_MEM_BARRIER and _GLIBCXX_WRITE_MEM_BARRIER but I have no way of testing this is sane. I'll leave that to the respective target maintainers for sparc and cris to deal as appropriate. 2015-06-09 Ramana Radhakrishnan PR c++/66192 PR target/66200 * config/cpu/generic/atomic_word.h (_GLIBCXX_READ_MEM_BARRIER): Define (_GLIBCXX_WRITE_MEM_BARRIER): Likewise * include/bits/shared_ptr_base.h: Use ACQ_REL barrier. * include/ext/atomicity.h: Likewise. * include/tr1/shared_ptr.h: Likewise. * libsupc++/guard.cc (__test_and_acquire): Rewrite with atomics. Update comment. (__set_and_release): Likewise. * testsuite/20_util/shared_ptr/cons/43820_neg.cc (void test01): Adjust for line numbers. * testsuite/20_util/shared_ptr/cons/void_neg.cc: Likewise. * testsuite/tr1/2_general_utilities/shared_ptr/cons/43820_neg.cc: Likewise. 2015-06-09 Ramana Radhakrishnan * config/cpu/alpha/atomic_word.h: Remove. * config/cpu/ia64/atomic_word.h: Remove. * config/cpu/powerpc/atomic_word.h: Remove. * config/os/aix/atomic_word.h: Remove. * configure.host (atomic_word_dir) [ia64, aix*, powerpc, alpha]: Use generic definition. Bootstrap and regression tested on aarch64-none-linux-gnu, arm-none-linux-gnueabihf and arm-none-eabi. regards Ramana >> >> #endif >