Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange

[Thomas Rodgers <trodgers@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 10418 bytes --]

This should address Jonathan's feedback and adds support for atomic_ref<T>

On Wed, Sep 29, 2021 at 5:14 AM Jonathan Wakely <jwakely@redhat.com> wrote:

> On Mon, 27 Sept 2021 at 15:11, Thomas Rodgers <rodgert@appliantology.com>
> wrote:
> >
> > From: Thomas Rodgers <rodgert@twrodgers.com>
> >
> > Now with checks for __has_builtin(__builtin_clear_padding)
> >
> > 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
> > before they are used in comparisons. This alrequires 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.
>
> Don't we also need this for std::atomic_ref, i.e. for the
> __atomic_impl free functions in <bits/atomic_base.h>?
>
> There we don't have any distinction between atomic_ref<integral type>
> and atomic_ref<struct with possible padding>, they both use the same
> implementations. But I think that's OK, as I think the built-in is
> smart enough to be a no-op for types with no padding.
>
> >         * testsuite/29_atomics/atomic/compare_exchange_padding.cc: New
> >         test.
> > ---
> >  libstdc++-v3/include/std/atomic               | 41 +++++++++++++++++-
> >  .../atomic/compare_exchange_padding.cc        | 42 +++++++++++++++++++
> >  2 files changed, 81 insertions(+), 2 deletions(-)
> >  create mode 100644
> libstdc++-v3/testsuite/29_atomics/atomic/compare_exchange_padding.cc
> >
> > diff --git a/libstdc++-v3/include/std/atomic
> b/libstdc++-v3/include/std/atomic
> > index 936dd50ba1c..4ac9ccdc1ab 100644
> > --- a/libstdc++-v3/include/std/atomic
> > +++ b/libstdc++-v3/include/std/atomic
> > @@ -228,7 +228,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >        atomic& operator=(const atomic&) = delete;
> >        atomic& operator=(const atomic&) volatile = delete;
> >
> > -      constexpr atomic(_Tp __i) noexcept : _M_i(__i) { }
> > +#if __cplusplus > 201703L && __has_builtin(__builtin_clear_padding)
> > +      constexpr atomic(_Tp __i) noexcept : _M_i(__i)
> > +      { __builtin_clear_padding(std::__addressof(_M_i)); }
> > +#else
> > +      constexpr atomic(_Tp __i) noexcept : _M_i(__i)
> > +      { }
> > +#endif
>
> Please write this as a single function with the preprocessor
> conditions in the body:
>
>       constexpr atomic(_Tp __i) noexcept : _M_i(__i)
>       {
> #if __cplusplus > 201703L && __has_builtin(__builtin_clear_padding)
>         __builtin_clear_padding(std::__addressof(_M_i)); }
> #endif
>       }
>
> This not only avoids duplication of the identical parts, but it avoids
> warnings from ld.gold if you use --detect-odr-violations. Otherwise,
> the linker can see a definition of that constructor on two different
> lines (233 and 236), and so warns about possible ODR violations,
> something like "warning: while linking foo: symbol
> 'std::atomic<int>::atomic(int)' defined in multiple places (possible
> ODR violation): ...atomic:233 ... atomic:236"
>
> Can't we clear the padding for >= 201402L instead of only C++20? Only
> C++11 has a problem with the built-in in a constexpr function, right?
> So we can DTRT for C++14 upwards.
>
>
> >
> >        operator _Tp() const noexcept
> >        { return load(); }
> > @@ -268,12 +274,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >        void
> >        store(_Tp __i, memory_order __m = memory_order_seq_cst) noexcept
> >        {
> > +#if __has_builtin(__builtin_clear_padding)
> > +       __builtin_clear_padding(std::__addressof(__i));
> > +#endif
>
> We repeat this *a lot*. When I started work on this I defined a
> non-member function in the __atomic_impl namespace:
>
>     template<typename _Tp>
>       _GLIBCXX_ALWAYS_INLINE void
>       __clear_padding(_Tp& __val) noexcept
>       {
> #if __has_builtin(__builtin_clear_padding)
>        __builtin_clear_padding(std::__addressof(__val));
> #endif
>       }
>
> Then you can just use that everywhere (except the constexpr
> constructor), without all the #if checks.
>
>
>
> >         __atomic_store(std::__addressof(_M_i), std::__addressof(__i),
> int(__m));
> >        }
> >
> >        void
> >        store(_Tp __i, memory_order __m = memory_order_seq_cst) volatile
> noexcept
> >        {
> > +#if __has_builtin(__builtin_clear_padding)
> > +       __builtin_clear_padding(std::__addressof(__i));
> > +#endif
> >         __atomic_store(std::__addressof(_M_i), std::__addressof(__i),
> int(__m));
> >        }
> >
> > @@ -300,6 +312,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >        {
> >          alignas(_Tp) unsigned char __buf[sizeof(_Tp)];
> >         _Tp* __ptr = reinterpret_cast<_Tp*>(__buf);
> > +#if __has_builtin(__builtin_clear_padding)
> > +       __builtin_clear_padding(std::__addressof(__i));
> > +#endif
> >         __atomic_exchange(std::__addressof(_M_i), std::__addressof(__i),
> >                           __ptr, int(__m));
> >         return *__ptr;
> > @@ -311,6 +326,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >        {
> >          alignas(_Tp) unsigned char __buf[sizeof(_Tp)];
> >         _Tp* __ptr = reinterpret_cast<_Tp*>(__buf);
> > +#if __has_builtin(__builtin_clear_padding)
> > +       __builtin_clear_padding(std::__addressof(__i));
> > +#endif
> >         __atomic_exchange(std::__addressof(_M_i), std::__addressof(__i),
> >                           __ptr, int(__m));
> >         return *__ptr;
> > @@ -322,6 +340,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >        {
> >         __glibcxx_assert(__is_valid_cmpexch_failure_order(__f));
> >
> > +#if __has_builtin(__builtin_clear_padding)
> > +       __builtin_clear_padding(std::__addressof(__e));
>
> This unconditionally clears the padding of __e, which I don't think is
> allowed. It potentially introduces a data race if another thread is
> doing the CAS at the same time, and the program assumes that only the
> CAS that fails will update expected.
>
> See the thread I started at
> https://lists.isocpp.org/parallel/2020/12/3443.php
> ("atomic compare_exchange and padding bits", 2020-12-03)
>
> The conclusion was that writing to __e is not allowed in the failure
> case, so you need to make a copy of it (into a buffer, using memcpy),
> then clear the padding in the copy, then try the
> __atomic_compare_exchange and if it fails, copy back from the buffer
> to __e. If all that extra work doesn't get inlined then we want to
> only do it for types which might have padding bits, so I had
> __atomic_impl::__maybe_has_padding in my unfinished patch:
>
>    template<typename _Tp>
>      constexpr bool
>      __maybe_has_padding()
>      {
> #if __has_builtin(__has_unique_object_representations)
>       return !__has_unique_object_representations(_Tp);
> #else
>       return true;
> #endif
>      }
>
> The MSVC implementation uses !__has_unique_object_representations(_Tp)
> && !is_floating_point<_Tp>::value here, which is better than mine
> above (FP types don't have unique object reps, but also don't have
> padding bits).
>
> And then do something like this in compare_exchange_weak:
>
>
> +      {
> +#if __has_builtin(__builtin_clear_padding)
> +       if _GLIBCXX_CONSTEXPR17 (__maybe_has_padding<_Tp>())
> +         {
> +           _Val<_Tp> __expected0 = __expected; // XXX should use memcpy
> +           auto* __exp = __atomic_impl::__clear_padding(__expected0);
> +           auto* __des = __atomic_impl::__clear_padding(__desired);
> +           if (__atomic_compare_exchange(__ptr, __exp, __des, true,
> +                                         int(__success), int(__failure)))
> +             return true;
> +           __builtin_memcpy(std::__addressof(__expected), __exp,
> sizeof(_Tp));
> +           return false;
> +         }
> +#endif
>        return __atomic_compare_exchange(__ptr,
> std::__addressof(__expected),
>
> And similarly for compare_exchange_strong (or refactor them into one
> function that takes a bool for weak/strong).
>
> If you do all that in __atomic_impl::compare_exchange_weak (making it
> take a bool for weak/strong) then you can reuse it from
> __atomic_impl:compare_exchange_strong, and then change the gneric
> atomic<T>::compare_exchange_{weak,strong} to use that as well.
>
>
>
>
> > 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..0875f168097
> > --- /dev/null
> > +++
> b/libstdc++-v3/testsuite/29_atomics/atomic/compare_exchange_padding.cc
> > @@ -0,0 +1,42 @@
> > +// { dg-options "-std=gnu++2a" }
> > +// { dg-do run { target c++2a } }
>
> We can (and should) use "20" not "2a".
>
> Does it need to be C++20 though, aren't all the clearings that are
> being tested going to happen unconditionally? (well ... as long as the
> builtin exists, which is true for GCC).
>
> > +// { 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) );
>
> Is it safe assume this won't fail spuriously? There is only one thread
> doing the RMW operation, is that enough to avoid spurious failures?
>
> > +  VERIFY( as.compare_exchange_strong(n, s) );
> > +  return 0;
> > +}
> > --
> > 2.31.1
> >
>
>

[-- Attachment #2: 0001-libstdc-Clear-padding-bits-in-atomic-compare_exchang.patch --]
[-- Type: text/x-patch, Size: 13372 bytes --]

From b2960902d114cb3440ced09ab187bcb725b7f06c Mon Sep 17 00:00:00 2001
From: Thomas Rodgers <rodgert@twrodgers.com>
Date: Tue, 5 Oct 2021 16:17:44 -0700
Subject: [PATCH] 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_clearpadding intrisic
before they are used in comparisons. This alrequires 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.
---
 libstdc++-v3/include/bits/atomic_base.h       | 113 ++++++++++++++++--
 libstdc++-v3/include/std/atomic               |  73 +++--------
 .../atomic_ref/compare_exchange_padding.cc    |  43 +++++++
 3 files changed, 160 insertions(+), 69 deletions(-)
 create mode 100644 libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc

diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/include/bits/atomic_base.h
index 71e1de078b5..beec412761f 100644
--- a/libstdc++-v3/include/bits/atomic_base.h
+++ b/libstdc++-v3/include/bits/atomic_base.h
@@ -936,6 +936,89 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       { return __atomic_fetch_sub(&_M_p, _M_type_size(__d), int(__m)); }
     };
 
+  // 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(__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 __has_builtin(__builtin_clear_padding)
+	if _GLIBCXX14_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Tp>())
+	  {
+	    alignas(_Tp) unsigned char __buf[sizeof(_Tp)];
+	    __builtin_memcpy(__buf, std::__addressof(__e), sizeof(_Tp));
+	    auto* __exp = __atomic_impl::__clear_padding(*reinterpret_cast<_Tp*>(__buf));
+	    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
+#endif
+	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 __has_builtin(__builtin_clear_padding)
+	if _GLIBCXX14_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Tp>())
+	  {
+	    alignas(_Tp) unsigned char __buf[sizeof(_Tp)];
+	    __builtin_memcpy(__buf, std::__addressof(__e), sizeof(_Tp));
+	    auto* __exp = __atomic_impl::__clear_padding(*reinterpret_cast<_Tp*>(__buf));
+	    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
+#endif
+	return __atomic_compare_exchange(std::__addressof(__val),
+					 std::__addressof(__e),
+					 std::__addressof(__i),
+					 __weak, int(__s), int(__f));
+      }
+  } // namespace __atomic_impl
+
 #if __cplusplus > 201703L
   // Implementation details of atomic_ref and atomic<floating-point>.
   namespace __atomic_impl
@@ -959,7 +1042,13 @@ _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 __has_builtin(__builtin_clear_padding)
+	if _GLIBCXX14_CONSTEXPR (__maybe_has_padding<_Tp>())
+	  __clear_padding(__t);
+#endif
+	__atomic_store(__ptr, std::__addressof(__t), int(__m));
+      }
 
     template<typename _Tp>
       _GLIBCXX_ALWAYS_INLINE _Val<_Tp>
@@ -977,6 +1066,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
         alignas(_Tp) unsigned char __buf[sizeof(_Tp)];
 	auto* __dest = reinterpret_cast<_Val<_Tp>*>(__buf);
+	__clear_padding(__desired);
 	__atomic_exchange(__ptr, std::__addressof(__desired), __dest, int(__m));
 	return *__dest;
       }
