On Thu, 5 Jun 2025 at 10:09, Tomasz Kaminski <tkami...@redhat.com> wrote:
>
>
>
> On Thu, Jun 5, 2025 at 12:10 AM Jonathan Wakely <jwak...@redhat.com> wrote:
>>
>> These overloads should never be used for proxy waits, so add assertions
>> to ensure that they aren't used accidentally.
>>
>> The reason they can't be used is that they don't call
>> __args._M_setup_wait to obtain a __wait_state pointer. Even if that was
>> changed, they would wait on a proxy wait which is potentially used by
>> many other threads waiting on other addresses, meaning spurious wake ups
>> are likely. In order to make the functions correct they would need to
>> perform additional loads and comparisons of the atomic variable before
>> calling __wait_impl or __wait_until_impl, which would make these
>> functions no faster than the general purpose overloads that take an
>> accessor function and predicate. That would be possible, and I think
>> they would then work for proxy waits, but doesn't seem necessary at this
>> time.
>>
>> In order to preseve the property that these functions are more
>> lightweight and efficient than the general ones, they should not be used
>> for proxy waits.
>>
>> libstdc++-v3/ChangeLog:
>>
>> * include/bits/atomic_timed_wait.h (__atomic_wait_address_until_v):
>> Add assertion to prevent use with proxy waits.
>> (__atomic_wait_address_for_v): Likewise.
>> * include/bits/atomic_wait.h (__atomic_wait_address_v):
>> Likewise.
>> ---
>>
>> Tested x86_64-linux and sparc-solaris.
>
> I would change all the static_assert into runtime asserts. This functions are
> dependent
> only on the duration parameters, and I writing something like"
> __atomic_wait_address_for_v(add, old, ACQUIRE, 0s);
> would lead to hard error even in the , even in dependent context like
> platform_semphore_impl:
> I think it is surprising that __atomic_wait_address_v works in such cases,
> but duration eversion sometimes
> does not.
I don't think we need to care about surprises, since these are purely
internal APIs and the assertions are just to catch mistakes during
libstdc++ maintenance. If any of the assertions (static or runtime)
fail, you've messed up and need to fix something.
But I'll make them all use __glibcxx_assert consistently.
>
>>
>>
>> libstdc++-v3/include/bits/atomic_timed_wait.h | 8 ++++++++
>> libstdc++-v3/include/bits/atomic_wait.h | 4 +++-
>> 2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/libstdc++-v3/include/bits/atomic_timed_wait.h
>> b/libstdc++-v3/include/bits/atomic_timed_wait.h
>> index bd2e6bf61ecf..9cd79e8829c7 100644
>> --- a/libstdc++-v3/include/bits/atomic_timed_wait.h
>> +++ b/libstdc++-v3/include/bits/atomic_timed_wait.h
>> @@ -156,6 +156,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> const chrono::time_point<_Clock, _Dur>&
>> __atime,
>> bool __bare_wait = false) noexcept
>> {
>> +#ifndef _GLIBCXX_HAVE_PLATFORM_TIMED_WAIT
>> + using namespace __detail;
>> + static_assert(__platform_wait_uses_type<__platform_wait_t>);
>> +#endif
>> __detail::__wait_args __args{ __addr, __old, __order, __bare_wait };
>> auto __res = __detail::__wait_until(__addr, __args, __atime);
>> return !__res._M_timeout; // C++26 will also return last observed
>> __val
>> @@ -205,6 +209,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> const chrono::duration<_Rep, _Period>&
>> __rtime,
>> bool __bare_wait = false) noexcept
>> {
>> +#ifndef _GLIBCXX_HAVE_PLATFORM_TIMED_WAIT
>> + using namespace __detail;
>> + static_assert(__platform_wait_uses_type<__platform_wait_t>);
>> +#endif
>> __detail::__wait_args __args{ __addr, __old, __order, __bare_wait };
>> auto __res = __detail::__wait_for(__addr, __args, __rtime);
>> return !__res._M_timeout; // C++26 will also return last observed
>> __val
>> diff --git a/libstdc++-v3/include/bits/atomic_wait.h
>> b/libstdc++-v3/include/bits/atomic_wait.h
>> index d32fda1c2435..95151479c120 100644
>> --- a/libstdc++-v3/include/bits/atomic_wait.h
>> +++ b/libstdc++-v3/include/bits/atomic_wait.h
>> @@ -250,12 +250,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> }
>>
>> // Wait on __addr while *__addr == __old is true.
>> - // Cannot be used for proxy waits.
>> inline void
>> __atomic_wait_address_v(const __detail::__platform_wait_t* __addr,
>> __detail::__platform_wait_t __old,
>> int __order, bool __bare_wait = false)
>> {
>> +#ifndef _GLIBCXX_HAVE_PLATFORM_WAIT
>> + __glibcxx_assert(false); // This function can't be used for proxy wait.
>> +#endif
>> __detail::__wait_args __args{ __addr, __old, __order, __bare_wait };
>> // C++26 will not ignore the return value here
>> __detail::__wait_impl(__addr, __args);
>> --
>> 2.49.0
>>
