Fix small structures calling convention issue for some big endian architectures

[spetrovic]

Summary

This patch fixes problem with passing structures and unions smaller than register as argument in variadic functions
on big endian architectures.
For example passing 3 chars in a structure as argument of variadic function LLVM is passing those chars
left-adjusted in register and trying to read them like they are right-adjusted, and that makes the problem.
I have changed reading of those arguments. Now LLVM reads them properly (left-adjusted). I detected
this problem on ARM32 big endian, MIPS32 big endian and MIPS64 big endian.

[rjmccall]

Hmm. On MIPS64, a slot is 64 bits, right? How is a float passed?

[rjmccall]

Oh, floats are promoted to doubles in varargs, of course, which neatly makes that an impossible situation.

My inclination is that the right condition here is that only integer types should be right-justified in their slot, but I'll admit to not having an easy example of a type for which your condition doesn't work.

[dsanders]

This change agrees with what I think the calling convention is and the documentation. However, I've hit quite a few discrepancies between the documented calling convention and the de-facto one implemented by gcc so I'm wary of going by that alone.

Have you tried putting the caller and callee side of this test in different compilation units and then linking a gcc-compiled caller with a clang-compiled callee (and then repeating that for the other three combinations) like the test generator in tools/clang/utils/ABITest does? Does this produce a program that executes correctly for all four combinations?

Hmm. On MIPS64, a slot is 64 bits, right? How is a float passed?

Oh, floats are promoted to doubles in varargs, of course, which neatly makes that an impossible situation.

My inclination is that the right condition here is that only integer types should be right-justified in their slot, but I'll admit to not having an easy example of a type for which your condition doesn't work.

Floats are left justified according to the 'MIPSproTM N32 ABI Handbook' (which also discusses the O32 and N64 ABI's) but I can't think of a test case that would expose a problem due to the promotion to double.

test/CodeGen/struct-union-BE.c

1–3 ↗

(On Diff #61557)

Do we need %clang and -O2? Can we use %clang_cc1 and the default optimization level instead?

[spetrovic]

Yes, I tried all caller - callee combinations for ARM32 big endian and MIPS/MIPS64 big endian, and it works properly with this patch.

[dsanders]

In that case the MIPS side of this LGTM. Someone more familiar with ARM should approve it for ARM.

[john.brawn]

Looks OK from the ARM side as well.

cfe/trunk/lib/CodeGen/TargetInfo.cpp

static Address emitVoidPtrDirectVAArg(CodeGenFunction &CGF,
   CharUnits FullDirectSize = DirectSize.alignTo(SlotSize);
   llvm::Value *NextPtr =
     CGF.Builder.CreateConstInBoundsByteGEP(Addr.getPointer(), FullDirectSize,
                                            "argp.next");
   CGF.Builder.CreateStore(NextPtr, VAListAddr);
 
   // If the argument is smaller than a slot, and this is a big-endian
   // target, the argument will be right-adjusted in its slot.
-  if (DirectSize < SlotSize && CGF.CGM.getDataLayout().isBigEndian()) {
+  if (DirectSize < SlotSize && CGF.CGM.getDataLayout().isBigEndian() &&
+      !DirectTy->isStructTy()) {
     Addr = CGF.Builder.CreateConstInBoundsByteGEP(Addr, SlotSize - DirectSize);
   }
 
   Addr = CGF.Builder.CreateElementBitCast(Addr, DirectTy);
   return Addr;
 }
 
 /// Emit va_arg for a platform using the common void* representation,

cfe/trunk/test/CodeGen/struct-union-BE.c

+// RUN: %clang -O2 -target mips-linux-gnu  -EB -S -emit-llvm %s -o - | FileCheck %s -check-prefix=MIPS
+// RUN: %clang -O2 -target mips64-linux-gnu  -EB -S -emit-llvm %s -o - | FileCheck %s -check-prefix=MIPS64
+// RUN: %clang -O2 -target armeb-linux-gnueabihf -march=armv7a  -EB -S -emit-llvm %s -o - | FileCheck %s -check-prefix=ARM
+
+#include <stdarg.h>
+#include <stdlib.h>
+
+struct tiny {
+  char c;
+};
+
+union data {
+  char c;
+};
+
+void fstr(int n, ...) {
+  struct tiny x;
+  va_list ap;
+  va_start (ap,n);
+  x = va_arg (ap, struct tiny);
+  if (x.c !=  10)
+    abort();
+  va_end (ap);
+// MIPS-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i32 3
+// MIPS64-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i64 7
+// ARM-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i32 3
+}
+
+void funi(int n, ...) {
+  union data x;
+  va_list ap;
+  va_start (ap,n);
+  x = va_arg (ap, union data);
+  if (x.c !=  10)
+    abort();
+  va_end (ap);
+// MIPS-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i32 3
+// MIPS64-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i64 7
+// ARM-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i32 3
+}
+
+void foo() {
+  struct tiny x[3];
+  union data y;
+  x[0].c = 10;
+  fstr(1, x[0]);
+  funi(1, y);
+}