public inbox for libstdc++@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
@ 2021-09-23 18:08 Thomas Rodgers
  2021-09-23 19:07 ` Jakub Jelinek
  2021-09-27 14:10 ` Thomas Rodgers
  0 siblings, 2 replies; 23+ messages in thread
From: Thomas Rodgers @ 2021-09-23 18:08 UTC (permalink / raw)
  To: gcc-patches, libstdc++; +Cc: trodgers, Thomas Rodgers

From: Thomas Rodgers <rodgert@twrodgers.com>

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 atomic compare_exchange. 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.
	* testsuite/29_atomics/atomic/compare_exchange_padding.cc: New
	test.
---
 libstdc++-v3/include/std/atomic               | 23 +++++++++-
 .../atomic/compare_exchange_padding.cc        | 42 +++++++++++++++++++
 2 files changed, 63 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..51450badace 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
+      constexpr atomic(_Tp __i) noexcept : _M_i(__i)
+      { __builtin_clear_padding(std::__addressof(_M_i)); }
+#else
+      atomic(_Tp __i) noexcept : _M_i(__i)
+      { }
+#endif
 
       operator _Tp() const noexcept
       { return load(); }
@@ -268,12 +274,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       void
       store(_Tp __i, memory_order __m = memory_order_seq_cst) noexcept
       {
+	__builtin_clear_padding(std::__addressof(__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
       {
+	__builtin_clear_padding(std::__addressof(__i));
 	__atomic_store(std::__addressof(_M_i), std::__addressof(__i), int(__m));
       }
 
@@ -300,6 +308,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
         alignas(_Tp) unsigned char __buf[sizeof(_Tp)];
 	_Tp* __ptr = reinterpret_cast<_Tp*>(__buf);
+	__builtin_clear_padding(std::__addressof(__i));
 	__atomic_exchange(std::__addressof(_M_i), std::__addressof(__i),
 			  __ptr, int(__m));
 	return *__ptr;
@@ -311,6 +320,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
         alignas(_Tp) unsigned char __buf[sizeof(_Tp)];
 	_Tp* __ptr = reinterpret_cast<_Tp*>(__buf);
+	__builtin_clear_padding(std::__addressof(__i));
 	__atomic_exchange(std::__addressof(_M_i), std::__addressof(__i),
 			  __ptr, int(__m));
 	return *__ptr;
@@ -322,6 +332,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
 	__glibcxx_assert(__is_valid_cmpexch_failure_order(__f));
 
+	__builtin_clear_padding(std::__addressof(__e));
+	__builtin_clear_padding(std::__addressof(__i));
 	return __atomic_compare_exchange(std::__addressof(_M_i),
 					 std::__addressof(__e),
 					 std::__addressof(__i),
@@ -334,6 +346,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
 	__glibcxx_assert(__is_valid_cmpexch_failure_order(__f));
 
+	__builtin_clear_padding(std::__addressof(__e));
+	__builtin_clear_padding(std::__addressof(__i));
 	return __atomic_compare_exchange(std::__addressof(_M_i),
 					 std::__addressof(__e),
 					 std::__addressof(__i),
@@ -358,6 +372,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
 	__glibcxx_assert(__is_valid_cmpexch_failure_order(__f));
 
+	__builtin_clear_padding(std::__addressof(__e));
+	__builtin_clear_padding(std::__addressof(__i));
 	return __atomic_compare_exchange(std::__addressof(_M_i),
 					 std::__addressof(__e),
 					 std::__addressof(__i),
@@ -370,6 +386,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
 	__glibcxx_assert(__is_valid_cmpexch_failure_order(__f));
 
+	__builtin_clear_padding(std::__addressof(__e));
+	__builtin_clear_padding(std::__addressof(__i));
 	return __atomic_compare_exchange(std::__addressof(_M_i),
 					 std::__addressof(__e),
 					 std::__addressof(__i),
@@ -392,6 +410,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     void
     wait(_Tp __old, memory_order __m = memory_order_seq_cst) const noexcept
     {
+      __builtin_clear_padding(std::__addressof(__old));
       std::__atomic_wait_address_v(&_M_i, __old,
 			 [__m, this] { return this->load(__m); });
     }
@@ -407,7 +426,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     { std::__atomic_notify_address(&_M_i, true); }
 #endif // __cpp_lib_atomic_wait 
 
-    };
+   };
 #undef _GLIBCXX20_INIT
 
   /// Partial specialization for pointer types.
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 } }
+// { 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;
+}
-- 
2.31.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
  2021-09-23 18:08 [PATCH] libstdc++: Clear padding bits in atomic compare_exchange Thomas Rodgers
@ 2021-09-23 19:07 ` Jakub Jelinek
  2021-09-23 20:15   ` Thomas Rodgers
  2021-09-23 20:15   ` Jonathan Wakely
  2021-09-27 14:10 ` Thomas Rodgers
  1 sibling, 2 replies; 23+ messages in thread
From: Jakub Jelinek @ 2021-09-23 19:07 UTC (permalink / raw)
  To: Thomas Rodgers; +Cc: gcc-patches, libstdc++, trodgers, Thomas Rodgers

On Thu, Sep 23, 2021 at 11:08:37AM -0700, Thomas Rodgers wrote:
> From: Thomas Rodgers <rodgert@twrodgers.com>
> 
> This change implements P0528 which requires that padding bits not
> participate in atomic compare exchange operations. All arguments to the

Thanks for working on this.

> generic template are 'sanitized' by the __builtin_clear_padding intrinsic
> before they are used in atomic compare_exchange. This alrequires that any
> stores also sanitize the incoming value.

Not a review, just random nit.
Shouldn't the __builtin_clear_padding calls be guarded with
#if __has_builtin(__builtin_clear_padding)
or #ifdef _GLIBCXX_HAVE_BUILTIN_CLEAR_PADDING defined based on that?
I think clang doesn't support it (yet?), and it doesn't support the MSVC
__builtin_zero_non_value_bits (with very similar, but slightly different,
behavior).

> 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.
> 	* testsuite/29_atomics/atomic/compare_exchange_padding.cc: New
> 	test.

	Jakub


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
  2021-09-23 19:07 ` Jakub Jelinek
@ 2021-09-23 20:15   ` Thomas Rodgers
  2021-09-23 20:15   ` Jonathan Wakely
  1 sibling, 0 replies; 23+ messages in thread
From: Thomas Rodgers @ 2021-09-23 20:15 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Thomas Rodgers, gcc Patches, libstdc++, Thomas Rodgers

Agreed, I'll revise the patch to do so.

On Thu, Sep 23, 2021 at 12:07 PM Jakub Jelinek <jakub@redhat.com> wrote:

> On Thu, Sep 23, 2021 at 11:08:37AM -0700, Thomas Rodgers wrote:
> > From: Thomas Rodgers <rodgert@twrodgers.com>
> >
> > This change implements P0528 which requires that padding bits not
> > participate in atomic compare exchange operations. All arguments to the
>
> Thanks for working on this.
>
> > generic template are 'sanitized' by the __builtin_clear_padding intrinsic
> > before they are used in atomic compare_exchange. This alrequires that any
> > stores also sanitize the incoming value.
>
> Not a review, just random nit.
> Shouldn't the __builtin_clear_padding calls be guarded with
> #if __has_builtin(__builtin_clear_padding)
> or #ifdef _GLIBCXX_HAVE_BUILTIN_CLEAR_PADDING defined based on that?
> I think clang doesn't support it (yet?), and it doesn't support the MSVC
> __builtin_zero_non_value_bits (with very similar, but slightly different,
> behavior).
>
> > 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.
> >       * testsuite/29_atomics/atomic/compare_exchange_padding.cc: New
> >       test.
>
>         Jakub
>
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
  2021-09-23 19:07 ` Jakub Jelinek
  2021-09-23 20:15   ` Thomas Rodgers
@ 2021-09-23 20:15   ` Jonathan Wakely
  1 sibling, 0 replies; 23+ messages in thread
From: Jonathan Wakely @ 2021-09-23 20:15 UTC (permalink / raw)
  To: Jakub Jelinek
  Cc: Thomas Rodgers, Thomas Rodgers, libstdc++, gcc-patches, Thomas Rodgers

On Thu, 23 Sep 2021, 20:07 Jakub Jelinek via Libstdc++, <
libstdc++@gcc.gnu.org> wrote:

> On Thu, Sep 23, 2021 at 11:08:37AM -0700, Thomas Rodgers wrote:
> > From: Thomas Rodgers <rodgert@twrodgers.com>
> >
> > This change implements P0528 which requires that padding bits not
> > participate in atomic compare exchange operations. All arguments to the
>
> Thanks for working on this.
>
> > generic template are 'sanitized' by the __builtin_clear_padding intrinsic
> > before they are used in atomic compare_exchange. This alrequires that any
> > stores also sanitize the incoming value.
>
> Not a review, just random nit.
> Shouldn't the __builtin_clear_padding calls be guarded with
> #if __has_builtin(__builtin_clear_padding)
> or #ifdef _GLIBCXX_HAVE_BUILTIN_CLEAR_PADDING defined based on that?
>

Yes. It can just use __has_builtin directly. All the compilers we care
about support that now.



I think clang doesn't support it (yet?), and it doesn't support the MSVC
> __builtin_zero_non_value_bits (with very similar, but slightly different,
> behavior).
>
> > 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.
> >       * testsuite/29_atomics/atomic/compare_exchange_padding.cc: New
> >       test.
>
>         Jakub
>
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
  2021-09-23 18:08 [PATCH] libstdc++: Clear padding bits in atomic compare_exchange Thomas Rodgers
  2021-09-23 19:07 ` Jakub Jelinek
@ 2021-09-27 14:10 ` Thomas Rodgers
  2021-09-29 12:13   ` Jonathan Wakely
  1 sibling, 1 reply; 23+ messages in thread
From: Thomas Rodgers @ 2021-09-27 14:10 UTC (permalink / raw)
  To: gcc-patches, libstdc++; +Cc: trodgers, Thomas Rodgers

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.
	* 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
 
       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
 	__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));
+	__builtin_clear_padding(std::__addressof(__i));
+#endif
 	return __atomic_compare_exchange(std::__addressof(_M_i),
 					 std::__addressof(__e),
 					 std::__addressof(__i),
@@ -334,6 +356,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));
+	__builtin_clear_padding(std::__addressof(__i));
+#endif
 	return __atomic_compare_exchange(std::__addressof(_M_i),
 					 std::__addressof(__e),
 					 std::__addressof(__i),
@@ -358,6 +384,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));
+	__builtin_clear_padding(std::__addressof(__i));
+#endif
 	return __atomic_compare_exchange(std::__addressof(_M_i),
 					 std::__addressof(__e),
 					 std::__addressof(__i),
@@ -370,6 +400,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));
+	__builtin_clear_padding(std::__addressof(__i));
+#endif
 	return __atomic_compare_exchange(std::__addressof(_M_i),
 					 std::__addressof(__e),
 					 std::__addressof(__i),
@@ -392,6 +426,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     void
     wait(_Tp __old, memory_order __m = memory_order_seq_cst) const noexcept
     {
+#if __has_builtin(__builtin_clear_padding)
+      __builtin_clear_padding(std::__addressof(__old));
+#endif
       std::__atomic_wait_address_v(&_M_i, __old,
 			 [__m, this] { return this->load(__m); });
     }
@@ -407,7 +444,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     { std::__atomic_notify_address(&_M_i, true); }
 #endif // __cpp_lib_atomic_wait 
 
-    };
+   };
 #undef _GLIBCXX20_INIT
 
   /// Partial specialization for pointer types.
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 } }
+// { 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;
+}
-- 
2.31.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
  2021-09-27 14:10 ` Thomas Rodgers
@ 2021-09-29 12:13   ` Jonathan Wakely
  2021-09-29 12:18     ` Jonathan Wakely
                       ` (3 more replies)
  0 siblings, 4 replies; 23+ messages in thread
From: Jonathan Wakely @ 2021-09-29 12:13 UTC (permalink / raw)
  To: Thomas Rodgers; +Cc: gcc Patches, libstdc++, Thomas Rodgers, Thomas Rodgers

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
>


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
  2021-09-29 12:13   ` Jonathan Wakely
@ 2021-09-29 12:18     ` Jonathan Wakely
  2021-09-29 12:28     ` Jakub Jelinek
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 23+ messages in thread
From: Jonathan Wakely @ 2021-09-29 12:18 UTC (permalink / raw)
  To: Thomas Rodgers; +Cc: gcc Patches, libstdc++, Thomas Rodgers, Thomas Rodgers

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

On Wed, 29 Sept 2021 at 13:13, Jonathan Wakely wrote:
> We repeat this *a lot*. When I started work on this I defined a
> non-member function in the __atomic_impl namespace:

I've attached my incomplete patch from when I was working on this.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 2929 bytes --]

diff --cc libstdc++-v3/include/bits/atomic_base.h
index 71e1de078b5,35023ad30a8..00000000000
--- a/libstdc++-v3/include/bits/atomic_base.h
+++ b/libstdc++-v3/include/bits/atomic_base.h
@@@ -944,6 -952,28 +944,27 @@@ _GLIBCXX_BEGIN_NAMESPACE_VERSIO
      template<typename _Tp>
        using _Val = remove_volatile_t<_Tp>;
  
+     template<typename _Tp>
+       constexpr bool
+       __maybe_has_padding()
+       {
 -#if _GLIBCXX_HAVE_BUILTIN_HAS_UNIQ_OBJ_REP
 -	return !__has_unique_object_representations(_Tp);
++#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 void
+       __clear_padding(_Tp& __val) noexcept
+       {
 -	auto* __ptr = std::__addressof(__val);
+ #if __has_builtin(__builtin_clear_padding)
 -	__builtin_clear_padding(__ptr);
++	__builtin_clear_padding(std::__addressof(__val));
+ #endif
 -	return __ptr;
+       }
+ 
      // As above, but for difference_type arguments.
      template<typename _Tp>
        using _Diff = conditional_t<is_pointer_v<_Tp>, ptrdiff_t, _Val<_Tp>>;
@@@ -984,13 -1015,26 +1006,26 @@@
      template<typename _Tp>
        _GLIBCXX_ALWAYS_INLINE bool
        compare_exchange_weak(_Tp* __ptr, _Val<_Tp>& __expected,
- 			    _Val<_Tp> __desired, memory_order __success,
- 			    memory_order __failure) noexcept
+ 			    _Val<_Tp> __desired,
+ 			    memory_order __success, memory_order __failure,
+ 			    bool __weak = true) noexcept
        {
 +	__glibcxx_assert(__is_valid_cmpexch_failure_order(__failure));
- 
+ #if __has_builtin(__builtin_clear_padding)
+ 	if _GLIBCXX_CONSTEXPR17 (__maybe_has_padding<_Tp>())
+ 	  {
+ 	    _Val<_Tp> __expected0 = __expected;
+ 	    auto* __exp = __atomic_impl::__clear_padding(__expected0);
+ 	    auto* __des = __atomic_impl::__clear_padding(__desired);
+ 	    if (__atomic_compare_exchange(__ptr, __exp, __des, __weak,
+ 					  int(__success), int(__failure)))
+ 	      return true;
+ 	    __builtin_memcpy(std::__addressof(__expected), __exp, sizeof(_Tp));
+ 	    return false;
+ 	  }
 -	else
+ #endif
  	return __atomic_compare_exchange(__ptr, std::__addressof(__expected),
- 					 std::__addressof(__desired), true,
+ 					 std::__addressof(__desired), __weak,
  					 int(__success), int(__failure));
        }
  
@@@ -1000,11 -1044,8 +1035,9 @@@
  			      _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_weak(__ptr, __expected, __desired, __success,
+ 				     __failure, false);
        }
  
  #if __cpp_lib_atomic_wait

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
  2021-09-29 12:13   ` Jonathan Wakely
  2021-09-29 12:18     ` Jonathan Wakely
@ 2021-09-29 12:28     ` Jakub Jelinek
  2021-09-29 18:22     ` Thomas Rodgers
  2021-11-02  1:25     ` Thomas Rodgers
  3 siblings, 0 replies; 23+ messages in thread
From: Jakub Jelinek @ 2021-09-29 12:28 UTC (permalink / raw)
  To: Jonathan Wakely
  Cc: Thomas Rodgers, Thomas Rodgers, libstdc++, gcc Patches, Thomas Rodgers

On Wed, Sep 29, 2021 at 01:13:46PM +0100, Jonathan Wakely via Gcc-patches wrote:
> 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.

Yes.  The only effect it will have is that during the initial optimization
passes the variable/parameter will be addressable where without the call and
__addressof it wouldn't be; but as soon as __builtin_clear_padding is folded
to nothing if there is no padding or something if there is (which happens
quite early) and TODO_update_address_taken is done at the end
of some pass, it will no longer be addressable (unless something different
keeps taking its address).

	Jakub


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
  2021-09-29 12:13   ` Jonathan Wakely
  2021-09-29 12:18     ` Jonathan Wakely
  2021-09-29 12:28     ` Jakub Jelinek
@ 2021-09-29 18:22     ` Thomas Rodgers
  2021-09-29 18:29       ` Jakub Jelinek
  2021-11-02  1:25     ` Thomas Rodgers
  3 siblings, 1 reply; 23+ messages in thread
From: Thomas Rodgers @ 2021-09-29 18:22 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: Thomas Rodgers, gcc Patches, libstdc++, Thomas Rodgers

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.
>
>
We can, I was being conservative expecting guiding elvish feedback :)


>
> >
> >        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
> >
>
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
  2021-09-29 18:22     ` Thomas Rodgers
@ 2021-09-29 18:29       ` Jakub Jelinek
  0 siblings, 0 replies; 23+ messages in thread
From: Jakub Jelinek @ 2021-09-29 18:29 UTC (permalink / raw)
  To: Thomas Rodgers; +Cc: Jonathan Wakely, gcc Patches, libstdc++, Thomas Rodgers

On Wed, Sep 29, 2021 at 11:22:29AM -0700, Thomas Rodgers via Gcc-patches wrote:
> > 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).

Except the 80-bit long double, that one has padding bits (similarly
_Complex long double).
As I said earlier, if you want a builtin using the __builtin_clear_padding
infrastructure that will return bool if __builtin_clear_padding expands to
something or not, it can be easily added.  GCC already uses something like
that internally.

	Jakub


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
  2021-09-29 12:13   ` Jonathan Wakely
                       ` (2 preceding siblings ...)
  2021-09-29 18:22     ` Thomas Rodgers
@ 2021-11-02  1:25     ` Thomas Rodgers
  2021-11-02  7:49       ` Jakub Jelinek
                         ` (2 more replies)
  3 siblings, 3 replies; 23+ messages in thread
From: Thomas Rodgers @ 2021-11-02  1:25 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: Thomas Rodgers, gcc Patches, libstdc++, Thomas Rodgers

[-- 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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
  2021-11-02  1:25     ` Thomas Rodgers
@ 2021-11-02  7:49       ` Jakub Jelinek
  2021-11-03  3:06         ` Thomas Rodgers
  2021-11-02  8:49       ` Daniel Krügler
  2022-01-18 21:48       ` Jonathan Wakely
  2 siblings, 1 reply; 23+ messages in thread
From: Jakub Jelinek @ 2021-11-02  7:49 UTC (permalink / raw)
  To: Thomas Rodgers; +Cc: Jonathan Wakely, gcc Patches, libstdc++, Thomas Rodgers

On Mon, Nov 01, 2021 at 06:25:45PM -0700, Thomas Rodgers via Gcc-patches wrote:
> +    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

I'm not sure I understand the && !is_floating_point<_Tp>::value.
Yes, float and double will never have padding, but long double often
will, e.g. on x86 or ia64 (but e.g. not on ppc, s390x, etc.).
So, unless we want to play with numeric_limits, it should be either
just return !__has_unique_object_representations(_Tp);
or return !__has_unique_object_representations(_Tp)
	  && (!is_floating_point<_Tp>::value
	      || is_same<__remove_cv_t<_Tp>,long double>::value);
or, with numeric_limits test numeric_limits<_Tp>::digits == 64
(but I'm sure Jonathan will not want including such a header dependency
unless it is already included).
Or I can always provide a new __builtin_clear_padding_p ...

	Jakub


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
  2021-11-02  1:25     ` Thomas Rodgers
  2021-11-02  7:49       ` Jakub Jelinek
@ 2021-11-02  8:49       ` Daniel Krügler
  2022-01-18 21:48       ` Jonathan Wakely
  2 siblings, 0 replies; 23+ messages in thread
From: Daniel Krügler @ 2021-11-02  8:49 UTC (permalink / raw)
  To: Thomas Rodgers; +Cc: Jonathan Wakely, gcc Patches, libstdc++, Thomas Rodgers

Am Di., 2. Nov. 2021 um 02:26 Uhr schrieb Thomas Rodgers via Libstdc++
<libstdc++@gcc.gnu.org>:
>
> This should address Jonathan's feedback and adds support for atomic_ref<T>
>

I'm wondering why __clear_padding doesn't refer to the computed __ptr
value in the case where __builtin_clear_padding is used?

Thanks,

- Daniel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
  2021-11-02  7:49       ` Jakub Jelinek
@ 2021-11-03  3:06         ` Thomas Rodgers
  0 siblings, 0 replies; 23+ messages in thread
From: Thomas Rodgers @ 2021-11-03  3:06 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Jonathan Wakely, Thomas Rodgers, gcc Patches, libstdc++

This version of the patch specifically doesn’t deal with long double.
Mostly looking for Jonathan’s review to ensure his previous feedback is
addressed. I plan to rev the patch to handle long doubles after some
further discussion with you and Jonathan.

On Tue, Nov 2, 2021 at 12:49 AM Jakub Jelinek <jakub@redhat.com> wrote:

> On Mon, Nov 01, 2021 at 06:25:45PM -0700, Thomas Rodgers via Gcc-patches
> wrote:
> > +    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
>
> I'm not sure I understand the && !is_floating_point<_Tp>::value.
> Yes, float and double will never have padding, but long double often
> will, e.g. on x86 or ia64 (but e.g. not on ppc, s390x, etc.).
> So, unless we want to play with numeric_limits, it should be either
> just return !__has_unique_object_representations(_Tp);
> or return !__has_unique_object_representations(_Tp)
>           && (!is_floating_point<_Tp>::value
>               || is_same<__remove_cv_t<_Tp>,long double>::value);
> or, with numeric_limits test numeric_limits<_Tp>::digits == 64
> (but I'm sure Jonathan will not want including such a header dependency
> unless it is already included).
> Or I can always provide a new __builtin_clear_padding_p ...
>
>         Jakub
>
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
  2021-11-02  1:25     ` Thomas Rodgers
  2021-11-02  7:49       ` Jakub Jelinek
  2021-11-02  8:49       ` Daniel Krügler
@ 2022-01-18 21:48       ` Jonathan Wakely
  2022-08-25 10:11         ` Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange) Jakub Jelinek
  2 siblings, 1 reply; 23+ messages in thread
From: Jonathan Wakely @ 2022-01-18 21:48 UTC (permalink / raw)
  To: Thomas Rodgers; +Cc: Thomas Rodgers, gcc Patches, libstdc++, Thomas Rodgers

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.

>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:

Typo

>
> * include/std/atomic (atomic<T>::atomic(_Tp): clear padding for

Unclosed paren.




>+#if __has_builtin(__builtin_clear_padding)

Instead of checking this built-in at every call site, can't we just make
__maybe_has_padding return false if the built-in isn't supported?
__clear_padding already handles the case where the built-in isn't supported.

>+    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
>+      }

So make that:

    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
     }


>+ if _GLIBCXX14_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Tp>())
>+  {

This needs to be _GLIBCXX17_CONSTEXPR (everywhere that `if constexpr` is
used).

>+  {
>+    alignas(_Tp) unsigned char __buf[sizeof(_Tp)];
>+    __builtin_memcpy(__buf, std::__addressof(__e), sizeof(_Tp));

alignas(_Tp) unsigned char __buf[sizeof(_Tp)];
_Tp* __exp = ::new((void*)__buf) _Tp(__e);

>+    auto* __exp =
__atomic_impl::__clear_padding(*reinterpret_cast<_Tp*>(__buf));

And then you don't need the reinterpret_cast:

__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;


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

All calls to __clear_padding need to be qualified.

>+ return __compare_exchange(*__ptr, __expected, __desired, true,
>+  __success, __failure);

So do calls to __compare_exchange.


>
>       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.


>--- 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
>+      }
>

Is this an incremental patch relative to the first one?
The changes to this file look correct.

>--- /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".

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)
  2022-01-18 21:48       ` Jonathan Wakely
@ 2022-08-25 10:11         ` Jakub Jelinek
  2022-09-01 22:57           ` Thomas Rodgers
  0 siblings, 1 reply; 23+ messages in thread
From: Jakub Jelinek @ 2022-08-25 10:11 UTC (permalink / raw)
  To: Thomas Rodgers, Jonathan Wakely; +Cc: gcc-patches, libstdc++, Thomas Rodgers

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)
  2022-08-25 10:11         ` Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange) Jakub Jelinek
@ 2022-09-01 22:57           ` Thomas Rodgers
  2022-09-07 11:56             ` Jonathan Wakely
  0 siblings, 1 reply; 23+ messages in thread
From: Thomas Rodgers @ 2022-09-01 22:57 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Jonathan Wakely, gcc Patches, libstdc++, Thomas Rodgers

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

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
>
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)
  2022-09-01 22:57           ` Thomas Rodgers
