GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

[dblaikie]

Summary

This matches GCC: https://godbolt.org/z/sM5q95PGY

I realize this is an API break for clang+clang - so I'm totally open to
discussing how we should deal with that. If Apple wants to keep the
Clang layout indefinitely, if we want to put a flag on this so non-Apple
folks can opt out of this fix/new behavior.

[dblaikie]

Ping

[tstellar]

So to summarize: this will change the ABI of clang built objects from clang13 -> clang14 in order to make clang14 compatible with gcc?

[dblaikie]

In D117616#3270205, @tstellar wrote:

So to summarize: this will change the ABI of clang built objects from clang13 -> clang14 in order to make clang14 compatible with gcc?

That's the idea, yes. (only for non-pod members of packed structs)

I /think/ we've done this sort of thing in the past when we've found corner case ABI differences between clang and gcc - but I don't have concrete examples to link to off hand.

[rnk]

In addition to the ABI compatibility concern, IMO, GCC's behavior here is better and more user friendly:

• C++ classes have methods
• calling a method on a field takes the address of a field
• if the field is in a packed struct, it may be misaligned
• misaligned loads and stores cause UB
• the UB is not trivial or hypothetical: it manifests as a crash, or load/store tearing or non-atomicity

GCC prevents users from getting into this situation by ignoring the packed attribute in the presence of non-C++98-POD fields and warning the user about it. Clang should do the same. Currently, Clang's stance seems to be, if the user asks permission to shoot themselves in the foot, they can go right ahead. That's less than ideal.

Between the usability concern and the ABI concern, it seems worth changing behavior here, even if it means leaving behind a flag or a new attribute such as __attribute__((packed_nonpod)) to get the old behavior. I don't see a PS4 reviewer here, but every time I've asked them if we can tweak some ABI detail, they ask to be opted out.

[dblaikie]

@probinson for Sony ABI input

[tstellar]

Can you add a release note to go along with this change?

[rjmccall]

In addition to the ABI compatibility concern, IMO, GCC's behavior here is better and more user friendly:

• C++ classes have methods
• calling a method on a field takes the address of a field

I mean, this argument is hardly limited to non-POD types. I suppose you could argue that with trivially-copyable types, you could at least copy the object somewhere else before calling the method. But then the condition ought to be "has a trivial, non-deleted copy/move constructor" rather than "POD for the purposes of the ABI."

Well, anyway, as always, GCC is entitled to set the ABI for the platforms on which it is the system compiler. I assume the POD definition actually comes from C++03 and not C++98, in which IIRC member pointers were non-trivial. Darwin will opt out, yes, and I think we should assume that PS4 will as well.

[probinson]

I think we should assume that PS4 will as well.

Yes, please. We are essentially locked to the ABI as of clang 3.2.

[dblaikie]

@Xiangling_L - I see you made an ABI change for AIX in rG05ad8e942996f36cc694478542ccd84aa5bbb80f - any idea if this new ABI break/fix is relevant to AIX? Would you want to stay with the current Clang layout (like Apple and Sony are) or would you want the fix to be compatible with GCC?

[dblaikie]

Relnotes
Opt out PS4 and Darwin
Add support for -fclang-abi-compat=13

[dblaikie]

I didn't find any mention of Darwin in RecordLayoutBuilder, which surprised me - I thought maybe Darwin was using the ClangABICompat to express its ABI compatibility requirements (which would sort of be handy rather than having to test multiple target features, for those targets that define their ABI by Clang's behavior - though I guess even those targets might be willing to take some ABI breaks against themselves sometimes (looks like they have, historically - looking at the other ClangABICompat values and where/how they're tested - I don't see target tests against Darwin and PS4 for all of them)) but doesn't look like it.

So - should I be testing Darwin/MacOS/something support differently than I have?

[rsmith]

clang/docs/ReleaseNotes.rst

239 ↗