@@ -987,11 +1077,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 __compare_exchange(*__ptr, __expected, __desired, true,
+				  __success, __failure);
       }
 
     template<typename _Tp>
@@ -1000,11 +1087,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 __compare_exchange(*__ptr, __expected, __desired, false,
+				  __success, __failure);
       }
 
 #if __cpp_lib_atomic_wait
@@ -1376,7 +1460,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;
 
diff --git a/libstdc++-v3/include/std/atomic b/libstdc++-v3/include/std/atomic
index 4ac9ccdc1ab..9fa7c95e7e3 100644
--- a/libstdc++-v3/include/std/atomic
+++ b/libstdc++-v3/include/std/atomic
@@ -228,13 +228,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       atomic& operator=(const atomic&) = delete;
       atomic& operator=(const atomic&) volatile = delete;
 
-#if __cplusplus > 201703L && __has_builtin(__builtin_clear_padding)
       constexpr atomic(_Tp __i) noexcept : _M_i(__i)
-      { __builtin_clear_padding(std::__addressof(_M_i)); }
-#else
-      constexpr atomic(_Tp __i) noexcept : _M_i(__i)
-      { }
+      {
+#if __cplusplus > 201402L && __has_builtin(__builtin_clear_padding)
+	__builtin_clear_padding(std::__addressof(_M_i));
 #endif
+      }
 
       operator _Tp() const noexcept
       { return load(); }
