[PATCH 2/4] libstdc++: Refactor __semaphore_base member functions

[Tomasz Kaminski]

On Tue, Jun 3, 2025 at 10:32 AM Jonathan Wakely <jwakely@redhat.com> wrote:

> Replace the _S_get_current and _S_do_try_acquire static member functions
> with non-static member functions _M_get_current and _M_do_try_acquire.
> This means they don't need the address of _M_counter passed in.
>
> libstdc++-v3/ChangeLog:
>
>         * include/bits/semaphore_base.h (_S_get_current): Replace with
>         non-static _M_get_current.
>         (_S_do_try_acquire): Replace with non-static _M_do_try_acquire.
> ---
>
> I'm not sure why these were static member functions in the first place,
> it doesn't seem to have any benefit.
>
> Tested x86_64-linux.
>
This one LGTM.

>
>  libstdc++-v3/include/bits/semaphore_base.h | 39 ++++++++++++----------
>  1 file changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/libstdc++-v3/include/bits/semaphore_base.h
> b/libstdc++-v3/include/bits/semaphore_base.h
> index 526da4d82b6a..3f7a33ccd51a 100644
> --- a/libstdc++-v3/include/bits/semaphore_base.h
> +++ b/libstdc++-v3/include/bits/semaphore_base.h
> @@ -64,20 +64,23 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>      __semaphore_base(const __semaphore_base&) = delete;
>      __semaphore_base& operator=(const __semaphore_base&) = delete;
>
> -    static _GLIBCXX_ALWAYS_INLINE __count_type
> -    _S_get_current(__count_type* __counter) noexcept
> -    {
> -      return __atomic_impl::load(__counter, memory_order::acquire);
> -    }
> +    // Load the current counter value.
> +    _GLIBCXX_ALWAYS_INLINE __count_type
> +    _M_get_current() const noexcept
> +    { return __atomic_impl::load(&_M_counter, memory_order::acquire); }
>
> -    static _GLIBCXX_ALWAYS_INLINE bool
> -    _S_do_try_acquire(__count_type* __counter, __count_type& __old)
> noexcept
> +    // Try to acquire the semaphore (i.e. decrement the counter).
> +    // Returns false if the current counter is zero, or if another thread
> +    // decrements the value first. In the latter case, __cur is set to the
> +    // new value.
> +    _GLIBCXX_ALWAYS_INLINE bool
> +    _M_do_try_acquire(__count_type& __cur) noexcept
>      {
> -      if (__old == 0)
> -       return false;
> +      if (__cur == 0)
> +       return false; // Cannot decrement when it's already zero.
>
> -      return __atomic_impl::compare_exchange_strong(__counter,
> -                                                   __old, __old - 1,
> +      return __atomic_impl::compare_exchange_strong(&_M_counter,
> +                                                   __cur, __cur - 1,
>                                                     memory_order::acquire,
>                                                     memory_order::relaxed);
>      }
> @@ -85,7 +88,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>      void
>      _M_acquire() noexcept
>      {
> -      auto const __vfn = [this]{ return
> _S_get_current(&this->_M_counter); };
> +      auto const __vfn = [this]{ return _M_get_current(); };
>        auto __val = __vfn();
>        auto const __pred = [&__val](__count_type __cur) {
>         if (__cur > 0)
> @@ -95,7 +98,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>           }
>         return false;
>        };
> -      while (!_S_do_try_acquire(&_M_counter, __val))
> +      while (!_M_do_try_acquire(__val))
>         if (__val == 0)
>           std::__atomic_wait_address(&_M_counter, __pred, __vfn, true);
>      }
> @@ -103,7 +106,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>      bool
>      _M_try_acquire() noexcept
>      {
> -      // The fastest implementation of this function is just
> _S_do_try_acquire
> +      // The fastest implementation of this function is just
> _M_do_try_acquire
>        // but that can fail under contention even when _M_count > 0.
>        // Using _M_try_acquire_for(0ns) will retry a few times in a loop.
>        return _M_try_acquire_for(__detail::__wait_clock_t::duration{});
> @@ -113,7 +116,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>        bool
>        _M_try_acquire_until(const chrono::time_point<_Clock, _Duration>&
> __atime) noexcept
>        {
> -       auto const __vfn = [this]{ return
> _S_get_current(&this->_M_counter); };
> +       auto const __vfn = [this]{ return _M_get_current(); };
>         auto __val = __vfn();
>         auto const __pred = [&__val](__count_type __cur) {
>           if (__cur > 0)
> @@ -123,7 +126,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>             }
>           return false;
>         };
> -       while (!_S_do_try_acquire(&this->_M_counter, __val))
> +       while (!_M_do_try_acquire(__val))
>           if (__val == 0)
>             {
>               if (!std::__atomic_wait_address_until(&_M_counter, __pred,
> @@ -137,7 +140,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>        bool
>        _M_try_acquire_for(const chrono::duration<_Rep, _Period>& __rtime)
> noexcept
>        {
> -       auto const __vfn = [this]{ return
> _S_get_current(&this->_M_counter); };
> +       auto const __vfn = [this]{ return _M_get_current(); };
>         auto __val = __vfn();
>         auto const __pred = [&__val](__count_type __cur) {
>           if (__cur > 0)
> @@ -147,7 +150,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>             }
>           return false;
>         };
> -       while (!_S_do_try_acquire(&this->_M_counter, __val))
> +       while (!_M_do_try_acquire(__val))
>           if (__val == 0)
>             {
>               if (!std::__atomic_wait_address_for(&_M_counter, __pred,
> --
> 2.49.0
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20250604/e07030fa/attachment.htm>