[wasm][bulk-memory] Adjust throw behavior to match new proposal

InstanceBuilder::LoadTableSegments - Throw RuntimeError instead of
  LinkError
WasmGraphBuilder::TableInit & WasmGraphBuilder::MemoryInit - Do not
  check for active/dropped status if size == 0
WasmGraphBuilder::MemoryFill - Throw out-of-bounds error BEFORE
  attempting any memory operations if necessary

R=ahaas@chromium.org

Bug: v8:9865
Change-Id: I6a67779dc99fdc1c6bda6a2526d0e9ee5385f3ed
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1924442Reviewed-by: Andreas Haas <ahaas@chromium.org>
Commit-Queue: Emanuel Ziegler <ecmziegler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65098}

src/compiler/wasm-compiler.cc

@@ -4835,7 +4835,6 @@ Node* WasmGraphBuilder::CheckDataSegmentIsPassiveAndNotDropped(
 Node* WasmGraphBuilder::MemoryInit(uint32_t data_segment_index, Node* dst,
                                    Node* src, Node* size,
                                    wasm::WasmCodePosition position) {
-  CheckDataSegmentIsPassiveAndNotDropped(data_segment_index, position);
   auto m = mcgraph()->machine();
   auto common = mcgraph()->common();
   Node* size_null_check =
@@ -4847,6 +4846,7 @@ Node* WasmGraphBuilder::MemoryInit(uint32_t data_segment_index, Node* dst,
   Node* size_null_if_false =
       graph()->NewNode(common->IfFalse(), size_null_branch);
   SetControl(size_null_if_false);
+  CheckDataSegmentIsPassiveAndNotDropped(data_segment_index, position);
 
   Node* dst_fail = BoundsCheckMemRange(&dst, &size, position);
   TrapIfTrue(wasm::kTrapMemOutOfBounds, dst_fail, position);
@@ -4965,13 +4965,13 @@ Node* WasmGraphBuilder::MemoryFill(Node* dst, Node* value, Node* size,
       graph()->NewNode(common->IfFalse(), size_null_branch);
   SetControl(size_null_if_false);
   Node* fail = BoundsCheckMemRange(&dst, &size, position);
+  TrapIfTrue(wasm::kTrapMemOutOfBounds, fail, position);
   Node* function = graph()->NewNode(mcgraph()->common()->ExternalConstant(
       ExternalReference::wasm_memory_fill()));
   MachineType sig_types[] = {MachineType::Pointer(), MachineType::Uint32(),
                              MachineType::Uint32()};
   MachineSignature sig(0, 3, sig_types);
   BuildCCall(&sig, function, dst, value, size);
-  TrapIfTrue(wasm::kTrapMemOutOfBounds, fail, position);
   Node* size_null_if_true =
       graph()->NewNode(common->IfTrue(), size_null_branch);
 
@@ -5001,6 +5001,18 @@ Node* WasmGraphBuilder::TableInit(uint32_t table_index,
                                   uint32_t elem_segment_index, Node* dst,
                                   Node* src, Node* size,
                                   wasm::WasmCodePosition position) {
+  auto machine = mcgraph()->machine();
+  auto common = mcgraph()->common();
+  // If size == 0, then table.init is a no-op.
+  Node* size_zero_check = graph()->NewNode(machine->Word32Equal(), size,
+                                           mcgraph()->Int32Constant(0));
+  Node* size_zero_branch = graph()->NewNode(common->Branch(BranchHint::kFalse),
+                                            size_zero_check, Control());
+
+  Node* size_zero_etrue = Effect();
+  Node* size_zero_if_false =
+      graph()->NewNode(common->IfFalse(), size_zero_branch);
+  SetControl(size_zero_if_false);
   CheckElemSegmentIsPassiveAndNotDropped(elem_segment_index, position);
   Node* args[] = {
       graph()->NewNode(mcgraph()->common()->NumberConstant(table_index)),
@@ -5008,10 +5020,15 @@ Node* WasmGraphBuilder::TableInit(uint32_t table_index,
       BuildConvertUint32ToSmiWithSaturation(dst, FLAG_wasm_max_table_size),
       BuildConvertUint32ToSmiWithSaturation(src, FLAG_wasm_max_table_size),
       BuildConvertUint32ToSmiWithSaturation(size, FLAG_wasm_max_table_size)};
-  Node* result =
-      BuildCallToRuntime(Runtime::kWasmTableInit, args, arraysize(args));
+  BuildCallToRuntime(Runtime::kWasmTableInit, args, arraysize(args));
+  Node* size_zero_if_true =
+      graph()->NewNode(common->IfTrue(), size_zero_branch);
 
-  return result;
+  Node* merge = SetControl(
+      graph()->NewNode(common->Merge(2), size_zero_if_true, Control()));
+  SetEffect(
+      graph()->NewNode(common->EffectPhi(2), size_zero_etrue, Effect(), merge));
+  return merge;
 }
 
 Node* WasmGraphBuilder::ElemDrop(uint32_t elem_segment_index,

src/wasm/module-instantiate.cc

@@ -671,7 +671,7 @@ void InstanceBuilder::LoadDataSegments(Handle<WasmInstanceObject> instance) {
                          segment.source.offset();
       memory_copy_wrapper(dest_addr, src_addr, size);
       if (!ok) {
-        thrower_->LinkError("data segment is out of bounds");
+        thrower_->RuntimeError("data segment is out of bounds");
         return;
       }
     } else {
@@ -1713,7 +1713,7 @@ void InstanceBuilder::LoadTableSegments(Handle<WasmInstanceObject> instance) {
         table_index, elem_segment, dst, src, count);
     if (enabled_.bulk_memory) {
       if (!success) {
-        thrower_->LinkError("table initializer is out of bounds");
+        thrower_->RuntimeError("table initializer is out of bounds");
         // Break out instead of returning; we don't want to continue to
         // initialize any further element segments, but still need to add
         // dispatch tables below.

src/wasm/wasm-interpreter.cc

@@ -1792,16 +1792,16 @@ class ThreadImpl {
         MemoryInitImmediate<Decoder::kNoValidate> imm(decoder, code->at(pc));
         DCHECK_LT(imm.data_segment_index, module()->num_declared_data_segments);
         *len += imm.length;
-        if (!CheckDataSegmentIsPassiveAndNotDropped(imm.data_segment_index,
-                                                    pc)) {
-          return false;
-        }
         auto size = Pop().to<uint32_t>();
         auto src = Pop().to<uint32_t>();
         auto dst = Pop().to<uint32_t>();
         if (size == 0) {
           return true;
         }
+        if (!CheckDataSegmentIsPassiveAndNotDropped(imm.data_segment_index,
+                                                    pc)) {
+          return false;
+        }
         Address dst_addr;
         auto src_max =
             instance_object_->data_segment_sizes()[imm.data_segment_index];
@@ -1857,20 +1857,26 @@ class ThreadImpl {
         }
         Address dst_addr;
         bool ok = BoundsCheckMemRange(dst, &size, &dst_addr);
+        if (!ok) {
+          DoTrap(kTrapMemOutOfBounds, pc);
+          return false;
+        }
         memory_fill_wrapper(dst_addr, value, size);
-        if (!ok) DoTrap(kTrapMemOutOfBounds, pc);
-        return ok;
+        return true;
       }
       case kExprTableInit: {
         TableInitImmediate<Decoder::kNoValidate> imm(decoder, code->at(pc));
         *len += imm.length;
+        auto size = Pop().to<uint32_t>();
+        auto src = Pop().to<uint32_t>();
+        auto dst = Pop().to<uint32_t>();
+        if (size == 0) {
+          return true;
+        }
         if (!CheckElemSegmentIsPassiveAndNotDropped(imm.elem_segment_index,
                                                     pc)) {
           return false;
         }
-        auto size = Pop().to<uint32_t>();
-        auto src = Pop().to<uint32_t>();
-        auto dst = Pop().to<uint32_t>();
         HandleScope scope(isolate_);  // Avoid leaking handles.
         bool ok = WasmInstanceObject::InitTableEntries(
             instance_object_->GetIsolate(), instance_object_, imm.table.index,

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

@@ -295,7 +295,7 @@ WASM_EXEC_TEST(MemoryFillOutOfBoundsData) {
       kExprI32Const, 0);
   const byte v = 123;
   CHECK_EQ(0xDEADBEEF, r.Call(kWasmPageSize - 5, v, 999));
-  CheckMemoryEquals(&r.builder(), kWasmPageSize - 6, {0, v, v, v, v, v});
+  CheckMemoryEquals(&r.builder(), kWasmPageSize - 6, {0, 0, 0, 0, 0, 0});
 }
 
 WASM_EXEC_TEST(MemoryFillOutOfBounds) {
@@ -870,14 +870,16 @@ WASM_EXEC_TEST(ElemDropTwice) {
 
 WASM_EXEC_TEST(ElemDropThenTableInit) {
   EXPERIMENTAL_FLAG_SCOPE(bulk_memory);
-  WasmRunner<uint32_t> r(execution_tier);
+  WasmRunner<uint32_t, uint32_t> r(execution_tier);
   r.builder().AddIndirectFunctionTable(nullptr, 1);
   r.builder().AddPassiveElementSegment({});
-  BUILD(r, WASM_ELEM_DROP(0),
-        WASM_TABLE_INIT(0, 0, WASM_I32V_1(0), WASM_I32V_1(0), WASM_I32V_1(0)),
-        kExprI32Const, 0);
+  BUILD(
+      r, WASM_ELEM_DROP(0),
+      WASM_TABLE_INIT(0, 0, WASM_I32V_1(0), WASM_I32V_1(0), WASM_GET_LOCAL(0)),
+      kExprI32Const, 0);
 
-  r.CheckCallViaJS(0xDEADBEEF);
+  r.CheckCallViaJS(0, 0);
+  r.CheckCallViaJS(0xDEADBEEF, 1);
 }
 
 }  // namespace test_run_wasm_bulk_memory

test/mjsunit/wasm/bulk-memory.js

@@ -64,11 +64,11 @@ function getMemoryInit(mem, segment_data) {
   builder.addMemory(1);
   builder.addPassiveDataSegment([1, 2, 3]);
   builder.addDataSegment(0, [4, 5, 6]);
-  builder.addFunction('init', kSig_v_v)
+  builder.addFunction('init', kSig_v_i)
       .addBody([
         kExprI32Const, 0,  // Dest.
         kExprI32Const, 0,  // Source.
-        kExprI32Const, 0,  // Size in bytes.
+        kExprLocalGet, 0,  // Size in bytes.
         kNumericPrefix, kExprMemoryInit,
         1,  // Data segment index.
         0,  // Memory index.
@@ -79,7 +79,11 @@ function getMemoryInit(mem, segment_data) {
   // is a trap, not a validation error.
   const instance = builder.instantiate();
 
-  assertTraps(kTrapDataSegmentDropped, () => instance.exports.init());
+  // Initialization succeeds, because the size is 0 which is always valid.
+  instance.exports.init(0);
+
+  // Initialization fails, because the size > 0 on dropped segment
+  assertTraps(kTrapDataSegmentDropped, () => instance.exports.init(1));
 })();
 
 (function TestDataDropOnActiveSegment() {
@@ -177,7 +181,7 @@ function getMemoryFill(mem) {
   assertEquals(0, view[kPageSize - 1]);
 
   // Instantiation fails, but still modifies memory.
-  assertThrows(() => builder.instantiate({m: {memory}}), WebAssembly.LinkError);
+  assertThrows(() => builder.instantiate({m: {memory}}), WebAssembly.RuntimeError);
 
   assertEquals(42, view[kPageSize - 1]);
   // The second segment is not initialized.
@@ -202,7 +206,7 @@ function getMemoryFill(mem) {
   // Instantiation fails, but still modifies the table. The memory is not
   // modified, since data segments are initialized after element segments.
   assertThrows(
-      () => builder.instantiate({m: {memory, table}}), WebAssembly.LinkError);
+      () => builder.instantiate({m: {memory, table}}), WebAssembly.RuntimeError);
 
   assertEquals(0, view[0]);
 })();

test/wasm-spec-tests/wasm-spec-tests.status

@@ -17,12 +17,6 @@
   'proposals/js-types/exports': [FAIL],
   'proposals/js-types/globals': [FAIL],
   'proposals/js-types/linking': [FAIL],
-  # TODO(v8:9865): Throwing behavior changed for these tests.
-  'proposals/bulk-memory-operations/linking': [FAIL],
-  'proposals/bulk-memory-operations/bulk': [FAIL],
-  'proposals/bulk-memory-operations/elem': [FAIL],
-  'proposals/bulk-memory-operations/data': [FAIL],
-
   # TODO(thibaudm): Spec tests do not check multi-return functions correctly.
   'proposals/multi-value/call': [FAIL],
   'proposals/multi-value/if': [FAIL],