@@ -274,18 +273,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       void
       store(_Tp __i, memory_order __m = memory_order_seq_cst) noexcept
       {
-#if __has_builtin(__builtin_clear_padding)
-	__builtin_clear_padding(std::__addressof(__i));
-#endif
+	__atomic_impl::__clear_padding(__i);
 	__atomic_store(std::__addressof(_M_i), std::__addressof(__i), int(__m));
       }
 
       void
       store(_Tp __i, memory_order __m = memory_order_seq_cst) volatile noexcept
       {
-#if __has_builtin(__builtin_clear_padding)
-	__builtin_clear_padding(std::__addressof(__i));
-#endif
+	__atomic_impl::__clear_padding(__i);
 	__atomic_store(std::__addressof(_M_i), std::__addressof(__i), int(__m));
       }
 
@@ -312,9 +307,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
         alignas(_Tp) unsigned char __buf[sizeof(_Tp)];
 	_Tp* __ptr = reinterpret_cast<_Tp*>(__buf);
-#if __has_builtin(__builtin_clear_padding)
-	__builtin_clear_padding(std::__addressof(__i));
-#endif
+	__atomic_impl::__clear_padding(__i);
 	__atomic_exchange(std::__addressof(_M_i), std::__addressof(__i),
 			  __ptr, int(__m));
 	return *__ptr;
