[wasm] Call imports via import table in js-to-wasm wrappers

The js-to-wasm wrappers are shared across instances, so we cannot
directly call the instance-specific wasm-to-js wrappers. Instead, we
need to call via the import table.

R=titzer@chromium.org

Bug: chromium:843563
Change-Id: Ia882604f6769472fe2eb69176cbed728215ced29
Reviewed-on: https://chromium-review.googlesource.com/1064610Reviewed-by: Ben Titzer <titzer@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53254}

src/compiler/wasm-compiler.cc

@@ -2559,7 +2559,8 @@ Node* WasmGraphBuilder::BuildWasmCall(wasm::FunctionSig* sig, Node** args,
       GetWasmCallDescriptor(mcgraph()->zone(), sig, use_retpoline);
   const Operator* op = mcgraph()->common()->Call(call_descriptor);
   Node* call = graph()->NewNode(op, static_cast<int>(count), args);
-  SetSourcePosition(call, position);
+  DCHECK(position == wasm::kNoCodePosition || position > 0);
+  if (position > 0) SetSourcePosition(call, position);
 
   *effect_ = call;
   size_t ret_count = sig->return_count();
@@ -2579,38 +2580,43 @@ Node* WasmGraphBuilder::BuildWasmCall(wasm::FunctionSig* sig, Node** args,
   return call;
 }
 
+Node* WasmGraphBuilder::BuildImportWasmCall(wasm::FunctionSig* sig, Node** args,
+                                            Node*** rets,
+                                            wasm::WasmCodePosition position,
+                                            int func_index) {
+  // Load the instance from the imported_instances array at a known offset.
+  Node* imported_instances = LOAD_INSTANCE_FIELD(ImportedFunctionInstances,
+                                                 MachineType::TaggedPointer());
+  Node* instance_node = LOAD_FIXED_ARRAY_SLOT(imported_instances, func_index);
+
+  // Load the target from the imported_targets array at a known offset.
+  Node* imported_targets =
+      LOAD_INSTANCE_FIELD(ImportedFunctionTargets, MachineType::Pointer());
+  Node* target_node = graph()->NewNode(
+      mcgraph()->machine()->Load(MachineType::Pointer()), imported_targets,
+      mcgraph()->Int32Constant(func_index * sizeof(Address)),
+      mcgraph()->graph()->start(), mcgraph()->graph()->start());
+  args[0] = target_node;
+  return BuildWasmCall(sig, args, rets, position, instance_node,
+                       untrusted_code_mitigations_ ? kRetpoline : kNoRetpoline);
+}
+
 Node* WasmGraphBuilder::CallDirect(uint32_t index, Node** args, Node*** rets,
                                    wasm::WasmCodePosition position) {
   DCHECK_NULL(args[0]);
   wasm::FunctionSig* sig = env_->module->functions[index].sig;
 
   if (env_ && index < env_->module->num_imported_functions) {
-    // A call to an imported function.
-    // Load the instance from the imported_instances array at a known offset.
-    Node* imported_instances = LOAD_INSTANCE_FIELD(
-        ImportedFunctionInstances, MachineType::TaggedPointer());
-    Node* instance_node = LOAD_FIXED_ARRAY_SLOT(imported_instances, index);
-
-    // Load the target from the imported_targets array at a known offset.
-    Node* imported_targets =
-        LOAD_INSTANCE_FIELD(ImportedFunctionTargets, MachineType::Pointer());
-    Node* target_node = graph()->NewNode(
-        mcgraph()->machine()->Load(MachineType::Pointer()), imported_targets,
-        mcgraph()->Int32Constant(index * sizeof(Address)),
-        mcgraph()->graph()->start(), mcgraph()->graph()->start());
-    args[0] = target_node;
-    return BuildWasmCall(
-        sig, args, rets, position, instance_node,
-        untrusted_code_mitigations_ ? kRetpoline : kNoRetpoline);
+    // Call to an imported function.
+    return BuildImportWasmCall(sig, args, rets, position, index);
+  }
 
-  } else {
-    // A call to a function in this module.
-    // Just encode the function index. This will be patched at instantiation.
-    Address code = static_cast<Address>(index);
-    args[0] = mcgraph()->RelocatableIntPtrConstant(code, RelocInfo::WASM_CALL);
+  // A direct call to a wasm function defined in this module.
+  // Just encode the function index. This will be patched at instantiation.
+  Address code = static_cast<Address>(index);
+  args[0] = mcgraph()->RelocatableIntPtrConstant(code, RelocInfo::WASM_CALL);
 
-    return BuildWasmCall(sig, args, rets, position, nullptr, kNoRetpoline);
-  }
+  return BuildWasmCall(sig, args, rets, position, nullptr, kNoRetpoline);
 }
 
 Node* WasmGraphBuilder::CallIndirect(uint32_t sig_index, Node** args,
@@ -4298,11 +4304,8 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
     return instance;
   }
 
