Details
- Reviewers
rsmith Bhramar.vatsa rjmccall rnk - Group Reviewers
Restricted Project - Commits
- rG7846d590033e: Extend the C++03 definition of POD to include defaulted functions
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
@aaron.ballman is this something you're comfortable reviewing or could recommend anyone else who might be suitable?
Oh wow that's an awful lot of pings without any response; I'm very sorry you had that experience, so thank you for tagging me to try to get this unstuck!
The precommit CI test failures definitely look relevant and should be fixed up.
| clang/include/clang/Basic/LangOptions.def | ||
|---|---|---|
| 219 ↗ | (On Diff #433472) | |
| clang/include/clang/Driver/Options.td | ||
| 1116 ↗ | (On Diff #433472) | Lol, well that's going to be fun when said out loud -- I come up with "defaulted smurfs are pod". :-D |
| 1118–1119 ↗ | (On Diff #433472) | |
| clang/lib/Driver/ToolChains/Clang.cpp | ||
| 5592 ↗ | (On Diff #433472) | Are we going to get duplicates passed to -cc1, as we also do: https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L5645 |
| 5594 ↗ | (On Diff #433472) | Is Clang 13 still the correct thing to test for here, or should this be 16 these days? |
Still waiting for the pre-merge checks to complete, but hopefully this is clean now.
Realized maybe we don't need a separate driver flag for this at all, and rely only on the abi-compat flag? That seems to be how (at least some) other ABI compat changes have been handled, looking at other uses of ClangABI enum values.
There could be more testing than only the indirect result of the packing problem that first inspired this patch. Any suggestions on what might be the most direct way to test whether the type's been considered pod in this sense?
| clang/lib/Driver/ToolChains/Clang.cpp | ||
|---|---|---|
| 5594 ↗ | (On Diff #433472) | Hmm - trunk is currently destined to be released as 16, yeah? So I think the right version would be 15, here. "If you want the old clang's ABI, ask for that, otherwise this is 16's ABI (new)"? Off-by-one error, of sorts. |
Agreed, I think this is the better approach.
There could be more testing than only the indirect result of the packing problem that first inspired this patch. Any suggestions on what might be the most direct way to test whether the type's been considered pod in this sense?
I would have thought use of __is_pod would tell us, but I'm not seeing the behavior described in the test case when using that: https://godbolt.org/z/1vr3MK4KW Oddly, it seems that QualType::isCXX11PODType() doesn't look at PlainOldData at all! What is your expectation as to how the type trait should be behaving?
| clang/test/SemaCXX/class-layout.cpp | ||
|---|---|---|
| 9–16 | No need for -Wno-c++11-extensions on these RUN lines, right (they already specify c++11 specifically, so there are no extensions to warn about)? | |
| 663–665 | ||
There could be more testing than only the indirect result of the packing problem that first inspired this patch. Any suggestions on what might be the most direct way to test whether the type's been considered pod in this sense?
I would have thought use of __is_pod would tell us, but I'm not seeing the behavior described in the test case when using that: https://godbolt.org/z/1vr3MK4KW Oddly, it seems that QualType::isCXX11PODType() doesn't look at PlainOldData at all! What is your expectation as to how the type trait should be behaving?
Ah, yeah, my understanding is the POD we're talking about is basically "POD for the purposes of Itanium layout" - independent of various language definitions of POD.
Which got me searching around in the code for uses of the data - it's used in some warnings, but also in the MSVC layout ( https://github.com/llvm/llvm-project/blob/c8cf669f6025bdf0fc227f3ddbbf3523a5b32f0b/clang/lib/CodeGen/MicrosoftCXXABI.cpp#L1123 and it looks like MSVC proper uses the non-GCC-compatible choice here, so this patch shouldn't apply to MSVC compatibility mode... https://godbolt.org/z/Mvroof8n4 ).
I realized when removing the explicit flag I lost some of the overrides - for PS and Darwin. Since we're checking this property in two places and now it'll involve about 4 different checks (or should all these platforms imply the older ABI? At least that seems correct for PS and Darwin where their ABIs are defined by the older version of clang, but the MSVC case is weirder - since their ABI isn't defined by the older version of clang, and the ABI may need to change as we discover mismatches/mistakes in Clang's ABI implementation)
I guess the other packed behavior/ABI checking 277123376ce08c98b07c154bf83e4092a5d4d3c6
Oh, yeah, seems @rsmith and I discussed this naming/expectations issue a bit over here previously: https://reviews.llvm.org/D117616#inline-1132622
So I could test this other ways that actually impact layout - like whether things can be packed in tail padding (can pack in tail padding for non-pod, right?). Or we could ast dump and inspect the property directly? Maybe there are some other options?
So I could test this other ways that actually impact layout - like whether things can be packed in tail padding (can pack in tail padding for non-pod, right?). Or we could ast dump and inspect the property directly? Maybe there are some other options?
Here's what the tail-padding based testing looks like: https://godbolt.org/z/Te3d4fYjj (though the asserts all fail on MSVC... not sure what size MSVC makes these structs at all)
namespace trailing {
struct t1 {
int x;
char c;
};
struct t2 : t1 {
char c;
};
static_assert(sizeof(t2) == sizeof(int) * 3, "");
} // namespace trailing
namespace trailing_nonpod {
struct t1 {
protected:
int x;
char c;
};
struct t2 : t1 {
char c;
};
static_assert(sizeof(t2) == sizeof(int) * 2, "");
} // namespace trailing_nonpod
namespace trailing_smf_defaulted_pod {
struct t1 {
t1() = default;
int a;
char c;
};
struct t2 : t1 {
char c;
};
static_assert(sizeof(t2) == sizeof(int) * 3, "");
}(GCC passes all these assertions, Clang (without this patch we're reviewing) fails the last of them)
I guess the other way to test for pod-for-purposes-of-ABI is IRgen. Looks like MSVC isn't observable based on sizeof or alignof on these issues (the previous godbolt shows MSVC's answer to alignment for the packed and size for the trailing packing don't change based on any of the variations (pod, non-pod, pod-with-defaulted-special-members)) - but should be observable based on the returning ABI.
Ah, here we go: https://godbolt.org/z/sd88zTjPP
So MSVC does consider the defaulted special member as still a valid pod-for-purposes-of-ABI and clang is incorrect/not ABI compatible with this. So MSVC does want this fix.
Oh, yeah, seems @rsmith and I discussed this naming/expectations issue a bit over here previously: https://reviews.llvm.org/D117616#inline-1132622
Ah, thank you for that!
Yeah this strikes me as the right kind of test to add for testing the ABI (codegen cares more about ABI than Sema, broadly speaking).
| clang/lib/AST/DeclCXX.cpp | ||
|---|---|---|
| 893 | Does this language change much if there are constraints on this method? We might need to consider that if we are breaking ABI here. | |
Add PS4/Darwin handling
Remove unnecessary warning suppression
Add MSVC return ABI testing
Add tail-padding based testing as well as the alignment based testing
Minor correction - this test was incorrect & I think demonstrates more/multiple MS ABI bugs. Looks like MS's ABI considers a NSDMI to still be pod-for-the-purposes-of-returning. So that's another bug (@hansw / @rnk - any interest in that? Want a bug or it or the like - here's a godbolt that shows it more clearly, using a base class instead, which MS does consider non-pod-for-purposes-of-returning: https://godbolt.org/z/aMhbe11a8 )
So using the base class instead (something both clang and MSVC agree on), here's a good demo of the defaulted SMF issue: https://godbolt.org/z/K31vGsejh
So MSVC does consider the defaulted special member as still a valid pod-for-purposes-of-ABI and clang is incorrect/not ABI compatible with this. So MSVC does want this fix.
Adding a test (clang/test/CodeGenCXX/return-abi.cpp) here to test this. Seems that the Itanium ABI doesn't care about pod-ness for return (or parameter) ABI, probably only cares about "trivially copyable" ( https://godbolt.org/z/7n91YqecM ) - so doesn't seem like I can test the Itanium preferences for pod-ness in this codegen test. It seems to only show up in layout.
The quirk of the SemaCXX/class-layout.cpp that it's currently not testing the behavior at 14 and below (because of the packing-non-pod ABI bug that was fixed previously/recently/lead to this work) - I could change the test to use tail padding instead as a way to observe the pod-ness a bit more robustly? What do you think? I added the padding based test as well for comparison - could keep one, or the other, or both.
| clang/lib/AST/DeclCXX.cpp | ||
|---|---|---|
| 893 | Oh, sorry, I should've used more words - but this is intended as an ABI break/fix. It's a place where Clang's ABI implementation is inconsistent with GCC and MSVC - so this is intended to fix/make Clang compatible with those ABIs where we are trying to be compatible with them. (& why we're checking the ClangABI version here - so if you're asking for the old ABI you still get it) Perhaps I'm misunderstanding your questions/concerns? | |
| clang/test/SemaCXX/class-layout.cpp | ||
| 663–665 | This doesn't compile under the first RUN line which uses C++98, I think? | |
| clang/test/SemaCXX/class-layout.cpp | ||
|---|---|---|
| 9–16 | ah, yeah - just need it on the first one | |
| clang/lib/AST/DeclCXX.cpp | ||
|---|---|---|
| 893 | Yeah, my concern is whether this behavior is different with a constraint on it. That is, does the eligibility of the constructor matter for this case? This ctor/copy assign/etc might not pass a constraint check/never be callable. So the question is whether we need to consider that here. | |
Do we need to gate this on use of -fms-compatibility as well? I'm not certain how cygwin factors in where it's sort of gcc and sort of msvc (perhaps the triple is sufficient for that?).
| clang/lib/AST/DeclCXX.cpp | ||
|---|---|---|
| 891 | Ah, it took me a moment to realize that move assignment is called out here but not move construction because move construction is handled above. | |
| clang/test/SemaCXX/class-layout.cpp | ||
| 663–665 | Ah, fair point. I don't have a strong opinion, but you could do -Dstatic_assert=_Static_assert for that RUN line and not use the extension version. But it doesn't matter for the test functionality, so whichever approach sparks the most joy for you is fine by me. | |
Regarding the usage of isPOD in the MSVC ABI code, we should have a routine similar to isTrivialForAArch64MSVC that works for the other MSVC architectures that we support (x86, x64, and arm32). I don't think we should try to rely on definition data bits. Clang is almost always tracking something slightly different than what MSVC tracks, so we should write down MSVC's logic once and keep it separate from Clang's notion of POD.
I think you can work out MSVC's behavior with godbolt, but if you need a hand, I volunteer @hans to take a stab at refactoring MicrosoftCXXABI::classifyReturnType. I remember making lots of comments to try to simplify the code during review, but everyone is always wary of scope creep.
Well, my first attempt at this - essentially using isTrivialForAArch64MSVC for everything, including x86, doesn't fail any Clang tests...
Do we still have those various fuzzers Majnemer setup? I'd guess we'd need to fuzz around here again to discover what the intended behavior is?
Hmm, let's compare the things that make PlainOldData false...
- has base classes
- any virtual member function
- non-implicit ctor (so even user-declared-but-defaulted ctor)
- non-implicit member function (so even user-declared-but-defaulted assignment operators or dtor)
- any non-public field member
- some ObjC thing (non-trivial ObjC lifetime)
- any field that is of C++98 POD type... (mostly a recursive definition, when it comes to record types - but then there's some rules about incomplete array types, which couldn't be field types anyway, and some other cases)
- non-static data member initializers
Let's compare that with isTrivialForAArch64MSVC...
- checked (second)
- checked (third) - well, it checks "is polymorphic" rather than virtual functions, so does that change things if you've got virtual bases but no virtual functions, can you have that?
- only user-provided ctors, so it can have non-implicit (user-declared) but defaulted ctors
- specifically checking non-trivial copy assignment (no restriction on move assignment and it could have any number of other member functions, I think?)
- checked (first)
- nope/not relevant?
- Presumably tested by "hasNonTrivialDestructor" and "hasNonTrivialCopyAssignment"?
- would a non-static data member initializer make the ctor user-provided? Presumably not, so I don't think this property is checked at all, which seems fair - how you pass something shouldn't be changed by how its constructed
I'll test the cases where these things differ with the godbolt-like testing earlier...
Each godbolt link shows MSVC 19.latest and clang, both in x86 and aarch64, with a reference pod and non-pod example at the start and end, and the variable test in the middle to see whether the codegen matches the pod or non-pod baselines above and below.
- checked (second): has base classes
https://godbolt.org/z/d76fEGP1M - all agreed
- checked (third): virtual functions/polymorphic
Oh, I guess if you can't have base classes then testing 'is polymorphic' is equivalent to 'has virtual functions'
This one's slightly less obvious to verify due to the existence of a virtual function changing the codegen of the f1 test functions so comparing it to the pod/non-pod reference & maybe me think about how to better isolate the test - though I don't think it's possible to totally isolate it, adding a virtual functions adds a vptr that has to be copied ,etc and extra globals for the vtalbe itself (MSVC doesn't home vtables), etc.
So I've got this: https://godbolt.org/z/nGa4drG4h - but it looks like they all agree, despite the extra variations in the output
- only user-provided ctors, so it can have non-implicit (user-declared) but defaulted ctors
User-defined ctor (t1(int) unrelated to any usage/copy operation, etc): https://godbolt.org/z/e79b1zeqr - everyone agrees
User-declared-but-not-user-defined ctor (t1() = default): Clang x86 is incorrect, believes it to be non-trivial - that's the original issue under discussion in this bug
- specifically checking non-trivial copy assignment (no restriction on move assignment and it could have any number of other member functions, I think?)
Oh, everyone allows a non-trivial non-special member function, I misread the Clang code - those don't affect retro-POD either, so everyone agrees on that. https://godbolt.org/z/hnMfqjov7
Restricting this to Special Member Functions, defaulted copy constructor... https://godbolt.org/z/sGvxcEPjo Clang x86 is incorrect
- checked (first)
Used this as the baseline - everyone agrees.
- nope/not relevant?
- Presumably tested by "hasNonTrivialDestructor" and "hasNonTrivialCopyAssignment"?
Having a field of a type with a protected member (property 5) - Clang x86 is incorrect: https://godbolt.org/z/GY48qxh3G
Having a field of a type with a non-trivial dtor - everyone agrees: https://godbolt.org/z/4dq9b43ac
Having a field of a type with a non-trivial copy-assignment - everyone agrees: https://godbolt.org/z/WEnKcz7oW
- would a non-static data member initializer make the ctor user-provided? Presumably not, so I don't think this property is checked at all, which seems fair - how you pass something shouldn't be changed by how its constructed
Non-static data member initializer: https://godbolt.org/z/Mb1PYhjrP - Clang x86 is incorrect
So... my conclusion is that Clang's AArch64 appears to be correct for x86 as well, and we should just rename the function and use it unconditionally, removing any use of Clang's AST POD property in the MSVC ABI handling?
Sounds good to me, I think you did the hard work of verifying, thanks for that. :)
Anyway, removing this isPOD usage should be it's own patch, with tests. We should already have test coverage for aarch64 that can be reused. @ayzhao, can you help David with this? This is not C++20-related, but it is clang frontend related.
Remove Microsoft return ABI test, now that the Microsoft ABI implementation no longer depends on the AST isPOD property
Popping the stack, the use of AST's isPOD in Microsoft's ABI has been removed in favor of more nuanced custom implementation in the Microsoft ABI handling that fixed a few bugs along the way (D133817, D134688).
I've rebased this patch and addressed a test failure I hadn't spotted before, in clang/test/AST/conditionally-trivial-smfs.cpp - added some details to the patch description.
And now that the Microsoft ABI doesn't depend on the AST isPOD property at all, I've removed the previously proposed new test clang/test/CodeGenCXX/return-abi.cpp that was testing the Microsoft return ABI.
Relatedly, if we put this POD behavior query on TargetInfo, I wonder if it makes sense to move the tail padding predicate from TargetCXXABI to TargetInfo:
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/TargetCXXABI.h#L282
The switch there is basically a switch over architectures.
| clang/lib/AST/DeclCXX.cpp | ||
|---|---|---|
| 774–775 | I think this ought to be factored into a TargetInfo method, so we can share the logic here and below somehow. Compare this for example with TargetInfo::getCallingConvKind, which has a similar purpose. | |
Hmm, looked into this - but isn't most of TargetCXXABI a switch over CXXABIs, which are almost architectures/operating systems? Why is tail padding predicate different than the other tests in TargetCXXABI?
But I took a look at changing this in D135326 in case you want to check if this is what you had in mind - I'm not entirely sure it's better. The OS seems not be quite 1:1 with the CXXABI groupings we have in TargetCXXABI... so one end result could just be calling back into the ABI to get the CXXABI kind and then returning based on that, which doesn't seem to be better...
| clang/lib/AST/DeclCXX.cpp | ||
|---|---|---|
| 774–775 | Seems plausible - though the version is stored in LangOpts, which isn't currently plumbed through into TargetInfo - should it be plumbed through, or is there some layering thing there where targets shouldn't depend on lang opts? Looks like it'd mostly involve passing LangOpts down here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/CompilerInstance.cpp#L107 and plumbing it through all the TargetInfo ctors, possibly either storing LangOpts& in the TargetInfo, or computing the DefaultedSMFArePOD property in the ctor and storing that as a bool member in TargetInfo to return from some query function to be added to that hierarchy. Or I guess like getOSDefines have getDefaultedSMFArePOD takes LangOptions as a parameter? | |
| clang/lib/AST/DeclCXX.cpp | ||
|---|---|---|
| 774–775 | My main concern is keeping complex target-specific conditions out of DeclCXX to improve readability. Any way to achieve that sounds good to me. I think I'd lean towards passing LangOpts as a parameter. After that, storing DefaultedSMFArePOD on TargetInfo as a bool sounds good. You can set the field in TargetInfo::adjust to read LangOpts, and then override that in PS & Darwin specific ::adjust method overrides. | |
| clang/lib/AST/DeclCXX.cpp | ||
|---|---|---|
| 774–775 | Fair enough - tried this out with a LongOpts parameter (I guess getCallingConvKind passes in just the boolean about whether it's the right ABI compatibility version - requiring the caller to check, but that seems a bit weird/specific? So I prefer passing in the whole LangOpts here and querying the right property inside TargetInfo) | |
+#clang-vendors for ABI changes
| clang/include/clang/Basic/TargetInfo.h | ||
|---|---|---|
| 1569 | Please add a doc comment for this, with some history about how previously explicitly defaulting certain special members would make types non-POD, and that changing what is POD can change record layout, so some targets return false to opt for the old, stable behavior. | |
| clang/include/clang/Basic/TargetInfo.h | ||
|---|---|---|
| 1569 | Done | |
Hmm - pinging this since my last comment didn't seem to send mail (at least not that appeared on llvm-commits archive)
Ping (hoping to ensure enough progress is made on this that we can get it in before the LLVM 16 branch - since it's complicated the last couple of releases already due to not having all the fallout of the ABI fixes in together)
I realized I forgot some things I should've mentioned:
- This probably deserves a release note, if it doesn't have one already that I missed or forgot about
- We should tag #clang-vendors, since this does change ABI. You've already done the work of guarding it with -fclang-abi-compat, so that should be OK, we just need to use the notification mechansim.
I noticed in a downstream that this changes how we emit structs to IR sometimes (see https://godbolt.org/z/74xeq9rTj for an example, but the 'no-opaque-pointers' isn't necessary to cause this, just anything that causes emission of the struct).
Are we OK with that? Is the 'padding' arrays important in any way?
The actual layout is different, it now agrees with GCC, see:
https://godbolt.org/z/EK7Tb6YTT
This is actually a quite significant ABI change, and there are various mechanisms to stay on the old ABI.
e4ec6ce8a75c208b49b163c81cda90dc7373c791
- We should tag #clang-vendors, since this does change ABI. You've already done the work of guarding it with -fclang-abi-compat, so that should be OK, we just need to use the notification mechansim.
Yep yep - thanks @aaron.ballman for adding that.
Yep, +1 to that. (another dimension to look at, in terms of "is this the right LLVM IR" - basically the GCC Itanium interpretation is that defaulted special members shouldn't, themselves, change the ABI - so comparing Clang with/without the defaulted special member does (now, with the fix applied) have the same IR: https://godbolt.org/z/88WKxP6G9 )