Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)

  On Tue, Jan 18, 2022 at 09:48:19PM +0000, Jonathan Wakely via Gcc-patches wrote:
> On Tue, 2 Nov 2021 at 01:26, Thomas Rodgers <trodgers@redhat.com> wrote:
> 
> > This should address Jonathan's feedback and adds support for atomic_ref<T>
> >
> 
> 
> >This change implements P0528 which requires that padding bits not
> >participate in atomic compare exchange operations. All arguments to the
> >generic template are 'sanitized' by the __builtin_clearpadding intrisic
> 
> The name of the intrinsic and the word "instrinsic" have typos.

I'd like to ping this patch.
To make some progress, I've tried to incorporate some of Jonathan's
review comments below, but it is incomplete.

ChangeLog + wording above it fixed.

> >
> >       explicit
> >       __atomic_ref(_Tp& __t) : _M_ptr(std::__addressof(__t))
> >-      { __glibcxx_assert(((uintptr_t)_M_ptr % required_alignment) == 0); }
> >+      {
> >+ __glibcxx_assert(((uintptr_t)_M_ptr % required_alignment) == 0);
> >+#if __cplusplus > 201402L && __has_builtin(__builtin_clear_padding)
> >+ __builtin_clear_padding(_M_ptr);
> >+#endif
> >+      }
> 
> Is this safe to do?
> 
> What if multiple threads all create a std::atomic_ref round the same object
> at once, they'll all try to clear padding, and so race, won't they?
> I don't think we can clear padding on atomic_ref construction, only on
> store and RMW operations.

Didn't touch the above.
> 
> 
> >--- a/libstdc++-v3/include/std/atomic
> >+++ b/libstdc++-v3/include/std/atomic

The patch against this file doesn't apply it all.

> >--- /dev/null
> >+++
> b/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc
> >@@ -0,0 +1,43 @@
> >+// { dg-options "-std=gnu++2a" }
> >+// { dg-do run { target c++2a } }
> 
> This new test is using "2a" not "20".

Fixed thus, but the other testcase wasn't in the patch at all.

Here it is:

libstdc++: Clear padding bits in atomic compare_exchange

This change implements P0528 which requires that padding bits not
participate in atomic compare exchange operations. All arguments to the
generic template are 'sanitized' by the __builtin_clear_padding intrinsic
before they are used in comparisons. This requires that any stores
also sanitize the incoming value.

Signed-off-by: Thomas Rodgers <trodgers@redhat.com>

libstdc++-v3/ChangeLog:

	* include/std/atomic (atomic<T>::atomic(_Tp)): Clear padding for
	__cplusplus > 201703L.
	(atomic<T>::store()): Clear padding.
	(atomic<T>::exchange()): Likewise.
	(atomic<T>::compare_exchange_weak()): Likewise.
	(atomic<T>::compare_exchange_strong()): Likewise.
	* include/bits/atomic_base.h (__atomic_impl::__clear_padding()):
	New function.
	(__atomic_impl::__maybe_has_padding()): Likewise.
	(__atomic_impl::__compare_exchange()): Likewise.
	(__atomic_impl::compare_exchange_weak()): Delegate to
	__compare_exchange().
	(__atomic_impl::compare_exchange_strong()): Likewise.
	* testsuite/29_atomics/atomic/compare_exchange_padding.cc: New
	test.
	* testsuite/28_atomics/atomic_ref/compare_exchange_padding.cc:
	Likewise.



	Jakub
  

Sorry for the delay in getting to this.

I am currently working on moving the bulk of the atomic wait implementation
into the .so. I'd like to get that work to a stable state before revisiting
this patch, but obviously if we want this to make it into GCC13, it needs
to happen sooner rather than later.

On Thu, Aug 25, 2022 at 3:11 AM Jakub Jelinek <jakub@redhat.com> wrote:

