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::_AtomicRef): Define.
(_Atomic_count::~_Atomic_count, _Atomic_count::lock)
(_Atomic_count::unlock, _Atomic_count::_M_swap_unlock)
(_Atomic_count::notify_one, _Atomic_count::notify_all):
Use _AtomicRef for atomic operations on _M_val.
(_Sp_atomic::element_type): Define.
(_Sp_atomic::_M_val): Change type to uintptr_t, add appropariate
alignas.
(_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.
Signed-off-by: Tomasz Kamiński <[email protected]>
---
changes in v3:
* break the lines to fit 80 collumns in commit description
* fix typos in commit message.
* make sure that _M_val is sufficiently aligned
(atomic_ref<uintptr_t>::required_aligment)
* using __atomic_ref<uintptr_t> for atomic operations on _M_val.
* remove copyright notice and fix signaller in test.
Testing on x86_64-linux. OK for trunk when test passes?
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(-)
create mode 100644 libstdc++-v3/testsuite/20_util/shared_ptr/atomic/pr118757.cc
create mode 100644 libstdc++-v3/testsuite/20_util/weak_ptr/pr118757.cc
diff --git a/libstdc++-v3/include/bits/shared_ptr_atomic.h
b/libstdc++-v3/include/bits/shared_ptr_atomic.h
index cc7841a8775..cbc4bf621f4 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 e5336b7fa89..1822d425f88 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 00000000000..d54abd8a039
--- /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 00000000000..f048f13aec2
--- /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();
+}
--
2.51.0
