Details
- Reviewers
rsmith Bhramar.vatsa rjmccall rnk
Diff Detail
- Repository
- rG LLVM Github Monorepo
Time | Test | |
---|---|---|
80 ms | x64 debian > Clang.AST::ast-dump-record-definition-data-json.cpp Script:
--
: 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -triple x86_64-unknown-unknown -fclang-abi-compat=7.0 -std=c++17 -ast-dump=json /var/lib/buildkite-agent/builds/llvm-project/clang/test/AST/ast-dump-record-definition-data-json.cpp | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/clang/test/AST/ast-dump-record-definition-data-json.cpp
| |
40 ms | x64 debian > Clang.SemaCXX::class-layout.cpp Script:
--
: 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -triple x86_64-unknown-unknown /var/lib/buildkite-agent/builds/llvm-project/clang/test/SemaCXX/class-layout.cpp -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base -Wno-c++11-extensions
|
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, ""); #endif
Where 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 | 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 | 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 | 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 | 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 | 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 |
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 | isPS4() => isPS() please. This should be in effect for both PS4 and PS5. | |
clang/test/Driver/clang_f_opts.c | ||
602 | Please duplicate this line with -target x86_64-sie-ps5 |