[wasm] Call through dispatch table in interpreter

When running wasm tests, the interpreter previously used a static
collection of function indexes stored in WasmTable to perform
call_indirect calls internal to that module. This has the wrong behavior
if the table is changed (via WasmTableObject::Set, `table.copy`, or
`table.init`).

This CL changes the cctests to always generate an intepreter entry for
all functions, and stores those entries in the dispatch table. This
allows us to use the same execution path as for non-testing code.

The interpreter entry compiler needed to be changed to support
multi-value returns too, since a 64-bit integer return value may be
lowered to two 32-bit integer returns.

Bug: v8:9016
Change-Id: I277df21ffde5c2eee0b691fcc9bab2b1a43eeffc
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1531137
Commit-Queue: Andreas Haas <ahaas@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#60380}

src/compiler/wasm-compiler.cc

@@ -5479,13 +5479,10 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
     }
 
     // The return value is also passed via this buffer:
-    DCHECK_GE(wasm::kV8MaxWasmFunctionReturns, sig_->return_count());
-    // TODO(wasm): Handle multi-value returns.
-    DCHECK_EQ(1, wasm::kV8MaxWasmFunctionReturns);
-    int return_size_bytes =
-        sig_->return_count() == 0
-            ? 0
-            : wasm::ValueTypes::ElementSizeInBytes(sig_->GetReturn());
+    int return_size_bytes = 0;
+    for (wasm::ValueType type : sig_->returns()) {
+      return_size_bytes += wasm::ValueTypes::ElementSizeInBytes(type);
+    }
 
     // Get a stack slot for the arguments.
     Node* arg_buffer =
@@ -5515,17 +5512,22 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
                        arraysize(parameters));
 
     // Read back the return value.
