On Wed, Apr 30, 2025 at 2:30 PM Jonathan Wakely <jwak...@redhat.com> wrote:
> 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. > Preserving the _GLIBCXX_ATOMIC_BUILTINS, while changing it semantic will be silent breaking change, in case of user code checked it to see if there are builtins atomic for bool, short, int, like libbacktrace configure did. Changing it, will hopefully make breakage loud.
