-
Notifications
You must be signed in to change notification settings - Fork 3k
Remove all attempts to use Clang builtins for trivial relocatability #1985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This simplifies the codepaths after a4950fb. We can't use Clang's current `__builtin_is_cpp_trivially_relocatable(T)` for the same reason we continue to be unable to use Clang's current `__is_trivially_relocatable(T)`: it returns true for polymorphic types and other types that aren't trivially relocatable in the Abseil/Folly/P1144 sense. We use memcpy for relocation; we use relocation to implement InlineVector::erase and InlineVector::swap. So, as the comment says, we care about wonky assignment as well as wonky copy-construction or destruction. Use the public (not private/compiler-builtin!) API of P1144 wherever it's available. Never use the Clang builtins otherwise. Always fall back to the public (not private/compiler-builtin!) std::is_trivially_copyable when P1144 isn't available. Remove references to P2786; it is not part of C++26 anymore, and even if it were, Abseil couldn't use it, because P2786 ignored move-assignment and also permitted "relocation" to fix up vptrs (which memcpy never does).
|
I agree with removing the Clang builtins given the reported bugs. However, as far as I can tell, Could we please remove the |
For a test case, you can use the code from https://godbolt.org/z/xaa98qxhz , where Abseil version 20250127.0 used to use memmove inside
That would be less buggy than the current post- a4950fb status quo, but Abseil would still not be getting the memmove optimizations when compiled with a P1144-compatible compiler. Getting those optimizations has been the point of patches like #1625, #1632, #1618; it is a shame to break those optimizations now. Using P1144's features on systems that define them today isn't "speculating" — if C++29/32/... ever ships something different, or a second suitable-yet-differently-shaped API ever materializes, then Abseil could simply add an Note it's only these lines of code we're debating over: Which is just, like, "if we have the feature then use it" (otherwise, emulate it). |
|
Regardless of the current compiler support or the performance benefits, trivial relocatability is not currently in the C++26 working draft, relying on this macro effectively treats a proposal as a standard feature. Even if P1144 is the, um, more useful approach, Abseil shouldn't claim support for a standard feature that doesn't legally exist yet. We should stick to the correct, non-speculative |
I'd say it's a "non-standard extension" — that's why you have to place the code under the guard macro rather than just assume it's OK to use on all platforms. I don't see this as fundamentally different from e.g. Abseil's conditional support for However, while e.g. Folly and Parlay are happy to guard their usage directly with (1) Suppose we replace with and then add a comment somewhere, similar to how you do (2) Or, again looking at how Abseil generally does this conditional-support/conditional-extension stuff — in Abseil's attributes.h — suppose you define something in there like and then guard that definition with compiler-support guards, e.g. |
This simplifies the codepaths after a4950fb. We can't use Clang's current
__builtin_is_cpp_trivially_relocatable(T)for the same reason we continue to be unable to use Clang's current__is_trivially_relocatable(T): it returns true for polymorphic types and other types that aren't trivially relocatable in the Abseil/Folly/P1144 sense. We use memcpy for relocation; we use relocation to implement InlineVector::erase and InlineVector::swap. So, as the comment says, we care about wonky assignment as well as wonky copy-construction or destruction.Use the public (not private/compiler-builtin!) API of P1144 wherever it's available. Never use the Clang builtins otherwise. Always fall back to the public (not private/compiler-builtin!) std::is_trivially_copyable when P1144 isn't available.
Remove references to P2786; it is not part of C++26 anymore, and even if it were, Abseil couldn't use it, because P2786 ignored move-assignment and also permitted "relocation" to fix up vptrs (which memcpy never does).
(Fixes #1885 in a different, shorter/correcter, way.)
Where P1144 is available, Abseil 20250127.0 has better performance than current Abseil trunk: i.e. trunk has regressed. This fixes that regression.