(On Diff #403449)

"non-packed" here seems a little unclear. Does this mean "GCC doesn't pack non-POD members in packed structs unless the packed attribute is also specified on the member." ?

(Also I believe the preferred spelling for GCC is capitalized.)

242 ↗

(On Diff #403449)

clang/include/clang/Basic/LangOptions.h

185 ↗

(On Diff #403449)

clang/lib/AST/RecordLayoutBuilder.cpp

1891

isPOD is C++ standard specific, and our ABI rule really shouldn't be. Does GCC use the C++98 rules here, the C++11 rules, or something else? (Hopefully the GCC behavior doesn't change between -std=c++98 and -std=c++11!)

From a quick test, it looks like GCC doesn't pack fields whose types are classes with base classes, even if they're trivially-copyable and standard-layout, suggesting that it's probably using the C++98 POD rules.

1895

Would it make sense to warn here if Packed && !FieldPacked, like GCC does?

[rnk]

clang/lib/AST/RecordLayoutBuilder.cpp

1890–1891

I think GCC implements this by ignoring the packed attribute on classes with such non-C++98-pod fields. See this case for example:
https://gcc.godbolt.org/z/fe8x1ne7o

class NonPod {
    void method();
    int x;
};
struct __attribute__((packed)) PackedNonPod {
    char a;
    NonPod b;
    char c;
    int d;
};
PackedNonPod gv;

static_assert(sizeof(PackedNonPod) == 4 * 4, "drop packed");

-->

<source>:7:12: warning: ignoring packed attribute because of unpacked non-POD field 'NonPod PackedNonPod::b'
    7 |     NonPod b;

So, in this case, the entire record is unpacked. d appears at offset 12, and the overall size is 16. Your code, as I understand it, handles each field individually, which isn't quite the same.

I think the fix is in Sema somewhere to drop the attribute.

[rsmith]

clang/lib/AST/RecordLayoutBuilder.cpp

1890–1891

GCC's diagnostic says it's ignoring the attribute, but that does not appear to actually be true. https://godbolt.org/z/jY1joGPjW (Note that the short field is being packed here, despite the warning saying the attribute was ignored.)

In your case, I think what's happening is that PackedNonPod::d is at offsets [9,13) within PackedNonPod, but the alignment of PackedNonPod is 4 because the NonPod field is not packed, so the size ends up being rounded up to 16 like it would be if it were not packed. You can add three more chars to the end of the struct without it getting larger: https://gcc.godbolt.org/z/qTzeosWWW

So I think the patch is correctly modeling the GCC behavior.

[dblaikie]

rsmith's fixes for release notes
added some more test coverage to demonstrate that other fields are still packed

clang/lib/AST/RecordLayoutBuilder.cpp

1891

I /think/ CXXRecordDecl::isPOD doesn't use a language-varying definition, according to its documentation at least:
https://github.com/llvm/llvm-project/blob/1e3a02162db20264e9615b1346420c8d199cb347/clang/include/clang/AST/DeclCXX.h#L1124
& the code that sets the field that the accessor returns looks, to me at least, consistent with that claim: https://github.com/llvm/llvm-project/blob/1e3a02162db20264e9615b1346420c8d199cb347/clang/lib/AST/DeclCXX.cpp#L983

But if there's another way I should spell this to make it more clear/correct, more test cases to add to show the difference between these definitions - I'm open to that...

1895

Probably? Happy to implement that in a follow-up.

[rsmith]

LGTM

clang/lib/AST/RecordLayoutBuilder.cpp

1891

Ah, right you are, I was thinking of QualType::isPODType, which does depend on the language mode: https://github.com/llvm/llvm-project/blob/1e3a02162db20264e9615b1346420c8d199cb347/clang/lib/AST/Type.cpp#L2350

No action necessary here, I think.

[dblaikie]

Will wait on this 'til next Monday at least. Wouldn't mind a double-check/confirmation from @rjmccall that the Darwin checking is suitable (since I didn't see any other Darwin checking in RecordLayoutBuilder).

clang/lib/AST/RecordLayoutBuilder.cpp

1891

Cool - reckon it's worth renaming it to be more clear about which POD it represents? IsClassicPOD, isRetroPOD, isCXX03POD?

[rjmccall]

Yes, I think that's an okay way to check this.

[dblaikie]

clang/lib/AST/RecordLayoutBuilder.cpp

1895

Warning sent for review here: https://reviews.llvm.org/D118511

[rsmith]

clang/lib/AST/RecordLayoutBuilder.cpp

1891

If anything I think it might make more sense to remove the language-sensitive version and always use POD to mean the C++03 thing. I think the only thing that should want standard-layout + trivially-copyable should be the __is_pod trait (and maybe some target-specific ABI rules?). The very notion of "POD" as a shorthand for "standard-layout and trivially-copyable" was deprecated in C++20.

[Bhramar.vatsa]

@dblaikie
The condition FieldClass->isPOD() returns false for the following case (when considering field 'struct foo t' of 'struct foo1') :

class foo {
   foo() = default;
   int _a;
};

struct foo1 {
    struct foo t;
} t1;

The same code though doesn't give any warning for gcc: https://godbolt.org/z/f4chraerY

This is because the way it works for CXXRecordDecl : https://github.com/llvm/llvm-project/blob/1e3a02162db20264e9615b1346420c8d199cb347/clang/lib/AST/DeclCXX.cpp#L928

So, there seems to be a difference the way GCC is handling this case, in comparison to how now clang handles it.

For the same case, D->getType().isCXX11PODType() (or isPODType()) indicates it to be a POD type. So, we think that this should be further changed such that it doesn't break the code that works with GCC.

[dblaikie]

In D117616#3295859, @Bhramar.vatsa wrote:

@dblaikie
The condition FieldClass->isPOD() returns false for the following case (when considering field 'struct foo t' of 'struct foo1') :

class foo {
   foo() = default;
   int _a;
};

struct foo1 {
    struct foo t;
} t1;

The same code though doesn't give any warning for gcc: https://godbolt.org/z/f4chraerY

This is because the way it works for CXXRecordDecl : https://github.com/llvm/llvm-project/blob/1e3a02162db20264e9615b1346420c8d199cb347/clang/lib/AST/DeclCXX.cpp#L928

So, there seems to be a difference the way GCC is handling this case, in comparison to how now clang handles it.

For the same case, D->getType().isCXX11PODType() (or isPODType()) indicates it to be a POD type. So, we think that this should be further changed such that it doesn't break the code that works with GCC.

Sorry, was a bit confused by the discussion of warnings and such - but, yes, this does seem to be a remaining divergence in the layout between Clang (after this patch was committed) and GCC: https://godbolt.org/z/GEM5q4fd3

@rsmith do you have a semi-exhaustive list of the variations in POD-ness I should probably test to better understand which definition GCC is using here? Reading the cppreference on POD, aggregate, standard layout, and trivial there are a lot of dimensions and I was wondering if you had a quick-ish summary so I hopefully don't miss cases & figure out exactly how this is meant to be sliced?

I'll work on some godbolt probes/test cases for now to see what I can come up with.

[dblaikie]

Hmm, I guess it might be the C++11 definition, as suggested - since a base class (even a public one) seems to classify the type as "non pod" as far as GCC is concerned (

In D117616#3298001, @dblaikie wrote:

In D117616#3295859, @Bhramar.vatsa wrote:

@dblaikie
The condition FieldClass->isPOD() returns false for the following case (when considering field 'struct foo t' of 'struct foo1') :

class foo {
   foo() = default;
   int _a;
};

struct foo1 {
    struct foo t;
} t1;

The same code though doesn't give any warning for gcc: https://godbolt.org/z/f4chraerY

This is because the way it works for CXXRecordDecl : https://github.com/llvm/llvm-project/blob/1e3a02162db20264e9615b1346420c8d199cb347/clang/lib/AST/DeclCXX.cpp#L928

So, there seems to be a difference the way GCC is handling this case, in comparison to how now clang handles it.

For the same case, D->getType().isCXX11PODType() (or isPODType()) indicates it to be a POD type. So, we think that this should be further changed such that it doesn't break the code that works with GCC.

Sorry, was a bit confused by the discussion of warnings and such - but, yes, this does seem to be a remaining divergence in the layout between Clang (after this patch was committed) and GCC: https://godbolt.org/z/GEM5q4fd3

@rsmith do you have a semi-exhaustive list of the variations in POD-ness I should probably test to better understand which definition GCC is using here? Reading the cppreference on POD, aggregate, standard layout, and trivial there are a lot of dimensions and I was wondering if you had a quick-ish summary so I hopefully don't miss cases & figure out exactly how this is meant to be sliced?

I'll work on some godbolt probes/test cases for now to see what I can come up with.

Posted https://reviews.llvm.org/D119051 at least with the existing test case - thanks for that!

Open to more robust test case suggestions.

[MatzeB]

clang/docs/ReleaseNotes.rst

239–243 ↗

(On Diff #404097)

Ugh... The release notes went missing in main now:

• We had them in clang-14, but then reverted in the release process of clang-14 so they never showed for clang-14.
• In main we removed all release nodes when going from 14->15.

So this notice is effectively lost now...

[dblaikie]

clang/docs/ReleaseNotes.rst

239–243 ↗

(On Diff #404097)

Ah, thanks - readded it in 0b903ef6aa0976a60d3f448837f3c43adaf09cc1

Anyone have feelings about whether we should move the old ABI under Ver14, instead of Ver13, since the break was never released in Ver14?

[tstellar]

clang/docs/ReleaseNotes.rst

239–243 ↗

(On Diff #404097)

That seems fine to me.

[dblaikie]

clang/docs/ReleaseNotes.rst

239–243 ↗

(On Diff #404097)

Sent D126334

[dblaikie]

clang/docs/ReleaseNotes.rst

239–243 ↗

(On Diff #404097)

Done in D126334 (posted, reviewed, and committed)

[abrachet]

While digging around failures we were seeing related to this I found some behavior that diverges from gcc. See https://godbolt.org/z/qe18Wh4jf

[tstellar]

@dblaikie Is there anything we need to do in the release branch for this still? cc @thieta

[dblaikie]

In D117616#3732173, @tstellar wrote:

@dblaikie Is there anything we need to do in the release branch for this still? cc @thieta

Sorry, yeah, I'd tagged you over here: https://reviews.llvm.org/D118511#3717753 for part of that discussion to see if we wanted to revert this from the release the same as we did the previous one... sorry this has dragged on so long, the second ABI fix (podness of structs with defaulted special members) and the warning (not wanting to warn/tell people to change things that were going to become a non-issue once the second ABI fix was in) aren't in yet, though the second ABI fix is getting some traction at least ( https://reviews.llvm.org/D119051 ).

[dblaikie]

In D117616#3731188, @abrachet wrote:

While digging around failures we were seeing related to this I found some behavior that diverges from gcc. See https://godbolt.org/z/qe18Wh4jf

thanks for the bug report, yeah - looks like we need "fieldClass.isPod && !fieldClass.packed" . I'll send out another patch & update here with a reference.

[dblaikie]

In D117616#3732261, @dblaikie wrote:

In D117616#3731188, @abrachet wrote:

While digging around failures we were seeing related to this I found some behavior that diverges from gcc. See https://godbolt.org/z/qe18Wh4jf

thanks for the bug report, yeah - looks like we need "fieldClass.isPod && !fieldClass.packed" . I'll send out another patch & update here with a reference.

Sorry it took a bit longer than ideal - posted here: D135916

[dblaikie]

clang/docs/ReleaseNotes.rst

239–243 ↗

(On Diff #404097)

Made the matching change for v15, since this was reverted on that release branch too (sorry this has been such a drawn out issue - thanks for all the help and such) in 9363071303ec59bc9e0d9b989f08390b37e3f5e4

clang/lib/AST/RecordLayoutBuilder.cpp

if (D->isBitField()) {
     return;
   }
 
   uint64_t UnpaddedFieldOffset = getDataSizeInBits() - UnfilledBitsInLastUnit;
   // Reset the unfilled bits.
   UnfilledBitsInLastUnit = 0;
   LastBitfieldStorageUnitSize = 0;
 
-  bool FieldPacked = Packed || D->hasAttr<PackedAttr>();
+  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD())) ||
+                     D->hasAttr<PackedAttr>();
 
   AlignRequirementKind AlignRequirement = AlignRequirementKind::None;
   CharUnits FieldSize;
   CharUnits FieldAlign;
   // The amount of this class's dsize occupied by the field.
   // This is equal to FieldSize unless we're permitted to pack
   // into the field's tail padding.
   CharUnits EffectiveFieldSize;
 
   auto setDeclInfo = [&](bool IsIncompleteArrayType) {
     auto TI = Context.getTypeInfoInChars(D->getType());
     FieldAlign = TI.Align;

clang/test/SemaCXX/address-packed.cpp

 // PR35509
 typedef long L1;
 struct Incomplete;
 struct S2 {
   L1 d;
   Incomplete *e() const;
 } __attribute__((packed));
 Incomplete *S2::e() const { return (Incomplete *)&d; } // no-warning
+
+
+namespace non_classic_pod {
+struct t1 { protected: int a; };
+static_assert(alignof(t1) == 4, "");
+// GCC prints warning: ignoring packed attribute because of unpacked non-POD field 't1 t2::f'`
+struct t2 { t1 v1; } __attribute__((packed));
+static_assert(alignof(t2) == 4, "");
+} // namespace non_classic_pod
+
+namespace non_classic_pod_packed {
+struct t1 { protected: int a; } __attribute__((packed));
+static_assert(alignof(t1) == 1, "");
+struct t2 { t1 v1; } __attribute__((packed));
+static_assert(alignof(t2) == 1, "");
+} // namespace non_classic_pod_packed