[WebAssembly] Narrow the scope of WebAssemblyFixFunctionBitcasts

[jgravelle-google]

Summary

The pass to fix function bitcasts generates thunks for functions that
are called directly with a mismatching signature. It was also generating
thunks in cases where the function was address-taken, causing aliasing
problems in otherwise valid cases.
This patch tightens the restrictions for when the pass runs.

[dschuff]

lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp

72 ↗

(On Diff #118051)

How about invoke? Also, are we filtering out intrinsics anywhere? (An IntrinsicInst is a CallInst but I forget if its opcode is Instruction::Call or not)

[sunfish]

Is the assumption here that the eventual caller will cast the argument back to the original type?

For example, in test_argument, if call_func bitcasts the i32 ()* argument to void (i32)* before calling it, then it doesn't want a wrapper. However, if it doesn't do that cast, and just calls it as i32 ()*, then it does seem to need the wrapper. So I think it's a guess either way. Do you think functions doing an extra cast are more common that functions that don't?

[jgravelle-google]

In D38640#891197, @sunfish wrote:

Is the assumption here that the eventual caller will cast the argument back to the original type?

Pretty much. In C, that is explicitly defined to be well-defined, and otherwise it's undefined behavior. Native targets ignore missing args and/or read junk bits from the argument registers, wasm traps at runtime, or with this pass we synthesize/ignore args.

For example, in test_argument, if call_func bitcasts the i32 ()* argument to void (i32)* before calling it, then it doesn't want a wrapper. However, if it doesn't do that cast, and just calls it as i32 ()*, then it does seem to need the wrapper. So I think it's a guess either way. Do you think functions doing an extra cast are more common that functions that don't?

And if call_func stores it to memory, or prints it to screen, or compares it to the pointer value of has_i32_arg, we have no idea. Because it's a guess either way, and because we don't have a way to reverse the bitcast-function, I think the most reasonable thing to do is nothing.
Functions doing an extra cast are probably equally common (I have no data for this), but more importantly they're not-undefined, so I think we should make sure to support those, and any undefined use cases that we can support are a bonus.

lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp

72 ↗

(On Diff #118051)

Not sure, will check and add tests

[sunfish]

That makes sense. This patch looks good to me, with Invokes handled.

lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp

72 ↗

(On Diff #118051)

The CallSite class is one way to handle both Calls and Invokes at the same time.

Also, it's invalid to do anything with the address of an intrinsic except directly call it, so we shouldn't have to worry about them.

[jgravelle-google]

• Add invoke test

[jgravelle-google]

• Use CallSites, properly test exceptions

[dschuff]

Cool, and the code is even shorter this way too 👍

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

 ///
 /// Note that LLVM already optimizes away function bitcasts in common cases by
 /// dropping arguments as needed, so this pass only ends up getting used in less
 /// common cases.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "WebAssembly.h"
+#include "llvm/IR/CallSite.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace llvm;

 // bitcasts of V.
 static void FindUses(Value *V, Function &F,
                      SmallVectorImpl<std::pair<Use *, Function *>> &Uses,
                      SmallPtrSetImpl<Constant *> &ConstantBCs) {
   for (Use &U : V->uses()) {
     if (BitCastOperator *BC = dyn_cast<BitCastOperator>(U.getUser()))
       FindUses(BC, F, Uses, ConstantBCs);
     else if (U.get()->getType() != F.getType()) {
+      CallSite CS(U.getUser());
+      if (!CS)
+        // Skip uses that aren't immediately called
+        continue;
+      Value *Callee = CS.getCalledValue();
+      if (Callee != V)
+        // Skip calls where the function isn't the callee
+        continue;
       if (isa<Constant>(U.get())) {
         // Only add constant bitcasts to the list once; they get RAUW'd
         auto c = ConstantBCs.insert(cast<Constant>(U.get()));
-        if (!c.second) continue;
+        if (!c.second)
+          continue;
       }
       Uses.push_back(std::make_pair(&U, &F));
     }
   }
 }
 
 // Create a wrapper function with type Ty that calls F (which may have a
 // different type). Attempt to support common bitcasted function idioms:

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

-; RUN: llc < %s -asm-verbose=false -disable-wasm-explicit-locals | FileCheck %s
+; RUN: llc < %s -asm-verbose=false -disable-wasm-explicit-locals -enable-emscripten-cxx-exceptions | 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-wasm"
 
+declare void @has_i32_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        .Lbitcast@FUNCTION{{$}}
 ; CHECK-NEXT: call        .Lbitcast@FUNCTION{{$}}
 ; CHECK-NEXT: call        .Lbitcast.1@FUNCTION{{$}}
 ; CHECK-NEXT: i32.const   $push[[L0:[0-9]+]]=, 0
 ; CHECK-NEXT: call        .Lbitcast.2@FUNCTION, $pop[[L0]]{{$}}
 ; CHECK-NEXT: i32.const   $push[[L1:[0-9]+]]=, 0
 ; CHECK-NEXT: call        .Lbitcast.2@FUNCTION, $pop[[L1]]{{$}}
 ; CHECK-NEXT: i32.const   $push[[L2:[0-9]+]]=, 0
 ; CHECK-NEXT: call        .Lbitcast.2@FUNCTION, $pop[[L2]]{{$}}
 ; CHECK-NEXT: call        foo0@FUNCTION
 ; CHECK-NEXT: i32.call    $drop=, .Lbitcast.3@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 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_varargs:
 ; CHECK:      set_global
 ; CHECK:      i32.const   $push[[L3:[0-9]+]]=, 0{{$}}
 ; CHECK-NEXT: call        vararg@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        plain@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
+
+@global_func = hidden local_unnamed_addr global void ()* null
+
+; CHECK-LABEL: test_store:
+; CHECK-NEXT: i32.const   $push[[L0:[0-9]+]]=, 0{{$}}
+; CHECK-NEXT: i32.const   $push[[L1:[0-9]+]]=, has_i32_ret@FUNCTION{{$}}
+; CHECK-NEXT: i32.store   global_func($pop[[L0]]), $pop[[L1]]{{$}}
+define void @test_store() {
+  %1 = bitcast i32 ()* @has_i32_ret to void ()*
+  store void ()* %1, void ()** @global_func
+  ret void
+}
+
+; CHECK-LABEL: test_load:
+; CHECK-NEXT: result      i32{{$}}
+; CHECK-NEXT: i32.const   $push[[L0:[0-9]+]]=, 0{{$}}
+; CHECK-NEXT: i32.load    $push[[L1:[0-9]+]]=, global_func($pop[[L0]]){{$}}
+; CHECK-NEXT: i32.call_indirect $push{{[0-9]+}}=, $pop[[L1]]{{$}}
+define i32 @test_load() {
+  %1 = load i32 ()*, i32 ()** bitcast (void ()** @global_func to i32 ()**)
+  %2 = call i32 %1()
+  ret i32 %2
+}
+
+; Don't use wrappers when the value is passed to a function call
+
+declare void @call_func(i32 ()*)
+
+; CHECK-LABEL: test_argument:
+; CHECK-NEXT: i32.const   $push[[L0:[0-9]+]]=, has_i32_ret@FUNCTION{{$}}
+; CHECK-NEXT: call        call_func@FUNCTION, $pop[[L0]]{{$}}
+; CHECK-NEXT: i32.const   $push[[L1:[0-9]+]]=, has_i32_arg@FUNCTION{{$}}
+; CHECK-NEXT: call        call_func@FUNCTION, $pop[[L1]]{{$}}
+define void @test_argument() {
+  call void @call_func(i32 ()* @has_i32_ret)
+  call void @call_func(i32 ()* bitcast (void (i32)* @has_i32_arg to i32 ()*))
+  ret void
+}
+
+; Invokes should be treated like calls
+
+; 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-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-NEXT: call        has_i32_arg@FUNCTION, $0{{$}}
 ; CHECK-NEXT: end_function
 
 ; CHECK-LABEL: .Lbitcast.1:
 ; CHECK-NEXT: call        $drop=, has_i32_ret@FUNCTION{{$}}
 ; CHECK-NEXT: end_function
 
 ; CHECK-LABEL: .Lbitcast.2:
 ; CHECK-NEXT: .param      i32
 ; CHECK-NEXT: call        foo0@FUNCTION{{$}}
 ; CHECK-NEXT: end_function
 
 ; CHECK-LABEL: .Lbitcast.3:
 ; CHECK-NEXT: .result     i32
 ; CHECK-NEXT: call        foo1@FUNCTION{{$}}
 ; CHECK-NEXT: copy_local  $push0=, $0
 ; CHECK-NEXT: end_function
-
-declare void @has_i32_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()
-
-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 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
-}
-
-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
-}