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

[Jonathan Wakely <jwakely@redhat.com>]

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

On Fri, 16 Feb 2024 at 15:15, Jonathan Wakely wrote:
>
> On Fri, 16 Feb 2024 at 14:10, Jakub Jelinek wrote:
> >
> > On Fri, Feb 16, 2024 at 01:51:54PM +0000, Jonathan Wakely wrote:
> > > Ah, although __atomic_compare_exchange only takes pointers, the
> > > compiler replaces that with a call to __atomic_compare_exchange_n
> > > which takes the newval by value, which presumably uses an 80-bit FP
> > > register and so the padding bits become indeterminate again.
> >
> > __atomic_compare_exchange_n only works with integers, so I guess
> > it is doing VIEW_CONVERT_EXPR (aka union-style type punning) on the
> > argument.
> >
> > Do you have preprocessed source for the testcase?
>
> Sent offlist.

Jakub fixed the compiler, so I've pushed the attached patch now.

Tested x86_64-linux.

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

commit 0adc8c5f146b108f99c4df09e43276e3a2419262
Author: xndcn <xndchn@gmail.com>
Date:   Fri Feb 16 11:00:13 2024

    libstdc++: Add missing clear_padding in __atomic_float constructor
    
    For 80-bit long double we need to clear the padding bits on
    construction.
    
    libstdc++-v3/ChangeLog:
    
            * include/bits/atomic_base.h (__atomic_float::__atomic_float(Fp)):
            Clear padding.
            * testsuite/29_atomics/atomic_float/compare_exchange_padding.cc:
            New test.
    
    Signed-off-by: xndcn <xndchn@gmail.com>
    
    Reviewed-by: Jonathan Wakely <jwakely@redhat.com>

diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/include/bits/atomic_base.h
index b857b441169..dd360302f80 100644
--- a/libstdc++-v3/include/bits/atomic_base.h
+++ b/libstdc++-v3/include/bits/atomic_base.h
@@ -1283,7 +1283,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       constexpr
       __atomic_float(_Fp __t) : _M_fp(__t)
-      { }
+      { __atomic_impl::__clear_padding(_M_fp); }
 
       __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 00000000000..49626ac6651
--- /dev/null
+++ b/libstdc++-v3/testsuite/29_atomics/atomic_float/compare_exchange_padding.cc
@@ -0,0 +1,53 @@
+// { dg-do run { target c++20 } }
+// { dg-options "-O0" }
+// { dg-additional-options "[atomic_link_flags [get_multilibs]] -latomic" }
+
+#include <atomic>
+#include <cstring>
+#include <limits>
+#include <testsuite_hooks.h>
+
+template<typename T>
+void __attribute__((noinline,noipa))
+fill_padding(T& f)
+{
+  T mask;
+  std::memset(&mask, 0xff, sizeof(T));
+  __builtin_clear_padding(&mask);
+  unsigned char* ptr_f = (unsigned char*)&f;
+  unsigned char* ptr_mask = (unsigned char*)&mask;
+  for (unsigned 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 has padding bits 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 comparison
+    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();
+}