Page MenuHomePhabricator

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

Authored by yronglin on Nov 22 2022, 10:16 AM.

Details

Summary

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

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

Diff Detail

Event Timeline

yronglin created this revision.Nov 22 2022, 10:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2022, 10:16 AM
yronglin requested review of this revision.Nov 22 2022, 10:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2022, 10:16 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
inclyc added a subscriber: inclyc.Nov 23 2022, 3:25 AM

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

yronglin added a comment.EditedNov 29 2022, 10:12 AM

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 updated this revision to Diff 478633.Nov 29 2022, 10:36 AM

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

rjmccall accepted this revision.Dec 5 2022, 12:28 PM

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.

This revision is now accepted and ready to land.Dec 5 2022, 12:28 PM

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 updated this revision to Diff 480853.Dec 7 2022, 4:35 AM

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

yronglin requested review of this revision.Dec 7 2022, 4:53 AM

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

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

This revision was not accepted when it landed; it landed in state Needs Review.Dec 20 2022, 6:06 AM
This revision was automatically updated to reflect the committed changes.