[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs and fields

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2118
+  if (Packed)
+    UnadjustedAlignment = std::max(UnadjustedAlignment, UnpackedFieldAlign);
   UpdateAlignment(FieldAlign, UnpackedFieldAlign, PreferredAlign);
----------------
I've always felt the data flow in this function was excessively convoluted.  Let's puzzle it out to figure out what's going on.  Ignoring the AIX stuff which I assume can't coincide with AArch64, we've got:

```
UnpackedFieldAlign = min(max(TyAlign, MaxAlignmentInChars), MaxFieldAlignment)
PackedFieldAlign = min(max(1, MaxAlignmentInChars), MaxFieldAlignment)
FieldAlign = FieldPacked ? PackedFieldAlign : UnpackedFieldAlign
```

where `MaxAlignmentInChars` is the highest value of all the alignment attributes on the field and `MaxFieldAlignment` is the value of `#pragma pack` that was active at the time of the struct definition.

Note that this gives us `PackedFieldAlign <= FieldAlign <= UnpackedFieldAlign`.

So:
1. I think it's wrong to be checking `Packed` instead of `FieldPacked` here.  But:
2. If `FieldPacked`, then because `UnpackedFieldAlign >= FieldAlign`, the net effect of these three lines is `UnadjustedAlignment = std::max(UnadjustedAlignment, UnpackedFieldAlign)`.
3. If `!FieldPacked`, then `UnpackedFieldAlign == FieldAlign`, so the net effect of these three lines is *also* `UnadjustedAlignment = std::max(UnadjustedAlignment, UnpackedFieldAlign)`.
4. So actually you don't need to check `FieldPacked` at all; you should remove the old line and just do your new one unconditionally.

Also, AAPCS64 seems to define UnadjustedAlignment as the "natural alignment", and there's a doc comment saying it's the max of the type alignments.  That makes me wonder if we should really be considering either the `aligned` attribute or `#pragma pack` in this computation at all; maybe we should just be looking at the type alignment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146242