libstdc++: Make atomic<shared_ptr<T>>::wait sensitive to stored pointer only changes...

Previously, atomic<shared_ptr<T>>::wait (and the weak_ptr version) was
equivalent to waiting directly on _M_val, which corresponds to the pointer
to the control block (_M_pi). Consequently, wakeups were not triggered if
the stored pointer value was changed to a pointer that uses the same control
block but stores pointer to a different object. Such a pointer can be
constructed using an aliasing constructor.

To address this, wait now uses a generic proxy wait
std::__atomic_wait_address function, which supports waiting until any
predicate is satisfied. The provided predicate now compares both the control
block (_M_pi) and the stored pointer (_M_ptr). Comparing the latter requires
locking the pointer.

Since this function operates on raw pointers, the type of _M_val was changed
from __atomic_base<uintptr_t> to uintptr_t. Invocations of the corresponding
member functions are now replaced with direct use of __atomic builtins.

	PR libstdc++/118757

libstdc++-v3/ChangeLog:

	* include/bits/shared_ptr_atomic.h (_Atomic_count::_M_wait_unlock):
	Add parameter capturing reference to _M_ptr. Reimplement in terms
	of __atomic_wait_address.
	(_Atomic_count::~_Atomic_count, _Atomic_count::lock)
	(_Atomic_count::unlock, _Atomic_count::_M_swap_unlock): Replace
	invocation of atomic member funcitons with __atomic builtins.
	(_Atomic_count::notify_one, _Atomic_count::notify_all):
	Use __atomic_notify_address.
	(_Sp_atomic::element_type): Define.
	(_Sp_atomic::_M_val): Change type to uintptr_t.
	(_Sp_atomic::wait): Pass _M_ptr to _M_wait_unlock.
	* python/libstdcxx/v6/printers.py:
	* testsuite/20_util/shared_ptr/atomic/pr118757.cc: New test.
	* testsuite/20_util/weak_ptr/pr118757.cc: New test.

Reviewed-by: Jonathan Wakely <jwakely@redhat.com>
Signed-off-by: Tomasz Kamiński <tkaminsk@redhat.com>

