https://gcc.gnu.org/g:59889eaa2b4822389794bbb7793c4d7d391d9614
commit r16-4419-g59889eaa2b4822389794bbb7793c4d7d391d9614 Author: Tomasz Kamiński <[email protected]> Date: Wed Aug 20 15:56:21 2025 +0200 libstdc++: Make atomic<shared_ptr<T>>::wait sensitive to stored pointer only changes [PR118757] 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 <[email protected]> Signed-off-by: Tomasz Kamiński <[email protected]> Diff: --- libstdc++-v3/include/bits/shared_ptr_atomic.h | 57 ++++++++++++++++------ libstdc++-v3/python/libstdcxx/v6/printers.py | 6 ++- .../20_util/shared_ptr/atomic/pr118757.cc | 29 +++++++++++ .../testsuite/20_util/weak_ptr/pr118757.cc | 30 ++++++++++++ 4 files changed, 107 insertions(+), 15 deletions(-) diff --git a/libstdc++-v3/include/bits/shared_ptr_atomic.h b/libstdc++-v3/include/bits/shared_ptr_atomic.h index cc7841a8775c..cbc4bf621f44 100644 --- 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 e5336b7fa89b..1822d425f88e 100644 --- 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 index 000000000000..d54abd8a0392 --- /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 index 000000000000..f048f13aec25 --- /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(); +}
