Re: [PATCH] [libstdc++] Fix "bare" notifications dropped by waiters check

[Jonathan Wakely <jwakely@redhat.com>]

On 21/04/21 18:29 -0700, Thomas Rodgers wrote:
>From: Thomas Rodgers <rodgert@twrodgers.com>
>
>NOTE - This patch also needs to be backported to gcc-11 in order for
>semaphore release() to work correctly on non-futex platforms.
>
>Tested sparc-sun-solaris2.11
>
>For types that track whether or not there extant waiters (e.g.
>semaphore) internally, the __atomic_notify_address_bare() call was
>introduced to avoid the overhead of loading the atomic count of
>waiters. For platforms that don't have Futex, however, there was
>still a check for waiters, and seeing that there are none (because
>in the bare case, the count is not incremented), the notification
>is dropped. This commit addresses that case.
>
>libstdc++-v3/ChangeLog:
>	* include/bits/atomic_wait.h: Always notify waiters in the
>	in the case of 'bare' address notification.

Repeated text: "in the in the"


>---
> libstdc++-v3/include/bits/atomic_wait.h | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
>diff --git a/libstdc++-v3/include/bits/atomic_wait.h b/libstdc++-v3/include/bits/atomic_wait.h
>index 0ac5575190c..984ed70f16c 100644
>--- a/libstdc++-v3/include/bits/atomic_wait.h
>+++ b/libstdc++-v3/include/bits/atomic_wait.h
>@@ -226,9 +226,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       }
>
>       void
>-      _M_notify(const __platform_wait_t* __addr, bool __all) noexcept
>+      _M_notify(const __platform_wait_t* __addr, bool __all, bool __bare) noexcept
>       {
>-	if (!_M_waiting())
>+	if (!(__bare || _M_waiting()))

Maybe it's just me, but I would find (!__base && !__waiting) to be a
clearer expression of the logic here.

i.e. "return if the wait is not bare and there are no waiters"
rather than "return if the wait is not either bare or has waiters".
The latter makes me take a second to grok.

The patch is OK either way though, with the ChangeLog typo fix.


> 	  return;
>
> #ifdef _GLIBCXX_HAVE_PLATFORM_WAIT
>@@ -304,11 +304,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	  }
>
> 	void
>-	_M_notify(bool __all)
>+	_M_notify(bool __all, bool __bare = false)
> 	{
> 	  if (_M_addr == &_M_w._M_ver)
> 	    __atomic_fetch_add(_M_addr, 1, __ATOMIC_ACQ_REL);
>-	  _M_w._M_notify(_M_addr, __all);
>+	  _M_w._M_notify(_M_addr, __all, __bare);
> 	}
>
> 	template<typename _Up, typename _ValFn,
>@@ -452,7 +452,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     __atomic_notify_address(const _Tp* __addr, bool __all) noexcept
>     {
>       __detail::__bare_wait __w(__addr);
>-      __w._M_notify(__all);
>+      __w._M_notify(__all, true);
>     }
>
>   // This call is to be used by atomic types which track contention externally
>@@ -464,7 +464,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     __detail::__platform_notify(__addr, __all);
> #else
>     __detail::__bare_wait __w(__addr);
>-    __w._M_notify(__all);
>+    __w._M_notify(__all, true);
> #endif
>   }
> _GLIBCXX_END_NAMESPACE_VERSION
>-- 
>2.30.2
>