@ 2022-09-07 11:56             ` Jonathan Wakely
  2022-09-07 22:06               ` Thomas Rodgers
  2022-09-09 18:36               ` Rainer Orth
  0 siblings, 2 replies; 23+ messages in thread
From: Jonathan Wakely @ 2022-09-07 11:56 UTC (permalink / raw)
  To: Thomas Rodgers; +Cc: Jakub Jelinek, gcc Patches, libstdc++, Thomas Rodgers

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

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.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 13342 bytes --]

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;
+}

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)
  2022-09-07 11:56             ` Jonathan Wakely
@ 2022-09-07 22:06               ` Thomas Rodgers
  2022-09-09 18:36               ` Rainer Orth
  1 sibling, 0 replies; 23+ messages in thread
From: Thomas Rodgers @ 2022-09-07 22:06 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: Jakub Jelinek, gcc Patches, libstdc++, Thomas Rodgers

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

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.
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)
  2022-09-07 11:56             ` Jonathan Wakely
  2022-09-07 22:06               ` Thomas Rodgers
@ 2022-09-09 18:36               ` Rainer Orth
  2022-09-09 18:46                 ` Iain Sandoe
  1 sibling, 1 reply; 23+ messages in thread
From: Rainer Orth @ 2022-09-09 18:36 UTC (permalink / raw)
  To: Jonathan Wakely via Gcc-patches
  Cc: Thomas Rodgers, Jonathan Wakely, Jakub Jelinek, libstdc++,
	Thomas Rodgers

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

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)
  2022-09-09 18:36               ` Rainer Orth
@ 2022-09-09 18:46                 ` Iain Sandoe
  2022-09-09 19:01                   ` Thomas Rodgers
  0 siblings, 1 reply; 23+ messages in thread
From: Iain Sandoe @ 2022-09-09 18:46 UTC (permalink / raw)
  To: Rainer Orth, Jonathan Wakely
  Cc: GCC Patches, Jakub Jelinek, libstdc++, Thomas Rodgers



> 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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)
  2022-09-09 18:46                 ` Iain Sandoe
