libstdc++: optimize weak_ptr converting constructor/assignment

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 <jwakely@redhat.com>
Reviewed-by: Tomasz Kamiński <tkaminsk@redhat.com>
Signed-off-by: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>

diff --git a/libstdc++-v3/include/bits/shared_ptr_base.h b/libstdc++-v3/include/bits/shared_ptr_base.h
index fb868e7afc3677f0f2cfee4790bb217e9b1aabfd..a961c02e97994e3ef85a88e0d78d388cd1f629fd 100644 (file)
--- 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 (file)
index 0000000..ac3e4bc
--- /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>();
+}