[PowerPC] Fix va_arg in C++, Objective-C on 32-bit ELF targets

[kernigh]

Summary

In the PPC32 SVR4 ABI, a va_list has copies of registers from the function call. va_arg looked in the wrong registers for (the pointer representation of) an object in Objective-C, and for some types in C++. Fix va_arg to look in the general-purpose registers, not the floating-point registers. Also fix va_arg for some C++ types, like a member function pointer, that are aggregates for the ABI.

Anthony Richardby found the problem in Objective-C. Eli Friedman suggested part of this fix.

Fixes https://bugs.llvm.org/show_bug.cgi?id=47921

[kernigh]

I'm using this TargetInfo.cpp diff in clang 10.0.1 on OpenBSD/macppc to build GNUstep. For me, it fixes the build of gnustep-gui. I rebuilt libobjc2, gnustep-make, and gnustep-base before building gnustep-gui.

This diff doesn't change 64-bit PowerPC, because 64-bit does va_list in a different way.

[efriedma]

clang/lib/CodeGen/TargetInfo.cpp

4723

I suspect this code doesn't handle C++ member pointers correctly. But maybe we can leave that for a followup.

Could we simplify this to bool isInt = !Ty->isFloatingType();?

[kernigh]

clang/lib/CodeGen/TargetInfo.cpp

4723

Yes, C++ member pointers are broken. I declared a struct animal, tried typedef void (animal::*noise)(); ... va_arg(ap, noise), and it segfaulted. In the disassembly, I saw 2 problems: (1) va_arg was looking at the floating-point registers, and (2) va_arg was not indirecting through a pointer. The caller had passed a pointer to a noise in a general-purpose register.

I'm not sure about bool isInt = !Ty->isFloatingType();, because I haven't checked whether it would break other types.

[kernigh]

clang/lib/CodeGen/TargetInfo.cpp

4723

bool isInt = !Ty->isFloatingType(); is working for me. I'm doing more checks; if it continues to work, I will update this diff.

Another change in bool isIndirect = Ty->isAggregateType(); to instead call isAggregateTypeForABI(Ty) is fixing va_arg of a C++ member function pointer. I may want to put this change in the same diff because I will be running both changes together.

[kernigh]

I have updated the diff to use bool isInt = !Ty->isFloatingType(); and bool isIndirect = isAggregateTypeForABI(Ty);. This removes both calls to Ty->isAggregateType(). This fixes some C++ types, because aren't aggregates in the C++ language, but do become aggregates in the ABI. The new test ppc32-varargs-method.cpp checks a pointer to a member function, and passes only with both the isInt and isIndirect changes.

[brad]

@efriedma Eli?

[efriedma]

LGTM with one minor comment.

Do you have commit access? If not, I can merge for you; please specify your preferred git "Author" line.

clang/test/CodeGenCXX/ppc32-varargs-method.cpp

15 ↗

(On Diff #305335)

Not sure referring to numUsedRegs like this will work in a non-Asserts build. Please verify.

[kernigh]

Hi, Eli. I'm missing emails from Phabricator, so I didn't know about your recent post. I will respond to your question about numUsedRegs when I find time.

[nemanjai]

LGTM aside from a minor nit.

clang/lib/CodeGen/TargetInfo.cpp

4722–4723

nit (feel free to ignore): seems like a better name might be something like isSingleGPR since it would appear that this is for types that go into a (single) general purpose register.

[kernigh]

I forgot about this diff for a month.

clang/lib/CodeGen/TargetInfo.cpp

4722–4723

Hi. isInt is also true for a 64-bit integer, which goes into a pair of GPRs, because each register has 32 bits.

clang/test/CodeGenCXX/ppc32-varargs-method.cpp

15 ↗

(On Diff #305335)

I did a build with cmake -DLLVM_ENABLE_ASSERTIONS=OFF and verified that the ppc32*varargs* tests still pass. %numUsedRegs is still in the llvm output.

%numUsedRegs does not appear in the output of clang -S -emit-llvm ... (not cc1), but %numUsedRegs does appear in the output of clang -cc1 .... The test runs cc1 and sees the %numUsedRegs. I am confused by how llvm sometimes erases the name of %numUsedRegs and sometimes keeps the name, but I observe that turning LLVM_ENABLE_ASSERTIONS on or off doesn't affect the test.

clang/lib/CodeGen/TargetInfo.cpp

Address PPC32_SVR4_ABIInfo::EmitVAArg(CodeGenFunction &CGF, Address VAList,
   //   unsigned char gpr;
   //   unsigned char fpr;
   //   unsigned short reserved;
   //   void *overflow_arg_area;
   //   void *reg_save_area;
   // };
 
   bool isI64 = Ty->isIntegerType() && getContext().getTypeSize(Ty) == 64;
-  bool isInt =
-      Ty->isIntegerType() || Ty->isPointerType() || Ty->isAggregateType();
+  bool isInt = Ty->isIntegerType() || Ty->hasPointerRepresentation() ||
+               Ty->isAggregateType();
   bool isF64 = Ty->isFloatingType() && getContext().getTypeSize(Ty) == 64;
 
   // All aggregates are passed indirectly?  That doesn't seem consistent
   // with the argument-lowering code.
   bool isIndirect = Ty->isAggregateType();
 
   CGBuilderTy &Builder = CGF.Builder;
 

clang/test/CodeGenObjC/ppc32-varargs-id.m

+// REQUIRES: powerpc-registered-target
+// RUN: %clang_cc1 -triple powerpc-unknown-openbsd -fblocks -emit-llvm -o - %s | FileCheck %s
+
+#include <stdarg.h>
+
+id testObject(va_list ap) {
+  return va_arg(ap, id);
+}
+// CHECK: @testObject
+// CHECK: using_regs:
+// CHECK-NEXT: getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %{{[0-9]+}}, i32 0, i32 4
+// CHECK-NEXT: load i8*, i8** %{{[0-9]+}}, align 4
+// CHECK-NEXT: mul i8 %numUsedRegs, 4
+// CHECK-NEXT: getelementptr inbounds i8, i8* %{{[0-9]+}}, i8 %{{[0-9]+}}
+// CHECK-NEXT: bitcast i8* %{{[0-9]+}} to i8**
+// CHECK-NEXT: add i8 %numUsedRegs, 1
+// CHECK-NEXT: store i8 %{{[0-9]+}}, i8* %gpr, align 4
+// CHECK-NEXT: br label %cont
+
+typedef void (^block)(void);
+block testBlock(va_list ap) {
+  return va_arg(ap, block);
+}
+// CHECK: @testBlock
+// CHECK: using_regs:
+// CHECK-NEXT: getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %{{[0-9]+}}, i32 0, i32 4
+// CHECK-NEXT: load i8*, i8** %{{[0-9]+}}, align 4
+// CHECK-NEXT: mul i8 %numUsedRegs, 4
+// CHECK-NEXT: getelementptr inbounds i8, i8* %{{[0-9]+}}, i8 %{{[0-9]+}}
+// CHECK-NEXT: bitcast i8* %{{[0-9]+}} to void ()**
+// CHECK-NEXT: add i8 %numUsedRegs, 1
+// CHECK-NEXT: store i8 %{{[0-9]+}}, i8* %gpr, align 4
+// CHECK-NEXT: br label %cont