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

[Jonathan Wakely <jwakely@redhat.com>]

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