[PATCH] D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1890
 
-  bool FieldPacked = Packed || D->hasAttr<PackedAttr>();
+  llvm::Triple Target = Context.getTargetInfo().getTriple();
+  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
----------------
rnk wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117616/new/

https://reviews.llvm.org/D117616