[wasm-gc][bug] Fix signature canonicalization

We used not to emit canonical indexes for arrays and structs into
WasmModule::signature_ids, which resulted in signature_ids not referring
to the correct type indices in a WasmModule.

Changes:
- Rename signature_ids to canonical_type_ids.
- Emit trivial canonical type ids for structs and arrays.
- Add a test to catch the existing bug.
- Improve DCHECKs for module type accessors.

Bug: v8:7748
Change-Id: I67ad58865e35b459b21db12557564b652035db75
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2444989
Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70318}

src/compiler/wasm-compiler.cc

@@ -2906,7 +2906,7 @@ Node* WasmGraphBuilder::BuildIndirectCall(uint32_t table_index,
   const bool needs_typechecking =
       env_->module->tables[table_index].type == wasm::kWasmFuncRef;
   if (needs_typechecking) {
-    int32_t expected_sig_id = env_->module->signature_ids[sig_index];
+    int32_t expected_sig_id = env_->module->canonicalized_type_ids[sig_index];
     Node* sig_match = graph()->NewNode(machine->Word32Equal(), loaded_sig,
                                        Int32Constant(expected_sig_id));
     TrapIfFalse(wasm::kTrapFuncSigMismatch, sig_match, position);

src/wasm/baseline/liftoff-compiler.cc

@@ -3918,7 +3918,8 @@ class LiftoffCompiler {
     Label* invalid_func_label = AddOutOfLineTrap(
         decoder->position(), WasmCode::kThrowWasmTrapTableOutOfBounds);
 
-    uint32_t canonical_sig_num = env_->module->signature_ids[imm.sig_index];
+    uint32_t canonical_sig_num =
+        env_->module->canonicalized_type_ids[imm.sig_index];
     DCHECK_GE(canonical_sig_num, 0);
     DCHECK_GE(kMaxInt, canonical_sig_num);
 

src/wasm/module-instantiate.cc

@@ -1950,7 +1950,7 @@ bool LoadElemSegmentImpl(Isolate* isolate, Handle<WasmInstanceObject> instance,
 
     // Update the local dispatch table first if necessary.
     if (IsSubtypeOf(table_object->type(), kWasmFuncRef, module)) {
-      uint32_t sig_id = module->signature_ids[function->sig_index];
+      uint32_t sig_id = module->canonicalized_type_ids[function->sig_index];
       IndirectFunctionTableEntry(instance, table_index, entry_index)
           .Set(sig_id, instance, func_index);
     }

src/wasm/wasm-module.cc

@@ -618,10 +618,11 @@ size_t EstimateStoredSize(const WasmModule* module) {
          (module->signature_zone ? module->signature_zone->allocation_size()
                                  : 0) +
          VectorSize(module->types) + VectorSize(module->type_kinds) +
-         VectorSize(module->signature_ids) + VectorSize(module->functions) +
-         VectorSize(module->data_segments) + VectorSize(module->tables) +
-         VectorSize(module->import_table) + VectorSize(module->export_table) +
-         VectorSize(module->exceptions) + VectorSize(module->elem_segments);
+         VectorSize(module->canonicalized_type_ids) +
+         VectorSize(module->functions) + VectorSize(module->data_segments) +
+         VectorSize(module->tables) + VectorSize(module->import_table) +
+         VectorSize(module->export_table) + VectorSize(module->exceptions) +
+         VectorSize(module->elem_segments);
 }
 
 size_t PrintSignature(Vector<char> buffer, const wasm::FunctionSig* sig,

src/wasm/wasm-module.h

@@ -283,9 +283,12 @@ struct V8_EXPORT_PRIVATE WasmModule {
   uint32_t num_declared_data_segments = 0;  // From the DataCount section.
   WireBytesRef code = {0, 0};
   WireBytesRef name = {0, 0};
-  std::vector<TypeDefinition> types;    // by type index
-  std::vector<uint8_t> type_kinds;      // by type index
-  std::vector<uint32_t> signature_ids;  // by signature index
+  std::vector<TypeDefinition> types;  // by type index
+  std::vector<uint8_t> type_kinds;    // by type index
+  // Map from each type index to the index of its corresponding canonical type.
+  // Note: right now, only functions are canonicalized, and arrays and structs
+  // map to themselves.
+  std::vector<uint32_t> canonicalized_type_ids;
 
   bool has_type(uint32_t index) const { return index < types.size(); }
 
@@ -293,37 +296,43 @@ struct V8_EXPORT_PRIVATE WasmModule {
     types.push_back(TypeDefinition(sig));
     type_kinds.push_back(kWasmFunctionTypeCode);
     uint32_t canonical_id = sig ? signature_map.FindOrInsert(*sig) : 0;
-    signature_ids.push_back(canonical_id);
-  }
-  const FunctionSig* signature(uint32_t index) const {
-    DCHECK(type_kinds[index] == kWasmFunctionTypeCode);
-    return types[index].function_sig;
+    canonicalized_type_ids.push_back(canonical_id);
   }
   bool has_signature(uint32_t index) const {
     return index < types.size() && type_kinds[index] == kWasmFunctionTypeCode;
   }
+  const FunctionSig* signature(uint32_t index) const {
+    DCHECK(has_signature(index));
+    return types[index].function_sig;
+  }
+
   void add_struct_type(const StructType* type) {
     types.push_back(TypeDefinition(type));
     type_kinds.push_back(kWasmStructTypeCode);
-  }
-  const StructType* struct_type(uint32_t index) const {
-    DCHECK(type_kinds[index] == kWasmStructTypeCode);
-    return types[index].struct_type;
+    // No canonicalization for structs.
+    canonicalized_type_ids.push_back(0);
   }
   bool has_struct(uint32_t index) const {
     return index < types.size() && type_kinds[index] == kWasmStructTypeCode;
   }
+  const StructType* struct_type(uint32_t index) const {
+    DCHECK(has_struct(index));
+    return types[index].struct_type;
+  }
+
   void add_array_type(const ArrayType* type) {
     types.push_back(TypeDefinition(type));
     type_kinds.push_back(kWasmArrayTypeCode);
-  }
-  const ArrayType* array_type(uint32_t index) const {
-    DCHECK(type_kinds[index] == kWasmArrayTypeCode);
-    return types[index].array_type;
+    // No canonicalization for arrays.
+    canonicalized_type_ids.push_back(0);
   }
   bool has_array(uint32_t index) const {
     return index < types.size() && type_kinds[index] == kWasmArrayTypeCode;
   }
+  const ArrayType* array_type(uint32_t index) const {
+    DCHECK(has_array(index));
+    return types[index].array_type;
+  }
 
   std::vector<WasmFunction> functions;
   std::vector<WasmDataSegment> data_segments;

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

@@ -120,8 +120,8 @@ class TestingModuleBuilder {
   }
 
   byte AddSignature(const FunctionSig* sig) {
-    // TODO(7748): This will need updating for struct/array types support.
-    DCHECK_EQ(test_module_->types.size(), test_module_->signature_ids.size());
+    DCHECK_EQ(test_module_->types.size(),
+              test_module_->canonicalized_type_ids.size());
     test_module_->add_signature(sig);
     size_t size = test_module_->types.size();
     CHECK_GT(127, size);

test/common/wasm/wasm-interpreter.cc

@@ -3786,7 +3786,7 @@ class WasmInterpreterInternals {
   CallResult CallIndirectFunction(uint32_t table_index, uint32_t entry_index,
                                   uint32_t sig_index) {
     HandleScope handle_scope(isolate_);  // Avoid leaking handles.
-    uint32_t expected_sig_id = module()->signature_ids[sig_index];
+    uint32_t expected_sig_id = module()->canonicalized_type_ids[sig_index];
     DCHECK_EQ(expected_sig_id,
               module()->signature_map.Find(*module()->signature(sig_index)));
     // Bounds check against table size.

test/unittests/wasm/module-decoder-unittest.cc

@@ -1264,6 +1264,41 @@ TEST_F(WasmModuleVerifyTest, MultipleSignatures) {
   EXPECT_OFF_END_FAILURE(data, 1);
 }
 
+TEST_F(WasmModuleVerifyTest, CanonicalTypeIds) {
+  WASM_FEATURE_SCOPE(typed_funcref);
+  WASM_FEATURE_SCOPE(gc);
+  WASM_FEATURE_SCOPE(reftypes);
+
+  static const byte data[] = {
+      SECTION(Type,                               // --
+              ENTRY_COUNT(5),                     // --
+              WASM_STRUCT_DEF(                    // Struct definition
+                  FIELD_COUNT(1),                 // --
+                  STRUCT_FIELD(kI32Code, true)),  // --
+              SIG_ENTRY_x_x(kI32Code, kF32Code),  // f32 -> i32
+              SIG_ENTRY_x_x(kI32Code, kF64Code),  // f64 -> i32
+              SIG_ENTRY_x_x(kI32Code, kF32Code),  // f32 -> i32 (again)
+              WASM_ARRAY_DEF(kI32Code, true))     // Array definition
+  };
+
+  ModuleResult result = DecodeModule(data, data + sizeof(data));
+  EXPECT_OK(result);
+  const WasmModule* module = result.value().get();
+
+  EXPECT_EQ(5u, module->types.size());
+  EXPECT_EQ(5u, module->type_kinds.size());
+  EXPECT_EQ(5u, module->canonicalized_type_ids.size());
+  EXPECT_EQ(2u, module->signature_map.size());
+
+  // No canonicalization for structs.
+  EXPECT_EQ(0u, module->canonicalized_type_ids[0]);
+  EXPECT_EQ(0u, module->canonicalized_type_ids[1]);
+  EXPECT_EQ(1u, module->canonicalized_type_ids[2]);
+  EXPECT_EQ(0u, module->canonicalized_type_ids[3]);
+  // No canonicalization for arrays.
+  EXPECT_EQ(0u, module->canonicalized_type_ids[4]);
+}
+
 TEST_F(WasmModuleVerifyTest, DataSegmentWithImmutableImportedGlobal) {
   // Import 2 globals so that we can initialize data with a global index != 0.
   const byte data[] = {