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);
}
@@ -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
>
>