> On Tue, Jan 18, 2022 at 09:48:19PM +0000, Jonathan Wakely via Gcc-patches
> wrote:
> > On Tue, 2 Nov 2021 at 01:26, Thomas Rodgers <trodgers@redhat.com> wrote:
> >
> > > This should address Jonathan's feedback and adds support for
> atomic_ref<T>
> > >
> >
> >
> > >This change implements P0528 which requires that padding bits not
> > >participate in atomic compare exchange operations. All arguments to the
> > >generic template are 'sanitized' by the __builtin_clearpadding intrisic
> >
> > The name of the intrinsic and the word "instrinsic" have typos.
>
> I'd like to ping this patch.
> To make some progress, I've tried to incorporate some of Jonathan's
> review comments below, but it is incomplete.
>
> ChangeLog + wording above it fixed.
>
> > >
> > >       explicit
> > >       __atomic_ref(_Tp& __t) : _M_ptr(std::__addressof(__t))
> > >-      { __glibcxx_assert(((uintptr_t)_M_ptr % required_alignment) ==
> 0); }
> > >+      {
> > >+ __glibcxx_assert(((uintptr_t)_M_ptr % required_alignment) == 0);
> > >+#if __cplusplus > 201402L && __has_builtin(__builtin_clear_padding)
> > >+ __builtin_clear_padding(_M_ptr);
> > >+#endif
> > >+      }
> >
> > Is this safe to do?
> >
> > What if multiple threads all create a std::atomic_ref round the same
> object
> > at once, they'll all try to clear padding, and so race, won't they?
> > I don't think we can clear padding on atomic_ref construction, only on
> > store and RMW operations.
>
> Didn't touch the above.
> >
> >
> > >--- a/libstdc++-v3/include/std/atomic
> > >+++ b/libstdc++-v3/include/std/atomic
>
> The patch against this file doesn't apply it all.
>
> > >--- /dev/null
> > >+++
> >
> b/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc
> > >@@ -0,0 +1,43 @@
> > >+// { dg-options "-std=gnu++2a" }
> > >+// { dg-do run { target c++2a } }
> >
> > This new test is using "2a" not "20".
>
> Fixed thus, but the other testcase wasn't in the patch at all.
>
> Here it is:
>
> libstdc++: Clear padding bits in atomic compare_exchange
>
> This change implements P0528 which requires that padding bits not
> participate in atomic compare exchange operations. All arguments to the
> generic template are 'sanitized' by the __builtin_clear_padding intrinsic
> before they are used in comparisons. This requires that any stores
> also sanitize the incoming value.
>
> Signed-off-by: Thomas Rodgers <trodgers@redhat.com>
>
> libstdc++-v3/ChangeLog:
>
>         * include/std/atomic (atomic<T>::atomic(_Tp)): Clear padding for
>         __cplusplus > 201703L.
>         (atomic<T>::store()): Clear padding.
>         (atomic<T>::exchange()): Likewise.
>         (atomic<T>::compare_exchange_weak()): Likewise.
>         (atomic<T>::compare_exchange_strong()): Likewise.
>         * include/bits/atomic_base.h (__atomic_impl::__clear_padding()):
>         New function.
>         (__atomic_impl::__maybe_has_padding()): Likewise.
>         (__atomic_impl::__compare_exchange()): Likewise.
>         (__atomic_impl::compare_exchange_weak()): Delegate to
>         __compare_exchange().
>         (__atomic_impl::compare_exchange_strong()): Likewise.
>         * testsuite/29_atomics/atomic/compare_exchange_padding.cc: New
>         test.
>         * testsuite/28_atomics/atomic_ref/compare_exchange_padding.cc:
>         Likewise.
>
> --- a/libstdc++-v3/include/bits/atomic_base.h.jj        2022-05-16
> 09:46:02.361059682 +0200
> +++ b/libstdc++-v3/include/bits/atomic_base.h   2022-08-25
> 12:06:13.758883172 +0200
> @@ -954,6 +954,87 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
>    /// @endcond
>
> +  // Implementation details of atomic padding handling
> +  namespace __atomic_impl
> +  {
> +    template<typename _Tp>
> +      _GLIBCXX_ALWAYS_INLINE _Tp*
> +      __clear_padding(_Tp& __val) noexcept
> +      {
> +       auto* __ptr = std::__addressof(__val);
> +#if __has_builtin(__builtin_clear_padding)
> +       __builtin_clear_padding(std::__addressof(__val));
> +#endif
> +       return __ptr;
> +      }
> +
> +    template<typename _Tp>
> +      constexpr bool
> +      __maybe_has_padding()
> +      {
> +#if ! __has_builtin(__builtin_clear_padding)
> +       return false;
> +#elif __has_builtin(__has_unique_object_representations)
> +       return !__has_unique_object_representations(_Tp)
> +         && !is_floating_point<_Tp>::value;
> +#else
> +       return true;
> +#endif
> +      }
> +
> +    template<typename _Tp>
> +      _GLIBCXX_ALWAYS_INLINE bool
> +      __compare_exchange(_Tp& __val, _Tp& __e, _Tp& __i, bool __weak,
> +                        memory_order __s, memory_order __f) noexcept
> +      {
> +       __glibcxx_assert(__is_valid_cmpexch_failure_order(__f));
> +
> +       if _GLIBCXX17_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Tp>())
> +         {
> +           alignas(_Tp) unsigned char __buf[sizeof(_Tp)];
> +           _Tp* __exp = ::new((void*)__buf) _Tp(__e);
> +           __exp = __atomic_impl::__clear_padding(*__exp);
> +           auto* __des = __atomic_impl::__clear_padding(__i);
> +           if (__atomic_compare_exchange(std::__addressof(__val), __exp,
> __des, __weak,
> +                                         int(__s), int(__f)))
> +             return true;
> +           __builtin_memcpy(std::addressof(__e), __exp, sizeof(_Tp));
> +           return false;
> +         }
> +       else
> +         return __atomic_compare_exchange(std::__addressof(__val),
> +                                          std::__addressof(__e),
> +                                          std::__addressof(__i),
> +                                          __weak, int(__s), int(__f));
> +      }
> +
> +    template<typename _Tp>
> +      _GLIBCXX_ALWAYS_INLINE bool
> +      __compare_exchange(_Tp volatile& __val, _Tp& __e, _Tp& __i, bool
> __weak,
> +                        memory_order __s, memory_order __f) noexcept
> +      {
> +       __glibcxx_assert(__is_valid_cmpexch_failure_order(__f));
> +
> +       if _GLIBCXX17_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Tp>())
> +         {
> +           alignas(_Tp) unsigned char __buf[sizeof(_Tp)];
> +           _Tp* __exp = ::new((void*)__buf) _Tp(__e);
> +           __exp = __atomic_impl::__clear_padding(*__exp);
> +           auto* __des = __atomic_impl::__clear_padding(__i);
> +           if (__atomic_compare_exchange(std::__addressof(__val), __exp,
> __des, __weak,
> +                                         int(__s), int(__f)))
> +             return true;
> +           __builtin_memcpy(std::addressof(__e), __exp, sizeof(_Tp));
> +           return false;
> +         }
> +       else
> +         return __atomic_compare_exchange(std::__addressof(__val),
> +                                          std::__addressof(__e),
> +                                          std::__addressof(__i),
> +                                          __weak, int(__s), int(__f));
> +      }
> +  } // namespace __atomic_impl
> +
>  #if __cplusplus > 201703L
>    /// @cond undocumented
>
> @@ -979,7 +1060,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>      template<typename _Tp>
>        _GLIBCXX_ALWAYS_INLINE void
>        store(_Tp* __ptr, _Val<_Tp> __t, memory_order __m) noexcept
> -      { __atomic_store(__ptr, std::__addressof(__t), int(__m)); }
> +      {
> +       if _GLIBCXX17_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Tp>())
> +         __atomic_impl::__clear_padding(__t);
> +       __atomic_store(__ptr, std::__addressof(__t), int(__m));
> +      }
>
>      template<typename _Tp>
>        _GLIBCXX_ALWAYS_INLINE _Val<_Tp>
> @@ -997,6 +1082,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>        {
>          alignas(_Tp) unsigned char __buf[sizeof(_Tp)];
>         auto* __dest = reinterpret_cast<_Val<_Tp>*>(__buf);
> +       __atomic_impl::__clear_padding(__desired);
>         __atomic_exchange(__ptr, std::__addressof(__desired), __dest,
> int(__m));
>         return *__dest;
>        }
> @@ -1007,11 +1093,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>                             _Val<_Tp> __desired, memory_order __success,
>                             memory_order __failure) noexcept
>        {
> -       __glibcxx_assert(__is_valid_cmpexch_failure_order(__failure));
> -
> -       return __atomic_compare_exchange(__ptr,
> std::__addressof(__expected),
> -                                        std::__addressof(__desired), true,
> -                                        int(__success), int(__failure));
> +       return __atomic_impl::__compare_exchange(*__ptr, __expected,
> __desired,
> +                                                true, __success,
> __failure);
>        }
>
>      template<typename _Tp>
> @@ -1020,11 +1103,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>                               _Val<_Tp> __desired, memory_order __success,
>                               memory_order __failure) noexcept
>        {
> -       __glibcxx_assert(__is_valid_cmpexch_failure_order(__failure));
> -
> -       return __atomic_compare_exchange(__ptr,
> std::__addressof(__expected),
> -                                        std::__addressof(__desired),
> false,
> -                                        int(__success), int(__failure));
> +       return __atomic_impl::__compare_exchange(*__ptr, __expected,
> __desired,
> +                                                false, __success,
> __failure);
>        }
>
>  #if __cpp_lib_atomic_wait
> @@ -1396,7 +1476,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
>        explicit
>        __atomic_ref(_Tp& __t) : _M_ptr(std::__addressof(__t))
> -      { __glibcxx_assert(((uintptr_t)_M_ptr % required_alignment) == 0); }
> +      {
> +       __glibcxx_assert(((uintptr_t)_M_ptr % required_alignment) == 0);
> +#if __cplusplus > 201402L && __has_builtin(__builtin_clear_padding)
> +       __builtin_clear_padding(_M_ptr);
> +#endif
> +      }
>
>        __atomic_ref(const __atomic_ref&) noexcept = default;
>
> ---
> a/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc.jj
>      2022-08-25 11:54:56.094694979 +0200
> +++
> b/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc
> 2022-08-25 11:54:56.094694979 +0200
> @@ -0,0 +1,43 @@
> +// { dg-options "-std=gnu++20" }
> +// { dg-do run { target c++20 } }
> +// { dg-add-options libatomic }
> +
> +#include <atomic>
> +
> +#include <testsuite_hooks.h>
> +
> +struct S { char c; short s; };
> +
> +void __attribute__((noinline,noipa))
> +fill_struct(S& s)
> +{ __builtin_memset(&s, 0xff, sizeof(S)); }
> +
> +bool
> +compare_struct(const S& a, const S& b)
> +{ return __builtin_memcmp(&a, &b, sizeof(S)) == 0; }
> +
> +int
> +main ()
> +{
> +  S s;
> +  fill_struct(s);
> +  s.c = 'a';
> +  s.s = 42;
> +
> +  S ss{ s };
> +  std::atomic_ref<S> as{ s };
> +  auto ts = as.load();
> +  VERIFY( !compare_struct(ss, ts) ); // padding cleared on construction
> +  as.exchange(ss);
> +  auto es = as.load();
> +  VERIFY( compare_struct(ts, es) ); // padding cleared on exchange
> +
> +  S n;
> +  fill_struct(n);
> +  n.c = 'b';
> +  n.s = 71;
> +  // padding cleared on compexchg
> +  VERIFY( as.compare_exchange_weak(s, n) );
> +  VERIFY( as.compare_exchange_strong(n, s) );
> +  return 0;
> +}
>
>
>         Jakub
>
>
  

