On Wed, 30 Apr 2025 at 12:12, Tomasz Kaminski <tkami...@redhat.com> wrote: > > > > On Tue, Apr 29, 2025 at 10:11 PM Jonathan Wakely <jwak...@redhat.com> wrote: >> >> Currently the GLIBCXX_ENABLE_ATOMIC_BUILTINS macro checks for a variety >> of __atomic built-ins for bool, short and int. If all those checks pass, >> then it defines _GLIBCXX_ATOMIC_BUILTINS and uses the definitions from >> config/cpu/generic/atomicity_builtins/atomicity.h for the non-inline >> versions of __exchange_and_add and __atomic_add that get compiled into >> libsupc++. >> >> However, the config/cpu/generic/atomicity_builtins/atomicity.h >> definitions only depend on __atomic_fetch_add not on >> __atomic_test_and_set or __atomic_compare_exchange. And they only >> operate on a variable of type _Atomic word, which is not necessarily one >> of bool, short or int (e.g. for sparcv9 _Atomic_word is 64-bit long). >> >> This means that for a target where _Atomic_word is int but there are no >> 1-byte or 2-byte atomic instructions, GLIBCXX_ENABLE_ATOMIC_BUILTINS >> will fail the checks for bool and short and not define the macro >> _GLIBCXX_ATOMIC_BUILTINS. That means that we will use a single global >> mutex for reference counting in the COW std::string and std::locale, >> even though we could use __atomic_fetch_add to do it lock-free. >> >> This commit removes most of the GLIBCXX_ENABLE_ATOMIC_BUILTINS checks, >> so that it only checks __atomic_fetch_add on _Atomic_word. This will >> enable the atomic versions of __exchange_and_add and __atomic_add for >> more targets. This is not an ABI change, because for targets which >> didn't previously use the atomic definitions of those function, they >> always make a non-inlined call to the functions in the library. If the >> definition of those functions now start using atomics, that doesn't >> change the semantics for the code calling those functions. >> >> On affected targets, new code compiled after this change will see the >> _GLIBCXX_ATOMIC_BUILTINS macro and so will use the always-inline >> versions of __exchange_and_add and __atomic_add, which use >> __atomic_fetch_add directly. That is also compatible with older code >> which calls the non-inline definitions, because those non-inline >> definitions now also use __atomic_fetch_add. >> >> The only configuration where this could be an ABI change is for a target >> which currently defines _GLIBCXX_ATOMIC_BUILTINS (because all the atomic >> built-ins for bool, short and int are supported), but which defines >> _Atomic_word to some other type for which __atomic_fetch_add is _not_ >> supported. For such a target, we would previously have used inline calls >> to __atomic_fetch_add, which would have dependend on libatomic. After >> this commit, we would make non-inline calls into the library where >> __exchange_and_add and __atomic_add would use the global mutex. That >> would be an ABI break. I don't consider that a realistic scenario, >> because it wouldn't have made any sense to define _Atomic_word to a >> wider type than int, when doing so would have required libatomic to make >> libstdc++.so work. Surely such a target would have just used int for its >> _Atomic_word type. >> >> The GLIBCXX_ENABLE_BACKTRACE macro currently uses the >> glibcxx_ac_atomic_int macro defined by the checks that this commit >> removes from GLIBCXX_ENABLE_ATOMIC_BUILTINS. That wasn't a good check >> anyway, because libbacktrace actually depends on atomic loads+stores for >> pointers as well as int, and for atomic stores for size_t. This commit >> replaces the glibcxx_ac_atomic_int check with a proper test for all the >> required atomic operations on all three of int, void* and size_t. This >> ensures that the libbacktrace code used for std::stacktrace will either >> use native atomics, or implement those loads and stores only in terms of >> __sync_bool_compare_and_swap (possibly requiring that to come from >> libatomic or elsewhere). >> >> libstdc++-v3/ChangeLog: >> >> PR libstdc++/70560 >> PR libstdc++/119667 >> * acinclude.m4 (GLIBCXX_ENABLE_ATOMIC_BUILTINS): Only check for >> __atomic_fetch_add on _Atomic_word. >> (GLIBCXX_ENABLE_BACKTRACE): Check for __atomic_load_n and >> __atomic_store_n on int, void* and size_t. >> * config.h.in: Regenerate. >> * configure: Regenerate. >> * configure.host: Fix typo in comment. >> --- >> >> Tested x86_64-linux, no changes to the c++config.h results. >> I need to do more testing on other targets. > > O would rename _GLIBCXX_ATOMIC_BUILTINS to _GLIBCXX_ATOMIC_WORLD_BUILTINS, > to better reflect new reality. I have checked that it seems to be one > libstdc++-v3/include/ext/atomicity.h > file change. And if I mistaken, then the rename will show all affected files > in commit.
That seems like a good suggestion. There's a small chance of it breaking user code that is being naughty and referring to that macro, e.g. this advises to unset it: https://sources.debian.org/src/guitarix/0.46.0+dfsg-1/src/NAM/NeuralAmpModelerCore/Dependencies/eigen/doc/UsingNVCC.dox/?hl=20#L20 I'm not sure we should care about that, it's an internal macro that users should not be referring to.
