[v2] Re: libstdc++: use is_virtual_base_of in std::weak_ptr operations
Checks
Commit Message
On 03/12/2024 18:02, Jonathan Wakely wrote:
> 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!
>>
Thank you for the review!
I think I've incorporated all the changes, new patch is attached. Some
other comments...
>> We would usually use _S_safe_upcast for the name of a static member
>> function, although I think __safe_upcast is OK here.
Well, I've renamed the static helper, while at it.
>> 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.
Sure, it's a tradeoff between re-checking versus having an internal API
that might get accidentally misused (if there is no check).
[snip]
> We should probably even be using the _GLIBCXX_USE_BUILTIN_TRAIT macro
> here, so that its use can be disabled.
Out of curiosity, are there some docs regarding what libstdc++ does
require out of the compiler? For instance, in this case, I know that
both GCC and Clang trunk have the builtin (with the same name), and I
wouldn't expect this patch to be cherry-picked into old/stable branches
anyhow. Is new libstdc++ + "old" compiler a supported combination? (I
guess it may happen on a Linux distro.) But up to which point? Etc.
Thanks,
--
Giuseppe D'Angelo
Comments
On Wed, 4 Dec 2024 at 12:20, Giuseppe D'Angelo
<giuseppe.dangelo@kdab.com> wrote:
>
> On 03/12/2024 18:02, Jonathan Wakely wrote:
> > 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!
> >>
>
>
> Thank you for the review!
>
> I think I've incorporated all the changes, new patch is attached. Some
> other comments...
>
> >> We would usually use _S_safe_upcast for the name of a static member
> >> function, although I think __safe_upcast is OK here.
>
> Well, I've renamed the static helper, while at it.
Thanks.
> >> 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.
>
> Sure, it's a tradeoff between re-checking versus having an internal API
> that might get accidentally misused (if there is no check).
If you wanted to be sure, a static_assert in the function body would
prevent misuse but I think it should be cheaper to compile than
something that is checked during overload resolution and template
argument deduction.
Is the risk that calling _S_safe_upcast with weak_ptr<Derived[]> would
give us back a Base* where it shouldn't? I think that upcast would
still be "safe", it's just that we should never be in a situation
where that conversion is needed, because converting
weak_ptr<Derived[]> to weak_ptr<Base[]> isn't allowed. Is there
another form of accidental misuse that would be unsafe, rather than
just unecessary?
> [snip]
> > We should probably even be using the _GLIBCXX_USE_BUILTIN_TRAIT macro
> > here, so that its use can be disabled.
>
> Out of curiosity, are there some docs regarding what libstdc++ does
> require out of the compiler? For instance, in this case, I know that
> both GCC and Clang trunk have the builtin (with the same name), and I
> wouldn't expect this patch to be cherry-picked into old/stable branches
> anyhow.
Clang trunk has it, but no released version does.
> Is new libstdc++ + "old" compiler a supported combination? (I
> guess it may happen on a Linux distro.) But up to which point? Etc.
Yes. We support using libstdc++ with "older" versions of Clang. We
only support using libstdc++ headers with the matching version of GCC
(because there's no good reason you would ever have newer headers and
not have been able to also install the newer GCC, that's just not a
thing we need to support). But we can't require that all consumers of
libstdc++ headers are using the latest Clang, so we support
approximately the most recent 3 Clang releases, so currently that
would mean Clang 17, 18 and 19. None of those support the built-in
yet. By the time GCC 15 is released, there will probably be a Clang 20
with support for the built-in, but we probably can't rely on it being
"always" available until GCC 16 at the earliest. And we don't know
when it will be supported by Intel icx (how closely does that track
LLVM main? ... I haven't bothered to check because using the
__has_builtin checks means we don't really need to care).
So we need to always guard uses of new (and even not-so-new) built-ins
with feature checks, i.e. __has_builtin or _GLIBCXX_USE_BUILTIN_TRAIT.
There are no docs on exactly what we require, because it would keep
changing, and because using feature checks means there are no hard
requirements. Libstdc++ should gracefully adapt to work with whatever
the compiler supports, disabling certain features (like
std::is_virtual_base_of) as needed. The feature test macros in
<version> make it possible for users to deal with those
inconsistencies in a portable and version-agnostic way.
An additional benefit of using _GLIBCXX_USE_BUILTIN_TRAIT is that it
makes it easier to bisect when looking for compiler regressions. If
you use -D_GLIBCXX_DO_NOT_USE_BUILTIN_TRAITS to create preprocessed
output then you get something that doesn't depend on brand new
built-ins, and so can be tested with older versions of GCC. That was a
recurring pain point before we introduced the macro wrapping
__has_builtin.
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 196185cb2b1526719b2cc8719d252bf050eea2d9 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.
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.dangelo@kdab.com>
---
libstdc++-v3/include/bits/shared_ptr_base.h | 25 ++++--
.../20_util/weak_ptr/cons/virtual_bases.cc | 80 +++++++++++++++++++
2 files changed, 100 insertions(+), 5 deletions(-)
create mode 100644 libstdc++-v3/testsuite/20_util/weak_ptr/cons/virtual_bases.cc
@@ -1992,6 +1992,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
template<typename _Yp>
using _Assignable = _Compatible<_Yp, __weak_ptr&>;
+ // Helper for construction/assignment:
+ template<typename _Yp>
+ static _Tp* _S_safe_upcast(const __weak_ptr<_Yp, _Lp>& __r)
+ {
+#if _GLIBCXX_USE_BUILTIN_TRAIT(__builtin_is_virtual_base_of)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wc++17-extensions" // if constexpr
+ if constexpr (!__builtin_is_virtual_base_of(_Tp, _Yp))
+ return __r._M_ptr;
+ else
+#pragma GCC diagnostic pop
+#endif
+ return __r.lock().get();
+ }
+
public:
using element_type = typename remove_extent<_Tp>::type;
@@ -2019,8 +2034,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 +2048,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 +2058,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 +2083,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;
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