libstdc++: Fix proxy wait detection in atomic wait

A failed assertion was observed with std::atomic<bool>::wait when the
loop in __atomic_wait_address is entered and calls _M_setup_wait a
second time, after waking from __wait_impl. When the first call to
_M_setup_wait makes a call to _M_setup_proxy_wait that function decides
that a proxy wait is needed for an object of type bool, and it updates
the _M_obj and _M_obj_size members to refer to the futex in the proxy
state, instead of referring to the bool object itself. The next time
_M_setup_wait is called it calls _M_setup_proxy_wait again but now it
sees _M_obj_size == sizeof(futex) and so this time decides a proxy wait
is *not* needed, and then fails the __glibcxx_assert(_M_obj == addr)
check.

The problem is that _M_setup_proxy_wait wasn't correctly handling the
case where it's called a second time, after the decision to use a proxy
wait has already been made. That can be fixed in _M_setup_proxy_wait by
checking if _M_obj != addr, which implies that a proxy wait has already
been set up by a previous call. In that case, _M_setup_proxy_wait should
only update _M_old to the latest value of the proxy _M_ver.

This change means that _M_setup_proxy_wait is safe to call repeatedly
for a proxy wait, and will only update _M_wait_state, _M_obj, and
_M_obj_size on the first call. On the second and subsequent calls, those
variables are already correctly set for the proxy wait so don't need to
be set again.

For non-proxy waits, calling _M_setup_proxy_wait more than once is safe,
but pessimizes performance. The caller shouldn't make a second call to
_M_setup_proxy_wait because we don't need to check again if a proxy wait
should be used (the answer won't change) and we don't need to load a
value from the proxy _M_ver.

However, it was difficult to detect the case of a non-proxy wait,
because _M_setup_wait doesn't know if it's being called the first time
(when _M_setup_proxy_wait is called to make the initial decision) or a
subsequent time (in which case _M_obj == addr implies a non-proxy wait
was already decided on). As a result, _M_setup_proxy_wait was being used
every time to see if it's a proxy wait. We can resolve this by splitting
the _M_setup_wait function into _M_setup_wait and _M_on_wake, where the
former is only called once to do the initial setup and the latter is
called after __wait_impl returns, to prepare to check the predicate and
possibly wait again.  The new _M_on_wake function can avoid unnecessary
calls to _M_setup_proxy_wait by checking _M_obj == addr to identify a
non-proxy wait.

The three callers of _M_setup_wait are updated to use _M_on_wake instead
of _M_setup_wait after waking from a waiting function. This change
revealed a latent performance bug in __atomic_wait_address_for which was
not passing __res to _M_setup_wait, so a new value was always loaded
even when __res._M_has_val was true. By splitting _M_on_wake out of
_M_setup_wait this problem became more obvious, because we no longer
have _M_setup_wait doing two different jobs, depending on whether it was
passed the optional third argument or not.

libstdc++-v3/ChangeLog:

	* include/bits/atomic_timed_wait.h (__atomic_wait_address_until):
	Use _M_on_wake instead of _M_setup_wait after waking.
	(__atomic_wait_address_for): Likewise.
	* include/bits/atomic_wait.h (__atomic_wait_address): Likewise.
	(__wait_args::_M_setup_wait): Remove third parameter and move
	code to update _M_old to ...
	(__wait_args::_M_on_wake): New member function to update _M_old
	after waking, only calling _M_setup_proxy_wait if needed.
	(__wait_args::_M_store): New member function to update _M_old
	from a value, for non-proxy waits.
	* src/c++20/atomic.cc (__wait_args::_M_setup_proxy_wait): If
	_M_obj is not addr, only load a new value and return true.

Reviewed-by: Tomasz Kamiński <tkaminsk@redhat.com>

diff --git a/libstdc++-v3/include/bits/atomic_timed_wait.h b/libstdc++-v3/include/bits/atomic_timed_wait.h
index bca43e3222dd4adabf1d3242da1da677a54b47f0..c195cefc6b852276642710b63add1fcb446b79e9 100644 (file)
--- a/libstdc++-v3/include/bits/atomic_timed_wait.h
+++ b/libstdc++-v3/include/bits/atomic_timed_wait.h
@@ -140,7 +140,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
          auto __res = __detail::__wait_until(__addr, __args, __atime);
          if (__res._M_timeout)
            return false; // C++26 will also return last observed __val
-         __val = __args._M_setup_wait(__addr, __vfn, __res);
+         __val = __args._M_on_wake(__addr, __vfn, __res);
        }
       return true; // C++26 will also return last observed __val
     }
