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

[Thomas Rodgers]

On 2021-04-22 02:23, Jonathan Wakely wrote:

> 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.
> 

As discussed on IRC, I went back and forth on this a couple of times and 
neither option seemed particularly great to me. I started down the path 
of propagating the std::true_type/std::false_type bits that the RAII 
wrapper type knows about and making this a compile time choice, but felt 
the change was too big to make this late in the release cycle. I will 
revisit this for stage1 (though ideally all of this will be moved into 
the .so for GCC12 and partially rewritten as part of that process 
anyway).

> 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

Tested sparc-sun-solaris2.11.
Committed to master, backported to release/gcc-11.