On Mon, 8 Sept 2025 at 15:19, Tomasz Kamiński <tkami...@redhat.com> wrote:
>
> From: Giuseppe D'Angelo <giuseppe.dang...@kdab.com>
>
> Converting a weak_ptr<Derived> to a weak_ptr<Base> requires calling
> lock() on the source object in the general case.
>
> Although the source weak_ptr<Derived> does contain a raw pointer to
> Derived, we can't just get it and (up)cast it to Base, as that will
> dereference the pointer in case Base is a virtual base class of Derived.
>
> We don't know if the managed object is still alive, and therefore if
> this operation is safe to do; we therefore temporarily lock() the source
> weak_ptr, do the cast using the resulting shared_ptr, and then discard
> this shared_ptr. Simply checking the strong counter isn't sufficient,
> because if multiple threads are involved then we'd have a race / TOCTOU
> problem; the object may get destroyed after we check the strong counter
> and before we cast the pointer.
>
> However lock() is not necessary if we know that Base is *not* a virtual
> base class of Derived; in this case we can avoid the relatively
> expensive call to lock() and just cast the pointer. This commit uses
> the newly added builtin to detect this case and optimize std::weak_ptr's
> converting constructors and assignment operations.
>
> Apart from non-virtual bases, there's also another couple of interesting
> cases where we can also avoid locking. Specifically:
>
> 1) converting a weak_ptr<T[N]> to a weak_ptr<T cv[]>;
> 2) converting a weak_ptr<T*> to a weak_ptr<T const * const> or similar.
>
> Since this logic is going to be used by multiple places, I've
> centralized it in a new static helper.
>
> libstdc++-v3/ChangeLog:
>
> * include/bits/shared_ptr_base.h (__weak_ptr): Avoid calling
> lock() when converting or assigning a weak_ptr<Derived> to
> a weak_ptr<Base> in case Base is not a virtual base of Derived.
> This logic is centralized in _S_safe_upcast, called by the
> various converting constructors/assignment operators.
> (_S_safe_upcast): New helper function.
> * testsuite/20_util/weak_ptr/cons/virtual_bases.cc: New test.
>
> Reviewed-by: Jonathan Wakely <jwak...@redhat.com>
> Reviewed-by: Tomasz Kamiński <tkami...@redhat.com>
> Signed-off-by: Giuseppe D'Angelo <giuseppe.dang...@kdab.com>
> ---
> This was reviewed on forge:
> https://forge.sourceware.org/gcc/gcc-TEST/pulls/41,
> but we have decided to wait until v16 to merge it.
> I have rebased it since, without any issues, and added a line break
> before _S_safe_upcast.
>
> Tested on x86_64-linux locally and powerpc64le-linux.
> OK for trunk?
OK, thanks (and thanks, Giuseppe for the idea and implementation).
> libstdc++-v3/include/bits/shared_ptr_base.h | 53 ++++++++++--
> .../20_util/weak_ptr/cons/virtual_bases.cc | 80 +++++++++++++++++++
> 2 files changed, 126 insertions(+), 7 deletions(-)
> create mode 100644
> libstdc++-v3/testsuite/20_util/weak_ptr/cons/virtual_bases.cc
>
> diff --git a/libstdc++-v3/include/bits/shared_ptr_base.h
> b/libstdc++-v3/include/bits/shared_ptr_base.h
> index fb868e7afc3..a961c02e979 100644
> --- a/libstdc++-v3/include/bits/shared_ptr_base.h
> +++ b/libstdc++-v3/include/bits/shared_ptr_base.h
> @@ -2012,6 +2012,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> template<typename _Tp, _Lock_policy _Lp>
> class __weak_ptr
> {
> + public:
> + using element_type = typename remove_extent<_Tp>::type;
> +
> + private:
> template<typename _Yp, typename _Res = void>
> using _Compatible = typename
> enable_if<__sp_compatible_with<_Yp*, _Tp*>::value, _Res>::type;
> @@ -2020,9 +2024,44 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> template<typename _Yp>
> using _Assignable = _Compatible<_Yp, __weak_ptr&>;
>
> - public:
> - using element_type = typename remove_extent<_Tp>::type;
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wc++17-extensions" // if constexpr
> + // Helper for construction/assignment:
> + template<typename _Yp>
> + static element_type*
> + _S_safe_upcast(const __weak_ptr<_Yp, _Lp>& __r)
> + {
> + // We know that _Yp and _Tp are compatible, that is, either
> + // _Yp* is convertible to _Tp* or _Yp is U[N] and _Tp is U cv [].
> +
> + // If _Yp is the same as _Tp after removing extents and cv
> + // qualifications, there's no pointer adjustments to do. This
> + // also allows us to support incomplete types.
> + using _At = typename remove_cv<typename
> remove_extent<_Tp>::type>::type;
> + using _Bt = typename remove_cv<typename
> remove_extent<_Yp>::type>::type;
> + if constexpr (is_same<_At, _Bt>::value)
> + return __r._M_ptr;
> + // If they're not the same type, but they're both scalars,
> + // we again don't need any adjustment. This allows us to support
> e.g.
> + // pointers to a differently cv qualified type X.
> + else if constexpr (__and_<is_scalar<_At>, is_scalar<_Bt>>::value)
> + return __r._M_ptr;
> +#if _GLIBCXX_USE_BUILTIN_TRAIT(__builtin_is_virtual_base_of)
> + // If _Tp is not a virtual base class of _Yp, the pointer
> + // conversion does not require dereferencing __r._M_ptr; just
> + // rely on the implicit conversion.
> + else if constexpr (!__builtin_is_virtual_base_of(_Tp, _Yp))
> + return __r._M_ptr;
> +#endif
> + // Expensive path; must lock() and do the pointer conversion while
> + // a shared_ptr keeps the pointee alive (because we may need
> + // to dereference).
> + else
> + return __r.lock().get();
> + }
> +#pragma GCC diagnostic pop
>
> + public:
> constexpr __weak_ptr() noexcept
> : _M_ptr(nullptr), _M_refcount()
> { }
> @@ -2047,8 +2086,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> // in multithreaded programs __r._M_ptr may be invalidated at any
> point.
> template<typename _Yp, typename = _Compatible<_Yp>>
> __weak_ptr(const __weak_ptr<_Yp, _Lp>& __r) noexcept
> - : _M_refcount(__r._M_refcount)
> - { _M_ptr = __r.lock().get(); }
> + : _M_ptr(_S_safe_upcast(__r)), _M_refcount(__r._M_refcount)
> + { }
>
> template<typename _Yp, typename = _Compatible<_Yp>>
> __weak_ptr(const __shared_ptr<_Yp, _Lp>& __r) noexcept
> @@ -2061,7 +2100,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> template<typename _Yp, typename = _Compatible<_Yp>>
> __weak_ptr(__weak_ptr<_Yp, _Lp>&& __r) noexcept
> - : _M_ptr(__r.lock().get()), _M_refcount(std::move(__r._M_refcount))
> + : _M_ptr(_S_safe_upcast(__r)), _M_refcount(std::move(__r._M_refcount))
> { __r._M_ptr = nullptr; }
>
> __weak_ptr&
> @@ -2071,7 +2110,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> _Assignable<_Yp>
> operator=(const __weak_ptr<_Yp, _Lp>& __r) noexcept
> {
> - _M_ptr = __r.lock().get();
> + _M_ptr = _S_safe_upcast(__r);
> _M_refcount = __r._M_refcount;
> return *this;
> }
> @@ -2096,7 +2135,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> _Assignable<_Yp>
> operator=(__weak_ptr<_Yp, _Lp>&& __r) noexcept
> {
> - _M_ptr = __r.lock().get();
> + _M_ptr = _S_safe_upcast(__r);
> _M_refcount = std::move(__r._M_refcount);
> __r._M_ptr = nullptr;
> return *this;
> diff --git a/libstdc++-v3/testsuite/20_util/weak_ptr/cons/virtual_bases.cc
> b/libstdc++-v3/testsuite/20_util/weak_ptr/cons/virtual_bases.cc
> new file mode 100644
> index 00000000000..ac3e4bce9da
> --- /dev/null
> +++ b/libstdc++-v3/testsuite/20_util/weak_ptr/cons/virtual_bases.cc
> @@ -0,0 +1,80 @@
> +// { dg-do run { target c++11 } }
> +
> +#include <memory>
> +#include <testsuite_hooks.h>
> +
> +struct BaseBase { virtual ~BaseBase() = default; };
> +struct Base : BaseBase { virtual ~Base() = default; };
> +struct Derived1 : Base { virtual ~Derived1() = default; };
> +struct Derived2 : virtual Base { virtual ~Derived2() = default; };
> +struct Derived3 : virtual Base { virtual ~Derived3() = default; };
> +struct Derived4 : Derived2, Derived3 { virtual ~Derived4() = default; };
> +struct Derived5 : Derived4 { virtual ~Derived5() = default; };
> +
> +template<typename T>
> +void test01()
> +{
> + std::shared_ptr<T> ptr(new T);
> + VERIFY(ptr);
> +
> + std::weak_ptr<T> wptr1 = ptr;
> + VERIFY(wptr1.lock());
> +
> + std::weak_ptr<Base> wptr2 = ptr;
> + VERIFY(wptr2.lock());
> +
> + std::weak_ptr<Base> wptr3 = wptr1;
> + VERIFY(wptr3.lock());
> +
> + std::weak_ptr<BaseBase> wptr4 = ptr;
> + VERIFY(wptr4.lock());
> +
> + std::weak_ptr<BaseBase> wptr5 = std::move(wptr1);
> + VERIFY(wptr5.lock());
> +
> + ptr.reset();
> +
> + VERIFY(!wptr1.lock());
> + VERIFY(!wptr2.lock());
> + VERIFY(!wptr3.lock());
> + VERIFY(!wptr4.lock());
> + VERIFY(!wptr5.lock());
> +}
> +
> +template<typename T>
> +void test02()
> +{
> + std::shared_ptr<T> ptr(new T);
> + VERIFY(ptr);
> +
> + std::weak_ptr<T> wptr1 = ptr;
> + VERIFY(wptr1.lock());
> +
> + std::weak_ptr<Base> wptr2 = ptr;
> + VERIFY(wptr2.lock());
> +
> + ptr.reset();
> +
> + std::weak_ptr<Base> wptr3 = wptr1;
> + std::weak_ptr<BaseBase> wptr4 = wptr1;
> + std::weak_ptr<BaseBase> wptr5 = std::move(wptr1);
> +
> + VERIFY(!wptr1.lock());
> + VERIFY(!wptr2.lock());
> + VERIFY(!wptr3.lock());
> + VERIFY(!wptr4.lock());
> + VERIFY(!wptr5.lock());
> +}
> +
> +int main()
> +{
> + test01<Derived1>();
> + test01<Derived2>();
> + test01<Derived4>();
> + test01<Derived5>();
> +
> + test02<Derived1>();
> + test02<Derived2>();
> + test02<Derived4>();
> + test02<Derived5>();
> +}
> --
> 2.51.0
>
