With the change to the title, this looks good to me.
On Tue, Jun 3, 2025 at 12:02 PM Tomasz Kaminski <tkami...@redhat.com> wrote:
> The tile says that you are doing only comment fixes, while there is code
> change
> in __spin_until_impl. Could you please adjust it:
> Fix incorrect returns and comments on atomic timed waits
>
> On Mon, Jun 2, 2025 at 7:24 PM Jonathan Wakely <jwak...@redhat.com> wrote:
>
>> The __detail::__wait_until function has a comment that should have been
>> removed when r16-1000-g225622398a9631 changed the return type from a
>> std::pair to a struct with three members.
>>
>> __atomic_wait_address_until_v returns __res.first which should have been
>> changed when __res was no longer a std::pair.
>>
>> Fix _M_spin_until_impl so that the _M_has_val member of the result is
>> accurate. If the deadline has passed then it never enters the loop and
>> so never loads a fresh value, so _M_has_val should be false.
>>
>> libstdc++-v3/ChangeLog:
>>
>> * include/bits/atomic_timed_wait.h (__detail::__wait_until):
>> Remove incorrect comment.
>> (__atomic_wait_address_until_v): Fix return statement.
>> * src/c++20/atomic.cc (__spin_until_impl): Fix incorrect
>> return value. Reuse result of first call to clock.
>> ---
>>
>> Tested x86_64-linux.
>>
>> libstdc++-v3/include/bits/atomic_timed_wait.h | 3 +--
>> libstdc++-v3/src/c++20/atomic.cc | 24 ++++++++++++-------
>> 2 files changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/libstdc++-v3/include/bits/atomic_timed_wait.h
>> b/libstdc++-v3/include/bits/atomic_timed_wait.h
>> index 230afbc96e7d..e22c13f47b18 100644
>> --- a/libstdc++-v3/include/bits/atomic_timed_wait.h
>> +++ b/libstdc++-v3/include/bits/atomic_timed_wait.h
>> @@ -87,7 +87,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> __wait_until_impl(const void* __addr, __wait_args_base& __args,
>> const __wait_clock_t::duration& __atime);
>>
>> - // Returns {true, val} if wait ended before a timeout.
>> template<typename _Clock, typename _Dur>
>> __wait_result_type
>> __wait_until(const void* __addr, __wait_args_base& __args,
>> @@ -159,7 +158,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> {
>> __detail::__wait_args __args{ __addr, __old, __order, __bare_wait
>> };
>> auto __res = __detail::__wait_until(__addr, &__args, __atime);
>> - return __res.first; // C++26 will also return last observed __val
>> + return !__res._M_timeout; // C++26 will also return last observed
>> __val
>> }
>>
>> template<typename _Tp, typename _ValFn,
>> diff --git a/libstdc++-v3/src/c++20/atomic.cc
>> b/libstdc++-v3/src/c++20/atomic.cc
>> index a3ec92a10d56..4120e1a0817f 100644
>> --- a/libstdc++-v3/src/c++20/atomic.cc
>> +++ b/libstdc++-v3/src/c++20/atomic.cc
>> @@ -397,17 +397,18 @@ __cond_wait_until(__condvar& __cv, mutex& __mx,
>> }
>> #endif // ! HAVE_PLATFORM_TIMED_WAIT
>>
>> -// Like __spin_impl, always returns _M_has_val == true.
>> +// Unlike __spin_impl, does not always return _M_has_val == true.
>> +// If the deadline has already passed then no fresh value is loaded.
>>
> This is indeed true now.
>
>> __wait_result_type
>> __spin_until_impl(const __platform_wait_t* __addr,
>> const __wait_args_base& __args,
>> const __wait_clock_t::time_point& __deadline)
>> {
>> - auto __t0 = __wait_clock_t::now();
>> using namespace literals::chrono_literals;
>>
>> - __platform_wait_t __val{};
>> - auto __now = __wait_clock_t::now();
>> + __wait_result_type __res{};
>> + auto __t0 = __wait_clock_t::now();
>> + auto __now = __t0;
>> for (; __now < __deadline; __now = __wait_clock_t::now())
>> {
>> auto __elapsed = __now - __t0;
>> @@ -422,16 +423,21 @@ __spin_until_impl(const __platform_wait_t* __addr,
>> __thread_yield();
>> else
>> {
>> - auto __res = __detail::__spin_impl(__addr, __args);
>> + __res = __detail::__spin_impl(__addr, __args);
>> if (!__res._M_timeout)
>> return __res;
>> }
>>
>> - __atomic_load(__addr, &__val, __args._M_order);
>> - if (__val != __args._M_old)
>> - return { ._M_val = __val, ._M_has_val = true, ._M_timeout = false
>> };
>> + __atomic_load(__addr, &__res._M_val, __args._M_order);
>> + __res._M_has_val = true;
>> + if (__res._M_val != __args._M_old)
>> + {
>> + __res._M_timeout = false;
>> + return __res;
>> + }
>> }
>> - return { ._M_val = __val, ._M_has_val = true, ._M_timeout = true };
>> + __res._M_timeout = true;
>> + return __res;
>> }
>> } // namespace
>>
>> --
>> 2.49.0
>>
>>
