[wasm] Canonicalize function signature indices for matching in indirect calls.

R=bradnelson@chromium.org, ahaas@chromium.org, clemensh@chromium.org
BUG=chromium:575167

Review-Url: https://codereview.chromium.org/2403093002
Cr-Commit-Position: refs/heads/master@{#40169}

BUILD.gn

@@ -1696,6 +1696,8 @@ v8_source_set("v8_base") {
     "src/wasm/leb-helper.h",
     "src/wasm/module-decoder.cc",
     "src/wasm/module-decoder.h",
+    "src/wasm/signature-map.cc",
+    "src/wasm/signature-map.h",
     "src/wasm/wasm-debug.cc",
     "src/wasm/wasm-debug.h",
     "src/wasm/wasm-external-refs.cc",

src/api.cc

@@ -73,6 +73,7 @@
 #include "src/version.h"
 #include "src/vm-state-inl.h"
 #include "src/wasm/wasm-module.h"
+#include "src/wasm/wasm-result.h"
 
 namespace v8 {
 

src/compiler/wasm-compiler.cc

@@ -2148,9 +2148,11 @@ Node* WasmGraphBuilder::CallIndirect(uint32_t index, Node** args, Node*** rets,
                                           Int32Constant(kPointerSizeLog2)),
                          Int32Constant(fixed_offset)),
         *effect_, *control_);
+    int32_t key = module_->module->function_tables[0].map.Find(sig);
+    DCHECK_GE(key, 0);
     Node* sig_match =
         graph()->NewNode(machine->Word32Equal(),
-                         BuildChangeSmiToInt32(load_sig), Int32Constant(index));
+                         BuildChangeSmiToInt32(load_sig), Int32Constant(key));
     trap_->AddTrapIfFalse(wasm::kTrapFuncSigMismatch, sig_match, position);
   }
 

src/v8.gyp

@@ -1247,6 +1247,8 @@
         'wasm/leb-helper.h',
         'wasm/module-decoder.cc',
         'wasm/module-decoder.h',
+        'wasm/signature-map.cc',
+        'wasm/signature-map.h',
         'wasm/wasm-debug.cc',
         'wasm/wasm-debug.h',
         'wasm/wasm-external-refs.cc',

src/wasm/module-decoder.cc

