Re: [PATCH] libstdc++: Fix wrong thread waking on notify [PR100334]

[Thomas Rodgers <rodgert@appliantology.com>]

On 2021-05-17 09:43, Jonathan Wakely wrote:

> On 14/05/21 18:09 +0100, Jonathan Wakely wrote: On 13/05/21 18:54 
> -0700, Thomas Rodgers wrote: From: Thomas Rodgers 
> <rodgert@twrodgers.com>
> 
> Please ignore the previous patch. This one removes the need to carry 
> any
> extra state in the case of a 'laundered' atomic wait.
> 
> libstdc++/ChangeLog:
> * include/bits/atomic_wait.h (__waiter::_M_do_wait_v): loop
> until value change observed.
> (__waiter_base::_M_laundered): New member function.
> (__watier_base::_M_notify): Check _M_laundered() to determine
> whether to wake one or all.
> (__detail::__atomic_compare): Return true if call to
> __builtin_memcmp() == 0.
> (__waiter_base::_S_do_spin_v): Adjust predicate.
> * testsuite/29_atomics/atomic/wait_notify/100334.cc: New
> test.
> ---
> libstdc++-v3/include/bits/atomic_wait.h       | 28 ++++--
> .../29_atomics/atomic/wait_notify/100334.cc   | 94 +++++++++++++++++++
> 2 files changed, 114 insertions(+), 8 deletions(-)
> create mode 100644 
> libstdc++-v3/testsuite/29_atomics/atomic/wait_notify/100334.cc
> 
> diff --git a/libstdc++-v3/include/bits/atomic_wait.h 
> b/libstdc++-v3/include/bits/atomic_wait.h
> index 984ed70f16c..07bb744d822 100644
> --- a/libstdc++-v3/include/bits/atomic_wait.h
> +++ b/libstdc++-v3/include/bits/atomic_wait.h
> @@ -181,11 +181,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> return false;
> }
> 
> +    // return true if equal
> template<typename _Tp>
> bool __atomic_compare(const _Tp& __a, const _Tp& __b)
> {
> // TODO make this do the correct padding bit ignoring comparison
> -    return __builtin_memcmp(&__a, &__b, sizeof(_Tp)) != 0;
> +    return __builtin_memcmp(&__a, &__b, sizeof(_Tp)) == 0;
> }
> 
> struct __waiter_pool_base
> @@ -300,14 +301,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> explicit __waiter_base(const _Up* __addr) noexcept
> : _M_w(_S_for(__addr))
> , _M_addr(_S_wait_addr(__addr, &_M_w._M_ver))
> -      {
> -      }
> +      { }
> +
> +    bool
> +    _M_laundered() const
> +    { return _M_addr == &_M_w._M_ver; }
> 
> void
> _M_notify(bool __all, bool __bare = false)
> {
> -      if (_M_addr == &_M_w._M_ver)
> -        __atomic_fetch_add(_M_addr, 1, __ATOMIC_ACQ_REL);
> +      if (_M_laundered())
> +        {
> +          __atomic_fetch_add(_M_addr, 1, __ATOMIC_ACQ_REL);
> Please mention this increment in the changelog.

Ugh, sorry, I seem to have forgotten how to read a diff.

> OK for trunk and gcc-11 with that change, thanks.

OK to push, no changes needed.

Tested x86_64-pc-linux-gnu, committed to master and cherry-picked to 
releases/gcc-11.