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

[xndcn]

Thanks, so I add a test: atomic_float/compare_exchange_padding.cc, which
will fail due to timeout without the patch.

---
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-*-*])
        } {
  global TOOL_OPTIONS

-- 
2.25.1

H.J. Lu <hjl.to...@gmail.com> 于2024年1月15日周一 11:46写道：

> On Sun, Jan 7, 2024, 5:02 PM xndcn <xnd...@gmail.com> wrote:
>
>> Hi, I found __atomic_float constructor does not clear padding,
>> while __compare_exchange assumes it as zeroed padding. So it is easy to
>> reproducing a infinite loop in X86-64 with long double type like:
>> ---
>> -O0 -std=c++23 -mlong-double-80
>> #include <stdio.h>
>> #include <atomic>
>>
>> #define T long double
>> int main() {
>>     std::atomic<T> t(0.5);
>>     t.fetch_add(0.5);
>>     float x = t;
>>     printf("%f\n", x);
>> }
>> ---
>>
>> So we should add __builtin_clear_padding in __atomic_float constructor,
>> just like the generic atomic struct.
>>
>> regtested on x86_64-linux. Is it OK for trunk?
>>
>> ---
>> libstdc++: atomic: Add missing clear_padding in __atomic_float
>> constructor.
>>
>> libstdc++-v3/ChangeLog:
>>
>> * include/bits/atomic_base.h: add __builtin_clear_padding in
>> __atomic_float constructor.
>> ---
>>  libstdc++-v3/include/bits/atomic_base.h | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/libstdc++-v3/include/bits/atomic_base.h
>> b/libstdc++-v3/include/bits/atomic_base.h
>> index f4ce0fa53..d59c2209e 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 _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;
>> --
>> 2.25.1
>>
>
> Can you add a testcase?
>
> Thanks.
>
> H.J.
>
>>