On Fri, 11 Apr 2025 at 08:23, Tomasz Kaminski <tkami...@redhat.com> wrote:
>
>
>
> On Thu, Apr 10, 2025 at 6:27 PM Jonathan Wakely <jwak...@redhat.com> wrote:
>>
>> The std::atomic constructor clears padding bits so that compare_exchange
>> will not fail due to differences in padding bits. But we can only do
>> that for C++14 and later, because in C++11 a constexpr constructor must
>> have an empty body. However, the code in compare_exchange_strong assumes
>> that padding is always cleared, and so it fails in C++11 due to non-zero
>> padding.
>>
>> Since we can't clear the padding in C++11 mode, we shouldn't assume it's
>> been cleared when in C++11 mode. This fixes the reported bug. However,
>> the fix fails to handle the case where the std::atomic is constructed in
>> C++11 code (and so doesn't zero padding) but the CAS happens in C++14
>> code (and so assumes padding has been zeroed). We might need to use the
>> same loop as atomic_ref::compare_exchange_strong to properly fix this
>> bug for that case.
>>
>> Although the mixed C++11 / C++14 case isn't fixed, this is still an
>> incremental improvement. It fixes the pure-C++11 case and doesn't
>> preclude a more comprehensive fix later.
>
> Wouldn't alternative comprehensive fix be equivalent to doing just:
> diff --git a/libstdc++-v3/include/std/atomic b/libstdc++-v3/include/std/atomic
> index 9b1aca0fc09..238cf739161 100644
> --- a/libstdc++-v3/include/std/atomic
> +++ b/libstdc++-v3/include/std/atomic
> @@ -345,7 +345,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> compare_exchange_weak(_Tp& __e, _Tp __i, memory_order __s,
> memory_order __f) noexcept
> {
> - return __atomic_impl::__compare_exchange(_M_i, __e, __i, true,
> + return __atomic_impl::__compare_exchange<(__cplusplus <
> 201402L)>(_M_i, __e, __i, true,
> __s, __f);
If the std::atomic constructor happens in a translation unit compiled
as C++11 but the call to compare_exchange_weak happens in a
translation unit compiled as C++14, then padding won't be cleared, but
this will call __compare_exchange<false> which expects padding to have
been cleared.
> }
>
> @@ -353,7 +353,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> compare_exchange_weak(_Tp& __e, _Tp __i, memory_order __s,
> memory_order __f) volatile noexcept
> {
> - return __atomic_impl::__compare_exchange(_M_i, __e, __i, true,
> + return __atomic_impl::__compare_exchange<(__cplusplus <
> 201402L)>(_M_i, __e, __i, true,
> __s, __f);
> }
>
> @@ -373,7 +373,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> compare_exchange_strong(_Tp& __e, _Tp __i, memory_order __s,
> memory_order __f) noexcept
> {
> - return __atomic_impl::__compare_exchange(_M_i, __e, __i, false,
> + return __atomic_impl::__compare_exchange<(__cplusplus <
> 201402L)>(_M_i, __e, __i, false,
> __s, __f);
> }
>
> @@ -381,7 +381,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> compare_exchange_strong(_Tp& __e, _Tp __i, memory_order __s,
> memory_order __f) volatile noexcept
> {
> - return __atomic_impl::__compare_exchange(_M_i, __e, __i, false,
> + return __atomic_impl::__compare_exchange<(__cplusplus <
> 201402L)>(_M_i, __e, __i, false,
> __s, __f);
> }
>
>>
>> libstdc++-v3/ChangeLog:
>>
>> PR libstdc++/114865
>> * include/bits/atomic_base.h (__maybe_has_padding): Return false
>> for C++11.
>> * include/std/atomic (atomic::atomic(T)): Add comment.
>> * testsuite/29_atomics/atomic/114865.cc: New test.
>> ---
>>
>> Tested x86_64-linux.
>>
>> libstdc++-v3/include/bits/atomic_base.h | 4 +-
>> libstdc++-v3/include/std/atomic | 2 +
>> .../testsuite/29_atomics/atomic/114865.cc | 49 +++++++++++++++++++
>> 3 files changed, 54 insertions(+), 1 deletion(-)
>> create mode 100644 libstdc++-v3/testsuite/29_atomics/atomic/114865.cc
>>
>> diff --git a/libstdc++-v3/include/bits/atomic_base.h
>> b/libstdc++-v3/include/bits/atomic_base.h
>> index 92d1269493f..19fc7a77c1b 100644
>> --- a/libstdc++-v3/include/bits/atomic_base.h
>> +++ b/libstdc++-v3/include/bits/atomic_base.h
>> @@ -954,7 +954,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> constexpr bool
>> __maybe_has_padding()
>> {
>> -#if ! __has_builtin(__builtin_clear_padding)
>> + // We cannot clear padding in the constructor for C++11,
>> + // so return false here to disable all code for zeroing padding.
>> +#if __cplusplus < 201402L || ! __has_builtin(__builtin_clear_padding)
>> return false;
>> #elif __has_builtin(__has_unique_object_representations)
>> return !__has_unique_object_representations(_Tp)
>> diff --git a/libstdc++-v3/include/std/atomic
>> b/libstdc++-v3/include/std/atomic
>> index 9b1aca0fc09..949a9017862 100644
>> --- a/libstdc++-v3/include/std/atomic
>> +++ b/libstdc++-v3/include/std/atomic
>> @@ -243,6 +243,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>
>> constexpr atomic(_Tp __i) noexcept : _M_i(__i)
>> {
>> + // A constexpr constructor must be empty in C++11,
>> + // so we can only clear padding for C++14 and later.
>> #if __cplusplus >= 201402L && __has_builtin(__builtin_clear_padding)
>> if _GLIBCXX17_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Tp>())
>> __builtin_clear_padding(std::__addressof(_M_i));
>> diff --git a/libstdc++-v3/testsuite/29_atomics/atomic/114865.cc
>> b/libstdc++-v3/testsuite/29_atomics/atomic/114865.cc
>> new file mode 100644
>> index 00000000000..577cd480915
>> --- /dev/null
>> +++ b/libstdc++-v3/testsuite/29_atomics/atomic/114865.cc
>> @@ -0,0 +1,49 @@
>> +// { dg-do run { target c++11_only } }
>> +// { dg-require-atomic-cmpxchg-word "" }
>> +// { dg-add-options libatomic }
>> +
>> +// Bug 114865
>> +// std::atomic<X>::compare_exchange_strong seems to hang under GCC 13 for
>> C++11
>> +
>> +#include <atomic>
>> +#include <cstdint>
>> +
>> +struct type
>> +{
>> + std::uint32_t u32;
>> + std::uint16_t u16;
>> +};
>> +
>> +[[gnu::noipa,gnu::noinline,gnu::optimize("O0")]]
>> +type next(const type& old)
>> +{
>> + auto t = old;
>> + ++t.u16;
>> + return t;
>> +}
>> +
>> +[[gnu::noipa,gnu::noinline,gnu::optimize("O0")]]
>> +void
>> +test_pr116440()
>> +{
>> + constexpr auto mo = std::memory_order_relaxed;
>> +
>> + type t;
>> + t.u32 = t.u16 = 0;
>> + std::atomic<type> a(t);
>> +
>> + auto old = a.load(mo);
>> +
>> + while (true)
>> + {
>> + auto t = next(old);
>> +
>> + if (a.compare_exchange_strong(old, t, mo, mo))
>> + return;
>> + }
>> +}
>> +
>> +int main()
>> +{
>> + test_pr116440();
>> +}
>> --
>> 2.49.0
>>
