[wasm] Canonicalize signatures per module at module decode time.

This is needed for proper deserialization of code and has the nice
side effect of fixing the nasty race condition that led us to
introducing a lock on the signature map.

R=mtrofin@chromium.org
CC=clemensh@chromium.org

Bug: 
Change-Id: I6a018344ad8b58b088b20756d3b00ae08232bbb9
Reviewed-on: https://chromium-review.googlesource.com/718937
Commit-Queue: Ben Titzer <titzer@chromium.org>
Reviewed-by: Mircea Trofin <mtrofin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48589}

src/compiler/wasm-compiler.cc

@@ -2415,10 +2415,11 @@ Node* WasmGraphBuilder::CallIndirect(uint32_t sig_index, Node** args,
                                           Int32Constant(kPointerSizeLog2)),
                          Int32Constant(fixed_offset)),
         *effect_, *control_);
-    auto map = env_->signature_maps[table_index];
-    Node* sig_match = graph()->NewNode(
-        machine->WordEqual(), load_sig,
-        jsgraph()->SmiConstant(static_cast<int>(map->FindOrInsert(sig))));
+    int32_t canonical_sig_num = env_->module->signature_ids[sig_index];
+    CHECK_GE(sig_index, 0);
+    Node* sig_match =
+        graph()->NewNode(machine->WordEqual(), load_sig,
+                         jsgraph()->SmiConstant(canonical_sig_num));
     TrapIfFalse(wasm::kTrapFuncSigMismatch, sig_match, position);
   }
 
