[WebAssembly] Ensure all bitcasts that would be invalid in wasm are removed by FixFunctionBitcasts pass

[jgravelle-google]

lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp

147 ↗

(On Diff #156175)

Doesn't seem to fail in this case. Probably worth leaving the debug print though, just changing the wording to be more accurate.

150 ↗

(On Diff #156175)

Should annotate the false values with the argument names, like /*SrcIsSigned = */ false

156 ↗

(On Diff #156175)

Maybe debug-print something about how this branch successfully created a wrapper? We should probably print something along all the possible execution paths for this function.

test/CodeGen/WebAssembly/function-bitcasts-varargs.ll

23 ↗

(On Diff #156175)

This is probably less fragile test-wise, as well as being easier to read later.

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

25 ↗

(On Diff #156175)

Should add this test elsewhere, along with other mismatched-return tests (float-to-int comes to mind). We do support return value conversions now, so we should have tests that exercise it.

[sunfish]

Not all types are valid to cast to all other types. But beyond that, normal native platforms don't insert non-trivial casts in such situations, so even in many cases where it's valid to cast, it doesn't seem like programmers could reasonably expect a non-trivial cast to happen. What kinds of use cases are motivating this?

[sbc100]

My goal here is not so much to make it "work" as to not produce invalid webassembly. i.e. it should compile, even it if fails at runtime to do anything useful/expected.

The motivating use case is the way 'cmake' detects functions: https://github.com/Kitware/CMake/blob/master/Modules/CheckFunctionExists.c

I tries to compile this little program (but not run it) where the function in question is declared as "int XXX()" regardless of the actual signature. This fails when detecting, for example strtod becasue the resulting wasm binary doesn't validate (which means wasm-emscripten-finalize can't load it which mean a link failure for emccc).

[sbc100]

When you say not all types are valid to cast to all others.. how would I know which are valid? And how would this code fail if they are invalid? Would llvm bail out during the call to getCastOpcode()?

I ideally we would never actually produce invalid wasm and prefer to fail to compile.. but perhaps that should be done later on during codegen rather that during this pass?

[sunfish]

Ah, that's useful context, thanks. That'd be good to mention in the commit message and/or comments somewhere. Also, please add a testcase which approximates that use case.

getCastOpcode asserts if you ask it for something invalid. However, what about an approach like this:

• Handle the case of a pointer-to-pointer bitcast, or vector-to-vector bitcast, that we know will always be an actual no-op, and users could maybe expect that to work.
• Convert any other case to a call to @abort.

That way it'll still compile, so if I understand correctly, the CMake test will still work. Anything that actually aborts would be well into undefined behavior territory. And, this avoids our having to support the general case of non-trivial casts over the long term. Thoughts?

[sbc100]

Interesting, so you are proposing delaying the error until link time but still producing a valid wasm binary?

What about integer conversions? Sounds like you are saying they should also abort?

[sunfish]

In D49517#1172783, @sbc100 wrote:

Interesting, so you are proposing delaying the error until link time but still producing a valid wasm binary?

Yeah. I don't always like delaying errors, but it sounds like that's what we need to make the CMake use case work, right? If so, I'm ok with that here.

What about integer conversions? Sounds like you are saying they should also abort?

Yeah. Unless I'm missing something, this should be very rare in practice, and it's certainly undefined behavior, so hopefully it won't be important.

[sbc100]

gererate runtime calls to abort rather than failing to compile
add more tests

[sbc100]

• revert

[sbc100]

[sbc100]

• remove logging

[sbc100]

PTAL

[dschuff]

I like this approach too, assuming it's sufficient to handle the CMake (and autoconf?) ugliness. I am a bit concerned that this pass will become a large and difficult-to-maintain pile of heuristics with different purposes, so I think we should be as clear as possible about what use cases we are trying to handle.
Other than the comment nit, the code looks fine too.

lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp

113 ↗

(On Diff #158579)

I actually disagree that failing at runtime is preferable, I would much rather fail as early as possible :)
I wouldn't say (as you do on line 117) that C in general expects to be able to compile and link programs with incorrect signatures; that's UB in C.

Having said that of course, some programmers may expect this because C has no name mangling and you can usually get away with it, and in particular CMake expects this and that's an important enough use case that we are willing to do this anyway. So we should just come out and say that in the comment :)

I do agree though that generating an invalid wasm module is a bad failure mode. Delaying the error until link time does seem preferable to that.

[sbc100]

• update comment

[alexcrichton]

I'm a bit late to this diff but I've bisected a regression in the Rust repo to this revision -- https://bugs.llvm.org/show_bug.cgi?id=38711

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

 
 // Create a wrapper function with type Ty that calls F (which may have a
 // different type). Attempt to support common bitcasted function idioms:
 //  - Call with more arguments than needed: arguments are dropped
 //  - 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
 //
-// For now, return nullptr without creating a wrapper if the wrapper cannot
-// be generated due to incompatible types.
+// 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,
+// 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).
 static Function *CreateWrapper(Function *F, FunctionType *Ty) {
   Module *M = F->getParent();
 
-  Function *Wrapper =
-      Function::Create(Ty, Function::PrivateLinkage, "bitcast", M);
+  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;
+
+  if ((F->getFunctionType()->getNumParams() != Ty->getNumParams()) ||
+      (F->getFunctionType()->isVarArg() != Ty->isVarArg()))
+    WrapperNeeded = true;
+
   for (; AI != AE && PI != PE; ++AI, ++PI) {
-    if (AI->getType() != *PI) {
-      Wrapper->eraseFromParent();
-      return nullptr;
-    }
+    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 {
+        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) {
     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 (Ty->getReturnType()->isVoidTy())
+    if (RtnType->isVoidTy()) {
       ReturnInst::Create(M->getContext(), BB);
-  else if (F->getFunctionType()->getReturnType()->isVoidTy())
-    ReturnInst::Create(M->getContext(), UndefValue::get(Ty->getReturnType()),
-                       BB);
-  else if (F->getFunctionType()->getReturnType() == Ty->getReturnType())
+      WrapperNeeded = true;
+    } else if (ExpectedRtnType->isVoidTy()) {
+      ReturnInst::Create(M->getContext(), UndefValue::get(RtnType), BB);
+      WrapperNeeded = true;
+    } else if (RtnType == ExpectedRtnType) {
       ReturnInst::Create(M->getContext(), Call, BB);
-  else {
+    } 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 {
+      LLVM_DEBUG(dbgs() << "CreateWrapper: return type mismatch calling: "
+                        << F->getName() << "\n");
+      LLVM_DEBUG(dbgs() << "Expected: " << *ExpectedRtnType
+                        << " Got: " << *RtnType << "\n");
+      TypeMismatch = true;
+    }
+  }
+
+  if (TypeMismatch) {
+    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;
   }
-
   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/call.ll

 define void @coldcc_tail_call_void_nullary() {
   tail call coldcc void @void_nullary()
   ret void
 }
 
 ; CHECK-LABEL: call_constexpr:
 ; CHECK-NEXT: i32.const $push[[L0:[0-9]+]]=, 2{{$}}
 ; CHECK-NEXT: i32.const $push[[L1:[0-9]+]]=, 3{{$}}
-; CHECK-NEXT: call .Lbitcast@FUNCTION, $pop[[L0]], $pop[[L1]]{{$}}
+; CHECK-NEXT: call .Lvararg_func_bitcast@FUNCTION, $pop[[L0]], $pop[[L1]]{{$}}
 ; CHECK-NEXT: call other_void_nullary@FUNCTION{{$}}
 ; CHECK-NEXT: call void_nullary@FUNCTION{{$}}
 ; CHECK-NEXT: return{{$}}
 declare void @vararg_func(...)
 declare void @other_void_nullary()
 define void @call_constexpr() {
 bb0:
   call void bitcast (void (...)* @vararg_func to void (i32, i32)*)(i32 2, i32 3)

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

 }
 
 declare void @underspecified(...)
 declare void @specified(i32, i32)
 
 ; CHECK: callWithArgs:
 ; CHECK: i32.const  $push1=, 0
 ; CHECK-NEXT: i32.const $push0=, 1
-; CHECK-NEXT: call      .Lbitcast@FUNCTION, $pop1, $pop0
-; CHECK: call     .Lbitcast.1@FUNCTION, $pop{{[0-9]+$}}
+; CHECK-NEXT: call      .Lunderspecified_bitcast@FUNCTION, $pop1, $pop0
+; CHECK: call     .Lspecified_bitcast@FUNCTION, $pop{{[0-9]+$}}
 
-; CHECK: .Lbitcast:
+; CHECK: .Lunderspecified_bitcast:
 ; CHECK-NEXT: .param    i32, i32{{$}}
 ; CHECK: call     underspecified@FUNCTION, $pop{{[0-9]+$}}
 
-; CHECK: .Lbitcast.1:
+; CHECK: .Lspecified_bitcast:
 ; CHECK-NEXT: .param    i32{{$}}
 ; CHECK: call     specified@FUNCTION, $pop{{[0-9]+}}, $pop{{[0-9]+$}}

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

 declare void @plain(i32)
 
 declare void @foo0()
 declare void @foo1()
 declare void @foo2()
 declare void @foo3()
 
 ; CHECK-LABEL: test:
-; CHECK-NEXT: call        .Lbitcast@FUNCTION{{$}}
-; CHECK-NEXT: call        .Lbitcast@FUNCTION{{$}}
-; CHECK-NEXT: call        .Lbitcast.1@FUNCTION{{$}}
+; CHECK-NEXT: call        .Lhas_i32_arg_bitcast@FUNCTION{{$}}
+; CHECK-NEXT: call        .Lhas_i32_arg_bitcast@FUNCTION{{$}}
+; CHECK-NEXT: call        .Lhas_i32_ret_bitcast@FUNCTION{{$}}
 ; CHECK-NEXT: i32.const   $push[[L0:[0-9]+]]=, 0
-; CHECK-NEXT: call        .Lbitcast.4@FUNCTION, $pop[[L0]]{{$}}
+; CHECK-NEXT: call        .Lfoo0_bitcast@FUNCTION, $pop[[L0]]{{$}}
 ; CHECK-NEXT: i32.const   $push[[L1:[0-9]+]]=, 0
-; CHECK-NEXT: call        .Lbitcast.4@FUNCTION, $pop[[L1]]{{$}}
+; CHECK-NEXT: call        .Lfoo0_bitcast@FUNCTION, $pop[[L1]]{{$}}
 ; CHECK-NEXT: i32.const   $push[[L2:[0-9]+]]=, 0
-; CHECK-NEXT: call        .Lbitcast.4@FUNCTION, $pop[[L2]]{{$}}
+; CHECK-NEXT: call        .Lfoo0_bitcast@FUNCTION, $pop[[L2]]{{$}}
 ; CHECK-NEXT: call        foo0@FUNCTION
-; CHECK-NEXT: i32.call    $drop=, .Lbitcast.5@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 ()*)()
entry:
   call void @foo3()
 
   ret void
 }
 
 ; CHECK-LABEL: test_varargs:
 ; CHECK:      set_global
 ; CHECK:      i32.const   $push[[L3:[0-9]+]]=, 0{{$}}
-; CHECK-NEXT: call        .Lbitcast.2@FUNCTION, $pop[[L3]]{{$}}
+; 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        .Lbitcast.3@FUNCTION, $[[L5]]{{$}}
+; CHECK-NEXT: call        .Lplain_bitcast@FUNCTION, $[[L5]]{{$}}
 define void @test_varargs() {
   call void bitcast (void (...)* @vararg to void (i32)*)(i32 0)
   call void (...) bitcast (void (i32)* @plain to void (...)*)(i32 0)
   ret void
 }
 
 ; Don't use wrappers when the value is stored in memory
 

 
 ; 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]+]]=, .Lbitcast@FUNCTION{{$}}
+; CHECK:      i32.const   $push[[L4:[0-9]+]]=, .Lhas_i32_arg_bitcast@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:
   invoke void @call_func(i32 ()* bitcast (void (i32)* @has_i32_arg to i32 ()*))
           to label %cont2 unwind label %lpad
 
 cont2:
   invoke void bitcast (void (i32)* @has_i32_arg to void ()*)()
           to label %end unwind label %lpad
 
 lpad:
   %0 = landingpad { i8*, i32 }
           catch i8* null
   br label %end
 
 end:
   ret void
 }
 
-; CHECK-LABEL: .Lbitcast:
+; CHECK-LABEL: .Lhas_i32_arg_bitcast:
 ; CHECK-NEXT: call        has_i32_arg@FUNCTION, $0{{$}}
 ; CHECK-NEXT: end_function
 
-; CHECK-LABEL: .Lbitcast.1:
+; CHECK-LABEL: .Lhas_i32_ret_bitcast:
 ; CHECK-NEXT: call        $drop=, has_i32_ret@FUNCTION{{$}}
 ; CHECK-NEXT: end_function
 
-; CHECK-LABEL: .Lbitcast.2:
+; CHECK-LABEL: .Lvararg_bitcast:
 ; CHECK: call        vararg@FUNCTION, $1{{$}}
 ; CHECK: end_function
 
-; CHECK-LABEL: .Lbitcast.3:
+; CHECK-LABEL: .Lplain_bitcast:
 ; CHECK: call        plain@FUNCTION, $1{{$}}
 ; CHECK: end_function
 
-; CHECK-LABEL: .Lbitcast.4:
+; CHECK-LABEL: .Lfoo0_bitcast:
 ; CHECK-NEXT: .param      i32
 ; CHECK-NEXT: call        foo0@FUNCTION{{$}}
 ; CHECK-NEXT: end_function
 
-; CHECK-LABEL: .Lbitcast.5:
+; CHECK-LABEL: .Lfoo1_bitcast:
 ; CHECK-NEXT: .result     i32
 ; CHECK-NEXT: call        foo1@FUNCTION{{$}}
 ; CHECK-NEXT: copy_local  $push0=, $0
 ; CHECK-NEXT: end_function

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

 ; RUN: llc < %s -asm-verbose=false | FileCheck %s
 
-; Test that function pointer casts that require conversions are not converted
-; to wrappers. In theory some conversions could be supported, but currently no
-; conversions are implemented.
+; 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"
 
-; CHECK-LABEL: test:
+declare i32 @has_i64_arg(i64)
+declare i32 @has_ptr_arg(i8*)
+
+define void @test_invalid_rtn() {
+entry:
+  call i32 bitcast (i32 (i64)* @has_i64_arg to i32 (i32)*)(i32 0)
+  ret void
+}
+; CHECK-LABEL: test_invalid_rtn:
 ; CHECK-NEXT: i32.const   $push[[L0:[0-9]+]]=, 0{{$}}
-; CHECK-NEXT: call        has_i64_arg@FUNCTION, $pop[[L0]]{{$}}
-; CHECK-NEXT: i32.call    $push{{[0-9]+}}=, has_i64_ret@FUNCTION{{$}}
-; CHECK-NEXT: drop
+; CHECK-NEXT: i32.call $push1=, .Lhas_i64_arg_bitcast_invalid@FUNCTION, $pop[[L0]]{{$}}
+; CHECK-NEXT: drop $pop1
 ; CHECK-NEXT: end_function
 
-; CHECK-NOT: .Lbitcast
-
-declare void @has_i64_arg(i64)
-declare i64 @has_i64_ret()
-
-define void @test() {
+define void @test_invalid_arg() {
 entry:
-  call void bitcast (void (i64)* @has_i64_arg to void (i32)*)(i32 0)
-  %t = call i32 bitcast (i64 ()* @has_i64_ret to i32 ()*)()
+  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: 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:   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
+
+; CHECK-LABEL: .Lhas_i64_arg_bitcast_invalid:
+; 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-NEXT:   .param    i32
+; CHECK-NEXT:   .result   i32
+; CHECK-NEXT:   unreachable
+; CHECK-NEXT:   end_function