[ARM] Fixing ABI mismatch for packed structs and fields

[JiruiWu]

Summary

Previously the alignments used by the caller and its callee do not
match. This causes errors for packed structs and fields. According to
AAPCS64, the alignment should be either 8 or 16 bytes for a composite
type.

[tmatheson]

Looks sensible but I don't fully understand the context of the change. Please could you explain more what is wrong with the current behaviour, and which parts of the AAPCS you are referring to.

clang/lib/CodeGen/TargetInfo.cpp

5819

Does the similar code added in D100853 need updated too?

5822

Does this code definitely only apply when the ABI is AAPCS64, or should there be a check for that somewhere here? I can't tell whether the if on line 5806 is sufficient.

clang/test/CodeGen/aarch64-ABI-align-packed.cpp

2

Does this need the arm vendor in the triple?

2

Please add a brief comment explaining what this is testing.

[JiruiWu]

Addressing review comments.

[JiruiWu]

clang/lib/CodeGen/TargetInfo.cpp

5819

No, because this patch is on AArch64 and https://reviews.llvm.org/D100853 is on AArch32. The alignment is capped to 16 in AArch64 and capped to 8 in AArch32.

5822

The code on line 5814 only applies when the ABI is AAPCS64 because it is in the if block that starts on line 5805 and ends on line 5818. As a result, the if on line 5806 is sufficient.

[tmatheson]

LGTM, but I'm not that familiar with the code that selects the alignment so it would be good to get a second opinion.

clang/lib/CodeGen/TargetInfo.cpp

5814

Should this change cover AAPCS_VFP too?

[olista01]

LGTM

[rjmccall]

clang/lib/AST/RecordLayoutBuilder.cpp

2118

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.

[dblaikie]

clang/lib/AST/RecordLayoutBuilder.cpp

2118

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)

[rjmccall]

Yeah. To be clear, though, I'm not asking for the overall data flow of the function to be fixed in this patch; I'm just pointing out problems in the new logic being added by this patch.

[JiruiWu]

Addressing review comments.

[JiruiWu]

clang/lib/AST/RecordLayoutBuilder.cpp

2118

I think the logic here is correct.

clang/lib/CodeGen/TargetInfo.cpp

5814

This patch does not cover AAPCS_VFP because AAPCS_VFP is not listed in the ABIKind of the class AArch64ABIInfo.

[rjmccall]

Thank you. Per my comment here:

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.

Could you add tests for what happens when a struct is modified by the aligned attribute on a field and/or #pragma pack and make sure that we do... whatever the right thing here is?

[tmatheson]

Reverse-ping @JiruiWu