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

[Jonathan Wakely <jwakely@redhat.com>]

On Fri, 2 Feb 2024 at 16:52, xndcn <xndchn@gmail.com> 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 <cstring> 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<long double>::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
43        long double s2 = s;
(gdb) n
44        fill_padding(s2);
(gdb)
45        while (!as.compare_exchange_weak(s2, f)) // padding cleared
on compexchg
(gdb) p/x as._M_fp
$11 = 0x40008000000000000000
(gdb) step
std::__atomic_float<long double>::compare_exchange_weak
(this=0x7fffffffd8c0, __expected=@0x7fffffffd8a0: 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
1387            return compare_exchange_weak(__expected, __desired, __order,
(gdb) step
std::__atomic_float<long double>::compare_exchange_weak
(this=0x7fffffffd8c0, __expected=@0x7fffffffd8a0: 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
1347            return __atomic_impl::compare_exchange_weak(&_M_fp,
(gdb) step
std::__atomic_impl::compare_exchange_weak<false, long double>
(__check_padding=false, __failure=std::memory_order::seq_cst,
    __success=std::memory_order::seq_cst, __desired=0.5,
__expected=@0x7fffffffd8a0: 2, __ptr=0x7fffffffd8c0)
    at /home/jwakely/gcc/14/include/c++/14.0.1/bits/atomic_base.h:1123
1123            return __atomic_impl::__compare_exchange<_AtomicRef>(
(gdb)
std::__atomic_impl::__compare_exchange<false, long double>
(__f=std::memory_order::seq_cst, __s=std::memory_order::seq_cst,
__is_weak=true,
    __i=<optimized out>, __e=@0x7fffffffd8a0: 2,
__val=@0x7fffffffd8c0: 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)
1016                if (__atomic_compare_exchange(__pval, __pexp, __pi,
(gdb) p/x *__pval
$12 = 0x40008000000000000000
(gdb) p/x *__pexp
$13 = 0x40008000000000000000
(gdb) p/x *__pi
$14 = 0x3ffe8000000000000000
(gdb) n
1018                  return true;
(gdb) p/x *__pval
$15 = 0x7ffff7bf3ffe8000000000000000
(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(&s);
  VERIFY( std::memcmp(&s, &as, sizeof(s)) == 0 );

So the value stored by fetch_add (which uses compare_exchange_weak in
a loop) really doesn't have zero padding bits.

I'm not sure what's going on here yet, maybe __atomic_compare_exchange
recognizes that the pointers are long double* and so only copies the
non-padding bits into the value?

I think I'll push the fix to the __atomic_float constructor, but with
a simplified test case that doesn't include the
compare_exchange_{weak,strong} calls, until I figure out how to fix
it.