On Wed, 4 Dec 2024 at 12:20, Giuseppe D'Angelo <giuseppe.dang...@kdab.com> wrote: > > On 03/12/2024 18:02, Jonathan Wakely wrote: > > On Tue, 3 Dec 2024 at 16:56, Jonathan Wakely <jwak...@redhat.com> wrote: > >> > >> On Tue, 3 Dec 2024 at 16:19, Giuseppe D'Angelo > >> <giuseppe.dang...@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.
