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–3551

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–3551

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!

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;
void Verifier::visitFunction(const Function &F) {
   }
 
   Assert1(!F.hasDLLImportStorageClass() ||
           (F.isDeclaration() && F.hasExternalLinkage()) ||
           F.hasAvailableExternallyLinkage(),
           "Function is marked as dllimport, but not external.", &F);
 }
 
+static bool isCalleeCleanup(CallingConv::ID CC) {
+  switch (CC) {
+  case CallingConv::X86_FastCall:
+  case CallingConv::X86_StdCall:
+  case CallingConv::X86_ThisCall:
+    return true;
+  default:
+  case CallingConv::C:
+    return false;
+  }
+  llvm_unreachable("invalid enum");
+}
+
 // verifyBasicBlock - Verify that a basic block is well formed...
 //
 void Verifier::visitBasicBlock(BasicBlock &BB) {
   InstsInThisBlock.clear();
 
   // Ensure that basic blocks have terminators!
   Assert1(BB.getTerminator(), "Basic Block does not have terminator!", &BB);
 
void Verifier::visitBranchInst(BranchInst &BI) {
   if (BI.isConditional()) {
     Assert2(BI.getCondition()->getType()->isIntegerTy(1),
             "Branch condition is not 'i1' type!", &BI, BI.getCondition());
   }
   visitTerminatorInst(BI);
 }
 
 void Verifier::visitReturnInst(ReturnInst &RI) {
-  Function *F = RI.getParent()->getParent();
+  BasicBlock *BB = RI.getParent();
+  Function *F = BB->getParent();
   unsigned N = RI.getNumOperands();
   if (F->getReturnType()->isVoidTy())
     Assert2(N == 0,
             "Found return instr that returns non-void in Function of void "
             "return type!", &RI, F->getReturnType());
   else
     Assert2(N == 1 && F->getReturnType() == RI.getOperand(0)->getType(),
             "Function return type does not match operand "
             "type of return inst!", &RI, F->getReturnType());
 
+  // Verify that varargs callee cleanup functions never do a normal return. All
+  // returns must be preceded by a musttail call, or be in trivially unreachable
+  // blocks.
+  if (F->isVarArg() && isCalleeCleanup(F->getCallingConv())) {
+    Assert1(BB->getTerminatingMustTailCall() != nullptr ||
+                !DT.isReachableFromEntry(BB),
+            "Found impossible return in varargs callee cleanup function!", &RI);
+  }
+
   // Check to make sure that the return value has necessary properties for
   // terminators...
   visitTerminatorInst(RI);
 }
 
 void Verifier::visitSwitchInst(SwitchInst &SI) {
   // Check to make sure that all of the constants in the switch instruction
   // have the same type as the switched-on value.

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!");

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

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
 }
+
+; These should verify. Only normal, reachable returns should fail verification.
+
+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
+}

test/Verifier/varargs-callee-cleanup.ll

+; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
+
+define x86_stdcallcc void @stdcall_varargs(i8*, ...) {
+  ret void
+}
+; CHECK: Found impossible return in varargs callee cleanup function!
+
+define x86_thiscallcc void @thiscall_varargs(i8*, ...) {
+  ret void
+}
+; CHECK: Found impossible return in varargs callee cleanup function!
+
+define x86_fastcallcc void @fastcall_varargs(i8*, ...) {
+  ret void
+}
+; CHECK: Found impossible return in varargs callee cleanup function!