Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor

[xndcn <xndchn@gmail.com>]

Thanks!

> Why not just use -latomic unconditionally here?
testsuites of libstdc++ seems to have some tricks to find and link
libatomic.a by using "dg-add-options libatomic", which do nothing for
x86 target. In previous email, we decide not to pollute the current
option, so we add a new libatomic_16b option here.

> Why dg-timeout?
without the patch, "as.fetch_add(t);" will result in an infinite loop,
so I add timeout to help it escape. Should I keep it or not?

> Also, the target selectors above look wrong.
Thanks, fixed with checking "std::numeric_limits<long double>::digits == 64".

> This code is not compiled for C++11 and C++14, so this should just use
> constexpr not _GLIBCXX17_CONSTEXPR.
Thanks, fixed with "constexpr".

So here is the fixed patch, please review it again, thanks:
---
libstdc++-v3/ChangeLog:

 * include/bits/atomic_base.h: add __builtin_clear_padding in
__atomic_float constructor.
 * testsuite/lib/dg-options.exp: add new add-options for libatomic_16b.
 * testsuite/29_atomics/atomic_float/compare_exchange_padding.cc: New test.

Signed-off-by: xndcn <xndchn@gmail.com>
---
 libstdc++-v3/include/bits/atomic_base.h       |  7 ++-
 .../atomic_float/compare_exchange_padding.cc  | 54 +++++++++++++++++++
 libstdc++-v3/testsuite/lib/dg-options.exp     | 22 ++++++++
 3 files changed, 82 insertions(+), 1 deletion(-)
 create mode 100644
libstdc++-v3/testsuite/29_atomics/atomic_float/compare_exchange_padding.cc

diff --git a/libstdc++-v3/include/bits/atomic_base.h
b/libstdc++-v3/include/bits/atomic_base.h
index f4ce0fa53..90e3f0e4b 100644
--- a/libstdc++-v3/include/bits/atomic_base.h
+++ b/libstdc++-v3/include/bits/atomic_base.h
@@ -1283,7 +1283,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION

       constexpr
       __atomic_float(_Fp __t) : _M_fp(__t)
-      { }
+      {
+#if __has_builtin(__builtin_clear_padding)
+ if constexpr (__atomic_impl::__maybe_has_padding<_Fp>())
+   __builtin_clear_padding(std::__addressof(_M_fp));
+#endif
+      }

       __atomic_float(const __atomic_float&) = delete;
       __atomic_float& operator=(const __atomic_float&) = delete;
diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_float/compare_exchange_padding.cc
b/libstdc++-v3/testsuite/29_atomics/atomic_float/compare_exchange_padding.cc
new file mode 100644
index 000000000..e6e17e4b5
--- /dev/null
+++ b/libstdc++-v3/testsuite/29_atomics/atomic_float/compare_exchange_padding.cc
@@ -0,0 +1,54 @@
+// { dg-do run { target c++20 } }
+// { dg-options "-O0" }
+// { dg-timeout 10 }
+// { dg-do run { target { i?86-*-* x86_64-*-* } } }
+// { dg-add-options libatomic_16b }
+
+#include <atomic>
+#include <limits>
+#include <testsuite_hooks.h>
+
+template<typename T>
+void __attribute__((noinline,noipa))
+fill_padding(T& f)
+{
+  T mask;
+  __builtin_memset(&mask, 0xff, sizeof(T));
+  __builtin_clear_padding(&mask);
+  unsigned char* ptr_f = (unsigned char*)&f;
+  unsigned char* ptr_mask = (unsigned char*)&mask;
+  for (int i = 0; i < sizeof(T); i++)
+  {
+    if (ptr_mask[i] == 0x00)
+    {
+      ptr_f[i] = 0xff;
+    }
+  }
+}
+
+void
+test01()
+{
+  // test for long double with padding (float80)
+  if constexpr (std::numeric_limits<long double>::digits == 64)
+  {
+    long double f = 0.5f; // long double may contains padding on X86
+    fill_padding(f);
+    std::atomic<long double> as{ f }; // padding cleared on constructor
+    long double t = 1.5;
+
+    as.fetch_add(t);
+    long double s = f + t;
+    t = as.load();
+    VERIFY(s == t); // padding ignored on float comparing
+    fill_padding(s);
+    VERIFY(as.compare_exchange_weak(s, f)); // padding cleared on cmpexchg
+    fill_padding(f);
+    VERIFY(as.compare_exchange_strong(f, t)); // padding cleared on cmpexchg
+  }
+}
+
+int main()
+{
+  test01();
+}
diff --git a/libstdc++-v3/testsuite/lib/dg-options.exp
b/libstdc++-v3/testsuite/lib/dg-options.exp
index 850442b6b..75b27a136 100644
--- a/libstdc++-v3/testsuite/lib/dg-options.exp
+++ b/libstdc++-v3/testsuite/lib/dg-options.exp
@@ -356,6 +356,28 @@ proc add_options_for_libatomic { flags } {
     return $flags
 }

+# Add option to link to libatomic, if required for atomics on 16-byte (128-bit)
+proc add_options_for_libatomic_16b { flags } {
+    if { ([istarget i?86-*-*] || [istarget x86_64-*-*])
+       } {
+ global TOOL_OPTIONS
+
+ set link_flags ""
+ if ![is_remote host] {
+     if [info exists TOOL_OPTIONS] {
+ set link_flags "[atomic_link_flags [get_multilibs ${TOOL_OPTIONS}]]"
+     } else {
+ set link_flags "[atomic_link_flags [get_multilibs]]"
+     }
+ }
+
+ append link_flags " -latomic "
+
+ return "$flags $link_flags"
+    }
+    return $flags
+}
+
 # Add options to enable use of deprecated features.
 proc add_options_for_using-deprecated { flags } {
     return "$flags -U_GLIBCXX_USE_DEPRECATED -D_GLIBCXX_USE_DEPRECATED=1"
-- 
2.25.1