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

[xndcn <xndchn@gmail.com>]

Sorry about the mangled content...
So I add a new add-options for libatomic_16b:

---
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.
---
 libstdc++-v3/include/bits/atomic_base.h       |  7 ++-
 .../atomic_float/compare_exchange_padding.cc  | 50 +++++++++++++++++++
 libstdc++-v3/testsuite/lib/dg-options.exp     | 22 ++++++++
 3 files changed, 78 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..104ddfdbe 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 __cplusplus >= 201402L && __has_builtin(__builtin_clear_padding)
+ if _GLIBCXX17_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..d538b3d55
--- /dev/null
+++ b/libstdc++-v3/testsuite/29_atomics/atomic_float/compare_exchange_padding.cc
@@ -0,0 +1,50 @@
+// { dg-do run { target c++20 } }
+// { dg-options "-O0" }
+// { dg-timeout 10 }
+// { dg-additional-options "-mlong-double-80" { target x86_64-*-* } }
+// { dg-do run { target { ia32 || x86_64-*-* } } }
+// { dg-add-options libatomic_16b }
+
+#include <atomic>
+#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()
+{
+  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..25da20d58 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


Xi Ruoyao <xry111@xry111.site> 于2024年1月16日周二 18:12写道：
>
> On Tue, 2024-01-16 at 17:53 +0800, xndcn wrote:
> > Thanks, so I add a test: atomic_float/compare_exchange_padding.cc,
> > which will fail due to timeout without the patch.
>
> Please resend in plain text instead of HTML.  Sending in HTML causes the
> patch mangled.
>
> And libstdc++ patches should CC libstdc++@gcc.gnu.org, in addition to
> gcc-patches@gcc.gnu.org.
>
> > ---
> > libstdc++-v3/ChangeLog:
> >
> >  * include/bits/atomic_base.h: add __builtin_clear_padding in __atomic_float constructor.
> >  * testsuite/lib/dg-options.exp: enable libatomic for IA32 and X86-64.
> >  * testsuite/29_atomics/atomic_float/compare_exchange_padding.cc: New test.
> > ---
> >  libstdc++-v3/include/bits/atomic_base.h       |  7 ++-
> >  .../atomic_float/compare_exchange_padding.cc  | 50 +++++++++++++++++++
> >  libstdc++-v3/testsuite/lib/dg-options.exp     |  1 +
> >  3 files changed, 57 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 d3a2c4f3805..cbe3749e17f 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 __cplusplus >= 201402L && __has_builtin(__builtin_clear_padding)
> > + if _GLIBCXX17_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 00000000000..9376ab22850
> > --- /dev/null
> > +++ b/libstdc++-v3/testsuite/29_atomics/atomic_float/compare_exchange_padding.cc
> > @@ -0,0 +1,50 @@
> > +// { dg-do run { target c++20 } }
> > +// { dg-options "-O0" }
> > +// { dg-timeout 10 }
> > +// { dg-additional-options "-mlong-double-80" { target x86_64-*-* } }
> > +// { dg-do run { target { ia32 || x86_64-*-* } } }
> > +// { dg-add-options libatomic }
> > +
> > +#include <atomic>
> > +#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()
> > +{
> > +  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 bc387d17ed7..d9a19dadd7f 100644
> > --- a/libstdc++-v3/testsuite/lib/dg-options.exp
> > +++ b/libstdc++-v3/testsuite/lib/dg-options.exp
> > @@ -337,6 +337,7 @@ proc add_options_for_libatomic { flags } {
> >    || ([istarget powerpc*-*-*] && [check_effective_target_ilp32])
> >    || [istarget riscv*-*-*]
> >    || ([istarget sparc*-*-linux-gnu] && [check_effective_target_ilp32])
> > + || ([istarget i?86-*-*] || [istarget x86_64-*-*])
>
> This seems too overkill as "dg-add-options libatomic" is not intended to
> handle 16-byte atomics.  Maybe we can fork this to a new dg-add-options
> like "add_options_for_libatomic_16b"?
>
> >         } {
> >   global TOOL_OPTIONS
> >
> > --
> > 2.25.1
>
> --
> Xi Ruoyao <xry111@xry111.site>
> School of Aerospace Science and Technology, Xidian University