Here's a complete patch that combines the various incremental patches
that have been going around. I'm testing this now.

Please take a look.
commit 4a0a8ec5bc2a890a1568f99eace6111166e9f72d
Author: Thomas Rodgers <trodgers@redhat.com>
Date:   Thu Aug 25 11:11:40 2022

    libstdc++: Clear padding bits in atomic compare_exchange
    
    This change implements P0528 which requires that padding bits not
    participate in atomic compare exchange operations. All arguments to the
    generic template are 'sanitized' by the __builtin_clear_padding intrinsic
    before they are used in comparisons. This requires that any stores
    also sanitize the incoming value.
    
    Co-authored-by: Jakub Jelinek <jakub@redhat.com>
    Co-authored-by: Jonathan Wakely <jwakely@redhat.com>
    
    Signed-off-by: Thomas Rodgers <trodgers@redhat.com>
    
    libstdc++-v3/ChangeLog:
    
            * include/bits/atomic_base.h (__atomic_impl::__maybe_has_padding):
            New function.
            (__atomic_impl::clear_padding): Likewise.
            (__atomic_impl::__compare_exchange): Likewise.
            (__atomic_impl::compare_exchange_weak): Delegate to
            __compare_exchange.
            (__atomic_impl::compare_exchange_strong): Likewise.
            * include/std/atomic (atomic<T>::atomic(T)): Clear padding when
            possible in a constexpr function.
            (atomic::store): Clear padding.
            (atomic::exchange): Likewise.
            (atomic::compare_exchange_weak): Use __compare_exchange.
            (atomic::compare_exchange_strong): Likewise.
            * testsuite/29_atomics/atomic/compare_exchange_padding.cc: New
            test.
            * testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc:
            New test.

diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/include/bits/atomic_base.h
index d29e4434177..29315547aab 100644
--- a/libstdc++-v3/include/bits/atomic_base.h
+++ b/libstdc++-v3/include/bits/atomic_base.h
@@ -33,6 +33,7 @@
 #pragma GCC system_header
 
 #include <bits/c++config.h>
+#include <new> // For placement new
 #include <stdint.h>
 #include <bits/atomic_lockfree_defines.h>
 #include <bits/move.h>
@@ -952,19 +953,76 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       { return __atomic_fetch_sub(&_M_p, _M_type_size(__d), int(__m)); }
     };
 
-  /// @endcond
+  namespace __atomic_impl
+  {
+    // Implementation details of atomic padding handling
+
+    template<typename _Tp>
+      constexpr bool
+      __maybe_has_padding()
+      {
+#if ! __has_builtin(__builtin_clear_padding)
+	return false;
+#elif __has_builtin(__has_unique_object_representations)
+	return !__has_unique_object_representations(_Tp)
+	  && !is_same<_Tp, float>::value && !is_same<_Tp, double>::value;
+#else
+	return true;
+#endif
+      }
+
+    template<typename _Tp>
+      _GLIBCXX_ALWAYS_INLINE _Tp*
+      __clear_padding(_Tp& __val) noexcept
+      {
+	auto* __ptr = std::__addressof(__val);
+#if __has_builtin(__builtin_clear_padding)
+	if _GLIBCXX17_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Tp>())
+	  __builtin_clear_padding(__ptr);
+#endif
+	return __ptr;
+      }
+
+    // Remove volatile and create a non-deduced context for value arguments.
+    template<typename _Tp>
+      using _Val = typename remove_volatile<_Tp>::type;
+
+    template<typename _Tp>
+      _GLIBCXX_ALWAYS_INLINE bool
+      __compare_exchange(_Tp& __val, _Val<_Tp>& __e, _Val<_Tp>& __i,
+			 bool __weak, memory_order __s, memory_order __f) noexcept
+      {
+	__glibcxx_assert(__is_valid_cmpexch_failure_order(__f));
+
+	using _Vp = _Val<_Tp>;
+
+	if _GLIBCXX17_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Vp>())
+	  {
+	    // We must not modify __e on success, so cannot clear its padding.
+	    // Copy into a buffer and clear that, then copy back on failure.
+	    alignas(_Vp) unsigned char __buf[sizeof(_Vp)];
+	    _Vp* __exp = ::new((void*)__buf) _Vp(__e);
+	    __atomic_impl::__clear_padding(*__exp);
+	    if (__atomic_compare_exchange(std::__addressof(__val), __exp,
+					  __atomic_impl::__clear_padding(__i),
+					  __weak, int(__s), int(__f)))
+	      return true;
+	    __builtin_memcpy(std::__addressof(__e), __exp, sizeof(_Vp));
+	    return false;
+	  }
+	else
+	  return __atomic_compare_exchange(std::__addressof(__val),
+					   std::__addressof(__e),
+					   std::__addressof(__i),
+					   __weak, int(__s), int(__f));
+      }
+  } // namespace __atomic_impl
 
 #if __cplusplus > 201703L
