Paths
| Differential D62152
[ARM][AArch64] Fix incorrect handling of alignment in va_arg code generation ClosedPublic Authored by john.brawn on May 20 2019, 9:41 AM.
Details
Summary Overaligned and underaligned types (i.e. types where the alignment has been increased or decreased using the aligned and packed attributes) weren't being correctly handled in all cases, as the unadjusted alignment should be used. This patch also adjusts getTypeUnadjustedAlign to correctly handle typedefs of non-aggregate types, which it appears it never had to handle before.
Diff Detail
Event TimelineComment Actions Please verify my understanding of the following is correct:
I'm not confident about changing the calling convention again at this point for non-struct types. I guess it's obscure enough that changing it is probably okay. But would you mind splitting it into a separate followup, with a better description of the impact? Comment Actions
Yes, the only places it's called are AArch64ABIInfo::classifyArgumentType and ARMABIInfo::classifyArgumentType.
Yes.
Only in va_arg. For example if you look at typedef int aligned_int __attribute__((aligned(8)));
aligned_int called_fn(int a1, aligned_int a2) {
return a2;
}
aligned_int calling_fn() {
return called_fn(1,2);
}a2 is passed in r1 by calling_fn, and read from it in called_fn (both before and after this patch). However for typedef int aligned_int __attribute__((aligned(8)));
aligned_int called_fn(int a1, ...) {
va_list ap;
va_start(ap, a1);
aligned_int a2 = va_arg(ap, aligned_int);
va_end(ap);
return a2;
}
aligned_int calling_fn() {
return called_fn(1,2);
}a2 is still passed in r1 by calling_fn, but the current clang behaviour in called_fn is that it pushes r1-r3 then loads the next 8-byte-aligned value, which will be the pushed r2 value. This patch fixes this so that it doesn't 8-byte align the load address, and so gets the pushed r1.
This patch only changes the va_arg behaviour, and this whole thing with unadjusted alignment is from the published pcs (https://developer.arm.com/docs/ihi0042/latest/procedure-call-standard-for-the-arm-architecture-abi-2019q1-documentation and https://developer.arm.com/docs/ihi0055/latest/procedure-call-standard-for-the-arm-64-bit-architecture, though it's called "natural alignment" there). Comment Actions Oh, I see, ABIInfo::computeInfo() uses the canonical type to compute the calling convention, and that throws away alignment attributes because alignment attributes on non-struct are broken that way. In that case, LGTM. This revision is now accepted and ready to land.May 21 2019, 11:52 AM Closed by commit rC361372: [ARM][AArch64] Fix incorrect handling of alignment in va_arg code generation (authored by john.brawn). · Explain WhyMay 22 2019, 4:40 AM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 200705 lib/AST/ASTContext.cpp
lib/CodeGen/TargetInfo.cpp
test/CodeGen/aarch64-varargs.c
test/CodeGen/arm-varargs.c
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||