[CodeGen][AArch64] Fix AArch64ABIInfo::EmitAAPCSVAArg crash with empty record type in variadic arg

[yronglin]

Summary

Fix AArch64ABIInfo::EmitAAPCSVAArg crash with empty record type in variadic arg

Open issue: https://github.com/llvm/llvm-project/issues/59034

[yronglin]

pin~

[efriedma]

It looks like AArch64ABIInfo::classifyArgumentType does a slightly more complicated check for "empty"; does that matter here?

[yronglin]

In D138511#3954143, @efriedma wrote:

It looks like AArch64ABIInfo::classifyArgumentType does a slightly more complicated check for "empty"; does that matter here?

Thanks for your comments @efriedma. Good catch! Current patch has a issue in this case(C++ mode):

clang -Xclang -no-opaque-pointers -target aarch64-linux-gnu -emit-llvm -S ./va_arg.cpp

typedef __builtin_va_list va_list;
#define va_start(ap, param) __builtin_va_start(ap, param)
#define va_end(ap)          __builtin_va_end(ap)
#define va_arg(ap, type)    __builtin_va_arg(ap, type)

va_list the_list;
typedef struct {} empty;
empty empty_record_test(void) {
  return va_arg(the_list, empty);
}

When use isEmptyRecord directly to check empty record, the generated IR is:

define dso_local void @_Z17empty_record_testv() #0 {
entry:
  %0 = load i8*, i8** getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 0), align 8
  %1 = bitcast i8* %0 to %struct.empty*
  ret void
}

When use AArch64ABIInfo::classifyArgumentType(...).isIgnore() to check empty record, the generated IR is:

define dso_local void @_Z17empty_record_testv() #0 {
entry:
  %gr_offs = load i32, i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3), align 8
  %0 = icmp sge i32 %gr_offs, 0
  br i1 %0, label %vaarg.on_stack, label %vaarg.maybe_reg

vaarg.maybe_reg:                                  ; preds = %entry
  %new_reg_offs = add i32 %gr_offs, 8
  store i32 %new_reg_offs, i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3), align 8
  %inreg = icmp sle i32 %new_reg_offs, 0
  br i1 %inreg, label %vaarg.in_reg, label %vaarg.on_stack

vaarg.in_reg:                                     ; preds = %vaarg.maybe_reg
  %reg_top = load i8*, i8** getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 1), align 8
  %1 = getelementptr inbounds i8, i8* %reg_top, i32 %gr_offs
  %2 = bitcast i8* %1 to %struct.empty2*
  br label %vaarg.end

vaarg.on_stack:                                   ; preds = %vaarg.maybe_reg, %entry
  %stack = load i8*, i8** getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 0), align 8
  %new_stack = getelementptr inbounds i8, i8* %stack, i64 8
  store i8* %new_stack, i8** getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 0), align 8
  %3 = bitcast i8* %stack to %struct.empty2*
  br label %vaarg.end

vaarg.end:                                        ; preds = %vaarg.on_stack, %vaarg.in_reg
  %vaargs.addr = phi %struct.empty2* [ %2, %vaarg.in_reg ], [ %3, %vaarg.on_stack ]
  ret void
}

As far as I know, according to the comments of the AArch64ABIInfo::classifyArgumentType(...), "Empty records are always ignored on Darwin, but actually passed in C++ mode elsewhere for GNU compatibility." and "GNU C mode. The only argument that gets ignored is an empty one with size 0.", I think we should ignore arg only if on Darwin or the type of arg is an empty record in C mode. What do you all think about?

[yronglin]

Use AArch64ABIInfo::classifyArgumentType().isIgnore() to check empty record vaarg

[yronglin]

pin~

[yronglin]

pin~

[yronglin]

pin~

[rjmccall]

Please don't ping every day. We haven't lost track of your patch, we're just busy reviewing other things.

This seems reasonable to me.

[yronglin]

In D138511#3971913, @rjmccall wrote:

Please don't ping every day. We haven't lost track of your patch, we're just busy reviewing other things.

This seems reasonable to me.

Thanks very much for your review @rjmccall !

[yronglin]

Rebase and apply changes from https://reviews.llvm.org/D138295

[efriedma]

Not sure what this is waiting on; do you need someone to merge for you?

[yronglin]

Thanks @efriedma , I'm wait to see if there is any objection before landing it.

[yronglin]

commit in ebe530ef7a49988a531a8928eac3ce0925f1c199

clang/lib/CodeGen/TargetInfo.cpp

bool AArch64ABIInfo::isZeroLengthBitfieldPermittedInHomogeneousAggregate()
   // being homogeneous.
   return true;
 }
 
 Address AArch64ABIInfo::EmitAAPCSVAArg(Address VAListAddr, QualType Ty,
                                        CodeGenFunction &CGF) const {
   ABIArgInfo AI = classifyArgumentType(Ty, /*IsVariadic=*/true,
                                        CGF.CurFnInfo->getCallingConvention());
+  // Empty records are ignored for parameter passing purposes.
+  if (AI.isIgnore()) {
+    uint64_t PointerSize = getTarget().getPointerWidth(0) / 8;
+    CharUnits SlotSize = CharUnits::fromQuantity(PointerSize);
+    VAListAddr = CGF.Builder.CreateElementBitCast(VAListAddr, CGF.Int8PtrTy);
+    auto *Load = CGF.Builder.CreateLoad(VAListAddr);
+    Address Addr = Address(Load, CGF.Int8Ty, SlotSize);
+    return CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));
+  }
+
   bool IsIndirect = AI.isIndirect();
 
   llvm::Type *BaseTy = CGF.ConvertType(Ty);
   if (IsIndirect)
     BaseTy = llvm::PointerType::getUnqual(BaseTy);
   else if (AI.getCoerceToType())
     BaseTy = AI.getCoerceToType();
 

clang/test/CodeGen/aarch64-varargs.c

// CHECK-LABEL: define{{.*}} void @check_start(i32 noundef %n, ...)
 
   va_list the_list;
   va_start(the_list, n);
 // CHECK: [[THE_LIST:%[a-z_0-9]+]] = alloca %struct.__va_list
 // CHECK: [[VOIDP_THE_LIST:%[a-z_0-9]+]] = bitcast %struct.__va_list* [[THE_LIST]] to i8*
 // CHECK: call void @llvm.va_start(i8* [[VOIDP_THE_LIST]])
 }
 
-
+typedef struct {} empty;
+empty empty_record_test(void) {
+// CHECK-LABEL: define{{.*}} void @empty_record_test()
+  return va_arg(the_list, empty);
+// CHECK: [[GR_OFFS:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 0)
+// CHECK: [[ADDR:%[a-z._0-9]+]] = bitcast i8* [[GR_OFFS]] to %struct.empty*
+}