-  /// @cond undocumented
-
   // Implementation details of atomic_ref and atomic<floating-point>.
   namespace __atomic_impl
   {
-    // Remove volatile and create a non-deduced context for value arguments.
-    template<typename _Tp>
-      using _Val = remove_volatile_t<_Tp>;
-
-    // As above, but for difference_type arguments.
+    // Like _Val<T> above, but for difference_type arguments.
     template<typename _Tp>
       using _Diff = __conditional_t<is_pointer_v<_Tp>, ptrdiff_t, _Val<_Tp>>;
 
@@ -979,7 +1037,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     template<typename _Tp>
       _GLIBCXX_ALWAYS_INLINE void
       store(_Tp* __ptr, _Val<_Tp> __t, memory_order __m) noexcept
-      { __atomic_store(__ptr, std::__addressof(__t), int(__m)); }
+      {
+	__atomic_store(__ptr, __atomic_impl::__clear_padding(__t), int(__m));
+      }
 
     template<typename _Tp>
       _GLIBCXX_ALWAYS_INLINE _Val<_Tp>
@@ -997,7 +1057,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
         alignas(_Tp) unsigned char __buf[sizeof(_Tp)];
 	auto* __dest = reinterpret_cast<_Val<_Tp>*>(__buf);
-	__atomic_exchange(__ptr, std::__addressof(__desired), __dest, int(__m));
+	__atomic_exchange(__ptr, __atomic_impl::__clear_padding(__desired),
+			  __dest, int(__m));
 	return *__dest;
       }
 
@@ -1007,11 +1068,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 			    _Val<_Tp> __desired, memory_order __success,
 			    memory_order __failure) noexcept
       {
-	__glibcxx_assert(__is_valid_cmpexch_failure_order(__failure));
-
-	return __atomic_compare_exchange(__ptr, std::__addressof(__expected),
-					 std::__addressof(__desired), true,
-					 int(__success), int(__failure));
+	return __atomic_impl::__compare_exchange(*__ptr, __expected, __desired,
+						 true, __success, __failure);
       }
 
     template<typename _Tp>
@@ -1020,11 +1078,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 			      _Val<_Tp> __desired, memory_order __success,
 			      memory_order __failure) noexcept
       {
-	__glibcxx_assert(__is_valid_cmpexch_failure_order(__failure));
-
-	return __atomic_compare_exchange(__ptr, std::__addressof(__expected),
-					 std::__addressof(__desired), false,
-					 int(__success), int(__failure));
+	return __atomic_impl::__compare_exchange(*__ptr, __expected, __desired,
+						 false, __success, __failure);
       }
 
 #if __cpp_lib_atomic_wait
@@ -1955,9 +2010,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       _Tp** _M_ptr;
     };
+#endif // C++2a
 
   /// @endcond
-#endif // C++2a
 
   /// @} group atomics
 
diff --git a/libstdc++-v3/include/std/atomic b/libstdc++-v3/include/std/atomic
index 70055b8fa83..b913960336d 100644
--- a/libstdc++-v3/include/std/atomic
+++ b/libstdc++-v3/include/std/atomic
@@ -230,7 +230,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       atomic& operator=(const atomic&) = delete;
       atomic& operator=(const atomic&) volatile = delete;
 
-      constexpr atomic(_Tp __i) noexcept : _M_i(__i) { }
+      constexpr atomic(_Tp __i) noexcept : _M_i(__i)
+      {
+#if __cplusplus >= 201402L && __has_builtin(__builtin_clear_padding)
+	if _GLIBCXX17_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Tp>())
+	  __builtin_clear_padding(std::__addressof(_M_i));
+#endif
+      }
 
       operator _Tp() const noexcept
       { return load(); }
@@ -270,13 +276,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       void
       store(_Tp __i, memory_order __m = memory_order_seq_cst) noexcept
       {
-	__atomic_store(std::__addressof(_M_i), std::__addressof(__i), int(__m));
+	__atomic_store(std::__addressof(_M_i),
+		       __atomic_impl::__clear_padding(__i),
+		       int(__m));
       }
 
       void
       store(_Tp __i, memory_order __m = memory_order_seq_cst) volatile noexcept
       {
-	__atomic_store(std::__addressof(_M_i), std::__addressof(__i), int(__m));
+	__atomic_store(std::__addressof(_M_i),
+		       __atomic_impl::__clear_padding(__i),
+		       int(__m));
       }
 
       _Tp
