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
Oh, and demonstrating this applies (as best as I can figure) even when compiling in C++03 mode: https://godbolt.org/z/9eqbThrKs (but perhaps there are otther differences between 03 and 11 POD I could test?)
| clang/lib/AST/RecordLayoutBuilder.cpp | ||
|---|---|---|
| 1892 ↗ | (On Diff #406148) | Maybe irrespective of the field's type itself, only important thing is just if it is a POD type or not? |
Looks like the test fails on the Windows pre-merge bot: https://buildkite.com/llvm-project/premerge-checks/builds/77696#1836f181-a998-4695-b587-a832392222ea
The debian bot seems to be failing for unrelated (clang tooling) reasons.
| clang/lib/AST/RecordLayoutBuilder.cpp | ||
|---|---|---|
| 1892 ↗ | (On Diff #406148) | Seems reasonable. |
Ah, legit failure - should've caught that locally.
Seems I'm not really sure what POD rules GCC is using... https://godbolt.org/z/oczvTrdTM
My understanding is that this is C++03 non-POD, but C++11 POD:
struct t1 {
protected:
int a;
};And this is also C++03 non-POD, but C++11 POD:
struct t1 {
t1() = default; // C++11 POD, <C++11 non-POD
int a;
};Well, I guess, arguably, the second example has no definition in C++03, since it uses a C++11 feature, but we backport that as an extension. So perhaps our backport doesn't capture the POD-ness in the same way GCC does?
Because the original (FieldClass->isPOD()) classifies the first example as non-POD and the second example as POD, and the isCXX11PODType does the opposite, if I'm understanding the results of my program correctly. So I'm not sure what property to test for here to correctly model GCC's behavior. Perhaps I need to go and change Clang's definition for FieldClass->isPOD()
Yeah, looks like GCC believes cxx11_pod::t1 to be C++03 POD even though it doesn't quite follow the letter of the standard - but since teh "= default" is an extension, I guess they get to define what that extension means.
Here's an example:
struct t1 { int i; };
#if USER_DECLARED
struct t2 { t2() = default; int i; };
#endif
#if USER_DEFINED
struct t3 { t3(); int i; };
#endif
void f(int i) {
switch (i) {
t1 v1;
#if USER_DECLARED
t2 v2;
#endif
#if USER_DEFINED
t3 v3;
#endif
case 1: {
}
}
}$ g++ pod.cpp -std=c++03 -fsyntax-only -w -DUSER_DECLARED $ g++ pod.cpp -std=c++03 -fsyntax-only -w -DUSER_DEFINED pod.cpp: In function ‘void f(int)’: pod.cpp:17:10: error: jump to case label 17 | case 1: { | ^ pod.cpp:15:8: note: crosses initialization of ‘t3 v3’ 15 | t3 v3; | ^~
$ clang++ pod.cpp -std=c++03 -fsyntax-only -w -DUSER_DEFINED pod.cpp:17:5: error: cannot jump from switch statement to this case label case 1: { ^ pod.cpp:15:8: note: jump bypasses variable initialization t3 v3; ^ 1 error generated. $ clang++ pod.cpp -std=c++03 -fsyntax-only -w -DUSER_DECLARED pod.cpp:17:5: error: cannot jump from switch statement to this case label case 1: { ^ pod.cpp:12:8: note: jump bypasses initialization of non-POD variable t2 v2; ^ 1 error generated.
And with the updated version of this patch, we get this behavior, matching GCC's:
$ clang++-tot pod.cpp -std=c++03 -fsyntax-only -w -DUSER_DECLARED $ clang++-tot pod.cpp -std=c++03 -fsyntax-only -w -DUSER_DEFINED pod.cpp:17:5: error: cannot jump from switch statement to this case label case 1: { ^ pod.cpp:15:8: note: jump bypasses variable initialization t3 v3; ^ 1 error generated.
Sorry, but I can only add a bit more confusion: https://godbolt.org/z/fT19KTh34
There are two cases, only differing in terms of user-defined constructor.
Gcc and clang differs in the two cases. Gcc at least packs the second case (without user defined constructor), but clang (trunk version) doesn't.
Uncomment/define macro 'PROPS' to check that the type-traits indicate in both cases that it can be considered POD.
Right - but with this proposed patch applied, Clang's answer is consistent with GCC's behavior, I believe?
Uncomment/define macro 'PROPS' to check that the type-traits indicate in both cases that it can be considered POD.
Yes, I believe/understand that both of these types are C++11 POD, but the first example is not C++98/03 POD (and the second example isn't valid C++98/03, but as an extension, it is supported in C++98/03 and as far as I can tell (see the switch example in https://reviews.llvm.org/D119051#3305511 ) GCC deems the defaulted constructor to be still be C++98/03 POD, as an extension).
So, I believe your example is consistent with the theory I gave at the start of this patch: GCC is using the C++98/03 definition of POD, with an extension for defaulted functions, to determine whether to pack/align a member or not, and changing Clang's definition of 98/03 POD to match GCC's (by adding an extension for defaulted functions) is probably the right thing to do, so far as I can gather.
Changing the C++03 POD definition is going to be substantially ABI-breaking at this point. Any logic to change it needs to be platform-specific.
Thanks for chiming in!
Even if it's only related to the use of "= default"? Hrm. Got some examples I could try out?
Ah, I guess here: https://godbolt.org/z/P6n9xKTnM
struct t1 {
int i;
char c;
t1() = default;
};
struct t2: t1 {
char c;
};
#ifdef __clang__
static_assert(sizeof(t2) == 8, "");
#else
static_assert(sizeof(t2) == 12, "");
#endifWhere would be the right place for the platform specific detection to happen then? Can the places I've proposed changing in DeclCXX.cpp be made target-specific there, or do we have to compute two different properties there and do the target-specific selection between them at some later point/in the record layout logic?
Ah, looks like this is the existing https://github.com/itanium-cxx-abi/cxx-abi/issues/66
I would structure this as a LangOpt, and feed the target checks and ABI compat checks into the default setting for it. It could be something like DefaultedSMFArePOD / -f[no-]defaulted-smf-are-pod (smf being special member functions). The LangOpt default is true, and the driver fills in the value from the target and ABI version in the case of no explicit flag.
I think the DeclCXX changes are probably in the right place. I don't think it makes sense to carry around separate isPOD bits according to the C++11 and C++03 rules, just so we can use them later to make these two specific, known ABI-impacting decisions.
If you're going to change the ABI, you might as well tackle the rest of the differences mentioned in that issue while you're in there. That is, neither marking anything "= delete", nor creating a user-defined move assignment operator should mark it non-pod according to the GCC-compatible rules.
Add a -f driver flag for defaulted-smf-are-pod
Include the other cases from itanium ABI issue 66 (implicitly and explicitly deleted special members, and any form of move assignment operator)
Fair enough - had a go at that. Used your naming though in light of @jyknight's suggestion it might be overly specific (but figure we can bikeshed that in parallel with the implementation review)
Fair point - I think I've got those cases covered (see updated test case - happy to expand that further if desired - testing each case separately, or the like)? No member functions/ctor marked deleted, and no user-defined move assignment operator make the type non-pod.
I tested this a bit more robustly end-to-end like this:
struct t1 {
t1() = default;
t1(const t1&) = delete;
t1(t1&&) = default;
void operator=(t1&&);
~t1() = delete;
int a;
char c;
};
struct t2 : t1 {
char c;
};
int x[sizeof(t2) == 3 * sizeof(int) ? 1 : -1];$ clang-tot test.cpp -c -target x86_64-scei-ps4 test.cpp:13:7: error: 'x' declared as an array with a negative size int x[sizeof(t2) == 3 * sizeof(int) ? 1 : -1]; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated. $ clang-tot test.cpp -c -target x86_64-scei-ps4 -fdefaulted-smf-are-pod $ clang-tot test.cpp -c -target x86_64-unknown-darwin test.cpp:13:7: error: 'x' declared as an array with a negative size int x[sizeof(t2) == 3 * sizeof(int) ? 1 : -1]; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated. $ clang-tot test.cpp -c -target x86_64-unknown-darwin -fdefaulted-smf-are-pod $ clang-tot test.cpp -c -target x86_64-unknown-linux $ clang-tot test.cpp -c -target x86_64-unknown-linux -fno-defaulted-smf-are-pod test.cpp:13:7: error: 'x' declared as an array with a negative size int x[sizeof(t2) == 3 * sizeof(int) ? 1 : -1]; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated.
| clang/lib/Driver/ToolChains/Clang.cpp | ||
|---|---|---|
| 5596 ↗ | (On Diff #431871) | Does this mean -fclang-abi-compat will override the PS4/Darwin special case? I think we don't want to do that. |
| clang/lib/Driver/ToolChains/Clang.cpp | ||
|---|---|---|
| 5596 ↗ | (On Diff #431871) | I don't think so - this expression will make DefaultedSMFArePOD false when it's already false due to the target not being PS4 or Darwin, yeah? So it'll redundantly/harmlessly set it to false. |
| clang/lib/Driver/ToolChains/Clang.cpp | ||
|---|---|---|
| 5596 ↗ | (On Diff #431871) | No, I mean if it *is* PS4, it will turn true into false, and that's bad. If I'm reading this correctly. |
| clang/lib/Driver/ToolChains/Clang.cpp | ||
|---|---|---|
| 5596 ↗ | (On Diff #431871) | Right - let's see if I can explain how I'm understanding it at least - maybe I've got some code blindness from staring at it too long. If it /is/ PS4, then this code: bool DefaultedSMFArePOD = !RawTriple.isPS4() && !RawTriple.isOSDarwin(); Will amount to this: bool DefaultedSMFArePOD = !true && !RawTriple.isOSDarwin(); bool DefaultedSMFArePOD = false && !RawTriple.isOSDarwin(); bool DefaultedSMFArePOD = false; So then then the code at 5595 can only reinforce that, not change it - since it only sets the value to false. So my understanding is that the -fclang-abi-compat flag can not have any effect on the DefaultedSMFArePOD behavior of PS4. And it sounds like that's what you want? So I think we're good here? |
| clang/lib/Driver/ToolChains/Clang.cpp | ||
|---|---|---|
| 5596 ↗ | (On Diff #431871) | Those little bangs can be hard to see sometimes... Also, I think this bit
confused me, should have read
and so what the code actually does is in fact what we want. Thanks for your patience in explaining it, and I will go make an eye doctor appointment! |
| clang/lib/Driver/ToolChains/Clang.cpp | ||
|---|---|---|
| 5596 ↗ | (On Diff #431871) |
Ah, right you are, sorry about that!
Sure thing - certainly sometimes consider whether or not switching to the C++ alternative tokens would be an improvement... DefaultedSMFArePOD = not RawTriple.isPS4() and not RawTriple.isOSDarwin() ;) |
I'm in the middle of upstreaming PS5 support and I still didn't think of this... doh!
| clang/lib/Driver/ToolChains/Clang.cpp | ||
|---|---|---|
| 5588 ↗ | (On Diff #431871) | isPS4() => isPS() please. This should be in effect for both PS4 and PS5. |
| clang/test/Driver/clang_f_opts.c | ||
| 602 ↗ | (On Diff #431871) | Please duplicate this line with -target x86_64-sie-ps5 |
@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 )
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.