@@ -326,9 +319,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
         alignas(_Tp) unsigned char __buf[sizeof(_Tp)];
 	_Tp* __ptr = reinterpret_cast<_Tp*>(__buf);
-#if __has_builtin(__builtin_clear_padding)
-	__builtin_clear_padding(std::__addressof(__i));
-#endif
+	__atomic_impl::__clear_padding(__i);
 	__atomic_exchange(std::__addressof(_M_i), std::__addressof(__i),
 			  __ptr, int(__m));
 	return *__ptr;
@@ -338,32 +329,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));
-
-#if __has_builtin(__builtin_clear_padding)
-	__builtin_clear_padding(std::__addressof(__e));
-	__builtin_clear_padding(std::__addressof(__i));
-#endif
-	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));
-
-#if __has_builtin(__builtin_clear_padding)
-	__builtin_clear_padding(std::__addressof(__e));
-	__builtin_clear_padding(std::__addressof(__i));
-#endif
-	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
@@ -382,32 +357,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));
-
-#if __has_builtin(__builtin_clear_padding)
-	__builtin_clear_padding(std::__addressof(__e));
-	__builtin_clear_padding(std::__addressof(__i));
-#endif
-	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));
-
-#if __has_builtin(__builtin_clear_padding)
-	__builtin_clear_padding(std::__addressof(__e));
-	__builtin_clear_padding(std::__addressof(__i));
-#endif
-	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_ref/compare_exchange_padding.cc b/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc
new file mode 100644
index 00000000000..ffd87bc1461
--- /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 } }
+// { 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;
+}
-- 
2.31.1