[wasm] Unify treatment of expressions in elem. segments

We unify the implementation of element segment expression entries with
other initializer expressions: we represent them with a {WireBytesRef}
and decode them with {InitExprInterface}. Except for reducing code
duplication, this also fixes a bug where {global.get} entries in element
segments could reference invalid globals.

Changes:
- Change {WasmElemSegment::Entry} to a union of a {WireBytesRef}
  initializer expression and a {uint32_t} function index.
- In module-decoder, change parsing of expression entries to use
  {consume_init_expr}. Add type checking to
  {consume_element_func_index}, to complement type checking happening in
  {consume_init_expr}.
- In module-instantiate.cc:
  - Move instantiation of indirect tables before loading of element
    segments. This way, when we call {UpdateDispatchTables} in
    {SetTableEntry}, the indirect table for the current table will also
    be updated.
  - Consolidate table entry instantiation into {SetTableEntry}, which
    handles lazily instantiated functions, or dispatches to
    {WasmTableObject::Set}.
  - Rename {InitializeIndirectFunctionTables} to
    {InitializeNonDefaultableTables}.
  - Change {InitializeNonDefaultableTables} and {LoadElemSegmentImpl}
    to use {EvaluateInitExpression}.
- Add a test to exclude mutable/non-imported globals from the element
  section.
- Update tests as needed.
- Update .js module emission in wasm-fuzzer-common.

Change-Id: I29c541bbca8531e8d0312ed95869c8e78a5a0c57
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3364082Reviewed-by: Andreas Haas <ahaas@chromium.org>
Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78476}

src/wasm/module-decoder.cc

@@ -998,20 +998,13 @@ class ModuleDecoderImpl : public Decoder {
           consume_count("number of elements", max_table_init_entries());
 
       for (uint32_t j = 0; j < num_elem; j++) {
-        WasmElemSegment::Entry init =
+        using Entry = WasmElemSegment::Entry;
+        Entry entry =
             segment.element_type == WasmElemSegment::kExpressionElements
-                ? consume_element_expr()
-                : WasmElemSegment::Entry(WasmElemSegment::Entry::kRefFuncEntry,
-                                         consume_element_func_index());
+                ? Entry(consume_init_expr(module_.get(), segment.type))
+                : Entry(consume_element_func_index(segment.type));
         if (failed()) return;
-        if (!IsSubtypeOf(TypeOf(init), segment.type, module_.get())) {
-          errorf(pc_,
-                 "Invalid type in the init expression. The expected type is "
-                 "'%s', but the actual type is '%s'.",
-                 segment.type.name().c_str(), TypeOf(init).name().c_str());
-          return;
-        }
-        segment.entries.push_back(init);
+        segment.entries.push_back(entry);
       }
       module_->elem_segments.push_back(std::move(segment));
     }
@@ -1504,18 +1497,6 @@ class ModuleDecoderImpl : public Decoder {
   AccountingAllocator allocator_;
   Zone init_expr_zone_{&allocator_, "initializer expression zone"};
 
-  ValueType TypeOf(WasmElemSegment::Entry entry) {
-    switch (entry.kind) {
-      case WasmElemSegment::Entry::kGlobalGetEntry:
-        return module_->globals[entry.index].type;
-      case WasmElemSegment::Entry::kRefFuncEntry:
-        return ValueType::Ref(module_->functions[entry.index].sig_index,
-                              kNonNullable);
-      case WasmElemSegment::Entry::kRefNullEntry:
-        return ValueType::Ref(entry.index, kNullable);
-    }
-  }
-
   bool has_seen_unordered_section(SectionCode section_code) {
     return seen_unordered_sections_ & (1 << section_code);
   }
@@ -2046,51 +2027,23 @@ class ModuleDecoderImpl : public Decoder {
     }
   }
 