-    if (sig_->return_count() == 0) {
+    DCHECK_LT(sig_->return_count(), wasm::kV8MaxWasmFunctionMultiReturns);
+    unsigned return_count = static_cast<unsigned>(sig_->return_count());
+    if (return_count == 0) {
       Return(Int32Constant(0));
     } else {
-      // TODO(wasm): Implement multi-return.
-      DCHECK_EQ(1, sig_->return_count());
-      MachineType load_rep =
-          wasm::ValueTypes::MachineTypeFor(sig_->GetReturn());
-      Node* val = SetEffect(
-          graph()->NewNode(mcgraph()->machine()->Load(load_rep), arg_buffer,
-                           Int32Constant(0), Effect(), Control()));
-      Return(val);
+      Node** returns = Buffer(return_count);
+      offset = 0;
+      for (size_t i = 0; i < return_count; ++i) {
+        wasm::ValueType type = sig_->GetReturn(i);
+        Node* val = SetEffect(graph()->NewNode(
+            mcgraph()->machine()->Load(wasm::ValueTypes::MachineTypeFor(type)),
+            arg_buffer, Int32Constant(offset), Effect(), Control()));
+        returns[i] = val;
+        offset += wasm::ValueTypes::ElementSizeInBytes(type);
+      }
+      Return(return_count, returns);
     }
 
     if (ContainsInt64(sig_)) LowerInt64();

src/wasm/wasm-debug.cc

@@ -545,9 +545,7 @@ wasm::WasmInterpreter* WasmDebugInfo::SetupForTesting(
   auto interp_handle = Managed<wasm::InterpreterHandle>::Allocate(
       isolate, interpreter_size, isolate, debug_info);
   debug_info->set_interpreter_handle(*interp_handle);
-  auto ret = interp_handle->raw()->interpreter();
-  ret->SetCallIndirectTestMode();
-  return ret;
+  return interp_handle->raw()->interpreter();
 }
 
 void WasmDebugInfo::SetBreakpoint(Handle<WasmDebugInfo> debug_info,

src/wasm/wasm-interpreter.cc

@@ -966,9 +966,6 @@ class CodeMap {
   Zone* zone_;
   const WasmModule* module_;
   ZoneVector<InterpreterCode> interpreter_code_;
-  // TODO(wasm): Remove this testing wart. It is needed because interpreter
-  // entry stubs are not generated in testing the interpreter in cctests.
-  bool call_indirect_through_module_ = false;
 
  public:
   CodeMap(const WasmModule* module, const uint8_t* module_start, Zone* zone)
@@ -986,12 +983,6 @@ class CodeMap {
     }
   }
 
-  bool call_indirect_through_module() { return call_indirect_through_module_; }
-
-  void set_call_indirect_through_module(bool val) {
-    call_indirect_through_module_ = val;
-  }
-
   const WasmModule* module() const { return module_; }
 
   InterpreterCode* GetCode(const WasmFunction* function) {
@@ -1005,36 +996,6 @@ class CodeMap {
     return Preprocess(&interpreter_code_[function_index]);
   }
 
-  InterpreterCode* GetIndirectCode(uint32_t table_index, uint32_t entry_index) {
-    uint32_t saved_index;
-    USE(saved_index);
-    if (table_index >= module_->tables.size()) return nullptr;
-    // Mask table index for SSCA mitigation.
-    saved_index = table_index;
-    table_index &= static_cast<int32_t>((table_index - module_->tables.size()) &
-                                        ~static_cast<int32_t>(table_index)) >>
-                   31;
-    DCHECK_EQ(table_index, saved_index);
-    const WasmTable* table = &module_->tables[table_index];
-    if (entry_index >= table->values.size()) return nullptr;
-    // Mask entry_index for SSCA mitigation.
-    saved_index = entry_index;
-    entry_index &= static_cast<int32_t>((entry_index - table->values.size()) &
-                                        ~static_cast<int32_t>(entry_index)) >>
-                   31;
-    DCHECK_EQ(entry_index, saved_index);
-    uint32_t index = table->values[entry_index];
-    if (index >= interpreter_code_.size()) return nullptr;
-    // Mask index for SSCA mitigation.
-    saved_index = index;
-    index &= static_cast<int32_t>((index - interpreter_code_.size()) &
-                                  ~static_cast<int32_t>(index)) >>
-             31;
-    DCHECK_EQ(index, saved_index);
-
-    return GetCode(index);
-  }
-
   InterpreterCode* Preprocess(InterpreterCode* code) {
     DCHECK_EQ(code->function->imported, code->start == nullptr);
     if (!code->side_table && code->start) {
@@ -3337,28 +3298,6 @@ class ThreadImpl {
   ExternalCallResult CallIndirectFunction(uint32_t table_index,
                                           uint32_t entry_index,
                                           uint32_t sig_index) {
-    if (codemap()->call_indirect_through_module()) {
-      // Rely on the information stored in the WasmModule.
-      InterpreterCode* code =
-          codemap()->GetIndirectCode(table_index, entry_index);
-      if (!code) return {ExternalCallResult::INVALID_FUNC};
-      if (code->function->sig_index != sig_index) {
-        // If not an exact match, we have to do a canonical check.
-        int function_canonical_id =
-            module()->signature_ids[code->function->sig_index];
-        int expected_canonical_id = module()->signature_ids[sig_index];
-        DCHECK_EQ(function_canonical_id,
-                  module()->signature_map.Find(*code->function->sig));
-        if (function_canonical_id != expected_canonical_id) {
-          return {ExternalCallResult::SIGNATURE_MISMATCH};
-        }
-      }
-      if (code->function->imported) {
-        return CallImportedFunction(code->function->func_index);
-      }
-      return {ExternalCallResult::INTERNAL, code};
-    }
-
     Isolate* isolate = instance_object_->GetIsolate();
     uint32_t expected_sig_id = module()->signature_ids[sig_index];
     DCHECK_EQ(expected_sig_id,
@@ -3658,10 +3597,6 @@ void WasmInterpreter::SetFunctionCodeForTesting(const WasmFunction* function,
   internals_->codemap_.SetFunctionCode(function, start, end);
 }
 
-void WasmInterpreter::SetCallIndirectTestMode() {
-  internals_->codemap_.set_call_indirect_through_module(true);
-}
-
 ControlTransferMap WasmInterpreter::ComputeControlTransfersForTesting(
     Zone* zone, const WasmModule* module, const byte* start, const byte* end) {
   // Create some dummy structures, to avoid special-casing the implementation

src/wasm/wasm-interpreter.h

@@ -203,7 +203,6 @@ class V8_EXPORT_PRIVATE WasmInterpreter {
   // Manually adds code to the interpreter for the given function.
   void SetFunctionCodeForTesting(const WasmFunction* function,
                                  const byte* start, const byte* end);
-  void SetCallIndirectTestMode();
 
   // Computes the control transfers for the given bytecode. Used internally in
   // the interpreter, but exposed for testing.

src/wasm/wasm-module.h

@@ -103,8 +103,6 @@ struct WasmTable {
   uint32_t initial_size = 0;      // initial table size.
   uint32_t maximum_size = 0;      // maximum table size.
   bool has_maximum_size = false;  // true if there is a maximum size.
-  // TODO(titzer): Move this to WasmInstance. Needed by interpreter only.
-  std::vector<int32_t> values;  // function table, -1 indicating invalid.
   bool imported = false;        // true if imported.
   bool exported = false;        // true if exported.
 };

test/cctest/wasm/test-run-wasm-exceptions.cc

@@ -79,7 +79,6 @@ WASM_EXEC_TEST(TryCatchCallIndirect) {
       static_cast<uint16_t>(throw_func.function_index())};
   r.builder().AddIndirectFunctionTable(indirect_function_table,
                                        arraysize(indirect_function_table));
-  r.builder().PopulateIndirectFunctionTable();
 
   // Build the main test function.
   BUILD(r, WASM_TRY_CATCH_T(

test/cctest/wasm/test-run-wasm-interpreter.cc

@@ -228,7 +228,6 @@ TEST(Run_Wasm_returnCallIndirectFactorial) {
 
   r.builder().AddIndirectFunctionTable(indirect_function_table,
                                        arraysize(indirect_function_table));
-  r.builder().PopulateIndirectFunctionTable();
 
   BUILD(r, WASM_RETURN_CALL_INDIRECT(0, WASM_I32V(0), WASM_GET_LOCAL(0),
                                      WASM_I32V(1)));

test/cctest/wasm/test-run-wasm-js.cc

@@ -146,7 +146,6 @@ WASM_EXEC_TEST(Run_IndirectCallJSFunction) {
 
   r.builder().AddIndirectFunctionTable(indirect_function_table,
                                        arraysize(indirect_function_table));
-  r.builder().PopulateIndirectFunctionTable();
 
   BUILD(rc_fn, WASM_CALL_INDIRECT3(0, WASM_I32V(js_index), WASM_I32V(left),
                                    WASM_I32V(right), WASM_GET_LOCAL(0)));
@@ -544,7 +543,6 @@ void RunPickerTest(ExecutionTier tier, bool indirect) {
 
     r.builder().AddIndirectFunctionTable(indirect_function_table,
                                          arraysize(indirect_function_table));
-    r.builder().PopulateIndirectFunctionTable();
 
     BUILD(rc_fn,
           WASM_RETURN_CALL_INDIRECT(0, WASM_I32V(js_index), WASM_I32V(left),

test/cctest/wasm/test-run-wasm.cc

@@ -2466,7 +2466,6 @@ WASM_EXEC_TEST(ReturnCall_IndirectFactorial) {
 
   r.builder().AddIndirectFunctionTable(indirect_function_table,
                                        arraysize(indirect_function_table));
-  r.builder().PopulateIndirectFunctionTable();
 
   BUILD(r,
         WASM_RETURN_CALL_FUNCTION(f_ind_fn.function_index(), WASM_GET_LOCAL(0),
@@ -2906,7 +2905,6 @@ WASM_EXEC_TEST(SimpleCallIndirect) {
       static_cast<uint16_t>(t2.function_index())};
   r.builder().AddIndirectFunctionTable(indirect_function_table,
                                        arraysize(indirect_function_table));
-  r.builder().PopulateIndirectFunctionTable();
 
   // Build the caller function.
   BUILD(r, WASM_CALL_INDIRECT2(1, WASM_GET_LOCAL(0), WASM_I32V_2(66),
@@ -2940,7 +2938,6 @@ WASM_EXEC_TEST(MultipleCallIndirect) {
       static_cast<uint16_t>(t2.function_index())};
   r.builder().AddIndirectFunctionTable(indirect_function_table,
                                        arraysize(indirect_function_table));
-  r.builder().PopulateIndirectFunctionTable();
 
   // Build the caller function.
   BUILD(r, WASM_I32_ADD(
@@ -3012,7 +3009,6 @@ WASM_EXEC_TEST(CallIndirect_canonical) {
 
   r.builder().AddIndirectFunctionTable(indirect_function_table,
                                        arraysize(indirect_function_table));
-  r.builder().PopulateIndirectFunctionTable();
 
   // Build the caller function.
   BUILD(r, WASM_CALL_INDIRECT2(1, WASM_GET_LOCAL(0), WASM_I32V_2(77),

test/cctest/wasm/wasm-run-utils.cc

@@ -120,6 +120,16 @@ uint32_t TestingModuleBuilder::AddFunction(FunctionSig* sig, const char* name,
   }
   if (interpreter_) {
     interpreter_->AddFunctionForTesting(&test_module_->functions.back());
+    // Patch the jump table to call the interpreter for this function.
+    wasm::WasmCompilationResult result = compiler::CompileWasmInterpreterEntry(
+        isolate_->wasm_engine(), native_module_->enabled_features(), index,
+        sig);
+    std::unique_ptr<wasm::WasmCode> code = native_module_->AddCode(
+        index, result.code_desc, result.frame_slot_count,
+        result.tagged_parameter_slots, std::move(result.protected_instructions),
+        std::move(result.source_positions), wasm::WasmCode::kInterpreterEntry,
+        wasm::WasmCode::kOther);
+    native_module_->PublishCode(std::move(code));
   }
   DCHECK_LT(index, kMaxFunctions);  // limited for testing.
   return index;
@@ -145,51 +155,24 @@ Handle<JSFunction> TestingModuleBuilder::WrapCode(uint32_t index) {
   old_arr->CopyTo(0, *new_arr, 0, old_arr->length());
   new_arr->set(old_arr->length(), *ret_code);
   module_object->set_export_wrappers(*new_arr);
-
-  if (interpreter_) {
-    // Patch the jump table to call the interpreter for this function. This is
-    // only needed for functions with a wrapper. Other functions never get
-    // called through the jump table.
-    wasm::WasmCompilationResult result = compiler::CompileWasmInterpreterEntry(
-        isolate_->wasm_engine(), native_module_->enabled_features(), index,
-        sig);
-    std::unique_ptr<wasm::WasmCode> code = native_module_->AddCode(
-        index, result.code_desc, result.frame_slot_count,
-        result.tagged_parameter_slots, std::move(result.protected_instructions),
-        std::move(result.source_positions), wasm::WasmCode::kInterpreterEntry,
-        wasm::WasmCode::kOther);
-    native_module_->PublishCode(std::move(code));
-  }
   return ret;
 }
 
 void TestingModuleBuilder::AddIndirectFunctionTable(
     const uint16_t* function_indexes, uint32_t table_size) {
+  auto instance = instance_object();
   test_module_->tables.emplace_back();
   WasmTable& table = test_module_->tables.back();
   table.initial_size = table_size;
   table.maximum_size = table_size;
   table.has_maximum_size = true;
-  for (uint32_t i = 0; i < table_size; ++i) {
-    table.values.push_back(function_indexes[i]);
-  }
   WasmInstanceObject::EnsureIndirectFunctionTableWithMinimumSize(
       instance_object(), table_size);
-}
-
-void TestingModuleBuilder::PopulateIndirectFunctionTable() {
-  if (interpret()) return;
-  auto instance = instance_object();
-  uint32_t num_tables = 1;  // TODO(titzer): multiple tables.
-  for (uint32_t i = 0; i < num_tables; i++) {
-    WasmTable& table = test_module_->tables[i];
-    int table_size = static_cast<int>(instance->indirect_function_table_size());
-    for (int j = 0; j < table_size; j++) {
-      WasmFunction& function = test_module_->functions[table.values[j]];
-      int sig_id = test_module_->signature_map.Find(*function.sig);
-      IndirectFunctionTableEntry(instance, j)
-          .Set(sig_id, instance, function.func_index);
-    }
+  for (uint32_t i = 0; i < table_size; ++i) {
+    WasmFunction& function = test_module_->functions[function_indexes[i]];
+    int sig_id = test_module_->signature_map.Find(*function.sig);
+    IndirectFunctionTableEntry(instance, i)
+        .Set(sig_id, instance, function.func_index);
   }
 }
 

test/cctest/wasm/wasm-run-utils.h

@@ -184,8 +184,6 @@ class TestingModuleBuilder {
   void AddIndirectFunctionTable(const uint16_t* function_indexes,
                                 uint32_t table_size);
 
-  void PopulateIndirectFunctionTable();
-
   uint32_t AddBytes(Vector<const byte> bytes);
 
   uint32_t AddException(FunctionSig* sig);