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

[Jirui Wu via Phabricator via cfe-commits]

JiruiWu marked an inline comment as done.
JiruiWu added inline comments.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2118
+  if (Packed)
+    UnadjustedAlignment = std::max(UnadjustedAlignment, UnpackedFieldAlign);
   UpdateAlignment(FieldAlign, UnpackedFieldAlign, PreferredAlign);
----------------
dblaikie wrote:
> 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)
I think the logic here is correct.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:5806
   if (!IsWinVariadic && isHomogeneousAggregate(Ty, Base, Members)) {
     if (Kind != AArch64ABIInfo::AAPCS)
       return ABIArgInfo::getDirect(
----------------
tmatheson wrote:
> Should this change cover AAPCS_VFP too?
This patch does not cover AAPCS_VFP because AAPCS_VFP is not listed in the ABIKind of the class AArch64ABIInfo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146242