diff --git a/libstdc++-v3/include/bits/shared_ptr_atomic.h b/libstdc++-v3/include/bits/shared_ptr_atomic.h
index cc7841a8775c5067d9daaee595c4efe99c953c92..cbc4bf621f44343da2905464808329231855e58a 100644 (file)
--- a/libstdc++-v3/include/bits/shared_ptr_atomic.h
+++ b/libstdc++-v3/include/bits/shared_ptr_atomic.h
@@ -392,6 +392,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     class _Sp_atomic
     {
       using value_type = _Tp;
+      using element_type = typename _Tp::element_type;
 
       friend struct atomic<_Tp>;
 
@@ -420,7 +421,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
        ~_Atomic_count()
        {
-         auto __val = _M_val.load(memory_order_relaxed);
+         auto __val = _AtomicRef(_M_val).load(memory_order_relaxed);
          _GLIBCXX_TSAN_MUTEX_DESTROY(&_M_val);
          __glibcxx_assert(!(__val & _S_lock_bit));
          if (auto __pi = reinterpret_cast<pointer>(__val))
@@ -442,18 +443,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        {
          // To acquire the lock we flip the LSB from 0 to 1.
 
-         auto __current = _M_val.load(memory_order_relaxed);
+         _AtomicRef __aref(_M_val);
+         auto __current = __aref.load(memory_order_relaxed);
          while (__current & _S_lock_bit)
            {
 #if __glibcxx_atomic_wait
              __detail::__thread_relax();
 #endif
-             __current = _M_val.load(memory_order_relaxed);
+             __current = __aref.load(memory_order_relaxed);
            }
 
          _GLIBCXX_TSAN_MUTEX_TRY_LOCK(&_M_val);
 
-         while (!_M_val.compare_exchange_strong(__current,
+         while (!__aref.compare_exchange_strong(__current,
                                                 __current | _S_lock_bit,
                                                 __o,
                                                 memory_order_relaxed))
@@ -474,7 +476,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        unlock(memory_order __o) const noexcept
        {
          _GLIBCXX_TSAN_MUTEX_PRE_UNLOCK(&_M_val);
-         _M_val.fetch_sub(1, __o);
+         _AtomicRef(_M_val).fetch_sub(1, __o);
          _GLIBCXX_TSAN_MUTEX_POST_UNLOCK(&_M_val);
        }
 
@@ -487,7 +489,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
            __o = memory_order_release;
          auto __x = reinterpret_cast<uintptr_t>(__c._M_pi);
          _GLIBCXX_TSAN_MUTEX_PRE_UNLOCK(&_M_val);
-         __x = _M_val.exchange(__x, __o);
+         __x = _AtomicRef(_M_val).exchange(__x, __o);
          _GLIBCXX_TSAN_MUTEX_POST_UNLOCK(&_M_val);
          __c._M_pi = reinterpret_cast<pointer>(__x & ~_S_lock_bit);
        }
@@ -495,19 +497,45 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 #if __glibcxx_atomic_wait
        // Precondition: caller holds lock!
        void
-       _M_wait_unlock(memory_order __o) const noexcept
+       _M_wait_unlock(const element_type* const& __ptr, memory_order __o) const noexcept
        {
+         auto __old_ptr = __ptr;
          _GLIBCXX_TSAN_MUTEX_PRE_UNLOCK(&_M_val);
-         auto __v = _M_val.fetch_sub(1, memory_order_relaxed);
+         uintptr_t __old_pi 
+           = _AtomicRef(_M_val).fetch_sub(1, memory_order_relaxed) - 1u;
          _GLIBCXX_TSAN_MUTEX_POST_UNLOCK(&_M_val);
-         _M_val.wait(__v & ~_S_lock_bit, __o);
+
+         // Ensure that the correct value of _M_ptr is visible after locking,
+         // by upgrading relaxed or consume to acquire.
+         auto __lo = __o;
+         if (__o != memory_order_seq_cst)
+           __lo = memory_order_acquire;
+
+         std::__atomic_wait_address(
+           &_M_val,
+           [=, &__ptr, this](uintptr_t __new_pi)
+             {
+               if (__old_pi != (__new_pi & ~_S_lock_bit))
+                 // control block changed, we can wake up
+                 return true;
+
+               // control block is same, we need to check if ptr changed,
+               // the lock needs to be taken first, the value of pi may have
+               // also been updated in meantime, so reload it
+               __new_pi = reinterpret_cast<uintptr_t>(this->lock(__lo));
+               auto __new_ptr = __ptr;
+               this->unlock(memory_order_relaxed);
+               // wake up if either of the values changed
+               return __new_pi != __old_pi || __new_ptr != __old_ptr;
+             },
+           [__o, this] { return _AtomicRef(_M_val).load(__o); });
        }
 
        void
        notify_one() noexcept
        {
          _GLIBCXX_TSAN_MUTEX_PRE_SIGNAL(&_M_val);
-         _M_val.notify_one();
+         _AtomicRef(_M_val).notify_one();
          _GLIBCXX_TSAN_MUTEX_POST_SIGNAL(&_M_val);
        }
 
@@ -515,17 +543,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        notify_all() noexcept
        {
          _GLIBCXX_TSAN_MUTEX_PRE_SIGNAL(&_M_val);
-         _M_val.notify_all();
+         _AtomicRef(_M_val).notify_all();
          _GLIBCXX_TSAN_MUTEX_POST_SIGNAL(&_M_val);
        }
 #endif
 
       private:
-       mutable __atomic_base<uintptr_t> _M_val{0};
+       using _AtomicRef = __atomic_ref<uintptr_t>;
+       alignas(_AtomicRef::required_alignment) mutable uintptr_t _M_val{0};
        static constexpr uintptr_t _S_lock_bit{1};
       };
 
-      typename _Tp::element_type* _M_ptr = nullptr;
+      element_type* _M_ptr = nullptr;
       _Atomic_count _M_refcount;
 
       static typename _Atomic_count::pointer
@@ -608,7 +637,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
        auto __pi = _M_refcount.lock(memory_order_acquire);
        if (_M_ptr == __old._M_ptr && __pi == __old._M_refcount._M_pi)
-         _M_refcount._M_wait_unlock(__o);
+         _M_refcount._M_wait_unlock(_M_ptr, __o);
        else
          _M_refcount.unlock(memory_order_relaxed);
       }

diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py b/libstdc++-v3/python/libstdcxx/v6/printers.py
index e5336b7fa89b9092b456bde7172f130a12d52ab6..1822d425f88e5c241ac0cf96ce0b005404e4006e 100644 (file)
--- a/libstdc++-v3/python/libstdcxx/v6/printers.py
+++ b/libstdc++-v3/python/libstdcxx/v6/printers.py
@@ -287,7 +287,11 @@ class SharedPointerPrinter(printer_base):
     def _get_refcounts(self):
         if self._typename == 'std::atomic':
             # A tagged pointer is stored as uintptr_t.
-            ptr_val = self._val['_M_refcount']['_M_val']['_M_i']
+            val = self._val['_M_refcount']['_M_val']
+            if val.type.is_scalar: # GCC 16 stores uintptr_t
+                ptr_val = val
+            else: # GCC 12-15 stores std::atomic<uintptr_t>
+                ptr_val = val['_M_i']
             ptr_val = ptr_val - (ptr_val % 2)  # clear lock bit
             ptr_type = find_type(self._val['_M_refcount'].type, 'pointer')
             return ptr_val.cast(ptr_type)

diff --git a/libstdc++-v3/testsuite/20_util/shared_ptr/atomic/pr118757.cc b/libstdc++-v3/testsuite/20_util/shared_ptr/atomic/pr118757.cc
new file mode 100644 (file)
index 0000000..d54abd8
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/shared_ptr/atomic/pr118757.cc
@@ -0,0 +1,29 @@
+// { dg-do run { target c++20 } }
+// { dg-require-gthreads "" }
+// { dg-require-effective-target hosted }
+
+#include <memory>
+#include <chrono>
+#include <thread>
+#include <barrier>
+
+std::shared_ptr<int> q = std::make_shared<int>(42);
+std::atomic<std::shared_ptr<int>> p = q;
+
+std::barrier bar(2);
+
+void signaller()
+{
+  std::this_thread::sleep_for(std::chrono::seconds(1));
+  p.store(std::shared_ptr<int>(q, nullptr));
+  p.notify_one();
+  bar.arrive_and_wait();
+}
+
+int main(int, char**)
+{
+  std::thread thr(signaller);
+  p.wait(q);
+  bar.arrive_and_wait();
+  thr.join();
+}

diff --git a/libstdc++-v3/testsuite/20_util/weak_ptr/pr118757.cc b/libstdc++-v3/testsuite/20_util/weak_ptr/pr118757.cc
new file mode 100644 (file)
index 0000000..f048f13
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/weak_ptr/pr118757.cc
@@ -0,0 +1,30 @@
+// { dg-do run { target c++20 } }
+// { dg-require-gthreads "" }
+// { dg-require-effective-target hosted }
+
+#include <memory>
+#include <chrono>
+#include <thread>
+#include <barrier>
+
+std::shared_ptr<int> s = std::make_shared<int>(42);
+std::weak_ptr<int> q = s;
+std::atomic<std::weak_ptr<int>> p = q;
+
+std::barrier bar(2);
+
+void signaller()
+{
+  std::this_thread::sleep_for(std::chrono::seconds(1));
+  p.store(std::shared_ptr<int>(s, nullptr));
+  p.notify_one();
+  bar.arrive_and_wait();
+}
+
+int main(int, char**)
+{
+  std::thread thr(signaller);
+  p.wait(q);
+  bar.arrive_and_wait();
+  thr.join();
+}