[PATCH] libstdc++: Fix incorrect comments on atomic timed waits

[Tomasz Kaminski]

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 <jwakely@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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20250603/ac86f774/attachment.htm>