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

[Jonathan Wakely <jwakely@redhat.com>]

On Wed, 31 Jan 2024 at 17:20, xndcn <xndchn@gmail.com> 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<long double>::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 <xndchn@gmail.com>
> ---
>  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 000000000..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 <atomic>
> +#include <limits>
> +#include <testsuite_hooks.h>
> +
> +template<typename T>
> +void __attribute__((noinline,noipa))
> +fill_padding(T& f)
> +{
> +  T mask;
> +  __builtin_memset(&mask, 0xff, sizeof(T));

There's no reason to use __builtin_memset here, just include <cstring>
and use std::memcpy.


> +  __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()
> +{
> +  // test for long double with padding (float80)
> +  if constexpr (std::numeric_limits<long double>::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<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..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
>