This is an archive of the discontinued LLVM Phabricator instance.

Extend the C++03 definition of POD to include defaulted functions
ClosedPublic

Authored by dblaikie on Feb 4 2022, 8:07 PM.

Diff Detail

Event Timeline

dblaikie requested review of this revision.Feb 4 2022, 8:07 PM
dblaikie created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2022, 8:07 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Test case showing GCC's behavior here: https://godbolt.org/z/4W7j8Yd54

Test case showing GCC's behavior here: https://godbolt.org/z/4W7j8Yd54

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?)

Bhramar.vatsa added inline comments.Feb 5 2022, 6:43 AM
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?

rnk added a comment.Feb 7 2022, 11:48 AM

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.

dblaikie updated this revision to Diff 406628.Feb 7 2022, 3:09 PM

Remove FieldClass check

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.

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()

dblaikie updated this revision to Diff 406904.Feb 8 2022, 11:17 AM
  • Change the definition of C++03 POD to include defaulted functions

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.
dblaikie retitled this revision from Fix pod-packed functionality to use the C++11 definition of pod-ness to Extend the C++03 definition of POD to include defaulted functions.Feb 8 2022, 11:21 AM
Bhramar.vatsa added a comment.EditedFeb 11 2022, 6:38 AM

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.

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.

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.

And this should be raised as an Itanium issue.

Thanks for chiming in!

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.

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?

And this should be raised as an Itanium issue.

Ah, looks like this is the existing https://github.com/itanium-cxx-abi/cxx-abi/issues/66

rnk added a comment.Feb 28 2022, 4:31 PM

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.

Ah, looks like this is the existing https://github.com/itanium-cxx-abi/cxx-abi/issues/66

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.

dblaikie updated this revision to Diff 413641.Mar 7 2022, 3:23 PM

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)

Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 3:23 PM
dblaikie added a comment.EditedMar 7 2022, 3:27 PM

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.

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)

Ah, looks like this is the existing https://github.com/itanium-cxx-abi/cxx-abi/issues/66

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.

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.
probinson added inline comments.
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.

dblaikie added inline comments.May 25 2022, 11:19 AM
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.

probinson added inline comments.May 25 2022, 4:01 PM
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.

dblaikie added inline comments.May 26 2022, 12:01 PM
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?

probinson added inline comments.May 26 2022, 1:32 PM
clang/lib/Driver/ToolChains/Clang.cpp
5596 ↗(On Diff #431871)

Those little bangs can be hard to see sometimes... Also, I think this bit

already false due to the target not being PS4 or Darwin,

confused me, should have read

already false due to the target being PS4 or Darwin,

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!

dblaikie added inline comments.May 27 2022, 9:03 AM
clang/lib/Driver/ToolChains/Clang.cpp
5596 ↗(On Diff #431871)

I think this bit

already false due to the target not being PS4 or Darwin,

confused me, should have read

already false due to the target being PS4 or Darwin,

Ah, right you are, sorry about that!

Thanks for your patience in explaining it, and I will go make an eye doctor appointment!

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
(no need to duplicate both)

dblaikie updated this revision to Diff 433472.Jun 1 2022, 11:31 AM

Generalize to handle PS5

dblaikie marked 5 inline comments as done.Jun 1 2022, 11:32 AM

I'm in the middle of upstreaming PS5 support and I still didn't think of this... doh!

Ah, right - done!

@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?

dblaikie updated this revision to Diff 451686.Aug 10 2022, 5:03 PM

Remove driver flag (just rely on ABI compat) & tidy up testing

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.

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.

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
8–15

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

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.

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?

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.

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?

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!

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.

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).

erichkeane added inline comments.
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.

dblaikie updated this revision to Diff 452315.Aug 12 2022, 3:20 PM

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

dblaikie added a subscriber: hansw.Aug 12 2022, 3:20 PM

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

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?

dblaikie marked an inline comment as done and an inline comment as not done.Aug 12 2022, 3:21 PM
dblaikie added inline comments.
clang/test/SemaCXX/class-layout.cpp
8–15

ah, yeah - just need it on the first one

erichkeane added inline comments.Aug 15 2022, 6:03 AM
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.

rnk added a subscriber: hans.Aug 15 2022, 12:13 PM

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.

dblaikie marked an inline comment as done.Aug 24 2022, 12:49 PM

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...

  1. has base classes
  2. any virtual member function
  3. non-implicit ctor (so even user-declared-but-defaulted ctor)
  4. non-implicit member function (so even user-declared-but-defaulted assignment operators or dtor)
  5. any non-public field member
  6. some ObjC thing (non-trivial ObjC lifetime)
  7. 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)
  8. non-static data member initializers

Let's compare that with isTrivialForAArch64MSVC...

  1. checked (second)
  2. 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?
  3. only user-provided ctors, so it can have non-implicit (user-declared) but defaulted ctors
  4. specifically checking non-trivial copy assignment (no restriction on move assignment and it could have any number of other member functions, I think?)
  5. checked (first)
  6. nope/not relevant?
  7. Presumably tested by "hasNonTrivialDestructor" and "hasNonTrivialCopyAssignment"?
  8. 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.

  1. checked (second): has base classes

https://godbolt.org/z/d76fEGP1M - all agreed

  1. 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

  1. 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

  1. 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

  1. checked (first)

Used this as the baseline - everyone agrees.

  1. nope/not relevant?
  1. 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

  1. 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?

rnk added a subscriber: ayzhao.Aug 24 2022, 4:07 PM

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.

dblaikie updated this revision to Diff 465158.Oct 4 2022, 2:12 PM

rebase and fix an AST test

dblaikie updated this revision to Diff 465159.Oct 4 2022, 2:16 PM

Remove Microsoft return ABI test, now that the Microsoft ABI implementation no longer depends on the AST isPOD property

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.

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.

rnk added a comment.Oct 4 2022, 2:41 PM

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.

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.

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?

rnk added inline comments.Oct 5 2022, 3:55 PM
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.

dblaikie updated this revision to Diff 465956.Oct 6 2022, 6:48 PM

Move condition to TargetInfo

dblaikie added inline comments.Oct 6 2022, 6:49 PM
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)

rnk added a comment.Oct 10 2022, 5:03 PM

+#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.

dblaikie updated this revision to Diff 467024.Oct 11 2022, 10:45 PM

Add doc comment

dblaikie marked an inline comment as done.Oct 11 2022, 10:45 PM
dblaikie added inline comments.Oct 12 2022, 1:35 PM
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)

rnk accepted this revision.Oct 25 2022, 1:29 PM

lgtm

This revision is now accepted and ready to land.Oct 25 2022, 1:29 PM
This revision was landed with ongoing or failed builds.Oct 26 2022, 3:01 PM
This revision was automatically updated to reflect the committed changes.
rnk added a comment.Oct 27 2022, 8:40 AM

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?

aaron.ballman added a reviewer: Restricted Project.Oct 27 2022, 9:46 AM

Adding clang-vendors because of the potential for disruption.

rnk added a comment.Oct 27 2022, 11:55 AM

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.

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

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.

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.

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 )