@ 2022-09-09 19:01                   ` Thomas Rodgers
  2022-09-09 20:14                     ` Jonathan Wakely
  0 siblings, 1 reply; 23+ messages in thread
From: Thomas Rodgers @ 2022-09-09 19:01 UTC (permalink / raw)
  To: Iain Sandoe
  Cc: Rainer Orth, Jonathan Wakely, Jakub Jelinek, libstdc++,
	GCC Patches, Thomas Rodgers

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

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
>
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)
  2022-09-09 19:01                   ` Thomas Rodgers
@ 2022-09-09 20:14                     ` Jonathan Wakely
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Wakely @ 2022-09-09 20:14 UTC (permalink / raw)
  To: Thomas Rodgers
  Cc: Iain Sandoe, Rainer Orth, Jakub Jelinek, libstdc++,
	GCC Patches, Thomas Rodgers

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

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
>>

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 2311 bytes --]

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2022-09-09 20:14 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-23 18:08 [PATCH] libstdc++: Clear padding bits in atomic compare_exchange Thomas Rodgers
2021-09-23 19:07 ` Jakub Jelinek
2021-09-23 20:15   ` Thomas Rodgers
2021-09-23 20:15   ` Jonathan Wakely
2021-09-27 14:10 ` Thomas Rodgers
2021-09-29 12:13   ` Jonathan Wakely
2021-09-29 12:18     ` Jonathan Wakely
2021-09-29 12:28     ` Jakub Jelinek
2021-09-29 18:22     ` Thomas Rodgers
2021-09-29 18:29       ` Jakub Jelinek
2021-11-02  1:25     ` Thomas Rodgers
2021-11-02  7:49       ` Jakub Jelinek
2021-11-03  3:06         ` Thomas Rodgers
2021-11-02  8:49       ` Daniel Krügler
2022-01-18 21:48       ` Jonathan Wakely
2022-08-25 10:11         ` Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange) Jakub Jelinek
2022-09-01 22:57           ` Thomas Rodgers
2022-09-07 11:56             ` Jonathan Wakely
2022-09-07 22:06               ` Thomas Rodgers
2022-09-09 18:36               ` Rainer Orth
2022-09-09 18:46                 ` Iain Sandoe
2022-09-09 19:01                   ` Thomas Rodgers
2022-09-09 20:14                     ` Jonathan Wakely

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).