libstdc++: use is_virtual_base_of in std::weak_ptr operations
Checks
Commit Message
Hello,
The attached patch changes std::weak_ptr "converting move
constructor/assignment" -- that is, from a rvalue weak_ptr<Derived> to a
weak_ptr<Base>.
In the general case, such conversion requires lock()ing the weak_ptr
because the pointed-to object might have already been destroyed, and a
Derived->Base pointer conversion requires dereferencing the pointer iff
Base is a virtual base class of Derived. Therefore the correct thing to
do in the general case is to lock() the weak_ptr, call get() on the
resulting shared_ptr, and do the pointer conversion while this
shared_ptr is alive.
However, the newly added __is_virtual_base_of builtin allows us to
micro-optimize this pointer upcast, and avoid calling lock() (and paying
for the associated atomic operations) in case Base is *not* a virtual
base class of Derived. In this case we can "just" perform the upcast;
as far as I know, no ABI requires dereferencing the pointer in the
non-virtual inheritance case.
Thanks,
--
Giuseppe D'Angelo
Comments
On Tue, 3 Dec 2024 at 16:19, Giuseppe D'Angelo
<giuseppe.dangelo@kdab.com> wrote:
>
> Hello,
>
> The attached patch changes std::weak_ptr "converting move
> constructor/assignment" -- that is, from a rvalue weak_ptr<Derived> to a
> weak_ptr<Base>.
>
> In the general case, such conversion requires lock()ing the weak_ptr
> because the pointed-to object might have already been destroyed, and a
> Derived->Base pointer conversion requires dereferencing the pointer iff
> Base is a virtual base class of Derived. Therefore the correct thing to
> do in the general case is to lock() the weak_ptr, call get() on the
> resulting shared_ptr, and do the pointer conversion while this
> shared_ptr is alive.
>
> However, the newly added __is_virtual_base_of builtin allows us to
> micro-optimize this pointer upcast, and avoid calling lock() (and paying
> for the associated atomic operations) in case Base is *not* a virtual
> base class of Derived. In this case we can "just" perform the upcast;
> as far as I know, no ABI requires dereferencing the pointer in the
> non-virtual inheritance case.
This is a nice optimization!
We would usually use _S_safe_upcast for the name of a static member
function, although I think __safe_upcast is OK here.
Do we need the _Compatible constraint on __safe_upcast? It's only
called internally from code that has already enforced that constraint,
so checking again just slows compilation down.
I've been moving from using `if _GLIBCXX17_CONSTEXPR` to just
surrounding the function in diagnostic pragmas so that we can use `if
constexpr` unconditionally, i.e.
#pragma GCC diagnostic push
#pragma GCC diagnostic pushignored "-Wc++17-extensions" // if constexpr
...
#pragma GCC diagnostic pop
That way we get the benefits of `if constexpr` even in C++11 mode.
We still have lots of code using _GLIBCXX17_CONSTEXPR but I think we
should phase it out in favour of using `if constexpr` unconditionally
(or at least using `if _GLIBCXX_CONSTEXPR` for code which needs to
compile as C++98 too).
On Tue, 3 Dec 2024 at 16:56, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On Tue, 3 Dec 2024 at 16:19, Giuseppe D'Angelo
> <giuseppe.dangelo@kdab.com> wrote:
> >
> > Hello,
> >
> > The attached patch changes std::weak_ptr "converting move
> > constructor/assignment" -- that is, from a rvalue weak_ptr<Derived> to a
> > weak_ptr<Base>.
> >
> > In the general case, such conversion requires lock()ing the weak_ptr
> > because the pointed-to object might have already been destroyed, and a
> > Derived->Base pointer conversion requires dereferencing the pointer iff
> > Base is a virtual base class of Derived. Therefore the correct thing to
> > do in the general case is to lock() the weak_ptr, call get() on the
> > resulting shared_ptr, and do the pointer conversion while this
> > shared_ptr is alive.
> >
> > However, the newly added __is_virtual_base_of builtin allows us to
> > micro-optimize this pointer upcast, and avoid calling lock() (and paying
> > for the associated atomic operations) in case Base is *not* a virtual
> > base class of Derived. In this case we can "just" perform the upcast;
> > as far as I know, no ABI requires dereferencing the pointer in the
> > non-virtual inheritance case.
>
> This is a nice optimization!
>
> We would usually use _S_safe_upcast for the name of a static member
> function, although I think __safe_upcast is OK here.
>
> Do we need the _Compatible constraint on __safe_upcast? It's only
> called internally from code that has already enforced that constraint,
> so checking again just slows compilation down.
>
> I've been moving from using `if _GLIBCXX17_CONSTEXPR` to just
> surrounding the function in diagnostic pragmas so that we can use `if
> constexpr` unconditionally, i.e.
>
> #pragma GCC diagnostic push
> #pragma GCC diagnostic pushignored "-Wc++17-extensions" // if constexpr
> ...
> #pragma GCC diagnostic pop
>
> That way we get the benefits of `if constexpr` even in C++11 mode.
>
> We still have lots of code using _GLIBCXX17_CONSTEXPR but I think we
> should phase it out in favour of using `if constexpr` unconditionally
> (or at least using `if _GLIBCXX_CONSTEXPR` for code which needs to
> compile as C++98 too).
Ah, I think we also need to guard the use of the built-in with a
preprocessor check to ensure it's available. So maybe invert the sense
of the condition, so that the fast path is only present when the
built-in is supported:
#if __has_builtin(__builtin_is_virtual_base_of)
if constexpr (!__builtin_is_virtual_base_of(_Tp, _Yp))
return __r._M_ptr;
else
#endif
return __r.lock().get();
We should probably even be using the _GLIBCXX_USE_BUILTIN_TRAIT macro
here, so that its use can be disabled.
From c67349ec3ac53573a5a36edeedaea9e8f5e41c97 Mon Sep 17 00:00:00 2001
From: Giuseppe D'Angelo <giuseppe.dangelo@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.
* testsuite/20_util/weak_ptr/cons/virtual_bases.cc: New test.
Signed-off-by: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
---
libstdc++-v3/include/bits/shared_ptr_base.h | 19 +++--
.../20_util/weak_ptr/cons/virtual_bases.cc | 80 +++++++++++++++++++
2 files changed, 94 insertions(+), 5 deletions(-)
create mode 100644 libstdc++-v3/testsuite/20_util/weak_ptr/cons/virtual_bases.cc
@@ -1992,6 +1992,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
template<typename _Yp>
using _Assignable = _Compatible<_Yp, __weak_ptr&>;
+ // Helper for construction/assignment:
+ template<typename _Yp, typename = _Compatible<_Yp>>
+ static _Tp* __safe_upcast(const __weak_ptr<_Yp, _Lp> &__r)
+ {
+ if _GLIBCXX17_CONSTEXPR (__builtin_is_virtual_base_of(_Tp, _Yp))
+ return __r.lock().get();
+ return __r._M_ptr;
+ }
+
public:
using element_type = typename remove_extent<_Tp>::type;
@@ -2019,8 +2028,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(__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 +2042,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(__safe_upcast(__r)), _M_refcount(std::move(__r._M_refcount))
{ __r._M_ptr = nullptr; }
__weak_ptr&
@@ -2043,7 +2052,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_Assignable<_Yp>
operator=(const __weak_ptr<_Yp, _Lp>& __r) noexcept
{
- _M_ptr = __r.lock().get();
+ _M_ptr = __safe_upcast(__r);
_M_refcount = __r._M_refcount;
return *this;
}
@@ -2068,7 +2077,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_Assignable<_Yp>
operator=(__weak_ptr<_Yp, _Lp>&& __r) noexcept
{
- _M_ptr = __r.lock().get();
+ _M_ptr = __safe_upcast(__r);
_M_refcount = std::move(__r._M_refcount);
__r._M_ptr = nullptr;
return *this;
new file mode 100644
@@ -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