@@ -4206,12 +4207,11 @@ Handle<Code> CompileJSToWasmWrapper(Isolate* isolate, wasm::WasmModule* module,
 
   // TODO(titzer): compile JS to WASM wrappers without a {ModuleEnv}.
   ModuleEnv env = {
-      module,                              // module itself
-      std::vector<Address>(),              // function_tables
-      std::vector<Address>(),              // signature_tables
-      std::vector<wasm::SignatureMap*>(),  // signature_maps
-      std::vector<Handle<Code>>(),         // function_code
-      BUILTIN_CODE(isolate, Illegal)       // default_function_code
+      module,
+      std::vector<Address>(),         // function_tables
+      std::vector<Address>(),         // signature_tables
+      std::vector<Handle<Code>>(),    // function_code
+      BUILTIN_CODE(isolate, Illegal)  // default_function_code
   };
 
   WasmGraphBuilder builder(&env, &zone, &jsgraph,
@@ -4595,7 +4595,6 @@ SourcePositionTable* WasmCompilationUnit::BuildGraphForWasmFunction(
     size_t tables_size = env_->module->function_tables.size();
     DCHECK_EQ(tables_size, env_->function_tables.size());
     DCHECK_EQ(tables_size, env_->signature_tables.size());
-    DCHECK_EQ(tables_size, env_->signature_maps.size());
   }
 #endif
 

src/compiler/wasm-compiler.h

@@ -59,12 +59,7 @@ struct ModuleEnv {
   // (the same length as module.function_tables)
   //  We use the address to a global handle to the FixedArray.
   const std::vector<Address> signature_tables;
-  // Signature maps canonicalize {FunctionSig*} to indexes. New entries can be
-  // added to a signature map during graph building.
-  // Normally, these signature maps correspond to the signature maps in the
-  // function tables stored in the {module}.
-  const std::vector<wasm::SignatureMap*> signature_maps;
-  // Contains the code objects to call for each indirect call.
+  // Contains the code objects to call for each direct call.
   // (the same length as module.functions)
   const std::vector<Handle<Code>> function_code;
   // If the default code is not a null handle, always use it for direct calls.

src/wasm/module-compiler.cc

@@ -699,7 +699,6 @@ compiler::ModuleEnv CreateModuleEnvFromCompiledModule(
 
   std::vector<GlobalHandleAddress> function_tables;
   std::vector<GlobalHandleAddress> signature_tables;
-  std::vector<SignatureMap*> signature_maps;
 
   int num_function_tables = static_cast<int>(module->function_tables.size());
   for (int i = 0; i < num_function_tables; ++i) {
@@ -709,7 +708,6 @@ compiler::ModuleEnv CreateModuleEnvFromCompiledModule(
     // TODO(clemensh): defer these handles for concurrent compilation.
     function_tables.push_back(WasmCompiledModule::GetTableValue(ft, i));
     signature_tables.push_back(WasmCompiledModule::GetTableValue(st, i));
-    signature_maps.push_back(&module->function_tables[i].map);
   }
 
   std::vector<Handle<Code>> empty_code;
@@ -717,7 +715,6 @@ compiler::ModuleEnv CreateModuleEnvFromCompiledModule(
   compiler::ModuleEnv result = {module,            // --
                                 function_tables,   // --
                                 signature_tables,  // --
-                                signature_maps,    // --
                                 empty_code,        // --
                                 BUILTIN_CODE(isolate, WasmCompileLazy)};
   return result;
@@ -1405,7 +1402,6 @@ std::unique_ptr<compiler::ModuleEnv> CreateDefaultModuleEnv(
     Isolate* isolate, WasmModule* module, Handle<Code> illegal_builtin) {
   std::vector<GlobalHandleAddress> function_tables;
   std::vector<GlobalHandleAddress> signature_tables;
-  std::vector<SignatureMap*> signature_maps;
 
   for (size_t i = 0; i < module->function_tables.size(); i++) {
     Handle<Object> func_table =
@@ -1420,7 +1416,6 @@ std::unique_ptr<compiler::ModuleEnv> CreateDefaultModuleEnv(
                             v8::WeakCallbackType::kFinalizer);
     function_tables.push_back(func_table.address());
     signature_tables.push_back(sig_table.address());
-    signature_maps.push_back(&module->function_tables[i].map);
   }
 
   std::vector<Handle<Code>> empty_code;
@@ -1429,7 +1424,6 @@ std::unique_ptr<compiler::ModuleEnv> CreateDefaultModuleEnv(
       module,            // --
       function_tables,   // --
       signature_tables,  // --
-      signature_maps,    // --
       empty_code,        // --
       illegal_builtin    // --
   };
@@ -2321,12 +2315,15 @@ int InstanceBuilder::ProcessImports(Handle<FixedArray> code_table,
                                 index, i);
             return -1;
           }
+          // Look up the signature's canonical id. If there is no canonical
+          // id, then the signature does not appear at all in this module,
+          // so putting {-1} in the table will cause checks to always fail.
           auto target = Handle<WasmExportedFunction>::cast(val);
           FunctionSig* sig = nullptr;
           Handle<Code> code =
               MakeWasmToWasmWrapper(isolate_, target, nullptr, &sig,
                                     &imported_wasm_instances, instance);
-          int sig_index = table.map.FindOrInsert(sig);
+          int sig_index = module_->signature_map.Find(sig);
           table_instance.signature_table->set(i, Smi::FromInt(sig_index));
           table_instance.function_table->set(i, *code);
         }
@@ -2763,7 +2760,6 @@ void InstanceBuilder::LoadTableSegments(Handle<FixedArray> code_table,
                                         Handle<WasmInstanceObject> instance) {
   int function_table_count = static_cast<int>(module_->function_tables.size());
   for (int index = 0; index < function_table_count; ++index) {
-    WasmIndirectFunctionTable& table = module_->function_tables[index];
     TableInstance& table_instance = table_instances_[index];
 
     Handle<FixedArray> all_dispatch_tables;
@@ -2801,8 +2797,7 @@ void InstanceBuilder::LoadTableSegments(Handle<FixedArray> code_table,
         uint32_t func_index = table_init.entries[i];
         WasmFunction* function = &module_->functions[func_index];
         int table_index = static_cast<int>(i + base);
-        int32_t sig_index = table.map.Find(function->sig);
-        DCHECK_GE(sig_index, 0);
+        uint32_t sig_index = module_->signature_ids[function->sig_index];
         table_instance.signature_table->set(table_index,
                                             Smi::FromInt(sig_index));
         Handle<Code> wasm_code = EnsureTableExportLazyDeoptData(

src/wasm/module-decoder.cc

@@ -423,7 +423,9 @@ class ModuleDecoderImpl : public Decoder {
             static_cast<int>(pc_ - start_));
       FunctionSig* s = consume_sig(module_->signature_zone.get());
       module_->signatures.push_back(s);
+      module_->signature_ids.push_back(module_->signature_map.FindOrInsert(s));
     }
+    module_->signature_map.Freeze();
   }
 
   void DecodeImportSection() {
@@ -684,12 +686,10 @@ class ModuleDecoderImpl : public Decoder {
       if (table_index != 0) {
         errorf(pos, "illegal table index %u != 0", table_index);
       }
-      WasmIndirectFunctionTable* table = nullptr;
       if (table_index >= module_->function_tables.size()) {
         errorf(pos, "out of bounds table index %u", table_index);
         break;
       }
-      table = &module_->function_tables[table_index];
       WasmInitExpr offset = consume_init_expr(module_.get(), kWasmI32);
       uint32_t num_elem =
           consume_count("number of elements", kV8MaxWasmTableEntries);
@@ -702,8 +702,6 @@ class ModuleDecoderImpl : public Decoder {
         if (!ok()) break;
         DCHECK_EQ(index, func->func_index);
         init->entries.push_back(index);
-        // Canonicalize signature indices during decoding.
-        table->map.FindOrInsert(func->sig);
       }
     }
   }

src/wasm/signature-map.cc

@@ -8,10 +8,8 @@ namespace v8 {
 namespace internal {
 namespace wasm {
 
-SignatureMap::SignatureMap() : mutex_(new base::Mutex()) {}
-
 uint32_t SignatureMap::FindOrInsert(FunctionSig* sig) {
-  base::LockGuard<base::Mutex> guard(mutex_.get());
+  CHECK(!frozen_);
   auto pos = map_.find(sig);
   if (pos != map_.end()) {
     return pos->second;
@@ -23,7 +21,6 @@ uint32_t SignatureMap::FindOrInsert(FunctionSig* sig) {
 }
 
 int32_t SignatureMap::Find(FunctionSig* sig) const {
-  base::LockGuard<base::Mutex> guard(mutex_.get());
   auto pos = map_.find(sig);
   if (pos != map_.end()) {
     return static_cast<int32_t>(pos->second);

src/wasm/signature-map.h

@@ -22,8 +22,7 @@ class V8_EXPORT_PRIVATE SignatureMap {
   // Allow default construction and move construction (because we have vectors
   // of objects containing SignatureMaps), but disallow copy or assign. It's
   // too easy to get security bugs by accidentally updating a copy of the map.
-  SignatureMap();
-  SignatureMap(SignatureMap&&) = default;
+  MOVE_ONLY_WITH_DEFAULT_CONSTRUCTORS(SignatureMap);
 
   // Gets the index for a signature, assigning a new index if necessary.
   uint32_t FindOrInsert(FunctionSig* sig);
@@ -31,17 +30,17 @@ class V8_EXPORT_PRIVATE SignatureMap {
   // Gets the index for a signature, returning {-1} if not found.
   int32_t Find(FunctionSig* sig) const;
 
+  // Disallows further insertions to this signature map.
+  void Freeze() { frozen_ = true; }
+
  private:
   // TODO(wasm): use a hashmap instead of an ordered map?
   struct CompareFunctionSigs {
     bool operator()(FunctionSig* a, FunctionSig* b) const;
   };
   uint32_t next_ = 0;
-  // TODO(wasm): performance-critical, replace with a reader-writer lock
-  std::unique_ptr<base::Mutex> mutex_;
+  bool frozen_ = false;
   std::map<FunctionSig*, uint32_t, CompareFunctionSigs> map_;
-
-  DISALLOW_COPY_AND_ASSIGN(SignatureMap);
 };
 
 }  // namespace wasm

src/wasm/wasm-interpreter.cc

@@ -2301,13 +2301,12 @@ class ThreadImpl {
       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.
-        // TODO(titzer): make this faster with some kind of caching?
-        const WasmIndirectFunctionTable* table =
-            &module()->function_tables[table_index];
-        int function_key = table->map.Find(code->function->sig);
-        if (function_key < 0 ||
-            (function_key !=
-             table->map.Find(module()->signatures[sig_index]))) {
+        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};
         }
       }
@@ -2325,11 +2324,9 @@ class ThreadImpl {
       // changes to the tables.
 
       // Canonicalize signature index.
-      // TODO(titzer): make this faster with some kind of caching?
-      const WasmIndirectFunctionTable* table =
-          &module()->function_tables[table_index];
-      FunctionSig* sig = module()->signatures[sig_index];
-      uint32_t canonical_sig_index = table->map.Find(sig);
+      uint32_t canonical_sig_index = module()->signature_ids[sig_index];
+      DCHECK_EQ(canonical_sig_index,
+                module()->signature_map.Find(module()->signatures[sig_index]));
 
       // Check signature.
       FixedArray* sig_tables = compiled_module->ptr_to_signature_tables();

src/wasm/wasm-module.cc

@@ -133,19 +133,17 @@ void UpdateDispatchTables(Isolate* isolate, Handle<FixedArray> dispatch_tables,
                           Handle<Code> code) {
   DCHECK_EQ(0, dispatch_tables->length() % 4);
   for (int i = 0; i < dispatch_tables->length(); i += 4) {
-    int table_index = Smi::ToInt(dispatch_tables->get(i + 1));
     Handle<FixedArray> function_table(
         FixedArray::cast(dispatch_tables->get(i + 2)), isolate);
     Handle<FixedArray> signature_table(
         FixedArray::cast(dispatch_tables->get(i + 3)), isolate);
     if (function) {
-      // TODO(titzer): the signature might need to be copied to avoid
-      // a dangling pointer in the signature map.
       Handle<WasmInstanceObject> instance(
           WasmInstanceObject::cast(dispatch_tables->get(i)), isolate);
-      auto& func_table = instance->module()->function_tables[table_index];
-      uint32_t sig_index = func_table.map.FindOrInsert(function->sig);
-      signature_table->set(index, Smi::FromInt(static_cast<int>(sig_index)));
+      // Note that {SignatureMap::Find} may return {-1} if the signature is
+      // not found; it will simply never match any check.
+      auto sig_index = instance->module()->signature_map.Find(function->sig);
+      signature_table->set(index, Smi::FromInt(sig_index));
       function_table->set(index, *code);
     } else {
       signature_table->set(index, Smi::FromInt(-1));

src/wasm/wasm-module.h

@@ -98,7 +98,6 @@ struct WasmIndirectFunctionTable {
   std::vector<int32_t> values;  // function table, -1 indicating invalid.
   bool imported = false;        // true if imported.
   bool exported = false;        // true if exported.
-  SignatureMap map;             // canonicalizing map for sig indexes.
 };
 
 // Static representation of how to initialize a table.
@@ -157,7 +156,8 @@ struct V8_EXPORT_PRIVATE WasmModule {
   uint32_t num_exported_functions = 0;
   WireBytesRef name = {0, 0};
   // TODO(wasm): Add url here, for spec'ed location information.
-  std::vector<FunctionSig*> signatures;
+  std::vector<FunctionSig*> signatures;  // by signature index
+  std::vector<uint32_t> signature_ids;   // by signature index
   std::vector<WasmFunction> functions;
   std::vector<WasmDataSegment> data_segments;
   std::vector<WasmIndirectFunctionTable> function_tables;
@@ -165,6 +165,7 @@ struct V8_EXPORT_PRIVATE WasmModule {
   std::vector<WasmExport> export_table;
   std::vector<WasmException> exceptions;
   std::vector<WasmTableInit> table_inits;
+  SignatureMap signature_map;  // canonicalizing map for signature indexes.
 
   WasmModule() : WasmModule(nullptr) {}
   WasmModule(std::unique_ptr<Zone> owned);

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

@@ -140,7 +140,6 @@ void TestingModuleBuilder::AddIndirectFunctionTable(uint16_t* function_indexes,
   table.has_maximum_size = true;
   for (uint32_t i = 0; i < table_size; ++i) {
     table.values.push_back(function_indexes[i]);
-    table.map.FindOrInsert(test_module_.functions[function_indexes[i]].sig);
   }
 
   function_tables_.push_back(
@@ -165,7 +164,8 @@ void TestingModuleBuilder::PopulateIndirectFunctionTable() {
     int table_size = static_cast<int>(table.values.size());
     for (int j = 0; j < table_size; j++) {
       WasmFunction& function = test_module_.functions[table.values[j]];
-      signature_table->set(j, Smi::FromInt(table.map.Find(function.sig)));
+      signature_table->set(
+          j, Smi::FromInt(test_module_.signature_map.Find(function.sig)));
       function_table->set(j, *function_code_[function.func_index]);
     }
   }
@@ -189,13 +189,8 @@ uint32_t TestingModuleBuilder::AddBytes(Vector<const byte> bytes) {
 }
 
 compiler::ModuleEnv TestingModuleBuilder::CreateModuleEnv() {
-  std::vector<SignatureMap*> signature_maps;
-  for (size_t i = 0; i < test_module_.function_tables.size(); i++) {
-    auto& function_table = test_module_.function_tables[i];
-    signature_maps.push_back(&function_table.map);
-  }
-  return {&test_module_,  function_tables_, signature_tables_,
-          signature_maps, function_code_,   Handle<Code>::null()};
+  return {&test_module_, function_tables_, signature_tables_, function_code_,
+          Handle<Code>::null()};
 }
 
 const WasmGlobal* TestingModuleBuilder::AddGlobal(ValueType type) {

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

@@ -104,7 +104,11 @@ class TestingModuleBuilder {
   }
 
   byte AddSignature(FunctionSig* sig) {
+    DCHECK_EQ(test_module_.signatures.size(),
+              test_module_.signature_ids.size());
     test_module_.signatures.push_back(sig);
+    auto canonical_sig_num = test_module_.signature_map.FindOrInsert(sig);
+    test_module_.signature_ids.push_back(canonical_sig_num);
     size_t size = test_module_.signatures.size();
     CHECK_GT(127, size);
     return static_cast<byte>(size - 1);