@@ -192,7 +192,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
          auto __res = __detail::__wait_for(__addr, __args, __rtime);
          if (__res._M_timeout)
            return false; // C++26 will also return last observed __val
-         __val = __args._M_setup_wait(__addr, __vfn);
+         __val = __args._M_on_wake(__addr, __vfn, __res);
        }
       return true; // 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 b6240a95370ffce36daf1682b8a7238a172c442c..8511e003abcaa3a5706dc2707334f05218069632 100644 (file)
--- a/libstdc++-v3/include/bits/atomic_wait.h
+++ b/libstdc++-v3/include/bits/atomic_wait.h
@@ -205,43 +205,72 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       template<typename _Tp, typename _ValFn>
        _Tp
-       _M_setup_wait(const _Tp* __addr, _ValFn __vfn,
-                     __wait_result_type __res = {})
+       _M_setup_wait(const _Tp* __addr, _ValFn __vfn)
        {
          static_assert(is_same_v<_Tp, decay_t<decltype(__vfn())>>);
 
-         if (__res._M_has_val) // A previous wait loaded a recent value.
-           {
-             _M_old = __res._M_val;
-             if constexpr (!__platform_wait_uses_type<_Tp>)
-               {
-                 // __res._M_val might be the value of a proxy wait object,
-                 // not the value of *__addr. Call __vfn() to get new value.
-                 return __vfn();
-               }
-             // Not a proxy wait, so the value in __res._M_val was loaded
-             // from *__addr and we don't need to call __vfn().
-             else if constexpr (sizeof(_Tp) == sizeof(__UINT32_TYPE__))
-               return __builtin_bit_cast(_Tp, (__UINT32_TYPE__)_M_old);
-             else if constexpr (sizeof(_Tp) == sizeof(__UINT64_TYPE__))
-               return __builtin_bit_cast(_Tp, (__UINT64_TYPE__)_M_old);
-             else
-               static_assert(false); // Unsupported size
-           }
-
          if constexpr (!__platform_wait_uses_type<_Tp>)
            if (_M_setup_proxy_wait(__addr))
              {
                // We will use a proxy wait for this object.
-               // The library has set _M_obj and _M_obj_size and _M_old.
+               // The library has set _M_wait_state, _M_obj, _M_obj_size,
+               // and _M_old.
                // Call __vfn to load the current value from *__addr
                // (which must happen after the call to _M_setup_proxy_wait).
                return __vfn();
              }
 
          // We will use a futex-like operation to wait on this object,
-         // and so can just load the value and store it into _M_old.
-         auto __val = __vfn();
+         // so just load the value, store it into _M_old, and return it.
+         return _M_store(__vfn());
+       }
+
+      // Called after a wait returns, to prepare to wait again.
+      template<typename _Tp, typename _ValFn>
+       _Tp
+       _M_on_wake(const _Tp* __addr, _ValFn __vfn, __wait_result_type __res)
+       {
+         if constexpr (!__platform_wait_uses_type<_Tp>) // maybe a proxy wait
+           if (_M_obj != __addr) // definitely a proxy wait
+             {
+               if (__res._M_has_val)
+                 // Previous wait loaded a recent value from the proxy.
+                 _M_old = __res._M_val;
+               else // Load a new value from the proxy and store in _M_old.
+                 (void) _M_setup_proxy_wait(nullptr);
+               // Read the current value of *__addr
+               return __vfn();
+             }
+
+         if (__res._M_has_val) // The previous wait loaded a recent value.
+           {
+             _M_old = __res._M_val;
+
+             // Not a proxy wait, so the value in __res._M_val was loaded
+             // from *__addr and we don't need to call __vfn().
+             if constexpr (sizeof(_Tp) == sizeof(__UINT64_TYPE__))
+               return __builtin_bit_cast(_Tp, (__UINT64_TYPE__)_M_old);
+             else if constexpr (sizeof(_Tp) == sizeof(__UINT32_TYPE__))
+               return __builtin_bit_cast(_Tp, (__UINT32_TYPE__)_M_old);
+             else if constexpr (sizeof(_Tp) == sizeof(__UINT16_TYPE__))
+               return __builtin_bit_cast(_Tp, (__UINT16_TYPE__)_M_old);
+             else if constexpr (sizeof(_Tp) == sizeof(__UINT8_TYPE__))
+               return __builtin_bit_cast(_Tp, (__UINT8_TYPE__)_M_old);
+             else // Should be a proxy wait for this size!
+               __glibcxx_assert(false);
+           }
+         else
+           return _M_store(__vfn());
+       }
+
+    private:
+      // Store __val in _M_old.
+      // pre: This must be a non-proxy wait.
+      template<typename _Tp>
+       [[__gnu__::__always_inline__]]
+       _Tp
+       _M_store(_Tp __val)
+       {
          // We have to consider various sizes, because a future libstdc++.so
          // might enable non-proxy waits for additional sizes.
          if constexpr (sizeof(_Tp) == sizeof(__UINT64_TYPE__))
@@ -252,12 +281,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
            _M_old = __builtin_bit_cast(__UINT16_TYPE__, __val);
          else if constexpr (sizeof(_Tp) == sizeof(__UINT8_TYPE__))
            _M_old = __builtin_bit_cast(__UINT8_TYPE__, __val);
-         else // _M_setup_proxy_wait should have returned true for this type!
+         else // Should be a proxy wait for this size!
            __glibcxx_assert(false);
          return __val;
        }
 
