On Fri, Apr 11, 2025 at 2:25 PM Jonathan Wakely <jwak...@redhat.com> wrote:
> 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.
>
I see. This seems to be the same drawback as in case of the current fix,
however
my suggestion guarantees that padding bits are ignored/cleared in program
compiled only in C++11 mode. As I understand the suggested change, make a
comparison exchange
prone to fail due padding bits.
>
>
> > }
> >
> > @@ -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
> >>
>
>