-  uint32_t consume_element_func_index() {
+  uint32_t consume_element_func_index(ValueType expected) {
     WasmFunction* func = nullptr;
+    const byte* initial_pc = pc();
     uint32_t index =
         consume_func_index(module_.get(), &func, "element function index");
     if (failed()) return index;
-    func->declared = true;
-    DCHECK_NE(func, nullptr);
+    DCHECK_NOT_NULL(func);
     DCHECK_EQ(index, func->func_index);
-    return index;
-  }
-
-  // TODO(manoskouk): Implement this with consume_init_expr(). It will require
-  // changes in module-instantiate.cc, in {LoadElemSegmentImpl}.
-  WasmElemSegment::Entry consume_element_expr() {
-    uint8_t opcode = consume_u8("element opcode");
-    if (failed()) return {};
-    switch (opcode) {
-      case kExprRefNull: {
-        HeapTypeImmediate<kFullValidation> imm(WasmFeatures::All(), this,
-                                               this->pc(), module_.get());
-        consume_bytes(imm.length, "ref.null immediate");
-        expect_u8("end opcode", kExprEnd);
-        return {WasmElemSegment::Entry::kRefNullEntry,
-                static_cast<uint32_t>(imm.type.representation())};
-      }
-      case kExprRefFunc: {
-        uint32_t index = consume_element_func_index();
-        if (failed()) return {};
-        expect_u8("end opcode", kExprEnd);
-        return {WasmElemSegment::Entry::kRefFuncEntry, index};
-      }
-      case kExprGlobalGet: {
-        uint32_t index = this->consume_u32v("global index");
-        if (failed()) return {};
-        if (index >= module_->globals.size()) {
-          errorf("Out-of-bounds global index %d", index);
-          return {};
-        }
-        expect_u8("end opcode", kExprEnd);
-        return {WasmElemSegment::Entry::kGlobalGetEntry, index};
-      }
-      default:
-        error("invalid opcode in element");
-        return {};
+    ValueType entry_type = ValueType::Ref(func->sig_index, kNonNullable);
+    if (V8_UNLIKELY(!IsSubtypeOf(entry_type, expected, module_.get()))) {
+      errorf(initial_pc,
+             "Invalid type in element entry: expected %s, got %s instead.",
+             expected.name().c_str(), entry_type.name().c_str());
+      return index;
     }
+    func->declared = true;
+    return index;
   }
 };
 

src/wasm/module-instantiate.h

@@ -11,7 +11,6 @@
 
 #include <stdint.h>
 
-#include "include/v8-metrics.h"
 #include "include/v8config.h"
 
 namespace v8 {
@@ -20,7 +19,6 @@ namespace internal {
 class Isolate;
 class JSArrayBuffer;
 class JSReceiver;
-class WasmInitExpr;
 class WasmModuleObject;
 class WasmInstanceObject;
 class Zone;
@@ -43,9 +41,6 @@ bool LoadElemSegment(Isolate* isolate, Handle<WasmInstanceObject> instance,
                      uint32_t table_index, uint32_t segment_index, uint32_t dst,
                      uint32_t src, uint32_t count) V8_WARN_UNUSED_RESULT;
 
-uint32_t EvalUint32InitExpr(Handle<WasmInstanceObject> instance,
-                            const WasmInitExpr& expr);
-
 }  // namespace wasm
 }  // namespace internal
 }  // namespace v8

src/wasm/wasm-module.h

@@ -121,13 +121,17 @@ struct WasmElemSegment {
     kStatusPassive,     // copied explicitly after instantiation.
     kStatusDeclarative  // purely declarative and never copied.
   };
-  struct Entry {
-    enum Kind { kGlobalGetEntry, kRefFuncEntry, kRefNullEntry } kind;
+  enum ElementType { kFunctionIndexElements, kExpressionElements };
+  // An element segment entry. If {element_type == kExpressionElements}, it
+  // refers to an initializer expression (via a {WireBytesRef}); otherwise, it
+  // represents a function index.
+  union Entry {
+    WireBytesRef ref;
     uint32_t index;
-    Entry(Kind kind, uint32_t index) : kind(kind), index(index) {}
-    Entry() : kind(kRefNullEntry), index(0) {}
+
+    explicit Entry(uint32_t index) : index(index) {}
+    explicit Entry(WireBytesRef ref) : ref(ref) {}
   };
-  enum ElementType { kFunctionIndexElements, kExpressionElements };
 
   // Construct an active segment.
   WasmElemSegment(ValueType type, uint32_t table_index, WireBytesRef offset,

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

@@ -331,8 +331,7 @@ uint32_t TestingModuleBuilder::AddPassiveElementSegment(
       WasmElemSegment::kFunctionIndexElements);
   auto& elem_segment = test_module_->elem_segments.back();
   for (uint32_t entry : entries) {
-    elem_segment.entries.push_back(
-        WasmElemSegment::Entry(WasmElemSegment::Entry::kRefFuncEntry, entry));
+    elem_segment.entries.emplace_back(entry);
   }
 
   // The vector pointers may have moved, so update the instance object.

test/fuzzer/wasm-fuzzer-common.cc

@@ -298,22 +298,6 @@ std::ostream& operator<<(std::ostream& os, const PrintName& name) {
   return os.write(name.name.begin(), name.name.size());
 }
 
-std::ostream& operator<<(std::ostream& os, WasmElemSegment::Entry entry) {
-  os << "WasmInitExpr.";
-  switch (entry.kind) {
-    case WasmElemSegment::Entry::kGlobalGetEntry:
-      os << "GlobalGet(" << entry.index;
-      break;
-    case WasmElemSegment::Entry::kRefFuncEntry:
-      os << "RefFunc(" << entry.index;
-      break;
-    case WasmElemSegment::Entry::kRefNullEntry:
-      os << "RefNull(" << HeapType(entry.index).name().c_str();
-      break;
-  }
-  return os << ")";
-}
-
 // An interface for WasmFullDecoder used to decode initializer expressions. As
 // opposed to the one in src/wasm/, this emits {WasmInitExpr} as opposed to a
 // {WasmValue}.
@@ -664,10 +648,19 @@ void GenerateTestCase(Isolate* isolate, ModuleWireBytes wire_bytes,
     }
     os << "[";
     for (uint32_t i = 0; i < elem_segment.entries.size(); i++) {
-      os << elem_segment.entries[i];
+      if (elem_segment.element_type == WasmElemSegment::kExpressionElements) {
+        DecodeAndAppendInitExpr(os, &zone, module, wire_bytes,
+                                elem_segment.entries[i].ref, elem_segment.type);
+      } else {
+        os << elem_segment.entries[i].index;
+      }
       if (i < elem_segment.entries.size() - 1) os << ", ";
     }
-    os << "], " << ValueTypeToConstantName(elem_segment.type) << ");\n";
+    os << "], "
+       << (elem_segment.element_type == WasmElemSegment::kExpressionElements
+               ? ValueTypeToConstantName(elem_segment.type)
+               : "undefined")
+       << ");\n";
   }
 
   for (const WasmTag& tag : module->tags) {

test/mjsunit/wasm/element-segments-with-reftypes.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --experimental-wasm-typed-funcref
+// Flags: --experimental-wasm-gc
 
 d8.file.execute('test/mjsunit/wasm/wasm-module-builder.js');
 
@@ -97,3 +97,22 @@ d8.file.execute('test/mjsunit/wasm/wasm-module-builder.js');
   assertEquals(instance.exports.table.get(2)(10), 20);
   assertEquals(instance.exports.table.get(3)(10), 11);
 })();