-    private:
       // Returns true if a proxy wait will be used for __addr, false otherwise.
       // If true, _M_wait_state, _M_obj, _M_obj_size, and _M_old are set.
       // If false, data members are unchanged.
@@ -297,7 +325,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       while (!__pred(__val))
        {
          auto __res = __detail::__wait_impl(__addr, __args);
-         __val = __args._M_setup_wait(__addr, __vfn, __res);
+         __val = __args._M_on_wake(__addr, __vfn, __res);
        }
       // C++26 will return __val
     }

diff --git a/libstdc++-v3/src/c++20/atomic.cc b/libstdc++-v3/src/c++20/atomic.cc
index 2d5842df30c3c9d9e2ad492f41742d4ca7b1a837..cbff5e9ba1a64f4623c61b34537bf6a33e1f2d69 100644 (file)
--- a/libstdc++-v3/src/c++20/atomic.cc
+++ b/libstdc++-v3/src/c++20/atomic.cc
@@ -311,26 +311,32 @@ namespace
 
 // Return false (and don't change any data members) if we can do a non-proxy
 // wait for the object of size `_M_obj_size` at address `addr`.
-// Otherwise, the object at addr needs to use a proxy wait. Set _M_wait_state,
-// set _M_obj and _M_obj_size to refer to the _M_wait_state->_M_ver proxy,
-// load the current value from _M_obj and store it in _M_old, then return true.
+// Otherwise, we will use a proxy wait. If addr == _M_obj this is the initial
+// setup of the proxy wait, so set _M_wait_state to the proxy state for addr,
+// and set _M_obj and _M_obj_size to refer to the _M_wait_state->_M_ver proxy.
+// For both the initial setup of a proxy wait and for subsequent calls to this
+// function for proxy waits, we load the current value from _M_obj (the proxy)
+// and store it in _M_old, then return true.
 bool
 __wait_args::_M_setup_proxy_wait(const void* addr)
 {
-  if (!use_proxy_wait(*this, addr)) // We can wait on this address directly.
+  __waitable_state* state = nullptr;
+
+  if (addr == _M_obj)
     {
-      // Ensure the caller set _M_obj correctly, as that's what we'll wait on:
-      __glibcxx_assert(_M_obj == addr);
-      return false;
-    }
+      if (!use_proxy_wait(*this, addr)) // We can wait on this address directly.
+       return false;
 
-  // This will be a proxy wait, so get a waitable state.
-  auto state = set_wait_state(addr, *this);
+      // This will be a proxy wait, so get a waitable state.
+      state = set_wait_state(addr, *this);
 
-  // The address we will wait on is the version count of the waitable state:
-  _M_obj = &state->_M_ver;
-  // __wait_impl and __wait_until_impl need to know this size:
-  _M_obj_size = sizeof(state->_M_ver);
+      // The address we will wait on is the version count of the waitable state:
+      _M_obj = &state->_M_ver;
+      // __wait_impl and __wait_until_impl need to know this size:
+      _M_obj_size = sizeof(state->_M_ver);
+    }
+  else // This is not the first call to this function, already a proxy wait.
+    state = static_cast<__waitable_state*>(_M_wait_state);
 
   // Read the value of the _M_ver counter.
   _M_old = __atomic_load_n(&state->_M_ver, __ATOMIC_ACQUIRE);