Hi Mark, On Fri, Sep 5, 2025 at 10:49 AM Mark Wielaard <[email protected]> wrote: > > Hi Aaron, > > On Thu, 2025-09-04 at 14:10 -0400, Aaron Merey wrote: > > On Thu, Sep 4, 2025 at 10:40 AM Mark Wielaard <[email protected]> wrote: > > > On Mon, 2025-09-01 at 20:31 -0400, Aaron Merey wrote: > > > > __libdw_dieabbrev uses the abbrev_lock rwlock to synchronize access to > > > > the > > > > Dwarf_Die abbrev field as well as its lazy loading. Calls to > > > > rwlock_wrlock > > > > and unlock incur significant performance overhead even in > > > > single-threaded > > > > cases. > > > > > > > > This patch implements thread safety in __libdw_dieabbrev with GCC > > > > __atomic > > > > builtins instead of an rwlock, improving performance. > > > > > > configure checks for C11 plus atomics support. So you should be able to > > > just use #include <stdatomic.h> and atomic_compare_exchange* functions. > > > > My understanding is that stdatomic.h atomic_* functions are only > > compatible with values that are declared _Atomic, while the __atomic > > builtins can be used with non-atomic types. Since we can't change the > > declaration of Dwarf_Die.abbrev without breaking compatibility, we > > have to stick with __atomic builtins. > > Meh. But yeah. Changing the type in a public struct is not good. "the > atomic version of type-name may have a different size, alignment, and > object representation". We still have the long int padding__ field, but > that might of course also change size when Atomic is added. We could > maybe static assert the Atomic and non-Atomic types are exactly the > same, but that seems also a bit fragile :{ > > So, sigh, lets go with the __atomic functions for now. Pleas add a > comment to __libdw_dieabbrev explaining this.
I've added the following comment to __libdw_dieabbrev: /* __atomic_* compiler builtin functions are used instead of <stdatomic.h> because the builtins can operate on non-_Atomic types. Dwarf_Die.abbrev cannot be made _Atomic without possibly breaking ABI compatibility. */ I've merged this patch plus the 12 others you approved. Thanks for all the reviews. Aaron