-  void BuildJSToWasmWrapper(Address call_target) {
+  void BuildJSToWasmWrapper(uint32_t wasm_func_index, Address call_target) {
     const int wasm_count = static_cast<int>(sig_->parameter_count());
-    const int count =
-        wasm_count + 4;  // wasm_code, instance_node, effect, and control.
-    Node** args = Buffer(count);
 
     // Build the start and the JS parameter nodes.
     Node* start = Start(wasm_count + 5);
@@ -4327,8 +4330,6 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
     DCHECK_NULL(instance_node_);
     instance_node_ = BuildLoadInstanceFromExportedFunction(js_closure);
 
-    Node* wasm_code_node = mcgraph()->RelocatableIntPtrConstant(
-        call_target, RelocInfo::JS_TO_WASM_CALL);
     if (!wasm::IsJSCompatibleSignature(sig_)) {
       // Throw a TypeError. Use the js_context of the calling javascript
       // function (passed as a parameter), such that the generated code is
@@ -4339,36 +4340,40 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
       return;
     }
 
-    int pos = 0;
-    args[pos++] = wasm_code_node;
-    args[pos++] = instance_node_.get();
+    const int args_count = wasm_count + 1;  // +1 for wasm_code.
+    Node** args = Buffer(args_count);
+    Node** rets;
 
     // Convert JS parameters to wasm numbers.
     for (int i = 0; i < wasm_count; ++i) {
       Node* param = Param(i + 1);
       Node* wasm_param = FromJS(param, js_context, sig_->GetParam(i));
-      args[pos++] = wasm_param;
+      args[i + 1] = wasm_param;
     }
 
     // Set the ThreadInWasm flag before we do the actual call.
     BuildModifyThreadInWasmFlag(true);
 
-    args[pos++] = *effect_;
-    args[pos++] = *control_;
-
-    // Call the wasm code.
-    auto call_descriptor = GetWasmCallDescriptor(mcgraph()->zone(), sig_);
+    if (env_ && wasm_func_index < env_->module->num_imported_functions) {
+      // Call to an imported function.
+      DCHECK_EQ(kNullAddress, call_target);
+      BuildImportWasmCall(sig_, args, &rets, wasm::kNoCodePosition,
+                          wasm_func_index);
+    } else {
+      // Call to a wasm function defined in this module.
+      DCHECK_NE(kNullAddress, call_target);
+      args[0] = mcgraph()->RelocatableIntPtrConstant(
+          call_target, RelocInfo::JS_TO_WASM_CALL);
 
-    Node* call = graph()->NewNode(mcgraph()->common()->Call(call_descriptor),
-                                  count, args);
-    *effect_ = call;
+      BuildWasmCall(sig_, args, &rets, wasm::kNoCodePosition, nullptr,
+                    kNoRetpoline);
+    }
 
-    // Clear the ThreadInWasmFlag
+    // Clear the ThreadInWasm flag.
     BuildModifyThreadInWasmFlag(false);
 
-    Node* retval = call;
-    Node* jsval = ToJS(retval, sig_->return_count() == 0 ? wasm::kWasmStmt
-                                                         : sig_->GetReturn());
+    Node* jsval = sig_->return_count() == 0 ? jsgraph()->UndefinedConstant()
+                                            : ToJS(rets[0], sig_->GetReturn());
     Return(jsval);
   }
 
@@ -4694,7 +4699,7 @@ Handle<Code> CompileJSToWasmWrapper(Isolate* isolate, wasm::WasmModule* module,
   WasmWrapperGraphBuilder builder(&zone, &env, &jsgraph, func->sig, nullptr);
   builder.set_control_ptr(&control);
   builder.set_effect_ptr(&effect);
-  builder.BuildJSToWasmWrapper(call_target);
+  builder.BuildJSToWasmWrapper(index, call_target);
 
   //----------------------------------------------------------------------------
   // Run the compilation pipeline.

src/compiler/wasm-compiler.h

@@ -388,6 +388,8 @@ class WasmGraphBuilder {
   Node* BuildWasmCall(wasm::FunctionSig* sig, Node** args, Node*** rets,
                       wasm::WasmCodePosition position, Node* instance_node,
                       UseRetpoline use_retpoline);
+  Node* BuildImportWasmCall(wasm::FunctionSig* sig, Node** args, Node*** rets,
+                            wasm::WasmCodePosition position, int func_index);
 
   Node* BuildF32CopySign(Node* left, Node* right);
   Node* BuildF64CopySign(Node* left, Node* right);

src/wasm/module-compiler.cc

@@ -197,24 +197,34 @@ class JSToWasmWrapperCache {
   Handle<Code> CloneOrCompileJSToWasmWrapper(
       Isolate* isolate, wasm::WasmModule* module, Address call_target,
       uint32_t index, wasm::UseTrapHandler use_trap_handler) {
+    const bool is_import = index < module->num_imported_functions;
+    DCHECK_EQ(is_import, call_target == kNullAddress);
     const wasm::WasmFunction* func = &module->functions[index];
-    int cached_idx = sig_map_.Find(func->sig);
-    if (cached_idx >= 0) {
-      Handle<Code> code = isolate->factory()->CopyCode(code_cache_[cached_idx]);
-      // Now patch the call to wasm code.
-      RelocIterator it(*code, RelocInfo::ModeMask(RelocInfo::JS_TO_WASM_CALL));
-      // If there is no reloc info, then it's an incompatible signature or calls
-      // an import.
-      if (!it.done()) it.rinfo()->set_js_to_wasm_address(call_target);
-      return code;
+    // We cannot cache js-to-wasm wrappers for imports, as they hard-code the
+    // function index.
+    if (!is_import) {
+      int cached_idx = sig_map_.Find(func->sig);
+      if (cached_idx >= 0) {
+        Handle<Code> code =
+            isolate->factory()->CopyCode(code_cache_[cached_idx]);
+        // Now patch the call to wasm code.
+        RelocIterator it(*code,
+                         RelocInfo::ModeMask(RelocInfo::JS_TO_WASM_CALL));
+        // If there is no reloc info, then it's an incompatible signature or
+        // calls an import.
+        if (!it.done()) it.rinfo()->set_js_to_wasm_address(call_target);
+        return code;
+      }
     }
 
     Handle<Code> code = compiler::CompileJSToWasmWrapper(
         isolate, module, call_target, index, use_trap_handler);
-    uint32_t new_cache_idx = sig_map_.FindOrInsert(func->sig);
-    DCHECK_EQ(code_cache_.size(), new_cache_idx);
-    USE(new_cache_idx);
-    code_cache_.push_back(code);
+    if (!is_import) {
+      uint32_t new_cache_idx = sig_map_.FindOrInsert(func->sig);
+      DCHECK_EQ(code_cache_.size(), new_cache_idx);
+      USE(new_cache_idx);
+      code_cache_.push_back(code);
+    }
     return code;
   }
 
@@ -1828,14 +1838,10 @@ MaybeHandle<WasmInstanceObject> InstanceBuilder::Build() {
   if (module_->start_function_index >= 0) {
     int start_index = module_->start_function_index;
     Handle<WasmInstanceObject> start_function_instance = instance;
-    Address start_call_address;
-    if (static_cast<uint32_t>(start_index) < module_->num_imported_functions) {
-      ImportedFunctionEntry entry(instance, start_index);
-      start_function_instance = handle(entry.instance(), isolate_);
-      start_call_address = entry.target();
-    } else {
-      start_call_address = native_module->GetCallTargetForFunction(start_index);
-    }
+    Address start_call_address =
+        static_cast<uint32_t>(start_index) < module_->num_imported_functions
+            ? kNullAddress
+            : native_module->GetCallTargetForFunction(start_index);
     FunctionSig* sig = module_->functions[start_index].sig;
     Handle<Code> wrapper_code = js_to_wasm_cache_.CloneOrCompileJSToWasmWrapper(
         isolate_, module_, start_call_address, start_index, use_trap_handler());
@@ -2648,7 +2654,8 @@ void InstanceBuilder::LoadTableSegments(Handle<WasmInstanceObject> instance) {
         Address call_target =
             native_module->GetCallTargetForFunction(func_index);
 
-        if (func_index < module_->num_imported_functions) {
+        const bool is_import = func_index < module_->num_imported_functions;
+        if (is_import) {
           // Imported functions have the target instance put into the IFT.
           WasmInstanceObject* target_instance =
               ImportedFunctionEntry(instance, func_index).instance();
@@ -2669,8 +2676,8 @@ void InstanceBuilder::LoadTableSegments(Handle<WasmInstanceObject> instance) {
 
             Handle<Code> wrapper_code =
                 js_to_wasm_cache_.CloneOrCompileJSToWasmWrapper(
-                    isolate_, module_, call_target, func_index,
-                    use_trap_handler());
+                    isolate_, module_, is_import ? kNullAddress : call_target,
+                    func_index, use_trap_handler());
             MaybeHandle<String> func_name;
             if (module_->is_asm_js()) {
               // For modules arising from asm.js, honor the names section.
@@ -3648,7 +3655,10 @@ void CompileJsToWasmWrappers(Isolate* isolate,
   WasmModule* module = native_module->shared_module_data()->module();
   for (auto exp : module->export_table) {
     if (exp.kind != kExternalFunction) continue;
-    Address call_target = native_module->GetCallTargetForFunction(exp.index);
+    Address call_target =
+        exp.index < module->num_imported_functions
+            ? kNullAddress
+            : native_module->GetCallTargetForFunction(exp.index);
     Handle<Code> wrapper_code = js_to_wasm_cache.CloneOrCompileJSToWasmWrapper(
         isolate, module, call_target, exp.index, use_trap_handler);
     export_wrappers->set(wrapper_index, *wrapper_code);

test/mjsunit/regress/wasm/regress-843563.js

+// Copyright 2018 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+load('test/mjsunit/wasm/wasm-constants.js');
+load('test/mjsunit/wasm/wasm-module-builder.js');
+
+const builder = new WasmModuleBuilder();
+sig1 = makeSig([kWasmI32, kWasmI32, kWasmI32], [kWasmI32]);
+const imp_idx = builder.addImport('q', 'imp', kSig_i_i);
+builder.addExport('exp', imp_idx);
+const module = builder.toModule();
+
+function bad(a, b, c, d, e, f, g, h) {
+    print(JSON.stringify([a, b, c, d, e, f, g, h]));
+}
+const instance1 = new WebAssembly.Instance(module, {q: {imp: bad}});
+const instance2 = new WebAssembly.Instance(module, {q: {imp: i => i}});
+
+print(instance1.exports.exp(5));