@@ -302,7 +312,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
         alignas(_Tp) unsigned char __buf[sizeof(_Tp)];
 	_Tp* __ptr = reinterpret_cast<_Tp*>(__buf);
-	__atomic_exchange(std::__addressof(_M_i), std::__addressof(__i),
+	__atomic_exchange(std::__addressof(_M_i),
+			  __atomic_impl::__clear_padding(__i),
 			  __ptr, int(__m));
 	return *__ptr;
       }
@@ -313,7 +324,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
         alignas(_Tp) unsigned char __buf[sizeof(_Tp)];
 	_Tp* __ptr = reinterpret_cast<_Tp*>(__buf);
-	__atomic_exchange(std::__addressof(_M_i), std::__addressof(__i),
+	__atomic_exchange(std::__addressof(_M_i),
+			  __atomic_impl::__clear_padding(__i),
 			  __ptr, int(__m));
 	return *__ptr;
       }
@@ -322,24 +334,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       compare_exchange_weak(_Tp& __e, _Tp __i, memory_order __s,
 			    memory_order __f) noexcept
       {
-	__glibcxx_assert(__is_valid_cmpexch_failure_order(__f));
-
-	return __atomic_compare_exchange(std::__addressof(_M_i),
-					 std::__addressof(__e),
-					 std::__addressof(__i),
-					 true, int(__s), int(__f));
+	return __atomic_impl::__compare_exchange(_M_i, __e, __i, true,
+						 __s, __f);
       }
 
       bool
       compare_exchange_weak(_Tp& __e, _Tp __i, memory_order __s,
 			    memory_order __f) volatile noexcept
       {
-	__glibcxx_assert(__is_valid_cmpexch_failure_order(__f));
-
-	return __atomic_compare_exchange(std::__addressof(_M_i),
-					 std::__addressof(__e),
-					 std::__addressof(__i),
-					 true, int(__s), int(__f));
+	return __atomic_impl::__compare_exchange(_M_i, __e, __i, true,
+						 __s, __f);
       }
 
       bool
@@ -358,24 +362,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       compare_exchange_strong(_Tp& __e, _Tp __i, memory_order __s,
 			      memory_order __f) noexcept
       {
-	__glibcxx_assert(__is_valid_cmpexch_failure_order(__f));
-
-	return __atomic_compare_exchange(std::__addressof(_M_i),
-					 std::__addressof(__e),
-					 std::__addressof(__i),
-					 false, int(__s), int(__f));
+	return __atomic_impl::__compare_exchange(_M_i, __e, __i, false,
+						 __s, __f);
       }
 
       bool
       compare_exchange_strong(_Tp& __e, _Tp __i, memory_order __s,
 			      memory_order __f) volatile noexcept
       {
-	__glibcxx_assert(__is_valid_cmpexch_failure_order(__f));
-
-	return __atomic_compare_exchange(std::__addressof(_M_i),
-					 std::__addressof(__e),
-					 std::__addressof(__i),
-					 false, int(__s), int(__f));
+	return __atomic_impl::__compare_exchange(_M_i, __e, __i, false,
+						 __s, __f);
       }
 
       bool
diff --git a/libstdc++-v3/testsuite/29_atomics/atomic/compare_exchange_padding.cc b/libstdc++-v3/testsuite/29_atomics/atomic/compare_exchange_padding.cc
new file mode 100644
index 00000000000..c4ab876db2a
--- /dev/null
+++ b/libstdc++-v3/testsuite/29_atomics/atomic/compare_exchange_padding.cc
@@ -0,0 +1,42 @@
+// { dg-options "-std=gnu++20" }
+// { dg-do run { target c++20 } }
+// { dg-add-options libatomic }
+
+#include <atomic>
+
+#include <testsuite_hooks.h>
+
+struct S { char c; short s; };
+
+void __attribute__((noinline,noipa))
+fill_struct(S& s)
+{ __builtin_memset(&s, 0xff, sizeof(S)); }
+
+bool
+compare_struct(const S& a, const S& b)
+{ return __builtin_memcmp(&a, &b, sizeof(S)) == 0; }
+
+int
+main ()
+{
+  S s;
+  fill_struct(s);
+  s.c = 'a';
+  s.s = 42;
+
+  std::atomic<S> as{ s };
+  auto ts = as.load();
+  VERIFY( !compare_struct(s, ts) ); // padding cleared on construction
+  as.exchange(s);
+  auto es = as.load();
+  VERIFY( compare_struct(ts, es) ); // padding cleared on exchange
+
+  S n;
+  fill_struct(n);
+  n.c = 'b';
+  n.s = 71;
+  // padding cleared on compexchg
+  VERIFY( as.compare_exchange_weak(s, n) );
+  VERIFY( as.compare_exchange_strong(n, s) );
+  return 0;
+}
diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc b/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc
new file mode 100644
index 00000000000..1b1a12dddda
--- /dev/null
+++ b/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc
@@ -0,0 +1,43 @@
+// { dg-options "-std=gnu++20" }
+// { dg-do run { target c++20 } }
+// { dg-add-options libatomic }
+
+#include <atomic>
+
+#include <testsuite_hooks.h>
+
+struct S { char c; short s; };
+
+void __attribute__((noinline,noipa))
+fill_struct(S& s)
+{ __builtin_memset(&s, 0xff, sizeof(S)); }
+
+bool
+compare_struct(const S& a, const S& b)
+{ return __builtin_memcmp(&a, &b, sizeof(S)) == 0; }
+
+int
+main ()
+{
+  S s;
+  fill_struct(s);
+  s.c = 'a';
+  s.s = 42;
+
+  S ss{ s };
+  std::atomic_ref<S> as{ s };
+  auto ts = as.load();
+  VERIFY( !compare_struct(ss, ts) ); // padding cleared on construction
+  as.exchange(ss);
+  auto es = as.load();
+  VERIFY( compare_struct(ts, es) ); // padding cleared on exchange
+
+  S n;
+  fill_struct(n);
+  n.c = 'b';
+  n.s = 71;
+  // padding cleared on compexchg
+  VERIFY( as.compare_exchange_weak(s, n) );
+  VERIFY( as.compare_exchange_strong(n, s) );
+  return 0;
+}
  

