Verifier: Don't reject varargs callee cleanup functions

[rnk]

Summary

We've rejected these kinds of functions since r28405 in 2006 because
it's impossible to lower the return of a callee cleanup varargs
function. However there are lots of legal ways to leave such a function
without returning, such as aborting. Today we can leave a function with
a musttail call to another function with the correct prototype, and
everything works out.

Therefore I'd like to remove the verifier check just say that a normal
return from such a function is UB.

[rnk]

Add missing file

[rnk]

• Fix calls to callee cleanup varargs functions and test it

[nlewycky]

Hunh. I'd think I'd prefer it if the verifier could check that the function defined callee cleanup and variadic doesn't return (unless the return is a musttail). I dislike making musttail even more special, but I think I just have to learn to deal with it.

lib/Target/X86/X86ISelLowering.cpp

3550 ↗

(On Diff #13055)

Isn't there a fallthrough annotation or comment to use here?

[rnk]

Clang also used to generate code like:

define x86_thiscallcc i32 @my_vmemptr(i8*, i32, i32) {
entry:
  %retslot = alloca i32
  %r = musttail call @my_target(i8* %0, i32 %1, i32 %2)
  ret i32 %r
exit: ; no predecessors!
  %ret = load i32* %retslot
  ret i32 %ret
}

While we don't do that today, should the verifier reject such code? Should it ignore unreachable blocks?

We could report_fatal_error in TLI::LowerReturn if isVarArgs&&isCalleeCleanup, but then we would abort on this precise example at -O0.

You can also imagine replacing the musttail call with a call to abort + unreachable.

lib/Target/X86/X86ISelLowering.cpp

3550 ↗

(On Diff #13055)

I thought it was OK to have multiple case labels one after another without annotating fallthrough. The whitespace in phab isn't actually present in the code, see the missing line number on the left.

[rnk]

Add a verifier check for normal returns

[nlewycky]

Yup, I've changed my mind.

A pass should be able to fold "br i1 false, label %bb1, label %bb2" into "br label %bb2" without worrying about making the module fail verification. Hence, testing for "isReachableFromEntry" is a bad idea here. Please allow it, then commit it.

[rnk]

Closed by commit rL216779 (authored by @rnk).

[rnk]

In D5059#14, @nlewycky wrote:

Yup, I've changed my mind.

A pass should be able to fold "br i1 false, label %bb1, label %bb2" into "br label %bb2" without worrying about making the module fail verification. Hence, testing for "isReachableFromEntry" is a bad idea here. Please allow it, then commit it.

OK, sounds good. :) Thanks!

llvm/trunk/lib/IR/Verifier.cpp

void Verifier::visitFunction(const Function &F) {
   // On function declarations/definitions, we do not support the builtin
   // attribute. We do not check this in VerifyFunctionAttrs since that is
   // checking for Attributes that can/can not ever be on functions.
   Assert1(!Attrs.hasAttribute(AttributeSet::FunctionIndex,
                               Attribute::Builtin),
           "Attribute 'builtin' can only be applied to a callsite.", &F);
 
   // Check that this function meets the restrictions on this calling convention.
+  // Sometimes varargs is used for perfectly forwarding thunks, so some of these
+  // restrictions can be lifted.
   switch (F.getCallingConv()) {
   default:
-    break;
   case CallingConv::C:
     break;
   case CallingConv::Fast:
   case CallingConv::Cold:
-  case CallingConv::X86_FastCall:
-  case CallingConv::X86_ThisCall:
   case CallingConv::Intel_OCL_BI:
   case CallingConv::PTX_Kernel:
   case CallingConv::PTX_Device:
-    Assert1(!F.isVarArg(),
-            "Varargs functions must have C calling conventions!", &F);
+    Assert1(!F.isVarArg(), "Calling convention does not support varargs or "
+                           "perfect forwarding!", &F);
     break;
   }
 
   bool isLLVMdotName = F.getName().size() >= 5 &&
                        F.getName().substr(0, 5) == "llvm.";
 
   // Check that the argument values match the function type for this function...
   unsigned i = 0;

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

bool X86::isOffsetSuitableForCodeModel(int64_t Offset, CodeModel::Model M,
 
   return false;
 }
 
 /// isCalleePop - Determines whether the callee is required to pop its
 /// own arguments. Callee pop is necessary to support tail calls.
 bool X86::isCalleePop(CallingConv::ID CallingConv,
                       bool is64Bit, bool IsVarArg, bool TailCallOpt) {
-  if (IsVarArg)
-    return false;
-
   switch (CallingConv) {
   default:
     return false;
   case CallingConv::X86_StdCall:
-    return !is64Bit;
   case CallingConv::X86_FastCall:
-    return !is64Bit;
   case CallingConv::X86_ThisCall:
     return !is64Bit;
   case CallingConv::Fast:
-    return TailCallOpt;
   case CallingConv::GHC:
-    return TailCallOpt;
   case CallingConv::HiPE:
+    if (IsVarArg)
+      return false;
     return TailCallOpt;
   }
 }
 
 /// \brief Return true if the condition is an unsigned comparison operation.
 static bool isX86CCUnsigned(unsigned X86CC) {
   switch (X86CC) {
   default: llvm_unreachable("Invalid integer condition!");

llvm/trunk/test/CodeGen/X86/vararg-callee-cleanup.ll

+; RUN: llc -mtriple=i686-pc-windows < %s | FileCheck %s
+
+target datalayout = "e-m:w-p:32:32-i64:64-f80:32-n8:16:32-S32"
+
+declare x86_thiscallcc void @thiscall_thunk(i8* %this, ...)
+define i32 @call_varargs_thiscall_thunk(i8* %a, i32 %b, i32 %c, i32 %d) {
+  call x86_thiscallcc void (i8*, ...)* @thiscall_thunk(i8* %a, i32 1, i32 2)
+  call x86_thiscallcc void (i8*, ...)* @thiscall_thunk(i8* %a, i32 1, i32 2)
+  %t1 = add i32 %b, %c
+  %r = add i32 %t1, %d
+  ret i32 %r
+}
+
+; CHECK: _call_varargs_thiscall_thunk:
+; CHECK: calll _thiscall_thunk
+; CHECK-NEXT: subl $8, %esp
+
+; We don't mangle the argument size into variadic callee cleanup functions.
+
+declare x86_stdcallcc void @stdcall_thunk(i8* %this, ...)
+define i32 @call_varargs_stdcall_thunk(i8* %a, i32 %b, i32 %c, i32 %d) {
+  call x86_stdcallcc void (i8*, ...)* @stdcall_thunk(i8* %a, i32 1, i32 2)
+  call x86_stdcallcc void (i8*, ...)* @stdcall_thunk(i8* %a, i32 1, i32 2)
+  %t1 = add i32 %b, %c
+  %r = add i32 %t1, %d
+  ret i32 %r
+}
+
+; CHECK: _call_varargs_stdcall_thunk:
+; CHECK: calll _stdcall_thunk{{$}}
+; CHECK-NEXT: subl $12, %esp
+
+declare x86_fastcallcc void @fastcall_thunk(i8* %this, ...)
+define i32 @call_varargs_fastcall_thunk(i8* %a, i32 %b, i32 %c, i32 %d) {
+  call x86_fastcallcc void (i8*, ...)* @fastcall_thunk(i8* inreg %a, i32 inreg 1, i32 2)
+  call x86_fastcallcc void (i8*, ...)* @fastcall_thunk(i8* inreg %a, i32 inreg 1, i32 2)
+  %t1 = add i32 %b, %c
+  %r = add i32 %t1, %d
+  ret i32 %r
+}
+
+; CHECK: _call_varargs_fastcall_thunk:
+; CHECK: calll @fastcall_thunk{{$}}
+; CHECK-NEXT: subl $4, %esp
+
+; If you actually return from such a thunk, it will only pop the non-variadic
+; portion of the arguments, which is different from what the callee passes.
+
+define x86_stdcallcc void @varargs_stdcall_return(i32, i32, ...) {
+  ret void
+}
+
+; CHECK: _varargs_stdcall_return:
+; CHECK: retl $8

llvm/trunk/test/Verifier/musttail-valid.ll

 ; RUN: llvm-as %s -o /dev/null
 
 ; Should assemble without error.
 
 declare void @similar_param_ptrty_callee(i8*)
 define void @similar_param_ptrty(i32*) {
   musttail call void @similar_param_ptrty_callee(i8* null)
   ret void
 }
 
 declare i8* @similar_ret_ptrty_callee()
 define i32* @similar_ret_ptrty() {
   %v = musttail call i8* @similar_ret_ptrty_callee()
   %w = bitcast i8* %v to i32*
   ret i32* %w
 }
+
+declare x86_thiscallcc void @varargs_thiscall(i8*, ...)
+define x86_thiscallcc void @varargs_thiscall_thunk(i8* %this, ...) {
+  musttail call x86_thiscallcc void (i8*, ...)* @varargs_thiscall(i8* %this, ...)
+  ret void
+}
+
+declare x86_fastcallcc void @varargs_fastcall(i8*, ...)
+define x86_fastcallcc void @varargs_fastcall_thunk(i8* %this, ...) {
+  musttail call x86_fastcallcc void (i8*, ...)* @varargs_fastcall(i8* %this, ...)
+  ret void
+}
+
+define x86_thiscallcc void @varargs_thiscall_unreachable(i8* %this, ...) {
+  unreachable
+}
+
+define x86_thiscallcc void @varargs_thiscall_ret_unreachable(i8* %this, ...) {
+  musttail call x86_thiscallcc void (i8*, ...)* @varargs_thiscall(i8* %this, ...)
+  ret void
+bb1:
+  ret void
+}