Hello, On 04/12/2024 13:20, Giuseppe D'Angelo wrote:
Thank you for the review!I think I've incorporated all the changes, new patch is attached. Some other comments...
By doing some more testing, I've noticed that this patch causes a build issue when a weak_ptr<Incomplete> is used to construct a weak_ptr<const Incomplete>.
I think this is supposed to work as the two pointer types are compatible, but using the trait would require completeness and therefore cause problems.
Here's an amended patch that tries to work around the problem. Since _S_safe_upcast is now a bit complex, I've also added some comments... Thanks, -- Giuseppe D'Angelo
From 22dec25f64efe0f4516c0a561a9a900964d5a51f Mon Sep 17 00:00:00 2001
From: Giuseppe D'Angelo <giuseppe.dang...@kdab.com>
Date: Wed, 2 Oct 2024 18:14:34 +0200
Subject: [PATCH] libstdc++: use is_virtual_base_of in std::weak_ptr operations
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.
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.
Signed-off-by: Giuseppe D'Angelo <giuseppe.dang...@kdab.com>
---
libstdc++-v3/include/bits/shared_ptr_base.h | 47 +++++++++--
.../20_util/weak_ptr/cons/virtual_bases.cc | 80 +++++++++++++++++++
2 files changed, 120 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 bc87c402afb..2f90cb92eab 100644
--- a/libstdc++-v3/include/bits/shared_ptr_base.h
+++ b/libstdc++-v3/include/bits/shared_ptr_base.h
@@ -1984,6 +1984,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;
@@ -1992,9 +1996,38 @@ _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 _A = typename remove_cv<typename remove_extent<_Tp>::type>::type;
+ using _B = typename remove_cv<typename remove_extent<_Yp>::type>::type;
+ if constexpr (is_same<_A, _B>::value)
+ return __r._M_ptr;
+ else
+#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
+ if constexpr (!__builtin_is_virtual_base_of(_Tp, _Yp))
+ return __r._M_ptr;
+ else
+#endif
+ // Expensive path; must lock() and do the pointer conversion while
+ // a shared_ptr keeps the pointee alive (because we may need
+ // to dereference).
+ return __r.lock().get();
+ }
+#pragma GCC diagnostic pop
+ public:
constexpr __weak_ptr() noexcept
: _M_ptr(nullptr), _M_refcount()
{ }
@@ -2019,8 +2052,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
@@ -2033,7 +2066,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&
@@ -2043,7 +2076,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;
}
@@ -2068,7 +2101,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.34.1
smime.p7s
Description: S/MIME Cryptographic Signature
