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

[dblaikie]

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

[dblaikie]

In D119051#3298491, @dblaikie wrote:

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]

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]

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]

Remove FieldClass check

[dblaikie]

In D119051#3302125, @rnk wrote:

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]

• Change the definition of C++03 POD to include defaulted functions

[dblaikie]

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.

[Bhramar.vatsa]

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.

[dblaikie]

In D119051#3314138, @Bhramar.vatsa wrote:

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.

[rjmccall]

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.

[rjmccall]

And this should be raised as an Itanium issue.

[dblaikie]

Thanks for chiming in!

In D119051#3315741, @rjmccall wrote:

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?

[dblaikie]

In D119051#3315747, @rjmccall wrote:

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

[dblaikie]

Ping

[dblaikie]

Ping

[rnk]

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.

[jyknight]

In D119051#3316026, @dblaikie wrote:

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]

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)

[dblaikie]

In D119051#3350072, @rnk wrote:

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)

In D119051#3350276, @jyknight wrote:

In D119051#3316026, @dblaikie wrote:

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.

[dblaikie]

Rebase

[dblaikie]

Ping

[probinson]

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.

[dblaikie]

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.

[probinson]

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.

[dblaikie]

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?

[probinson]

clang/lib/Driver/ToolChains/Clang.cpp

5596

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]

clang/lib/Driver/ToolChains/Clang.cpp

5596

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

;)

[probinson]

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

[dblaikie]

Generalize to handle PS5

[dblaikie]

In D119051#3550653, @probinson wrote:

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

Ah, right - done!

[dblaikie]

Ping

[dblaikie]

Ping

[dblaikie]

Ping

[dblaikie]

Ping