[WebAssembly] Be a little more conservative in WebAssemblyFixFunctionBitcasts

[sbc100]

Summary

We don't have enough information to know if struct types being
bitcast will cause validation failures or not, so be conservative
and allow such cases to persist (not now).

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

[jgravelle-google]

The change feels a bit screwy to me, but I can't think of any alternative ways to factor this that aren't worse.

test/CodeGen/WebAssembly/function-bitcasts.ll

23 ↗

(On Diff #163204)

One too many spaces after call?

[alexcrichton]

In some local tests it looks like this fixes the issue we were seeing, thanks so much!

llvm/trunk/lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp

 //  - Call with fewer arguments than needed: arguments are filled in with undef
 //  - Return value is not needed: drop it
 //  - Return value needed but not present: supply an undef
 //
 // If the all the argument types of trivially castable to one another (i.e.
 // I32 vs pointer type) then we don't create a wrapper at all (return nullptr
 // instead).
 //
-// If there is a type mismatch that would result in an invalid wasm module
-// being written then generate wrapper that contains unreachable (i.e. abort
-// at runtime).  Such programs are deep into undefined behaviour territory,
+// If there is a type mismatch that we know would result in an invalid wasm
+// module then generate wrapper that contains unreachable (i.e. abort at 
+// runtime).  Such programs are deep into undefined behaviour territory,
 // but we choose to fail at runtime rather than generate and invalid module
 // or fail at compiler time.  The reason we delay the error is that we want
 // to support the CMake which expects to be able to compile and link programs
 // that refer to functions with entirely incorrect signatures (this is how
 // CMake detects the existence of a function in a toolchain).
+//
+// For bitcasts that involve struct types we don't know at this stage if they
+// would be equivalent at the wasm level and so we can't know if we need to 
+// generate a wrapper.
 static Function *CreateWrapper(Function *F, FunctionType *Ty) {
   Module *M = F->getParent();
 
   Function *Wrapper = Function::Create(Ty, Function::PrivateLinkage,
                                        F->getName() + "_bitcast", M);
   BasicBlock *BB = BasicBlock::Create(M->getContext(), "body", Wrapper);
   const DataLayout &DL = BB->getModule()->getDataLayout();
 
   // Determine what arguments to pass.
   SmallVector<Value *, 4> Args;
   Function::arg_iterator AI = Wrapper->arg_begin();
   Function::arg_iterator AE = Wrapper->arg_end();
   FunctionType::param_iterator PI = F->getFunctionType()->param_begin();
   FunctionType::param_iterator PE = F->getFunctionType()->param_end();
   bool TypeMismatch = false;
   bool WrapperNeeded = false;
 
+  Type *ExpectedRtnType = F->getFunctionType()->getReturnType();
+  Type *RtnType = Ty->getReturnType();
+
   if ((F->getFunctionType()->getNumParams() != Ty->getNumParams()) ||
-      (F->getFunctionType()->isVarArg() != Ty->isVarArg()))
+      (F->getFunctionType()->isVarArg() != Ty->isVarArg()) ||
+      (ExpectedRtnType != RtnType))
     WrapperNeeded = true;
 
   for (; AI != AE && PI != PE; ++AI, ++PI) {
     Type *ArgType = AI->getType();
     Type *ParamType = *PI;
 
     if (ArgType == ParamType) {
       Args.push_back(&*AI);
     } else {
       if (CastInst::isBitOrNoopPointerCastable(ArgType, ParamType, DL)) {
         Instruction *PtrCast =
             CastInst::CreateBitOrPointerCast(AI, ParamType, "cast");
         BB->getInstList().push_back(PtrCast);
         Args.push_back(PtrCast);
+      } else if (ArgType->isStructTy() || ParamType->isStructTy()) {
+        LLVM_DEBUG(dbgs() << "CreateWrapper: struct param type in bitcast: "
+                          << F->getName() << "\n");
+        WrapperNeeded = false;
       } else {
         LLVM_DEBUG(dbgs() << "CreateWrapper: arg type mismatch calling: "
                           << F->getName() << "\n");
         LLVM_DEBUG(dbgs() << "Arg[" << Args.size() << "] Expected: "
                           << *ParamType << " Got: " << *ArgType << "\n");
         TypeMismatch = true;
         break;
       }
     }
   }
 
-  if (!TypeMismatch) {
+  if (WrapperNeeded && !TypeMismatch) {
     for (; PI != PE; ++PI)
       Args.push_back(UndefValue::get(*PI));
     if (F->isVarArg())
       for (; AI != AE; ++AI)
         Args.push_back(&*AI);
 
     CallInst *Call = CallInst::Create(F, Args, "", BB);
 
     Type *ExpectedRtnType = F->getFunctionType()->getReturnType();
     Type *RtnType = Ty->getReturnType();
     // Determine what value to return.
     if (RtnType->isVoidTy()) {
       ReturnInst::Create(M->getContext(), BB);
-      WrapperNeeded = true;
     } else if (ExpectedRtnType->isVoidTy()) {
+      LLVM_DEBUG(dbgs() << "Creating dummy return: " << *RtnType << "\n");
       ReturnInst::Create(M->getContext(), UndefValue::get(RtnType), BB);
-      WrapperNeeded = true;
     } else if (RtnType == ExpectedRtnType) {
       ReturnInst::Create(M->getContext(), Call, BB);
     } else if (CastInst::isBitOrNoopPointerCastable(ExpectedRtnType, RtnType,
                                                     DL)) {
       Instruction *Cast =
           CastInst::CreateBitOrPointerCast(Call, RtnType, "cast");
       BB->getInstList().push_back(Cast);
       ReturnInst::Create(M->getContext(), Cast, BB);
+    } else if (RtnType->isStructTy() || ExpectedRtnType->isStructTy()) {
+      LLVM_DEBUG(dbgs() << "CreateWrapper: struct return type in bitcast: "
+                        << F->getName() << "\n");
+      WrapperNeeded = false;
     } else {
       LLVM_DEBUG(dbgs() << "CreateWrapper: return type mismatch calling: "
                         << F->getName() << "\n");
       LLVM_DEBUG(dbgs() << "Expected: " << *ExpectedRtnType
                         << " Got: " << *RtnType << "\n");
       TypeMismatch = true;
     }
   }
 
   if (TypeMismatch) {
+    // Create a new wrapper that simply contains `unreachable`.
+    Wrapper->eraseFromParent();
+    Wrapper = Function::Create(Ty, Function::PrivateLinkage, F->getName() + "_bitcast_invalid", M);
+    BasicBlock *BB = BasicBlock::Create(M->getContext(), "body", Wrapper);
     new UnreachableInst(M->getContext(), BB);
     Wrapper->setName(F->getName() + "_bitcast_invalid");
   } else if (!WrapperNeeded) {
     LLVM_DEBUG(dbgs() << "CreateWrapper: no wrapper needed: " << F->getName()
                       << "\n");
     Wrapper->eraseFromParent();
     return nullptr;
   }
+  LLVM_DEBUG(dbgs() << "CreateWrapper: " << F->getName() << "\n");
   return Wrapper;
 }
 
 bool FixFunctionBitcasts::runOnModule(Module &M) {
   Function *Main = nullptr;
   CallInst *CallMain = nullptr;
   SmallVector<std::pair<Use *, Function *>, 0> Uses;
   SmallPtrSet<Constant *, 2> ConstantBCs;

llvm/trunk/test/CodeGen/WebAssembly/function-bitcasts.ll

 ; RUN: llc < %s -asm-verbose=false -wasm-disable-explicit-locals -wasm-keep-registers -enable-emscripten-cxx-exceptions -wasm-temporary-workarounds=false | FileCheck %s
 
 ; Test that function pointer casts are replaced with wrappers.
 
 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32-unknown-unknown"
 
 declare void @has_i32_arg(i32)
+declare void @has_struct_arg({i32})
 declare i32 @has_i32_ret()
 declare void @vararg(...)
 declare void @plain(i32)
 
 declare void @foo0()
 declare void @foo1()
 declare void @foo2()
 declare void @foo3()
 
 ; CHECK-LABEL: test:
-; CHECK-NEXT: call        .Lhas_i32_arg_bitcast@FUNCTION{{$}}
-; CHECK-NEXT: call        .Lhas_i32_arg_bitcast@FUNCTION{{$}}
+; CHECK-NEXT: call        .Lhas_i32_arg_bitcast.2@FUNCTION{{$}}
+; CHECK-NEXT: call        .Lhas_i32_arg_bitcast.2@FUNCTION{{$}}
 ; CHECK-NEXT: call        .Lhas_i32_ret_bitcast@FUNCTION{{$}}
+; CHECK-NEXT: i32.call     $drop=, has_i32_ret@FUNCTION
 ; CHECK-NEXT: i32.const   $push[[L0:[0-9]+]]=, 0
 ; CHECK-NEXT: call        .Lfoo0_bitcast@FUNCTION, $pop[[L0]]{{$}}
 ; CHECK-NEXT: i32.const   $push[[L1:[0-9]+]]=, 0
 ; CHECK-NEXT: call        .Lfoo0_bitcast@FUNCTION, $pop[[L1]]{{$}}
 ; CHECK-NEXT: i32.const   $push[[L2:[0-9]+]]=, 0
 ; CHECK-NEXT: call        .Lfoo0_bitcast@FUNCTION, $pop[[L2]]{{$}}
 ; CHECK-NEXT: call        foo0@FUNCTION
 ; CHECK-NEXT: i32.call    $drop=, .Lfoo1_bitcast@FUNCTION{{$}}
 ; CHECK-NEXT: call        foo2@FUNCTION{{$}}
 ; CHECK-NEXT: call        foo1@FUNCTION{{$}}
 ; CHECK-NEXT: call        foo3@FUNCTION{{$}}
 ; CHECK-NEXT: end_function
 define void @test() {
 entry:
   call void bitcast (void (i32)* @has_i32_arg to void ()*)()
   call void bitcast (void (i32)* @has_i32_arg to void ()*)()
   call void bitcast (i32 ()* @has_i32_ret to void ()*)()
+  call i32 bitcast (i32 ()* @has_i32_ret to i32 ()*)()
   call void bitcast (void ()* @foo0 to void (i32)*)(i32 0)
   %p = bitcast void ()* @foo0 to void (i32)*
   call void %p(i32 0)
   %q = bitcast void ()* @foo0 to void (i32)*
   call void %q(i32 0)
   %r = bitcast void (i32)* %q to void ()*
   call void %r()
   %t = call i32 bitcast (void ()* @foo1 to i32 ()*)()
   call void bitcast (void ()* @foo2 to void ()*)()
   call void @foo1()
   call void @foo3()
 
   ret void
 }
 
+; CHECK-LABEL: test_structs:
+; CHECK: call     .Lhas_i32_arg_bitcast.1@FUNCTION, $pop{{[0-9]+}}, $pop{{[0-9]+$}}
+; CHECK: call     .Lhas_i32_arg_bitcast@FUNCTION, $0, $pop2
+; CHECK: call     .Lhas_struct_arg_bitcast@FUNCTION{{$}}
+define void @test_structs() {
+entry:
+  call void bitcast (void (i32)* @has_i32_arg to void (i32, {i32})*)(i32 5, {i32} {i32 6})
+  call {i32, i64} bitcast (void (i32)* @has_i32_arg to {i32, i64} (i32)*)(i32 7)
+  call void bitcast (void ({i32})* @has_struct_arg to void ()*)()
+  ret void
+}
+
+; CHECK-LABEL: test_structs_unhandled:
+; CHECK: call    has_struct_arg@FUNCTION, $pop{{[0-9]+$}}
+; CHECK: call    has_struct_arg@FUNCTION, $pop{{[0-9]+$}}
+; CHECK: call    has_i32_ret@FUNCTION, $pop{{[0-9]+$}}
+define void @test_structs_unhandled() {
+entry:
+  call void @has_struct_arg({i32} {i32 3})
+  call void bitcast (void ({i32})* @has_struct_arg to void ({i64})*)({i64} {i64 4})
+  call {i32, i32} bitcast (i32 ()* @has_i32_ret to {i32, i32} ()*)()
+  ret void
+}
+
 ; CHECK-LABEL: test_varargs:
 ; CHECK:      set_global
 ; CHECK:      i32.const   $push[[L3:[0-9]+]]=, 0{{$}}
 ; CHECK-NEXT: call        .Lvararg_bitcast@FUNCTION, $pop[[L3]]{{$}}
 ; CHECK-NEXT: i32.const   $push[[L4:[0-9]+]]=, 0{{$}}
 ; CHECK-NEXT: i32.store   0($[[L5:[0-9]+]]), $pop[[L4]]{{$}}
 ; CHECK-NEXT: call        .Lplain_bitcast@FUNCTION, $[[L5]]{{$}}
 define void @test_varargs() {

 
 ; CHECK-LABEL: test_invoke:
 ; CHECK:      i32.const   $push[[L1:[0-9]+]]=, call_func@FUNCTION{{$}}
 ; CHECK-NEXT: i32.const   $push[[L0:[0-9]+]]=, has_i32_ret@FUNCTION{{$}}
 ; CHECK-NEXT: call        "__invoke_void_i32()*"@FUNCTION, $pop[[L1]], $pop[[L0]]{{$}}
 ; CHECK:      i32.const   $push[[L3:[0-9]+]]=, call_func@FUNCTION{{$}}
 ; CHECK-NEXT: i32.const   $push[[L2:[0-9]+]]=, has_i32_arg@FUNCTION{{$}}
 ; CHECK-NEXT: call        "__invoke_void_i32()*"@FUNCTION, $pop[[L3]], $pop[[L2]]{{$}}
-; CHECK:      i32.const   $push[[L4:[0-9]+]]=, .Lhas_i32_arg_bitcast@FUNCTION{{$}}
+; CHECK:      i32.const   $push[[L4:[0-9]+]]=, .Lhas_i32_arg_bitcast.2@FUNCTION{{$}}
 ; CHECK-NEXT: call        __invoke_void@FUNCTION, $pop[[L4]]{{$}}
 declare i32 @personality(...)
 define void @test_invoke() personality i32 (...)* @personality {
 entry:
   invoke void @call_func(i32 ()* @has_i32_ret)
           to label %cont unwind label %lpad
 
 cont:
%0 = landingpad { i8*, i32 }
           catch i8* null
   br label %end
 
 end:
   ret void
 }
 
 ; CHECK-LABEL: .Lhas_i32_arg_bitcast:
+; CHECK-NEXT: .param      i32, i32
+; CHECK-NEXT: call        has_i32_arg@FUNCTION, $1{{$}}
+; CHECK-NEXT: end_function
+
+; CHECK-LABEL: .Lhas_i32_arg_bitcast.1:
+; CHECK-NEXT: .param      i32, i32
+; CHECK-NEXT: call        has_i32_arg@FUNCTION, $0{{$}}
+; CHECK-NEXT: end_function
+
+; CHECK-LABEL: .Lhas_i32_arg_bitcast.2:
 ; CHECK-NEXT: call        has_i32_arg@FUNCTION, $0{{$}}
 ; CHECK-NEXT: end_function
 
 ; CHECK-LABEL: .Lhas_i32_ret_bitcast:
 ; CHECK-NEXT: call        $drop=, has_i32_ret@FUNCTION{{$}}
 ; CHECK-NEXT: end_function
 
 ; CHECK-LABEL: .Lvararg_bitcast:

llvm/trunk/test/CodeGen/WebAssembly/unsupported-function-bitcasts.ll

 ; RUN: llc < %s -asm-verbose=false -wasm-keep-registers | FileCheck %s
 
 ; Test that function pointer casts that require conversions of arguments or
 ; return types are converted to unreachable.
 
 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32-unknown-unknown"
 
 declare i32 @has_i64_arg(i64)
 declare i32 @has_ptr_arg(i8*)
 
+; CHECK-LABEL: test_invalid_rtn:
+; CHECK-NEXT: i32.const   $push[[L0:[0-9]+]]=, 0{{$}}
+; CHECK-NEXT: i32.call $push[[L1:[0-9]+]]=, .Lhas_i64_arg_bitcast_invalid.2@FUNCTION, $pop[[L0]]{{$}}
+; CHECK-NEXT: drop $pop[[L1]]{{$}}
+; CHECK-NEXT: i64.const   $push[[L0:[0-9]+]]=, 0{{$}}
+; CHECK-NEXT: i64.call $push[[L1:[0-9]+]]=, .Lhas_i64_arg_bitcast_invalid@FUNCTION, $pop[[L0]]{{$}}
+; CHECK-NEXT: drop $pop[[L1]]{{$}}
+; CHECK-NEXT: end_function
 define void @test_invalid_rtn() {
 entry:
   call i32 bitcast (i32 (i64)* @has_i64_arg to i32 (i32)*)(i32 0)
+  call [1 x i64] bitcast (i32 (i64)* @has_i64_arg to [1 x i64] (i64)*)(i64 0)
   ret void
 }
-; CHECK-LABEL: test_invalid_rtn:
-; CHECK-NEXT: i32.const   $push[[L0:[0-9]+]]=, 0{{$}}
-; CHECK-NEXT: i32.call $push1=, .Lhas_i64_arg_bitcast_invalid@FUNCTION, $pop[[L0]]{{$}}
-; CHECK-NEXT: drop $pop1
-; CHECK-NEXT: end_function
 
-define void @test_invalid_arg() {
-entry:
-  call i32 bitcast (i32 (i8*)* @has_ptr_arg to i32 (i8)*)(i8 2)
-  call i32 bitcast (i32 (i8*)* @has_ptr_arg to i32 (i32)*)(i32 2)
-  call i32 bitcast (i32 (i8*)* @has_ptr_arg to i32 (i64)*)(i64 3)
+; CHECK-LABEL: test_struct_rtn:
+; CHECK:  call      has_i64_arg@FUNCTION, $pop6, $pop0
+define void @test_struct_rtn() {
+  call {i32, i32} bitcast (i32 (i64)* @has_i64_arg to {i32, i32} (i64)*)(i64 0)
   ret void
 }
 
 ; CHECK-LABEL: test_invalid_arg:
 ; CHECK-NEXT:   i32.const $push[[L0:[0-9]+]]=, 2{{$}}
-; CHECK-NEXT:   i32.call  $push[[L1:[0-9]+]]=, .Lhas_ptr_arg_bitcast_invalid.1@FUNCTION, $pop[[L0]]{{$}}
+; CHECK-NEXT:   i32.call  $push[[L1:[0-9]+]]=, .Lhas_ptr_arg_bitcast_invalid.4@FUNCTION, $pop[[L0]]{{$}}
 ; CHECK-NEXT:   drop  $pop[[L1]]{{$}}
 ; CHECK-NEXT:   i32.const $push[[L0:[0-9]+]]=, 2{{$}}
 ; CHECK-NEXT:   i32.call  $push[[L1:[0-9]+]]=, has_ptr_arg@FUNCTION, $pop[[L0]]{{$}}
 ; CHECK-NEXT:   drop  $pop[[L1]]{{$}}
 ; CHECK-NEXT:   i64.const $push[[L0:[0-9]+]]=, 3{{$}}
 ; CHECK-NEXT:   i32.call  $push[[L1:[0-9]+]]=, .Lhas_ptr_arg_bitcast_invalid@FUNCTION, $pop[[L0]]{{$}}
 ; CHECK-NEXT:   drop  $pop[[L1]]{{$}}
 ; CHECK-NEXT:   end_function
+define void @test_invalid_arg() {
+entry:
+  call i32 bitcast (i32 (i8*)* @has_ptr_arg to i32 (i8)*)(i8 2)
+  call i32 bitcast (i32 (i8*)* @has_ptr_arg to i32 (i32)*)(i32 2)
+  call i32 bitcast (i32 (i8*)* @has_ptr_arg to i32 (i64)*)(i64 3)
+  ret void
+}
 
 ; CHECK-LABEL: .Lhas_i64_arg_bitcast_invalid:
+; CHECK-NEXT:  .param   i64
+; CHECK-NEXT:  .result  i64
+; CHECK-NEXT:  unreachable
+; CHECK-NEXT:  end_function
+
+; CHECK-LABEL: .Lhas_i64_arg_bitcast_invalid.2:
 ; CHECK-NEXT:  .param   i32
 ; CHECK-NEXT:  .result  i32
 ; CHECK-NEXT:  unreachable
 ; CHECK-NEXT:  end_function
 
 ; CHECK-LABEL: .Lhas_ptr_arg_bitcast_invalid:
 ; CHECK-NEXT:   .param    i64
 ; CHECK-NEXT:   .result   i32
 ; CHECK-NEXT:   unreachable
 ; CHECK-NEXT:   end_function
 
-; CHECK-LABEL: .Lhas_ptr_arg_bitcast_invalid.1:
+; CHECK-LABEL: .Lhas_ptr_arg_bitcast_invalid.4:
 ; CHECK-NEXT:   .param    i32
 ; CHECK-NEXT:   .result   i32
 ; CHECK-NEXT:   unreachable
 ; CHECK-NEXT:   end_function