[wasm] Do not use WasmInitExpr for element segments

Changes:
- Use a lightweight WasmElemSegment::Entry struct to store element
  segment entries in a WasmModule.
- Also, restructure LoadElemSegmentImpl to handle all types of
  global.get entries correctly.
- Simplify InitializeIndirectFunctionTables and make it handle all types
  of entries correctly.
- In the above two cases, reject WasmJSFunctions for now.

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

src/wasm/module-decoder.cc

@@ -916,10 +916,11 @@ class ModuleDecoderImpl : public Decoder {
           consume_count("number of elements", max_table_init_entries());
 
       for (uint32_t j = 0; j < num_elem; j++) {
-        WasmInitExpr init =
+        WasmElemSegment::Entry init =
             expressions_as_elements
                 ? consume_element_expr()
-                : WasmInitExpr::RefFuncConst(consume_element_func_index());
+                : WasmElemSegment::Entry(WasmElemSegment::Entry::kRefFuncEntry,
+                                         consume_element_func_index());
         if (failed()) return;
         if (!IsSubtypeOf(TypeOf(init), segment.type, module_.get())) {
           errorf(pc_,
@@ -928,7 +929,7 @@ class ModuleDecoderImpl : public Decoder {
                  segment.type.name().c_str(), TypeOf(init).name().c_str());
           return;
         }
-        segment.entries.push_back(std::move(init));
+        segment.entries.push_back(init);
       }
       module_->elem_segments.push_back(std::move(segment));
     }
@@ -1423,8 +1424,16 @@ class ModuleDecoderImpl : public Decoder {
   AccountingAllocator allocator_;
   Zone init_expr_zone_{&allocator_, "initializer expression zone"};
 
-  ValueType TypeOf(const WasmInitExpr& expr) {
-    return expr.type(module_.get(), enabled_features_);
+  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) {
@@ -1991,9 +2000,10 @@ class ModuleDecoderImpl : public Decoder {
     return index;
   }
 
-  // TODO(manoskouk): When reftypes lands, remove this and use
-  // consume_init_expr() instead.
-  WasmInitExpr consume_element_expr() {
+  // TODO(manoskouk): When reftypes lands, consider if we can 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) {
@@ -2002,13 +2012,14 @@ class ModuleDecoderImpl : public Decoder {
                                                this->pc(), module_.get());
         consume_bytes(imm.length, "ref.null immediate");
         expect_u8("end opcode", kExprEnd);
-        return WasmInitExpr::RefNullConst(imm.type.representation());
+        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 WasmInitExpr::RefFuncConst(index);
+        return {WasmElemSegment::Entry::kRefFuncEntry, index};
       }
       case kExprGlobalGet: {
         if (!enabled_features_.has_reftypes()) {
@@ -2025,7 +2036,7 @@ class ModuleDecoderImpl : public Decoder {
           return {};
         }
         expect_u8("end opcode", kExprEnd);
-        return WasmInitExpr::GlobalGet(index);
+        return {WasmElemSegment::Entry::kGlobalGetEntry, index};
       }
       default:
         error("invalid opcode in element");

src/wasm/module-instantiate.cc

@@ -684,6 +684,7 @@ MaybeHandle<WasmInstanceObject> InstanceBuilder::Build() {
   //--------------------------------------------------------------------------
   if (table_count > 0) {
     InitializeIndirectFunctionTables(instance);
+    if (thrower_->error()) return {};
   }
 
   //--------------------------------------------------------------------------
@@ -1770,6 +1771,61 @@ void InstanceBuilder::ProcessExports(Handle<WasmInstanceObject> instance) {
   }
 }
 
+void SetNullTableEntry(Isolate* isolate, Handle<WasmInstanceObject> instance,
+                       Handle<WasmTableObject> table_object,
+                       uint32_t table_index, uint32_t entry_index) {
+  const WasmModule* module = instance->module();
+  if (IsSubtypeOf(table_object->type(), kWasmFuncRef, module)) {
+    IndirectFunctionTableEntry(instance, table_index, entry_index).clear();
+  }
+  WasmTableObject::Set(isolate, table_object, entry_index,
+                       isolate->factory()->null_value());
+}
+
+void SetFunctionTableEntry(Isolate* isolate,
+                           Handle<WasmInstanceObject> instance,
+                           Handle<WasmTableObject> table_object,
+                           uint32_t table_index, uint32_t entry_index,
+                           uint32_t func_index) {
+  const WasmModule* module = instance->module();
+  const WasmFunction* function = &module->functions[func_index];
+
+  // For externref tables, we have to generate the WasmExternalFunction eagerly.
+  // Later we cannot know if an entry is a placeholder or not.
+  if (table_object->type().is_reference_to(HeapType::kExtern)) {
+    Handle<WasmExternalFunction> wasm_external_function =
+        WasmInstanceObject::GetOrCreateWasmExternalFunction(isolate, instance,
+                                                            func_index);
+    WasmTableObject::Set(isolate, table_object, entry_index,
+                         wasm_external_function);
+  } else {
+    DCHECK(IsSubtypeOf(table_object->type(), kWasmFuncRef, module));
+
+    // Update the local dispatch table first if necessary.
+    uint32_t sig_id = module->canonicalized_type_ids[function->sig_index];
+    IndirectFunctionTableEntry(instance, table_index, entry_index)
+        .Set(sig_id, instance, func_index);
+
+    // Update the table object's other dispatch tables.
+    MaybeHandle<WasmExternalFunction> wasm_external_function =
+        WasmInstanceObject::GetWasmExternalFunction(isolate, instance,
+                                                    func_index);
+    if (wasm_external_function.is_null()) {
+      // No JSFunction entry yet exists for this function. Create a
+      // {Tuple2} holding the information to lazily allocate one.
+      WasmTableObject::SetFunctionTablePlaceholder(
+          isolate, table_object, entry_index, instance, func_index);
+    } else {
+      table_object->entries().set(entry_index,
+                                  *wasm_external_function.ToHandleChecked());
+    }
+    // UpdateDispatchTables() updates all other dispatch tables, since
+    // we have not yet added the dispatch table we are currently building.
+    WasmTableObject::UpdateDispatchTables(isolate, table_object, entry_index,
+                                          function->sig, instance, func_index);
+  }
+}
+
 void InstanceBuilder::InitializeIndirectFunctionTables(
     Handle<WasmInstanceObject> instance) {
   for (int table_index = 0;
@@ -1782,38 +1838,35 @@ void InstanceBuilder::InitializeIndirectFunctionTables(
     }
 
     if (!table.type.is_defaultable()) {
-      WasmValue value =
-          EvaluateInitExpression(table.initial_value, table.type, instance);
-      DCHECK(WasmExportedFunction::IsWasmExportedFunction(*value.to_ref()));
-      auto wasm_external_function =
-          Handle<WasmExportedFunction>::cast(value.to_ref());
-      uint32_t func_index = wasm_external_function->function_index();
-
-      uint32_t sig_id =
-          module_->canonicalized_type_ids[module_->functions[func_index]
-                                              .sig_index];
       auto table_object = handle(
           WasmTableObject::cast(instance->tables().get(table_index)), isolate_);
+      Handle<Object> value =
+          EvaluateInitExpression(table.initial_value, table.type, instance)
+              .to_ref();
+      if (value.is_null()) {
         for (uint32_t entry_index = 0; entry_index < table.initial_size;
              entry_index++) {
-        // Update the local dispatch table first.
-        IndirectFunctionTableEntry(instance, table_index, entry_index)
-            .Set(sig_id, instance, func_index);
-
-        // Update the table object's other dispatch tables.
-        if (wasm_external_function.is_null()) {
-          // No JSFunction entry yet exists for this function. Create a {Tuple2}
-          // holding the information to lazily allocate one.
-          WasmTableObject::SetFunctionTablePlaceholder(
-              isolate_, table_object, entry_index, instance, func_index);
+          SetNullTableEntry(isolate_, instance, table_object, table_index,
+                            entry_index);
+        }
+      } else if (WasmExportedFunction::IsWasmExportedFunction(*value)) {
+        uint32_t function_index =
+            Handle<WasmExportedFunction>::cast(value)->function_index();
+        for (uint32_t entry_index = 0; entry_index < table.initial_size;
+             entry_index++) {
+          SetFunctionTableEntry(isolate_, instance, table_object, table_index,
+                                entry_index, function_index);
+        }
+      } else if (WasmJSFunction::IsWasmJSFunction(*value)) {
+        // TODO(manoskouk): Support WasmJSFunction.
+        thrower_->TypeError(
+            "Initializing a table with a Webassembly.Function object is not "
+            "supported yet");
       } else {
-          table_object->entries().set(entry_index, *wasm_external_function);
+        for (uint32_t entry_index = 0; entry_index < table.initial_size;
+             entry_index++) {
+          WasmTableObject::Set(isolate_, table_object, entry_index, value);
         }
-        // UpdateDispatchTables() updates all other dispatch tables, since
-        // we have not yet added the dispatch table we are currently building.
-        WasmTableObject::UpdateDispatchTables(
-            isolate_, table_object, entry_index,
-            module_->functions[func_index].sig, instance, func_index);
       }
     }
   }
@@ -1837,68 +1890,39 @@ bool LoadElemSegmentImpl(Isolate* isolate, Handle<WasmInstanceObject> instance,
     return false;
   }
 
-  const WasmModule* module = instance->module();
   for (size_t i = 0; i < count; ++i) {
-    const WasmInitExpr* init = &elem_segment.entries[src + i];
+    WasmElemSegment::Entry init = elem_segment.entries[src + i];
     int entry_index = static_cast<int>(dst + i);
-
-    if (init->kind() == WasmInitExpr::kRefNullConst) {
-      if (IsSubtypeOf(table_object->type(), kWasmFuncRef, module)) {
-        IndirectFunctionTableEntry(instance, table_index, entry_index).clear();
-      }
-      WasmTableObject::Set(isolate, table_object, entry_index,
-                           isolate->factory()->null_value());
-      continue;
-    }
-
-    if (init->kind() == WasmInitExpr::kGlobalGet) {
-      WasmTableObject::Set(
-          isolate, table_object, entry_index,
+    switch (init.kind) {
+      case WasmElemSegment::Entry::kRefNullEntry:
+        SetNullTableEntry(isolate, instance, table_object, table_index,
+                          entry_index);
+        break;
+      case WasmElemSegment::Entry::kRefFuncEntry:
+        SetFunctionTableEntry(isolate, instance, table_object, table_index,
+                              entry_index, init.index);
+        break;
+      case WasmElemSegment::Entry::kGlobalGetEntry: {
+        Handle<Object> value =
             WasmInstanceObject::GetGlobalValue(
-              instance, module->globals[init->immediate().index])
-              .to_ref());
-      continue;
-    }
-
-    DCHECK_EQ(init->kind(), WasmInitExpr::kRefFuncConst);
-
-    const uint32_t func_index = init->immediate().index;
-    const WasmFunction* function = &module->functions[func_index];
-
-    // Update the local dispatch table first if necessary.
-    if (IsSubtypeOf(table_object->type(), kWasmFuncRef, module)) {
-      uint32_t sig_id = module->canonicalized_type_ids[function->sig_index];
-      IndirectFunctionTableEntry(instance, table_index, entry_index)
-          .Set(sig_id, instance, func_index);
-    }
-
-    // For ExternRef tables, we have to generate the WasmExternalFunction
-    // eagerly. Later we cannot know if an entry is a placeholder or not.
-    if (table_object->type().is_reference_to(HeapType::kExtern)) {
-      Handle<WasmExternalFunction> wasm_external_function =
-          WasmInstanceObject::GetOrCreateWasmExternalFunction(isolate, instance,
-                                                              func_index);
-      WasmTableObject::Set(isolate, table_object, entry_index,
-                           wasm_external_function);
-    } else {
-      // Update the table object's other dispatch tables.
-      MaybeHandle<WasmExternalFunction> wasm_external_function =
-          WasmInstanceObject::GetWasmExternalFunction(isolate, instance,
-                                                      func_index);
-      if (wasm_external_function.is_null()) {
-        // No JSFunction entry yet exists for this function. Create a {Tuple2}
-        // holding the information to lazily allocate one.
-        WasmTableObject::SetFunctionTablePlaceholder(
-            isolate, table_object, entry_index, instance, func_index);
+                instance, instance->module()->globals[init.index])
+                .to_ref();
+        if (value.is_null()) {
+          SetNullTableEntry(isolate, instance, table_object, table_index,
+                            entry_index);
+        } else if (WasmExportedFunction::IsWasmExportedFunction(*value)) {
+          uint32_t function_index =
+              Handle<WasmExportedFunction>::cast(value)->function_index();
+          SetFunctionTableEntry(isolate, instance, table_object, table_index,
+                                entry_index, function_index);
+        } else if (WasmJSFunction::IsWasmJSFunction(*value)) {
+          // TODO(manoskouk): Support WasmJSFunction.
+          return false;
         } else {
-        table_object->entries().set(entry_index,
-                                    *wasm_external_function.ToHandleChecked());
+          WasmTableObject::Set(isolate, table_object, entry_index, value);
+        }
+        break;
       }
-      // UpdateDispatchTables() updates all other dispatch tables, since
-      // we have not yet added the dispatch table we are currently building.
-      WasmTableObject::UpdateDispatchTables(isolate, table_object, entry_index,
-                                            function->sig, instance,
-                                            func_index);
     }
   }
   return true;

src/wasm/wasm-module.h

@@ -135,7 +135,13 @@ struct WasmElemSegment {
   ValueType type;
   uint32_t table_index;
   WireBytesRef offset;
-  std::vector<WasmInitExpr> entries;
+  struct Entry {
+    enum Kind { kGlobalGetEntry, kRefFuncEntry, kRefNullEntry } kind;
+    uint32_t index;
+    Entry(Kind kind, uint32_t index) : kind(kind), index(index) {}
+    Entry() : kind(kRefNullEntry), index(0) {}
+  };
+  std::vector<Entry> entries;
   enum Status {
     kStatusActive,      // copied automatically during instantiation.
     kStatusPassive,     // copied explicitly after instantiation.

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

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

test/fuzzer/wasm-fuzzer-common.cc

@@ -159,37 +159,17 @@ std::ostream& operator<<(std::ostream& os, const PrintName& name) {
   return os.write(name.name.begin(), name.name.size());
 }
 
-std::ostream& operator<<(std::ostream& os, const WasmInitExpr& expr) {
+std::ostream& operator<<(std::ostream& os, WasmElemSegment::Entry entry) {
   os << "WasmInitExpr.";
-  switch (expr.kind()) {
-    case WasmInitExpr::kNone:
-      UNREACHABLE();
-    case WasmInitExpr::kS128Const:
-    case WasmInitExpr::kRttCanon:
-    case WasmInitExpr::kRttSub:
-    case WasmInitExpr::kRttFreshSub:
-    case WasmInitExpr::kRefNullConst:
-    case WasmInitExpr::kStructNewWithRtt:
-    case WasmInitExpr::kArrayInit:
-      // TODO(manoskouk): Implement these.
-      UNIMPLEMENTED();
-    case WasmInitExpr::kGlobalGet:
-      os << "GlobalGet(" << expr.immediate().index;
-      break;
-    case WasmInitExpr::kI32Const:
-      os << "I32Const(" << expr.immediate().i32_const;
-      break;
-    case WasmInitExpr::kI64Const:
-      os << "I64Const(" << expr.immediate().i64_const;
-      break;
-    case WasmInitExpr::kF32Const:
-      os << "F32Const(" << expr.immediate().f32_const;
+  switch (entry.kind) {
+    case WasmElemSegment::Entry::kGlobalGetEntry:
+      os << "GlobalGet(" << entry.index;
       break;
-    case WasmInitExpr::kF64Const:
-      os << "F64Const(" << expr.immediate().f64_const;
+    case WasmElemSegment::Entry::kRefFuncEntry:
+      os << "RefFunc(" << entry.index;
       break;
-    case WasmInitExpr::kRefFuncConst:
-      os << "RefFunc(" << expr.immediate().index;
+    case WasmElemSegment::Entry::kRefNullEntry:
+      os << "RefNull(" << HeapType(entry.index).name().c_str();
       break;
   }
   return os << ")";

test/unittests/wasm/function-body-decoder-unittest.cc

@@ -161,16 +161,16 @@ class TestModuleBuilder {
     auto& init = mod.elem_segments.back();
     // Add 5 empty elements.
     for (uint32_t j = 0; j < 5; j++) {
-      init.entries.push_back(
-          WasmInitExpr::RefNullConst(type.heap_representation()));
+      init.entries.push_back(WasmElemSegment::Entry(
+          WasmElemSegment::Entry::kRefNullEntry, type.heap_representation()));
     }
     return static_cast<byte>(mod.elem_segments.size() - 1);
   }
 
   byte AddDeclarativeElementSegment() {
     mod.elem_segments.emplace_back(kWasmFuncRef, true);
-    mod.elem_segments.back().entries.push_back(
-        WasmInitExpr::RefNullConst(HeapType::kFunc));
+    mod.elem_segments.back().entries.push_back(WasmElemSegment::Entry(
+        WasmElemSegment::Entry::kRefNullEntry, HeapType::kFunc));
     return static_cast<byte>(mod.elem_segments.size() - 1);
   }