Looks good to me.

Tom.

On Wed, Sep 7, 2022 at 4:56 AM Jonathan Wakely <jwakely@redhat.com> wrote:

> Here's a complete patch that combines the various incremental patches
> that have been going around. I'm testing this now.
>
> Please take a look.
>
  

Hi Jonathan,

> Here's a complete patch that combines the various incremental patches
> that have been going around. I'm testing this now.
>
> Please take a look.

unfortunately, this patch broke macOS bootstrap (seen on
x86_64-apple-darwin11.4.2):

In file included from /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/shared_ptr_atomic.h:33,
                 from /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/memory:78,
                 from /vol/gcc/src/hg/master/darwin/libstdc++-v3/include/precompiled/stdc++.h:82:
/var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h: In function 'bool std::__atomic_impl::__compare_exchange(_Tp&, _Val<_Tp>&, _Val<_Tp>&, bool, std::memory_order, std::memory_order)':
/var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1008:49: error: expected primary-expression before ',' token
 1008 |                                           __weak, int(__s), int(__f)))
      |                                                 ^
/var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1017:50: error: expected primary-expression before ',' token
 1017 |                                            __weak, int(__s), int(__f));
      |                                                  ^

Darwin gcc predefines __weak= in gcc/config/darwin-c.cc (darwin_cpp_builtins).

	Rainer
  

> On 9 Sep 2022, at 19:36, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> wrote:
> 

>> Here's a complete patch that combines the various incremental patches
>> that have been going around. I'm testing this now.
>> 
>> Please take a look.
> 
> unfortunately, this patch broke macOS bootstrap (seen on
> x86_64-apple-darwin11.4.2):
> 
> In file included from /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/shared_ptr_atomic.h:33,
>                 from /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/memory:78,
>                 from /vol/gcc/src/hg/master/darwin/libstdc++-v3/include/precompiled/stdc++.h:82:
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h: In function 'bool std::__atomic_impl::__compare_exchange(_Tp&, _Val<_Tp>&, _Val<_Tp>&, bool, std::memory_order, std::memory_order)':
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1008:49: error: expected primary-expression before ',' token
> 1008 |                                           __weak, int(__s), int(__f)))
>      |                                                 ^
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1017:50: error: expected primary-expression before ',' token
> 1017 |                                            __weak, int(__s), int(__f));
>      |                                                  ^
> 
> Darwin gcc predefines __weak= in gcc/config/darwin-c.cc (darwin_cpp_builtins).

yes, __weak and __strong are Objective C things (in principle, applicable to non-Darwin targets
using NeXT runtime - there is at least one such target).

Iain
  

s/__weak/__is_weak/g  perhaps?

On Fri, Sep 9, 2022 at 11:46 AM Iain Sandoe via Libstdc++ <
libstdc++@gcc.gnu.org> wrote:

>
>
> > On 9 Sep 2022, at 19:36, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE>
> wrote:
> >
>
> >> Here's a complete patch that combines the various incremental patches
> >> that have been going around. I'm testing this now.
> >>
> >> Please take a look.
> >
> > unfortunately, this patch broke macOS bootstrap (seen on
> > x86_64-apple-darwin11.4.2):
> >
> > In file included from
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/shared_ptr_atomic.h:33,
> >                 from
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/memory:78,
> >                 from
> /vol/gcc/src/hg/master/darwin/libstdc++-v3/include/precompiled/stdc++.h:82:
> >
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:
> In function 'bool std::__atomic_impl::__compare_exchange(_Tp&, _Val<_Tp>&,
> _Val<_Tp>&, bool, std::memory_order, std::memory_order)':
> >
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1008:49:
> error: expected primary-expression before ',' token
> > 1008 |                                           __weak, int(__s),
> int(__f)))
> >      |                                                 ^
> >
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1017:50:
> error: expected primary-expression before ',' token
> > 1017 |                                            __weak, int(__s),
> int(__f));
> >      |                                                  ^
> >
> > Darwin gcc predefines __weak= in gcc/config/darwin-c.cc
> (darwin_cpp_builtins).
>
> yes, __weak and __strong are Objective C things (in principle, applicable
> to non-Darwin targets
> using NeXT runtime - there is at least one such target).
>
> Iain
>
>
  