+
+// Test that mutable globals cannot be used in element segments, even under
+// --experimental-wasm-gc.
+(function TestMutableGlobalInElementSegment() {
+  print(arguments.callee.name);
+  let builder = new WasmModuleBuilder();
+  let global = builder.addImportedGlobal("m", "g", kWasmFuncRef, true);
+  let table = builder.addTable(kWasmFuncRef, 10, 10);
+  builder.addActiveElementSegment(
+      table.index, WasmInitExpr.I32Const(0),
+      [WasmInitExpr.GlobalGet(global.index)], kWasmFuncRef);
+  builder.addExportOfKind("table", kExternalTable, table.index);
+
+  assertThrows(
+    () => builder.instantiate({m : {g :
+        new WebAssembly.Global({value: "anyfunc", mutable: true}, null)}}),
+    WebAssembly.CompileError,
+    /mutable globals cannot be used in initializer expressions/);
+})();

test/mjsunit/wasm/indirect-tables.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --expose-wasm --expose-gc
+// Flags: --expose-gc
 
 d8.file.execute("test/mjsunit/wasm/wasm-module-builder.js");
 
@@ -900,3 +900,19 @@ function js_div(a, b) { return (a / b) | 0; }
   assertEquals(300, main(2));
   assertEquals(400, main(3));
 })();
+
+(function TestNonImportedGlobalInElementSegment() {
+  print(arguments.callee.name);
+  let builder = new WasmModuleBuilder();
+  let global = builder.addGlobal(kWasmFuncRef, false,
+                                 WasmInitExpr.RefNull(kWasmFuncRef));
+  let table = builder.addTable(kWasmFuncRef, 10, 10);
+  builder.addActiveElementSegment(
+      table.index, WasmInitExpr.I32Const(0),
+      [WasmInitExpr.GlobalGet(global.index)], kWasmFuncRef);
+  builder.addExportOfKind("table", kExternalTable, table.index);
+
+  assertThrows(
+    () => builder.instantiate(), WebAssembly.CompileError,
+    /non-imported globals cannot be used in initializer expressions/);
+})();

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

@@ -2210,8 +2210,7 @@ TEST_F(WasmModuleVerifyTest, ElementSectionInitFuncRefTableWithExternRefNull) {
 
   EXPECT_FAILURE_WITH_MSG(
       data,
-      "Invalid type in the init expression. The expected "
-      "type is 'funcref', but the actual type is 'externref'.");
+      "type error in init. expression[0] (expected funcref, got externref)");
 }
 
 TEST_F(WasmModuleVerifyTest, ElementSectionDontInitExternRefImportedTable) {
@@ -2265,7 +2264,7 @@ TEST_F(WasmModuleVerifyTest, ElementSectionGlobalGetOutOfBounds) {
               kFuncRefCode,    // type
               ENTRY_COUNT(1),  // element count
               kExprGlobalGet, 0x00, kExprEnd)};  // init. expression
-  EXPECT_FAILURE_WITH_MSG(data, "Out-of-bounds global index 0");
+  EXPECT_FAILURE_WITH_MSG(data, "Invalid global index: 0");
 }
 
 TEST_F(WasmModuleVerifyTest, IndirectFunctionNoFunctions) {