Re: [PING][PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
Wow, thank you all, you guys! 在 2024年3月14日星期四，Jonathan Wakely 写道： > 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 +, 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. >
Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
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 +, 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.
commit 0adc8c5f146b108f99c4df09e43276e3a2419262
Author: xndcn
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
Reviewed-by: Jonathan Wakely
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 000..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
+#include
+#include
+#include
+
+template
+void __attribute__((noinline,noipa))
+fill_padding(T& f)
+{
+ T mask;
+ std::memset(, 0xff, sizeof(T));
+ __builtin_clear_padding();
+ unsigned char* ptr_f = (unsigned char*)
+ unsigned char* ptr_mask = (unsigned char*)
+ 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::digits == 64)
+ {
+long double f = 0.5f; // long double has padding bits on x86
+fill_padding(f);
+std::atomic 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();
+}
Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
On Fri, 16 Feb 2024 at 14:10, Jakub Jelinek wrote: > > On Fri, Feb 16, 2024 at 01:51:54PM +, 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.
Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
On Fri, Feb 16, 2024 at 01:51:54PM +, 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? Jakub
Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
On Fri, 16 Feb 2024 at 12:38, Jonathan Wakely wrote: > > On Fri, 2 Feb 2024 at 16:52, xndcn wrote: > > > > Thank you for your careful review! > > > > > But we don't need a new one if it's going to be used in exactly one test > > > and if the new option does the same thing for all targets that run the > > > test. > > Got it, thanks. Now add option "-latomic" directly, but it still rely > > on the trick "[atomic_link_flags [get_multilibs]]" > > > > > No, because the patch is supposed to prevent the infinite loop, and so > > > there's no need to stop it looping after 10s. It won't loop at all. > > Thanks, deleted. > > > > > We only need to clear padding for long double, not float and double, > > > right? > > Yes, actually there is a check "if constexpr > > (__atomic_impl::__maybe_has_padding<_Fp>())". > > But "__atomic_impl::__clear_padding(_M_fp); " is indeed simply, so fixed > > here. > > > > > Why can't we run this on all targets? > > Got it, now target option deleted. > > > > > There's no reason to use __builtin_memset here, just include > > > and use std::memcpy. > > Thanks, fixed. > > > > > It definitely does have padding, just say "long double has padding bits > > > on x86" > > Thanks, fixed. > > > > So here comes the latest patch: > > > Thanks. I've applied the patch to my tree, but the new test fails > pretty reliably. > > The infinite loop in std::atomic::fetch_add is fixed by > clearing padding in the constructor, but the test fails on the > compare_exchange_weak or compare_exchange_strong lines here: > > > > +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 > > > > > I think the problem is here in __atomic_impl::__compare_exchange: > >if (__atomic_compare_exchange(__pval, __pexp, __pi, > __is_weak, int(__s), int(__f))) > return true; > > Even though padding in *__pexp and *__pi has been cleared, the value > of *__pval after a successful __atomic_compare_exchange has non-zero > padding. That means that the next compare_exchange will fail, because > we assume that the stored value always has zeroed padding bits. > > Here's a gdb session showing that __atomic_compare_exchange stores a > value with non-zero padding: > > Breakpoint 2, test01 () at compare_exchange_padding.cc:43 > 43long double s2 = s; > (gdb) n > 44fill_padding(s2); > (gdb) > 45while (!as.compare_exchange_weak(s2, f)) // padding cleared > on compexchg > (gdb) p/x as._M_fp > $11 = 0x40008000 > (gdb) step > std::__atomic_float::compare_exchange_weak > (this=0x7fffd8c0, __expected=@0x7fffd8a0: 2, __desired=0.5, > __order=std::memory_order::seq_cst) at > /home/jwakely/gcc/14/include/c++/14.0.1/bits/atomic_base.h:1387 > 1387return compare_exchange_weak(__expected, __desired, __order, > (gdb) step > std::__atomic_float::compare_exchange_weak > (this=0x7fffd8c0, __expected=@0x7fffd8a0: 2, __desired=0.5, > __success=std::memory_order::seq_cst, > __failure=std::memory_order::seq_cst) at > /home/jwakely/gcc/14/include/c++/14.0.1/bits/atomic_base.h:1347 > 1347return __atomic_impl::compare_exchange_weak(&_M_fp, > (gdb) step > std::__atomic_impl::compare_exchange_weak > (__check_padding=false, __failure=std::memory_order::seq_cst, > __success=std::memory_order::seq_cst, __desired=0.5, > __expected=@0x7fffd8a0: 2, __ptr=0x7fffd8c0) > at /home/jwakely/gcc/14/include/c++/14.0.1/bits/atomic_base.h:1123 > 1123return __atomic_impl::__compare_exchange<_AtomicRef>( > (gdb) > std::__atomic_impl::__compare_exchange > (__f=std::memory_order::seq_cst, __s=std::memory_order::seq_cst, > __is_weak=true, > __i=, __e=@0x7fffd8a0: 2, > __val=@0x7fffd8c0: 2) at > /home/jwakely/gcc/14/include/c++/14.0.1/bits/atomic_base.h:994 > 994 __glibcxx_assert(__is_valid_cmpexch_failure_order(__f)); > (gdb) n > 997 _Tp* const __pval = std::__addressof(__val); > (gdb) > 1008_Vp* const __pi = __atomic_impl::__clear_padding(__i); > (gdb) > 1010_Vp __exp = __e; > (gdb) > 1012_Vp* const __pexp = __atomic_impl::__clear_padding(__exp); > (gdb) > 1016if (__atomic_compare_exchange(__pval, __pexp, __pi, > (gdb) p/x *__pval > $12 = 0x40008000 > (gdb) p/x *__pexp > $13 = 0x40008000 > (gdb) p/x *__pi > $14 = 0x3ffe8000 > (gdb) n > 1018 return true; > (gdb) p/x *__pval > $15 = 0x77bf3ffe8000 > (gdb) > > We stored *__pi which has zero padding, but the result in *__pval has > non-zero padding. This doesn't seem to be gdb being misleading by > loading
Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
On Fri, 2 Feb 2024 at 16:52, xndcn wrote: > > Thank you for your careful review! > > > But we don't need a new one if it's going to be used in exactly one test > > and if the new option does the same thing for all targets that run the test. > Got it, thanks. Now add option "-latomic" directly, but it still rely > on the trick "[atomic_link_flags [get_multilibs]]" > > > No, because the patch is supposed to prevent the infinite loop, and so > > there's no need to stop it looping after 10s. It won't loop at all. > Thanks, deleted. > > > We only need to clear padding for long double, not float and double, right? > Yes, actually there is a check "if constexpr > (__atomic_impl::__maybe_has_padding<_Fp>())". > But "__atomic_impl::__clear_padding(_M_fp); " is indeed simply, so fixed here. > > > Why can't we run this on all targets? > Got it, now target option deleted. > > > There's no reason to use __builtin_memset here, just include and > > use std::memcpy. > Thanks, fixed. > > > It definitely does have padding, just say "long double has padding bits on > > x86" > Thanks, fixed. > > So here comes the latest patch: Thanks. I've applied the patch to my tree, but the new test fails pretty reliably. The infinite loop in std::atomic::fetch_add is fixed by clearing padding in the constructor, but the test fails on the compare_exchange_weak or compare_exchange_strong lines here: > +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 > I think the problem is here in __atomic_impl::__compare_exchange: if (__atomic_compare_exchange(__pval, __pexp, __pi, __is_weak, int(__s), int(__f))) return true; Even though padding in *__pexp and *__pi has been cleared, the value of *__pval after a successful __atomic_compare_exchange has non-zero padding. That means that the next compare_exchange will fail, because we assume that the stored value always has zeroed padding bits. Here's a gdb session showing that __atomic_compare_exchange stores a value with non-zero padding: Breakpoint 2, test01 () at compare_exchange_padding.cc:43 43long double s2 = s; (gdb) n 44fill_padding(s2); (gdb) 45while (!as.compare_exchange_weak(s2, f)) // padding cleared on compexchg (gdb) p/x as._M_fp $11 = 0x40008000 (gdb) step std::__atomic_float::compare_exchange_weak (this=0x7fffd8c0, __expected=@0x7fffd8a0: 2, __desired=0.5, __order=std::memory_order::seq_cst) at /home/jwakely/gcc/14/include/c++/14.0.1/bits/atomic_base.h:1387 1387return compare_exchange_weak(__expected, __desired, __order, (gdb) step std::__atomic_float::compare_exchange_weak (this=0x7fffd8c0, __expected=@0x7fffd8a0: 2, __desired=0.5, __success=std::memory_order::seq_cst, __failure=std::memory_order::seq_cst) at /home/jwakely/gcc/14/include/c++/14.0.1/bits/atomic_base.h:1347 1347return __atomic_impl::compare_exchange_weak(&_M_fp, (gdb) step std::__atomic_impl::compare_exchange_weak (__check_padding=false, __failure=std::memory_order::seq_cst, __success=std::memory_order::seq_cst, __desired=0.5, __expected=@0x7fffd8a0: 2, __ptr=0x7fffd8c0) at /home/jwakely/gcc/14/include/c++/14.0.1/bits/atomic_base.h:1123 1123return __atomic_impl::__compare_exchange<_AtomicRef>( (gdb) std::__atomic_impl::__compare_exchange (__f=std::memory_order::seq_cst, __s=std::memory_order::seq_cst, __is_weak=true, __i=, __e=@0x7fffd8a0: 2, __val=@0x7fffd8c0: 2) at /home/jwakely/gcc/14/include/c++/14.0.1/bits/atomic_base.h:994 994 __glibcxx_assert(__is_valid_cmpexch_failure_order(__f)); (gdb) n 997 _Tp* const __pval = std::__addressof(__val); (gdb) 1008_Vp* const __pi = __atomic_impl::__clear_padding(__i); (gdb) 1010_Vp __exp = __e; (gdb) 1012_Vp* const __pexp = __atomic_impl::__clear_padding(__exp); (gdb) 1016if (__atomic_compare_exchange(__pval, __pexp, __pi, (gdb) p/x *__pval $12 = 0x40008000 (gdb) p/x *__pexp $13 = 0x40008000 (gdb) p/x *__pi $14 = 0x3ffe8000 (gdb) n 1018 return true; (gdb) p/x *__pval $15 = 0x77bf3ffe8000 (gdb) We stored *__pi which has zero padding, but the result in *__pval has non-zero padding. This doesn't seem to be gdb being misleading by loading *__pval into a FP register which doesn't preserve the zero padding, because if I do this then it fails: as.fetch_add(t); VERIFY(as.load() == s); __builtin_clear_padding(); VERIFY( std::memcmp(, , sizeof(s)) == 0 ); So the value stored by fetch_add (which uses compare_exchange_weak in a loop) really doesn't have
Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
Thank you for your careful review!
> But we don't need a new one if it's going to be used in exactly one test and
> if the new option does the same thing for all targets that run the test.
Got it, thanks. Now add option "-latomic" directly, but it still rely
on the trick "[atomic_link_flags [get_multilibs]]"
> No, because the patch is supposed to prevent the infinite loop, and so
> there's no need to stop it looping after 10s. It won't loop at all.
Thanks, deleted.
> We only need to clear padding for long double, not float and double, right?
Yes, actually there is a check "if constexpr
(__atomic_impl::__maybe_has_padding<_Fp>())".
But "__atomic_impl::__clear_padding(_M_fp); " is indeed simply, so fixed here.
> Why can't we run this on all targets?
Got it, now target option deleted.
> There's no reason to use __builtin_memset here, just include and
> use std::memcpy.
Thanks, fixed.
> It definitely does have padding, just say "long double has padding bits on
> x86"
Thanks, fixed.
So here comes the latest patch:
---
libstdc++-v3/ChangeLog:
* include/bits/atomic_base.h: add __builtin_clear_padding in
__atomic_float constructor.
* testsuite/29_atomics/atomic_float/compare_exchange_padding.cc: New test.
Signed-off-by: xndcn
---
libstdc++-v3/include/bits/atomic_base.h | 2 +-
.../atomic_float/compare_exchange_padding.cc | 53 +++
2 files changed, 54 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..cdd6f07da 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 0..eeace251c
--- /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
+#include
+#include
+#include
+
+template
+void __attribute__((noinline,noipa))
+fill_padding(T& f)
+{
+ T mask;
+ std::memset(, 0xff, sizeof(T));
+ __builtin_clear_padding();
+ unsigned char* ptr_f = (unsigned char*)
+ unsigned char* ptr_mask = (unsigned char*)
+ 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::digits == 64)
+ {
+long double f = 0.5f; // long double has padding bits on x86
+fill_padding(f);
+std::atomic 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();
+}
--
2.25.1
Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
On Wed, 31 Jan 2024 at 17:20, xndcn wrote:
>
> 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.
Right, we don't want to change the existing libatomic option. But we
don't need a new one if it's going to be used in exactly one test and
if the new option does the same thing for all targets that run the
test.
However, on that subject, does the test really need to be restricted
to x86? It shouldn't loop on any target, right?
>
> > 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?
No, because the patch is supposed to prevent the infinite loop, and so
there's no need to stop it looping after 10s. It won't loop at all.
>
> > Also, the target selectors above look wrong.
> Thanks, fixed with checking "std::numeric_limits::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".
Actually, why are you using the built-in directly in the first place? See below.
>
> 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
> ---
> 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
> + }
Why can't this be simply:
{ __atomic_impl::__clear_padding(_M_fp); }
?
We only need to clear padding for long double, not float and double, right?
>
>__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 0..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-*-* } } }
Why can't we run this on all targets?
> +// { dg-add-options libatomic_16b }
Let's just add -latomic to the dg-options line for this one test. We
don't want that in general because we want to know that atomics work
without it in most cases. It should be fine to use it for one test.
> +
> +#include
> +#include
> +#include
> +
> +template
> +void __attribute__((noinline,noipa))
> +fill_padding(T& f)
> +{
> + T mask;
> + __builtin_memset(, 0xff, sizeof(T));
There's no reason to use __builtin_memset here, just include
and use std::memcpy.
> + __builtin_clear_padding();
> + unsigned char* ptr_f = (unsigned char*)
> + unsigned char* ptr_mask = (unsigned char*)
> + 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::digits == 64)
> + {
> +long double f = 0.5f; // long double may contains padding on X86
It definitely does have padding, just say "long double has padding bits on x86"
> +fill_padding(f);
> +std::atomic 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
> +
Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
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::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
---
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 0..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
+#include
+#include
+
+template
+void __attribute__((noinline,noipa))
+fill_padding(T& f)
+{
+ T mask;
+ __builtin_memset(, 0xff, sizeof(T));
+ __builtin_clear_padding();
+ unsigned char* ptr_f = (unsigned char*)
+ unsigned char* ptr_mask = (unsigned char*)
+ 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::digits == 64)
+ {
+long double f = 0.5f; // long double may contains padding on X86
+fill_padding(f);
+std::atomic 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
Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
On Tue, Jan 30, 2024 at 04:34:30PM +, Jonathan Wakely wrote:
> > --- /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 }
Why dg-timeout?
I don't see it used in any of the libstdc++ tests and I don't see anything
expensive on this test. Just leave it out.
> > +// { dg-additional-options "-mlong-double-80" { target x86_64-*-* } }
> > +// { dg-do run { target { ia32 || x86_64-*-* } } }
Also, the target selectors above look wrong.
Generally, one should use { i?86-*-* x86_64-*-* } because e.g. on some
OSes like Solaris, one builds i?86 compiler which supports -m32 or -m64
options.
I wouldn't add the -mlong-double-80 at all, it is an ABI changing option,
if you rely in the test on some 80-bit long double properties (but on which
ones? Sure, if -mlong-double-64 or -mlong-double-128 is in effect, the
type won't have any padding, but __builtin_clear_padding should reflect
that), it is better to guard the part of the test with some checks, whether
std::numeric_limits::digits == 64, or
preprocessor __LDBL_MANT_DIG__ == 64.
> > +// { dg-add-options libatomic_16b }
>
> Why not just use -latomic unconditionally here?
>
> You're adding a new libatomic_16b option that expands to -latomic for
> x86 and nothing otherwise, and then using it in a test that only runs
> for x86. So it's just adding -latomic to all targets that run the
> test, right?
>
> Also, this patch needs either a copyright assignment or a DCO sign-off:
> https://gcc.gnu.org/contribute.html#legal
And if somebody would need to commit it for you, also patch in non-garbled
state (e.g. sent as an attachment, or even compressed attachment if using
really bad MUAs).
Jakub
Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
On Tue, 16 Jan 2024 at 16:17, xndcn wrote:
>
> 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 0..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 }
Why not just use -latomic unconditionally here?
You're adding a new libatomic_16b option that expands to -latomic for
x86 and nothing otherwise, and then using it in a test that only runs
for x86. So it's just adding -latomic to all targets that run the
test, right?
Also, this patch needs either a copyright assignment or a DCO sign-off:
https://gcc.gnu.org/contribute.html#legal
> +
> +#include
> +#include
> +
> +template
> +void __attribute__((noinline,noipa))
> +fill_padding(T& f)
> +{
> + T mask;
> + __builtin_memset(, 0xff, sizeof(T));
> + __builtin_clear_padding();
> + unsigned char* ptr_f = (unsigned char*)
> + unsigned char* ptr_mask = (unsigned char*)
> + 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 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 于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
Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
On Tue, 16 Jan 2024 at 16:17, xndcn wrote:
>
> 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>())
This code is not compiled for C++11 and C++14, so this should just use
constexpr not _GLIBCXX17_CONSTEXPR.
Apart from that I think this patch looks OK.
> + __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 0..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
> +#include
> +
> +template
> +void __attribute__((noinline,noipa))
> +fill_padding(T& f)
> +{
> + T mask;
> + __builtin_memset(, 0xff, sizeof(T));
> + __builtin_clear_padding();
> + unsigned char* ptr_f = (unsigned char*)
> + unsigned char* ptr_mask = (unsigned char*)
> + 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 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 于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 libstd...@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.
> > > *
[PING][PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
Ping, thanks.
I do not have access to the repo, anyone can please help me submit the
patch? Thanks.
xndcn 于2024年1月17日周三 00:16写道：
>
> 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 0..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
> +#include
> +
> +template
> +void __attribute__((noinline,noipa))
> +fill_padding(T& f)
> +{
> + T mask;
> + __builtin_memset(, 0xff, sizeof(T));
> + __builtin_clear_padding();
> + unsigned char* ptr_f = (unsigned char*)
> + unsigned char* ptr_mask = (unsigned char*)
> + 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 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 于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 libstd...@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.
> > > *
Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
Hi, is it OK for trunk? I do not have access to the repo, can you
please help me submit the patch? Thanks.
xndcn 于2024年1月17日周三 00:16写道：
>
> 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 0..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
> +#include
> +
> +template
> +void __attribute__((noinline,noipa))
> +fill_padding(T& f)
> +{
> + T mask;
> + __builtin_memset(, 0xff, sizeof(T));
> + __builtin_clear_padding();
> + unsigned char* ptr_f = (unsigned char*)
> + unsigned char* ptr_mask = (unsigned char*)
> + 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 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 于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 libstd...@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.
>
Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
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 0..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
+#include
+
+template
+void __attribute__((noinline,noipa))
+fill_padding(T& f)
+{
+ T mask;
+ __builtin_memset(, 0xff, sizeof(T));
+ __builtin_clear_padding();
+ unsigned char* ptr_f = (unsigned char*)
+ unsigned char* ptr_mask = (unsigned char*)
+ 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 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 于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 libstd...@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
Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
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 libstd...@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 000..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
> +#include
> +
> +template
> +void __attribute__((noinline,noipa))
> +fill_padding(T& f)
> +{
> + T mask;
> + __builtin_memset(, 0xff, sizeof(T));
> + __builtin_clear_padding();
> + unsigned char* ptr_f = (unsigned char*)
> + unsigned char* ptr_mask = (unsigned char*)
> + 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 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
School of Aerospace Science and Technology, Xidian University
Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
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 000..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
+#include
+
+template
+void __attribute__((noinline,noipa))
+fill_padding(T& f)
+{
+ T mask;
+ __builtin_memset(, 0xff, sizeof(T));
+ __builtin_clear_padding();
+ unsigned char* ptr_f = (unsigned char*)
+ unsigned char* ptr_mask = (unsigned char*)
+ 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 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 于2024年1月15日周一 11:46写道：
> On Sun, Jan 7, 2024, 5:02 PM xndcn 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
>> #include
>>
>> #define T long double
>> int main() {
>> std::atomic 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));
>>
Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
On Sun, Jan 7, 2024, 5:02 PM xndcn 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
> #include
>
> #define T long double
> int main() {
> std::atomic 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.
>
[PING][PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
Ping. Thanks.
xndcn 于2024年1月8日周一 09:01写道：
> 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
> #include
>
> #define T long double
> int main() {
> std::atomic 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
>
[PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor
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
#include
#define T long double
int main() {
std::atomic 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
