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

[Jonathan Wakely <jwakely@redhat.com>]

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.