On Tue, 14 Oct 2025 at 13:53 +0200, Tomasz Kaminski wrote:
On Tue, Oct 14, 2025 at 12:38 PM Jonathan Wakely <[email protected]> wrote:
On Thu, 21 Aug 2025 at 08:22 +0200, Tomasz Kamiński wrote:
>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 uninptr. Invocations of the corresponding
s/uninptr/uintptr_t/
>member functions are now replaced with direct use of __atomic builtins.
The text above doesn't fit in 80 columns, and will look even worse in
the output of 'git log' where it's indented by another 4 columns.
See https://cbea.ms/git-commit/#wrap-72 for recommendations on commit
message formatting.
>
> 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.
>
>Signed-off-by: Tomasz Kamiński <[email protected]>
>---
>Changes in v2:
> * update pretty printers
> * use __atomic_sub_fetch in _M_wait_unlock
>
>OK for trunk?
>As atomic wait fixes are in v16, not planning to backport this one.
>
> libstdc++-v3/include/bits/shared_ptr_atomic.h | 60 +++++++++++++------
> libstdc++-v3/python/libstdcxx/v6/printers.py | 2 +-
> .../20_util/shared_ptr/atomic/pr118757.cc | 46 ++++++++++++++
> .../testsuite/20_util/weak_ptr/pr118757.cc | 47 +++++++++++++++
> 4 files changed, 137 insertions(+), 18 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..5f1e1b50514 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 = __atomic_load_n(&_M_val, int(memory_order_relaxed));
Would it be cleaner to use the __atomic_impl::xxx wrappers (or just
use std::atomic_ref) instead of the raw __atomic_xxx built-ins?
You'd still need to use __atomic_wait_address directly, but everything
else could use the nicer API, and not need to cast memory_order to
int.
I used __atomic_ref that is defined in bits/atomic_base.h class.
Also added alignas(_AtomicRef::required_alignment) to _M_val.
> _GLIBCXX_TSAN_MUTEX_DESTROY(&_M_val);
> __glibcxx_assert(!(__val & _S_lock_bit));
> if (auto __pi = reinterpret_cast<pointer>(__val))
Unrelated to this patch, but I wonder if we should do:
__val = __val & ~_S_lock_bit;
here, so that if the destructor _does_ get run while it's locked (and
assertions are disabled), we don't get a segfault or bus error for the
unaligned pointer access.
I am not sure here if I would not prefer to get crash close to the problem
when the date race was occuring.
Yes. Users who want memory safety should be enabling the assertions,
and then the destructor will abort if it's locked.
We could consider checking and using __builtin_trap() when assertions
are disabled, but that's a decision for another day.
That would mean potentially decrementing the refcount while another
thread thinks it has it locked, which is still bad, and still a memory
safety issue.
In particular this thread could make a copy of it and return it.
The safest thing to do here would be to acquire the lock in the
destructor, but that would add overhead for correct programs.
