Page MenuHomePhabricator

[ARM][AArch64] Fix incorrect handling of alignment in va_arg code generation
ClosedPublic

Authored by john.brawn on Mon, May 20, 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

Repository
rC Clang

Event Timeline

john.brawn created this revision.Mon, May 20, 9:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, May 20, 9:41 AM

Please verify my understanding of the following is correct:

  1. getTypeUnadjustedAlign() is currently only used to compute calling convention alignment for ARM and AArch64.
  2. Without this patch, we use the unadjusted alignment to pass arguments, but the adjusted alignment to compute the alignment for the corresponding va_arg calls.
  3. Without this patch, the unadjusted alignment for non-struct types is actually adjusted based on attributes on typedefs.

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?

carwil added a subscriber: carwil.Tue, May 21, 3:42 AM

Please verify my understanding of the following is correct:

  1. getTypeUnadjustedAlign() is currently only used to compute calling convention alignment for ARM and AArch64.

Yes, the only places it's called are AArch64ABIInfo::classifyArgumentType and ARMABIInfo::classifyArgumentType.

  1. Without this patch, we use the unadjusted alignment to pass arguments, but the adjusted alignment to compute the alignment for the corresponding va_arg calls.

Yes.

  1. Without this patch, the unadjusted alignment for non-struct types is actually adjusted based on attributes on typedefs.

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.

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?

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).

efriedma accepted this revision.Tue, May 21, 11:52 AM

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.Tue, May 21, 11:52 AM
This revision was automatically updated to reflect the committed changes.