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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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. |
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? |
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:
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. |
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) |
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.
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?