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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2118
+  if (Packed)
+    UnadjustedAlignment = std::max(UnadjustedAlignment, UnpackedFieldAlign);
   UpdateAlignment(FieldAlign, UnpackedFieldAlign, PreferredAlign);
----------------
rjmccall wrote:
> 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.
I think I had a go at this over here & failed, might have some relevant notes: https://reviews.llvm.org/D118511#inline-1140212

But, yeah, would love to see it simplified, if possible - just the data point that I tried and failed recently :/ (& contributed to some of the current complexity)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146242