Page MenuHomePhabricator

[ARM] Fixing ABI mismatch for packed structs and fields
Needs ReviewPublic

Authored by JiruiWu on Mar 16 2023, 10:03 AM.

Details

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.

Diff Detail

Event Timeline

JiruiWu created this revision.Mar 16 2023, 10:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 10:03 AM
JiruiWu requested review of this revision.Mar 16 2023, 10:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 10:03 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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 updated this revision to Diff 507313.Mar 22 2023, 4:55 AM
JiruiWu marked 2 inline comments as done.

Addressing review comments.

JiruiWu marked 2 inline comments as done.Mar 22 2023, 4:56 AM
JiruiWu added inline comments.
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.

JiruiWu marked 2 inline comments as done.Mar 22 2023, 4:56 AM

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 accepted this revision.Apr 4 2023, 2:57 AM

LGTM

This revision is now accepted and ready to land.Apr 4 2023, 2:57 AM
rjmccall added inline comments.Apr 4 2023, 5:22 PM
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 added inline comments.Apr 18 2023, 4:50 PM
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 requested changes to this revision.Apr 19 2023, 3:07 PM

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.

This revision now requires changes to proceed.Apr 19 2023, 3:07 PM
JiruiWu updated this revision to Diff 515261.Apr 20 2023, 2:31 AM
JiruiWu marked 2 inline comments as done.

Addressing review comments.

JiruiWu marked an inline comment as done.Apr 20 2023, 2:42 AM
JiruiWu added inline comments.
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.

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?