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

[Jakub Jelinek <jakub@redhat.com>]

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