@@ -301,7 +301,7 @@ class ModuleDecoder : public Decoder {
             import->index =
                 static_cast<uint32_t>(module->function_tables.size());
             module->function_tables.push_back(
-                {0, 0, std::vector<int32_t>(), true, false});
+                {0, 0, std::vector<int32_t>(), true, false, SignatureMap()});
             expect_u8("element type", 0x20);
             WasmIndirectFunctionTable* table = &module->function_tables.back();
             consume_resizable_limits("element count", "elements", kMaxUInt32,
@@ -363,10 +363,12 @@ class ModuleDecoder : public Decoder {
       if (table_count > 1) {
         error(pos, pos, "invalid table count %d, maximum 1", table_count);
       }
+      if (module->function_tables.size() < 1) {
+        module->function_tables.push_back(
+            {0, 0, std::vector<int32_t>(), false, false, SignatureMap()});
+      }
 
       for (uint32_t i = 0; ok() && i < table_count; i++) {
-        module->function_tables.push_back(
-            {0, 0, std::vector<int32_t>(), false, false});
         WasmIndirectFunctionTable* table = &module->function_tables.back();
         expect_u8("table type", kWasmAnyFunctionTypeForm);
         consume_resizable_limits("table elements", "elements", kMaxUInt32,
@@ -503,8 +505,17 @@ class ModuleDecoder : public Decoder {
     if (section_iter.section_code() == kElementSectionCode) {
       uint32_t element_count = consume_u32v("element count");
       for (uint32_t i = 0; ok() && i < element_count; ++i) {
+        const byte* pos = pc();
         uint32_t table_index = consume_u32v("table index");
-        if (table_index != 0) error("illegal table index != 0");
+        if (table_index != 0) {
+          error(pos, pos, "illegal table index %u != 0", table_index);
+        }
+        WasmIndirectFunctionTable* table = nullptr;
+        if (table_index >= module->function_tables.size()) {
+          error(pos, pos, "out of bounds table index %u", table_index);
+        } else {
+          table = &module->function_tables[table_index];
+        }
         WasmInitExpr offset = consume_init_expr(module, kAstI32);
         uint32_t num_elem = consume_u32v("number of elements");
         std::vector<uint32_t> vector;
@@ -513,7 +524,13 @@ class ModuleDecoder : public Decoder {
         init->entries.reserve(SafeReserve(num_elem));
         for (uint32_t j = 0; ok() && j < num_elem; j++) {
           WasmFunction* func = nullptr;
-          init->entries.push_back(consume_func_index(module, &func));
+          uint32_t index = consume_func_index(module, &func);
+          init->entries.push_back(index);
+          if (table && index < module->functions.size()) {
+            // Canonicalize signature indices during decoding.
+            // TODO(titzer): suboptimal, redundant when verifying only.
+            table->map.FindOrInsert(module->functions[index].sig);
+          }
         }
       }
 

src/wasm/module-decoder.h

@@ -7,10 +7,17 @@
 
 #include "src/wasm/ast-decoder.h"
 #include "src/wasm/wasm-module.h"
+#include "src/wasm/wasm-result.h"
 
 namespace v8 {
 namespace internal {
 namespace wasm {
+
+typedef Result<const WasmModule*> ModuleResult;
+typedef Result<WasmFunction*> FunctionResult;
+typedef std::vector<std::pair<int, int>> FunctionOffsets;
+typedef Result<FunctionOffsets> FunctionOffsetsResult;
+
 // Decodes the bytes of a WASM module between {module_start} and {module_end}.
 V8_EXPORT_PRIVATE ModuleResult DecodeWasmModule(Isolate* isolate, Zone* zone,
                                                 const byte* module_start,

src/wasm/signature-map.cc

@@ -0,0 +1,51 @@
+// Copyright 2016 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.
+
+#include "src/wasm/signature-map.h"
+
+namespace v8 {
+namespace internal {
+namespace wasm {
+
+uint32_t SignatureMap::FindOrInsert(FunctionSig* sig) {
+  auto pos = map_.find(sig);
+  if (pos != map_.end()) {
+    return pos->second;
+  } else {
+    uint32_t index = next_++;
+    map_[sig] = index;
+    return index;
+  }
+}
+
+int32_t SignatureMap::Find(FunctionSig* sig) const {
+  auto pos = map_.find(sig);
+  if (pos != map_.end()) {
+    return static_cast<int32_t>(pos->second);
+  } else {
+    return -1;
+  }
+}
+
+bool SignatureMap::CompareFunctionSigs::operator()(FunctionSig* a,
+                                                   FunctionSig* b) const {
+  if (a == b) return false;
+  if (a->return_count() < b->return_count()) return true;
+  if (a->return_count() > b->return_count()) return false;
+  if (a->parameter_count() < b->parameter_count()) return true;
+  if (a->parameter_count() > b->parameter_count()) return false;
+  for (size_t r = 0; r < a->return_count(); r++) {
+    if (a->GetReturn(r) < b->GetReturn(r)) return true;
+    if (a->GetReturn(r) > b->GetReturn(r)) return false;
+  }
+  for (size_t p = 0; p < a->parameter_count(); p++) {
+    if (a->GetParam(p) < b->GetParam(p)) return true;
+    if (a->GetParam(p) > b->GetParam(p)) return false;
+  }
+  return false;
+}
+
+}  // namespace wasm
+}  // namespace internal
+}  // namespace v8

src/wasm/signature-map.h

@@ -0,0 +1,41 @@
+// Copyright 2016 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.
+
+#ifndef V8_WASM_SIGNATURE_MAP_H_
+#define V8_WASM_SIGNATURE_MAP_H_
+
+#include <map>
+
+#include "src/signature.h"
+#include "src/wasm/wasm-opcodes.h"
+
+namespace v8 {
+namespace internal {
+namespace wasm {
+
+// A signature map canonicalizes signatures into a range of indices so that
+// two different {FunctionSig} instances with the same contents map to the
+// same index.
+class SignatureMap {
+ public:
+  // Gets the index for a signature, assigning a new index if necessary.
+  uint32_t FindOrInsert(FunctionSig* sig);
+
+  // Gets the index for a signature, returning {-1} if not found.
+  int32_t Find(FunctionSig* sig) const;
+
+ private:
+  // TODO(wasm): use a hashmap instead of an ordered map?
+  struct CompareFunctionSigs {
+    bool operator()(FunctionSig* a, FunctionSig* b) const;
+  };
+  uint32_t next_ = 0;
+  std::map<FunctionSig*, uint32_t, CompareFunctionSigs> map_;
+};
+
+}  // namespace wasm
+}  // namespace internal
+}  // namespace v8
+
+#endif  // V8_WASM_SIGNATURE_MAP_H_

src/wasm/wasm-interpreter.cc

@@ -1419,7 +1419,16 @@ class ThreadImpl : public WasmInterpreter::Thread {
           if (target == nullptr) {
             return DoTrap(kTrapFuncInvalid, pc);
           } else if (target->function->sig_index != operand.index) {
-            return DoTrap(kTrapFuncSigMismatch, pc);
+            // If not an exact match, we have to do a canonical check.
+            // TODO(titzer): make this faster with some kind of caching?
+            const WasmIndirectFunctionTable* table =
+                &module()->function_tables[0];
+            int function_key = table->map.Find(target->function->sig);
+            if (function_key < 0 ||
+                (function_key !=
+                 table->map.Find(module()->signatures[operand.index]))) {
+              return DoTrap(kTrapFuncSigMismatch, pc);
+            }
           }
 
           DoCall(target, &pc, pc + 1 + operand.length, &limit);

src/wasm/wasm-module-builder.h

@@ -229,6 +229,8 @@ class V8_EXPORT_PRIVATE WasmModuleBuilder : public ZoneObject {
   // Writing methods.
   void WriteTo(ZoneBuffer& buffer) const;
 
+  // TODO(titzer): use SignatureMap from signature-map.h here.
+  // This signature map is zone-allocated, but the other is heap allocated.
   struct CompareFunctionSigs {
     bool operator()(FunctionSig* a, FunctionSig* b) const;
   };

src/wasm/wasm-module.cc

@@ -2059,7 +2059,9 @@ Handle<FixedArray> BuildFunctionTable(Isolate* isolate, uint32_t index,
       isolate->factory()->NewFixedArray(2 * table->max_size, TENURED);
   for (uint32_t i = 0; i < table->size; ++i) {
     const WasmFunction* function = &module->functions[table->values[i]];
-    values->set(i, Smi::FromInt(function->sig_index));
+    int32_t index = table->map.Find(function->sig);
+    DCHECK_GE(index, 0);
+    values->set(i, Smi::FromInt(index));
     values->set(i + table->max_size, Smi::FromInt(table->values[i]));
   }
   // Set the remaining elements to -1 (instead of "undefined"). These

src/wasm/wasm-module.h

@@ -11,8 +11,8 @@
 #include "src/handles.h"
 #include "src/parsing/preparse-data.h"
 
+#include "src/wasm/signature-map.h"
 #include "src/wasm/wasm-opcodes.h"
-#include "src/wasm/wasm-result.h"
 
 namespace v8 {
 namespace internal {
@@ -23,6 +23,8 @@ class WasmCompilationUnit;
 }
 
 namespace wasm {
+class ErrorThrower;
+
 const size_t kMaxModuleSize = 1024 * 1024 * 1024;
 const size_t kMaxFunctionSize = 128 * 1024;
 const size_t kMaxStringSize = 256;
@@ -134,6 +136,7 @@ struct WasmIndirectFunctionTable {
   std::vector<int32_t> values;  // function table, -1 indicating invalid.
   bool imported;                // true if imported.
   bool exported;                // true if exported.
+  SignatureMap map;             // canonicalizing map for sig indexes.
 };
 
 // Static representation of how to initialize a table.
@@ -339,11 +342,6 @@ std::ostream& operator<<(std::ostream& os, const WasmModule& module);
 std::ostream& operator<<(std::ostream& os, const WasmFunction& function);
 std::ostream& operator<<(std::ostream& os, const WasmFunctionName& name);
 
-typedef Result<const WasmModule*> ModuleResult;
-typedef Result<WasmFunction*> FunctionResult;
-typedef std::vector<std::pair<int, int>> FunctionOffsets;
-typedef Result<FunctionOffsets> FunctionOffsetsResult;
-
 class WasmCompiledModule : public FixedArray {
  public:
   static WasmCompiledModule* cast(Object* fixed_array) {

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

@@ -2632,6 +2632,49 @@ WASM_EXEC_TEST(CallIndirect_NoTable) {
   CHECK_TRAP(r.Call(2));
 }
 
+WASM_EXEC_TEST(CallIndirect_canonical) {
+  TestSignatures sigs;
+  TestingModule module(execution_mode);
+
+  WasmFunctionCompiler t1(sigs.i_ii(), &module);
+  BUILD(t1, WASM_I32_ADD(WASM_GET_LOCAL(0), WASM_GET_LOCAL(1)));
+  t1.CompileAndAdd(/*sig_index*/ 0);
+
+  WasmFunctionCompiler t2(sigs.i_ii(), &module);
+  BUILD(t2, WASM_I32_SUB(WASM_GET_LOCAL(0), WASM_GET_LOCAL(1)));
+  t2.CompileAndAdd(/*sig_index*/ 1);
+
+  WasmFunctionCompiler t3(sigs.f_ff(), &module);
+  BUILD(t3, WASM_F32_SUB(WASM_GET_LOCAL(0), WASM_GET_LOCAL(1)));
+  t3.CompileAndAdd(/*sig_index*/ 2);
+
+  // Signature table.
+  module.AddSignature(sigs.i_ii());
+  module.AddSignature(sigs.i_ii());
+  module.AddSignature(sigs.f_ff());
+
+  // Function table.
+  uint16_t i1 = static_cast<uint16_t>(t1.function_index());
+  uint16_t i2 = static_cast<uint16_t>(t2.function_index());
+  uint16_t i3 = static_cast<uint16_t>(t3.function_index());
+  uint16_t indirect_function_table[] = {i1, i2, i3, i1, i2};
+
+  module.AddIndirectFunctionTable(indirect_function_table,
+                                  arraysize(indirect_function_table));
+  module.PopulateIndirectFunctionTable();
+
+  // Builder the caller function.
+  WasmRunner<int32_t> r(&module, MachineType::Int32());
+  BUILD(r, WASM_CALL_INDIRECT2(1, WASM_GET_LOCAL(0), WASM_I8(77), WASM_I8(11)));
+
+  CHECK_EQ(88, r.Call(0));
+  CHECK_EQ(66, r.Call(1));
+  CHECK_TRAP(r.Call(2));
+  CHECK_EQ(88, r.Call(3));
+  CHECK_EQ(66, r.Call(4));
+  CHECK_TRAP(r.Call(5));
+}
+
 WASM_EXEC_TEST(F32Floor) {
   WasmRunner<float> r(execution_mode, MachineType::Float32());
   BUILD(r, WASM_F32_FLOOR(WASM_GET_LOCAL(0)));

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

@@ -227,10 +227,13 @@ class TestingModule : public ModuleEnv {
   }
 
   void AddIndirectFunctionTable(uint16_t* functions, uint32_t table_size) {
-    module_.function_tables.push_back(
-        {table_size, table_size, std::vector<int32_t>(), false, false});
+    module_.function_tables.push_back({table_size, table_size,
+                                       std::vector<int32_t>(), false, false,
+                                       SignatureMap()});
+    WasmIndirectFunctionTable& table = module_.function_tables.back();
     for (uint32_t i = 0; i < table_size; ++i) {
-      module_.function_tables.back().values.push_back(functions[i]);
+      table.values.push_back(functions[i]);
+      table.map.FindOrInsert(module_.functions[functions[i]].sig);
     }
 
     Handle<FixedArray> values = BuildFunctionTable(

test/mjsunit/wasm/indirect-calls.js

@@ -54,19 +54,19 @@ module = (function () {
 
   var sig_i_ii = builder.addType(kSig_i_ii);
   var sig_i_i = builder.addType(kSig_i_i);
-  builder.addImport("mul", sig_i_ii);
-  builder.addFunction("add", sig_i_ii)
+  var mul = builder.addImport("mul", sig_i_ii);
+  var add = builder.addFunction("add", sig_i_ii)
     .addBody([
       kExprGetLocal, 0,  // --
       kExprGetLocal, 1,  // --
       kExprI32Add        // --
     ]);
-  builder.addFunction("popcnt", sig_i_i)
+  var popcnt = builder.addFunction("popcnt", sig_i_i)
     .addBody([
       kExprGetLocal, 0,  // --
       kExprI32Popcnt     // --
     ]);
-  builder.addFunction("main", kSig_i_iii)
+  var main = builder.addFunction("main", kSig_i_iii)
     .addBody([
       kExprGetLocal, 1,
       kExprGetLocal, 2,
@@ -74,7 +74,7 @@ module = (function () {
       kExprCallIndirect, sig_i_ii
     ])
     .exportFunc()
-  builder.appendToTable([0, 1, 2, 3]);
+  builder.appendToTable([mul.index, add.index, popcnt.index, main.index]);
 
   return builder.instantiate({mul: function(a, b) { return a * b | 0; }});
 })();
@@ -84,3 +84,46 @@ assertEquals(99, module.exports.main(1, 22, 77));
 assertTraps(kTrapFuncSigMismatch, "module.exports.main(2, 12, 33)");
 assertTraps(kTrapFuncSigMismatch, "module.exports.main(3, 12, 33)");
 assertTraps(kTrapFuncInvalid, "module.exports.main(4, 12, 33)");
+
+
+module = (function () {
+  var builder = new WasmModuleBuilder();
+
+  var mul = builder.addFunction("mul", kSig_i_ii)
+    .addBody([
+      kExprGetLocal, 0,  // --
+      kExprGetLocal, 1,  // --
+      kExprI32Mul        // --
+    ]);
+  var add = builder.addFunction("add", kSig_i_ii)
+    .addBody([
+      kExprGetLocal, 0,  // --
+      kExprGetLocal, 1,  // --
+      kExprI32Add        // --
+    ]);
+  var sub = builder.addFunction("sub", kSig_i_ii)
+    .addBody([
+      kExprGetLocal, 0,  // --
+      kExprGetLocal, 1,  // --
+      kExprI32Sub        // --
+    ]);
+  builder.addFunction("main", kSig_i_ii)
+    .addBody([
+      kExprI32Const, 33,  // --
+      kExprGetLocal, 0,   // --
+      kExprGetLocal, 1,   // --
+      kExprCallIndirect, 0])  // --
+    .exportAs("main");
+
+  builder.appendToTable([mul.index, add.index, sub.index]);
+
+  return builder.instantiate();
+})();
+
+assertEquals(33, module.exports.main(1, 0));
+assertEquals(66, module.exports.main(2, 0));
+assertEquals(34, module.exports.main(1, 1));
+assertEquals(35, module.exports.main(2, 1));
+assertEquals(32, module.exports.main(1, 2));
+assertEquals(31, module.exports.main(2, 2));
+assertTraps(kTrapFuncInvalid, "module.exports.main(12, 3)");