[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs and fields
John McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 4 17:22:42 PDT 2023
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
More information about the cfe-commits
mailing list