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

2024-03-25 Thread xndcn
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.
>


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

2024-01-30 Thread xndcn
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.
> > >  * 

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

2024-01-14 Thread xndcn
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
>