On Fri, 9 Sept 2022 at 20:01, Thomas Rodgers wrote:
>
> s/__weak/__is_weak/g  perhaps?

Yes, that'll do. Fixed by the attached, with a test to avoid it happening again.

Tested x86_64-linux, pushed to trunk.




>
> On Fri, Sep 9, 2022 at 11:46 AM Iain Sandoe via Libstdc++ <libstdc++@gcc.gnu.org> wrote:
>>
>>
>>
>> > On 9 Sep 2022, at 19:36, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> wrote:
>> >
>>
>> >> Here's a complete patch that combines the various incremental patches
>> >> that have been going around. I'm testing this now.
>> >>
>> >> Please take a look.
>> >
>> > unfortunately, this patch broke macOS bootstrap (seen on
>> > x86_64-apple-darwin11.4.2):
>> >
>> > In file included from /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/shared_ptr_atomic.h:33,
>> >                 from /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/memory:78,
>> >                 from /vol/gcc/src/hg/master/darwin/libstdc++-v3/include/precompiled/stdc++.h:82:
>> > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h: In function 'bool std::__atomic_impl::__compare_exchange(_Tp&, _Val<_Tp>&, _Val<_Tp>&, bool, std::memory_order, std::memory_order)':
>> > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1008:49: error: expected primary-expression before ',' token
>> > 1008 |                                           __weak, int(__s), int(__f)))
>> >      |                                                 ^
>> > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1017:50: error: expected primary-expression before ',' token
>> > 1017 |                                            __weak, int(__s), int(__f));
>> >      |                                                  ^
>> >
>> > Darwin gcc predefines __weak= in gcc/config/darwin-c.cc (darwin_cpp_builtins).
>>
>> yes, __weak and __strong are Objective C things (in principle, applicable to non-Darwin targets
>> using NeXT runtime - there is at least one such target).
>>
>> Iain
>>
commit 007680f946eaffa3c6321624129e1ec18e673091
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Sep 9 21:03:58 2022

    libstdc++: Rename parameter to avoid darwin __weak qualifier
    
    libstdc++-v3/ChangeLog:
    
            * include/bits/atomic_base.h (__atomic_impl::__compare_exchange):
            Rename __weak to __is_weak.
            * testsuite/17_intro/names.cc: Add __weak and __strong.

diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/include/bits/atomic_base.h
index 29315547aab..6ea3268fdf0 100644
--- a/libstdc++-v3/include/bits/atomic_base.h
+++ b/libstdc++-v3/include/bits/atomic_base.h
@@ -990,7 +990,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     template<typename _Tp>
       _GLIBCXX_ALWAYS_INLINE bool
       __compare_exchange(_Tp& __val, _Val<_Tp>& __e, _Val<_Tp>& __i,
-			 bool __weak, memory_order __s, memory_order __f) noexcept
+			 bool __is_weak,
+			 memory_order __s, memory_order __f) noexcept
       {
 	__glibcxx_assert(__is_valid_cmpexch_failure_order(__f));
 
@@ -1005,7 +1006,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	    __atomic_impl::__clear_padding(*__exp);
 	    if (__atomic_compare_exchange(std::__addressof(__val), __exp,
 					  __atomic_impl::__clear_padding(__i),
-					  __weak, int(__s), int(__f)))
+					  __is_weak, int(__s), int(__f)))
 	      return true;
 	    __builtin_memcpy(std::__addressof(__e), __exp, sizeof(_Vp));
 	    return false;
@@ -1014,7 +1015,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  return __atomic_compare_exchange(std::__addressof(__val),
 					   std::__addressof(__e),
 					   std::__addressof(__i),
-					   __weak, int(__s), int(__f));
+					   __is_weak, int(__s), int(__f));
       }
   } // namespace __atomic_impl
 
diff --git a/libstdc++-v3/testsuite/17_intro/names.cc b/libstdc++-v3/testsuite/17_intro/names.cc
index ede2fe8caa7..86fb8f8999b 100644
--- a/libstdc++-v3/testsuite/17_intro/names.cc
+++ b/libstdc++-v3/testsuite/17_intro/names.cc
@@ -129,6 +129,10 @@
 // This clashes with newlib so don't use it.
 # define __lockable		cannot be used as an identifier
 
+#ifndef __APPLE__
+#define __weak   predefined qualifier on darwin
+#define __strong predefined qualifier on darwin
+#endif
 
 // Common template parameter names
 #define OutputIterator		OutputIterator is not a reserved name