-
Notifications
You must be signed in to change notification settings - Fork 12k
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
[clang] __is_trivially_relocatable isn't correct on Windows #69394
Comments
|
@llvm/issue-subscribers-clang-frontend Author: Reid Kleckner (rnk)
As implemented in https://reviews.llvm.org/D119017 and https://reviews.llvm.org/D114732, the `__is_trivially_relocatable` type trait is built on top of `canPassInRegisters`, which was never meant to implement this type trait:
https://github.com/llvm/llvm-project/blob/e90ec58b132a7244bdd8d45dd482fd78fe487f37/clang/lib/AST/Type.cpp#L2649
llvm-project/clang/lib/Sema/SemaDeclCXX.cpp Lines 6909 to 6915 in 340d746
This means There are extensive comments on the original review which I haven't read through, but it seems like the proper solution is to track trivial relocatability as a type trait, and then use that to inform whether things get passed in registers. I think (at risk of misremembering) that I suggested the name of "canPassInRegisters", and what I think I had in mind is that we should name it something intentionally hyper-specific and low-level so that folks don't rely on it for any other purpose, but I guess that didn't work out as planned. I guess it wasn't that great of a plan in the first place. cc @zygoloid @ssbr @zmodem @danakj |
|
I think the logic was: "if a type can be passed in registers, then that means you can move it from one location to another via a trivial copy, because that's what happens when you pass it in registers", but we didn't take into account that the ABI might do that even in cases where it's not correct to do so! Adding a separate but related computation for trivial relocatability seems reasonable to me. If we can implement |
|
Are false negatives for I seem to recall that Rust cares a lot about trivial relocatability, but I don't have a good handle on what they intended to do with it. Their use cases might not tolerate false negatives. @ssbr , can you speak to that? Ultimately, I agree, the best resolution would be to implement |
Related: https://reviews.llvm.org/D119385 (It's just been really hard to put time into it. Also there were some ~events that made me pull away from it, at the time...) As long as it's just a missing optimization that wouldn't otherwise be there, then it's surely fine. But it would not be good if we replaced a check around e.g. trivial copyability, with a check around trivial relocatability, and doing so created a regression.
Sure, let me try to give more detail. First off, yeah, false negatives are OK in principle for C++/Rust interop. The consequence is that C++/Rust interop becomes more painful for that type because it requires calling into C++ for constructors and C++ and Rust's object models, when it comes to relocation and so on, differ in that a Rust assignment / initialization is always a relocation, and a C++ assignment / initialization is always an overloaded operation which runs (potentially user-defined) assignment logic. A type which is trivially relocatable is "safe" to use in Rust in the normal Rust way, using Rust assignment, and Rust references -- it won't invoke C++ object lifetime functions, but this is allowed, because it is And so the consequence of a false negative, here, is that we unnecessarily fall back to the more painful but more general mode of C++/Rust interop, instead of using a more "native" feeling approach. So eliminating the false negative is desirable, but mostly for the same reasons that a true negative is undesirable -- in any case, we'd like things to be known to be trivially relocatable. A perhaps bigger issue from an interop POV is that it differs by platform -- the consequence being that Rust code which assumes trivial relocatability of a type might not be portable to Windows or PS4. I think that by itself is a sufficient reason to want to detach P.S. I've never actually been sure what the PS4 platform is. If it's literally just the PS4, I don't mind so much, but if my PlayStation®5 is included then indeed this is very unfortunate, as that means every single one of my gaming devices are going to be slower than they could be. :'( |
These are the semantics relied upon by BSL, Folly, Qt, Thrust, HPX, Parlay, Abseil, Amadeus; and compatible with the semantics currently relied upon by libc++. User-provided special members now prevent natural trivial relocatability. This is important for Abseil, Amadeus, BSL, Folly, Qt at least, because they assume that the assignment operator "doesn't do anything fancy," so that relocation can be used for `vector::erase`. The `[[clang::trivial_abi]]` attribute now always implies trivial relocatability, even on platforms where such types aren't physically passed in registers. Vice versa, just because a type is passed in registers (e.g. on Windows) doesn't automatically mean that it's trivially relocatable. This is llvm#69394. Add a lot of new tests for both old and new behaviors. Test aggregation of both kinds of types as data members; inheritance; virtual member functions and virtual bases; const and reference data members; reference types. Fixes llvm#69394
These are the semantics relied upon by BSL, Folly, Qt, Thrust, HPX, Parlay, Abseil, Amadeus; and compatible with the semantics currently relied upon by libc++. User-provided special members now prevent natural trivial relocatability. This is important for Abseil, Amadeus, BSL, Folly, Qt at least, because they assume that the assignment operator "doesn't do anything fancy," so that relocation can be used for `vector::erase`. The `[[clang::trivial_abi]]` attribute now always implies trivial relocatability, even on platforms where such types aren't physically passed in registers. Vice versa, just because a type is passed in registers (e.g. on Windows) doesn't automatically mean that it's trivially relocatable. This is llvm#69394. Add a lot of new tests for both old and new behaviors. Test aggregation of both kinds of types as data members; inheritance; virtual member functions and virtual bases; const and reference data members; reference types. Fixes llvm#69394
These are the semantics relied upon by BSL, Folly, Qt, Thrust, HPX, Parlay, Abseil, Amadeus; and compatible with the semantics currently relied upon by libc++. User-provided special members now prevent natural trivial relocatability. This is important for Abseil, Amadeus, BSL, Folly, Qt at least, because they assume that the assignment operator "doesn't do anything fancy," so that relocation can be used for `vector::erase`. The `[[clang::trivial_abi]]` attribute now always implies trivial relocatability, even on platforms where such types aren't physically passed in registers. Vice versa, just because a type is passed in registers (e.g. on Windows) doesn't automatically mean that it's trivially relocatable. This is llvm#69394. Add a lot of new tests for both old and new behaviors. Test aggregation of both kinds of types as data members; inheritance; virtual member functions and virtual bases; const and reference data members; reference types. Fixes llvm#69394
To resolve llvm#69394, this patch separates trivial-relocatability's logic from `canPassInRegisters` to decide if a type is trivial-relocatable. A type passed in registers doesn't necessarily mean trivial-relocatability of that type(e.g. on Windows) i.e. it gives us an unintended false positive. This change would be beneficial for Abseil since they rely upon these semantics. By these changes now: User-provided special members prevent natural trivial-relocatabilitiy. It's important because Abseil and maybe others assume the assignment operator doesn't have an impact on the trivial-relocatability of a type. In fact, it does have an effect, and with a user-provided assignment operator, the compiler should only accept it as trivial-relocatable if it's implied by the `[[clang::trivial_abi]]` attribute. Just because a type can pass in registers doesn't necessarily mean it's trivial-relocatable. The `[[clang::trivial_abi]]` attribute always implies trivial-relocatability, even if it can't pass in registers. The trait has extensive tests for both old and new behaviors. Test aggregation of both kinds of types as data members; inheritance; virtual member functions and virtual bases; const and reference data members; and reference types. Fixes llvm#69394
To resolve llvm#69394, this patch separates trivial-relocatability's logic from `canPassInRegisters` to decide if a type is trivial-relocatable. A type passed in registers doesn't necessarily mean trivial-relocatability of that type(e.g. on Windows) i.e. it gives us an unintended false positive. This change would be beneficial for Abseil since they rely upon these semantics. By these changes now: User-provided special members prevent natural trivial-relocatabilitiy. It's important because Abseil and maybe others assume the assignment operator doesn't have an impact on the trivial-relocatability of a type. In fact, it does have an effect, and with a user-provided assignment operator, the compiler should only accept it as trivial-relocatable if it's implied by the `[[clang::trivial_abi]]` attribute. Just because a type can pass in registers doesn't necessarily mean it's trivial-relocatable. The `[[clang::trivial_abi]]` attribute always implies trivial-relocatability, even if it can't pass in registers. The trait has extensive tests for both old and new behaviors. Test aggregation of both kinds of types as data members; inheritance; virtual member functions and virtual bases; const and reference data members; and reference types. Fixes llvm#69394
To resolve llvm#69394, this patch separates trivial-relocatability's logic from `canPassInRegisters` to decide if a type is trivial-relocatable. A type passed in registers doesn't necessarily mean trivial-relocatability of that type(e.g. on Windows) i.e. it gives us an unintended false positive. This change would be beneficial for Abseil since they rely upon these semantics. By these changes now: User-provided special members prevent natural trivial-relocatabilitiy. It's important because Abseil and maybe others assume the assignment operator doesn't have an impact on the trivial-relocatability of a type. In fact, it does have an effect, and with a user-provided assignment operator, the compiler should only accept it as trivial-relocatable if it's implied by the `[[clang::trivial_abi]]` attribute. Just because a type can pass in registers doesn't necessarily mean it's trivial-relocatable. The `[[clang::trivial_abi]]` attribute always implies trivial-relocatability, even if it can't pass in registers. The trait has extensive tests for both old and new behaviors. Test aggregation of both kinds of types as data members; inheritance; virtual member functions and virtual bases; const and reference data members; and reference types. Fixes llvm#69394
To resolve llvm#69394, this patch separates trivial-relocatability's logic from `canPassInRegisters` to decide if a type is trivial-relocatable. A type passed in registers doesn't necessarily mean trivial-relocatability of that type(e.g. on Windows) i.e. it gives us an unintended false positive. This change would be beneficial for Abseil since they rely upon these semantics. By these changes now: User-provided special members prevent natural trivial-relocatabilitiy. It's important because Abseil and maybe others assume the assignment operator doesn't have an impact on the trivial-relocatability of a type. In fact, it does have an effect, and with a user-provided assignment operator, the compiler should only accept it as trivial-relocatable if it's implied by the `[[clang::trivial_abi]]` attribute. Just because a type can pass in registers doesn't necessarily mean it's trivial-relocatable. The `[[clang::trivial_abi]]` attribute always implies trivial-relocatability, even if it can't pass in registers. The trait has extensive tests for both old and new behaviors. Test aggregation of both kinds of types as data members; inheritance; virtual member functions and virtual bases; const and reference data members; and reference types. Fixes llvm#69394
These are the semantics relied upon by BSL, Folly, Qt, Thrust, HPX, Parlay, Abseil, Amadeus; and compatible with the semantics currently relied upon by libc++. User-provided special members now prevent natural trivial relocatability. This is important for Abseil, Amadeus, BSL, Folly, Qt at least, because they assume that the assignment operator "doesn't do anything fancy," so that relocation can be used for `vector::erase`. The `[[clang::trivial_abi]]` attribute now always implies trivial relocatability, even on platforms where such types aren't physically passed in registers. Vice versa, just because a type is passed in registers (e.g. on Windows) doesn't automatically mean that it's trivially relocatable. This is llvm#69394. Add a lot of new tests for both old and new behaviors. Test aggregation of both kinds of types as data members; inheritance; virtual member functions and virtual bases; const and reference data members; reference types.
As implemented in https://reviews.llvm.org/D119017 and https://reviews.llvm.org/D114732, the
__is_trivially_relocatabletype trait is built on top ofcanPassInRegisters, which was never meant to implement this type trait:llvm-project/clang/lib/AST/Type.cpp
Line 2649 in e90ec58
canPassInRegistersis full of various size checks that have nothing to do with trivial relocatability:llvm-project/clang/lib/Sema/SemaDeclCXX.cpp
Lines 6909 to 6915 in 340d746
This means
__is_trivial_relocatabilitydoesn't work correctly on Windows or PS4.There are extensive comments on the original review which I haven't read through, but it seems like the proper solution is to track trivial relocatability as a type trait, and then use that to inform whether things get passed in registers.
I think (at risk of misremembering) that I suggested the name of "canPassInRegisters", and what I think I had in mind is that we should name it something intentionally hyper-specific and low-level so that folks don't rely on it for any other purpose, but I guess that didn't work out as planned. I guess it wasn't that great of a plan in the first place.
cc @zygoloid @ssbr @zmodem @danakj
The text was updated successfully, but these errors were encountered: