[PATCH v2] Re: libstdc++: use is_virtual_base_of in std::weak_ptr operations

[Giuseppe D'Angelo]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libstdc-use-is_virtual_base_of-in-std-weak_ptr-opera.patch
Type: text/x-patch
Size: 6586 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20241204/5ddd2a15/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4244 bytes
Desc: S/MIME Cryptographic Signature
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20241204/5ddd2a15/attachment.p7s>