From 0861b4b6582ea5c641316e6f301804f81f4741c6 Mon Sep 17 00:00:00 2001 From: Manos Koukoutos Date: Wed, 16 Nov 2022 05:33:44 +0100 Subject: [PATCH] [wasm-gc] Disallow array.new_{data, elem} as constant expressions Additionally: - Remove the early data-count section from module-decoder and wasm-module-builder.js. - Move a test from gc-nominal.js to array-init-from-segment.js. - Comment-out relevant tests. Bug: v8:7748 Change-Id: I5e038e0b6227c28ce79ffe39529ada59c34187eb Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4028144 Reviewed-by: Jakob Kummerow Commit-Queue: Manos Koukoutos Cr-Commit-Position: refs/heads/main@{#84301} --- src/wasm/constant-expression-interface.cc | 3 + src/wasm/function-body-decoder-impl.h | 2 + src/wasm/module-decoder-impl.h | 3 - test/mjsunit/wasm/array-init-from-segment.js | 64 ++++++++++++++++++++ test/mjsunit/wasm/gc-nominal.js | 59 ------------------ test/mjsunit/wasm/wasm-module-builder.js | 16 +---- 6 files changed, 70 insertions(+), 77 deletions(-) diff --git a/src/wasm/constant-expression-interface.cc b/src/wasm/constant-expression-interface.cc index d4f8153291..1deb23a822 100644 --- a/src/wasm/constant-expression-interface.cc +++ b/src/wasm/constant-expression-interface.cc @@ -236,6 +236,9 @@ void ConstantExpressionInterface::ArrayNewFixed( ValueType::Ref(HeapType(imm.index))); } +// TODO(7748): These expressions are non-constant for now. There are plans to +// make them constant in the future, so we retain the required infrastructure +// here. void ConstantExpressionInterface::ArrayNewSegment( FullDecoder* decoder, const ArrayIndexImmediate& array_imm, const IndexImmediate& segment_imm, const Value& offset_value, diff --git a/src/wasm/function-body-decoder-impl.h b/src/wasm/function-body-decoder-impl.h index ac015fc431..db08b2f7c6 100644 --- a/src/wasm/function-body-decoder-impl.h +++ b/src/wasm/function-body-decoder-impl.h @@ -4588,6 +4588,7 @@ class WasmFullDecoder : public WasmDecoder { return opcode_length + imm.length; } case kExprArrayNewData: { + NON_CONST_ONLY ArrayIndexImmediate array_imm(this, this->pc_ + opcode_length, validate); if (!this->Validate(this->pc_ + opcode_length, array_imm)) return 0; @@ -4630,6 +4631,7 @@ class WasmFullDecoder : public WasmDecoder { return opcode_length + array_imm.length + data_segment.length; } case kExprArrayNewElem: { + NON_CONST_ONLY ArrayIndexImmediate array_imm(this, this->pc_ + opcode_length, validate); if (!this->Validate(this->pc_ + opcode_length, array_imm)) return 0; diff --git a/src/wasm/module-decoder-impl.h b/src/wasm/module-decoder-impl.h index afa54fed62..023a929e15 100644 --- a/src/wasm/module-decoder-impl.h +++ b/src/wasm/module-decoder-impl.h @@ -447,9 +447,6 @@ class ModuleDecoderTemplate : public Decoder { // Now check the ordering constraints of specific unordered sections. switch (section_code) { case kDataCountSectionCode: - // If wasm-gc is enabled, we allow the data count section anywhere in - // the module. - if (enabled_features_.has_gc()) return true; return check_order(kElementSectionCode, kCodeSectionCode); case kTagSectionCode: return check_order(kMemorySectionCode, kGlobalSectionCode); diff --git a/test/mjsunit/wasm/array-init-from-segment.js b/test/mjsunit/wasm/array-init-from-segment.js index f98c86e8b8..b66445be4c 100644 --- a/test/mjsunit/wasm/array-init-from-segment.js +++ b/test/mjsunit/wasm/array-init-from-segment.js @@ -78,6 +78,8 @@ d8.file.execute("test/mjsunit/wasm/wasm-module-builder.js"); assertTraps(kTrapElementSegmentOutOfBounds, () => init_and_get_active(1, 0)); })(); +// TODO(7748): Reenable when we have constant array.new_elem. +/* (function TestArrayNewElemConstant() { print(arguments.callee.name); let builder = new WasmModuleBuilder(); @@ -158,6 +160,7 @@ d8.file.execute("test/mjsunit/wasm/wasm-module-builder.js"); assertTraps(kTrapNullDereference, () => table_get(0, 2)); assertTraps(kTrapArrayOutOfBounds, () => table_get(0, 3)); })(); +*/ (function TestArrayNewElemMistypedSegment() { print(arguments.callee.name); @@ -185,6 +188,8 @@ d8.file.execute("test/mjsunit/wasm/wasm-module-builder.js"); /segment type.*is not a subtype of array element type.*/); })(); +// TODO(7748): Reenable when we have constant array.new_elem. +/* // Element segments are defined after globals, so currently it is not valid // to refer to an element segment in the global section. (function TestArrayNewFixedFromElemInGlobal() { @@ -326,3 +331,62 @@ d8.file.execute("test/mjsunit/wasm/wasm-module-builder.js"); // An active segment counts as having 0 length. assertTraps(kTrapElementSegmentOutOfBounds, () => instance.exports.init()); })(); + +(function TestArrayNewData() { + print(arguments.callee.name); + let builder = new WasmModuleBuilder(); + let array_type_index = builder.addArray(kWasmI16, true); + + let dummy_byte = 0xff; + let element_0 = 1000; + let element_1 = -2222; + + let data_segment = builder.addPassiveDataSegment( + [dummy_byte, element_0 & 0xff, (element_0 >> 8) & 0xff, + element_1 & 0xff, (element_1 >> 8) & 0xff]); + + let global = builder.addGlobal( + wasmRefType(array_type_index), true, + [...wasmI32Const(1), ...wasmI32Const(2), + kGCPrefix, kExprArrayNewData, array_type_index, data_segment], + builder); + + builder.addFunction("global_get", kSig_i_i) + .addBody([ + kExprGlobalGet, global.index, + kExprLocalGet, 0, + kGCPrefix, kExprArrayGetS, array_type_index]) + .exportFunc(); + + // parameters: (segment offset, array length, array index) + builder.addFunction("init_from_data", kSig_i_iii) + .addBody([ + kExprLocalGet, 0, kExprLocalGet, 1, + kGCPrefix, kExprArrayNewData, + array_type_index, data_segment, + kExprLocalGet, 2, + kGCPrefix, kExprArrayGetS, array_type_index]) + .exportFunc(); + + builder.addFunction("drop_segment", kSig_v_v) + .addBody([kNumericPrefix, kExprDataDrop, data_segment]) + .exportFunc(); + + let instance = builder.instantiate(); + + assertEquals(element_0, instance.exports.global_get(0)); + assertEquals(element_1, instance.exports.global_get(1)); + + let init = instance.exports.init_from_data; + + assertEquals(element_0, init(1, 2, 0)); + assertEquals(element_1, init(1, 2, 1)); + + assertTraps(kTrapArrayTooLarge, () => init(1, 1000000000, 0)); + assertTraps(kTrapDataSegmentOutOfBounds, () => init(2, 2, 0)); + + instance.exports.drop_segment(); + + assertTraps(kTrapDataSegmentOutOfBounds, () => init(1, 2, 0)); +})(); +*/ diff --git a/test/mjsunit/wasm/gc-nominal.js b/test/mjsunit/wasm/gc-nominal.js index 5a43d5230f..367d2044d1 100644 --- a/test/mjsunit/wasm/gc-nominal.js +++ b/test/mjsunit/wasm/gc-nominal.js @@ -44,62 +44,3 @@ d8.file.execute("test/mjsunit/wasm/wasm-module-builder.js"); () => builder.instantiate(), WebAssembly.CompileError, /subtyping depth is greater than allowed/); })(); - -(function TestArrayNewData() { - print(arguments.callee.name); - let builder = new WasmModuleBuilder(); - builder.setEarlyDataCountSection(); - let array_type_index = builder.addArray(kWasmI16, true); - - let dummy_byte = 0xff; - let element_0 = 1000; - let element_1 = -2222; - - let data_segment = builder.addPassiveDataSegment( - [dummy_byte, element_0 & 0xff, (element_0 >> 8) & 0xff, - element_1 & 0xff, (element_1 >> 8) & 0xff]); - - let global = builder.addGlobal( - wasmRefType(array_type_index), true, - [...wasmI32Const(1), ...wasmI32Const(2), - kGCPrefix, kExprArrayNewData, array_type_index, data_segment], - builder); - - builder.addFunction("global_get", kSig_i_i) - .addBody([ - kExprGlobalGet, global.index, - kExprLocalGet, 0, - kGCPrefix, kExprArrayGetS, array_type_index]) - .exportFunc(); - - // parameters: (segment offset, array length, array index) - builder.addFunction("init_from_data", kSig_i_iii) - .addBody([ - kExprLocalGet, 0, kExprLocalGet, 1, - kGCPrefix, kExprArrayNewData, - array_type_index, data_segment, - kExprLocalGet, 2, - kGCPrefix, kExprArrayGetS, array_type_index]) - .exportFunc(); - - builder.addFunction("drop_segment", kSig_v_v) - .addBody([kNumericPrefix, kExprDataDrop, data_segment]) - .exportFunc(); - - let instance = builder.instantiate(); - - assertEquals(element_0, instance.exports.global_get(0)); - assertEquals(element_1, instance.exports.global_get(1)); - - let init = instance.exports.init_from_data; - - assertEquals(element_0, init(1, 2, 0)); - assertEquals(element_1, init(1, 2, 1)); - - assertTraps(kTrapArrayTooLarge, () => init(1, 1000000000, 0)); - assertTraps(kTrapDataSegmentOutOfBounds, () => init(2, 2, 0)); - - instance.exports.drop_segment(); - - assertTraps(kTrapDataSegmentOutOfBounds, () => init(1, 2, 0)); -})(); diff --git a/test/mjsunit/wasm/wasm-module-builder.js b/test/mjsunit/wasm/wasm-module-builder.js index 9670f72031..a121372ef4 100644 --- a/test/mjsunit/wasm/wasm-module-builder.js +++ b/test/mjsunit/wasm/wasm-module-builder.js @@ -1282,7 +1282,6 @@ class WasmModuleBuilder { // a separate rec. group instead. // TODO(7748): Support more flexible rec. groups. this.singleton_rec_groups = false; - this.early_data_count_section = false; return this; } @@ -1613,10 +1612,6 @@ class WasmModuleBuilder { this.singleton_rec_groups = true; } - setEarlyDataCountSection() { - this.early_data_count_section = true; - } - setName(name) { this.name = name; return this; @@ -1727,14 +1722,6 @@ class WasmModuleBuilder { }); } - // If there are any passive data segments, add the DataCount section. - if (this.early_data_count_section && - wasm.data_segments.some(seg => !seg.is_active)) { - binary.emit_section(kDataCountSectionCode, section => { - section.emit_u32v(wasm.data_segments.length); - }); - } - // Add table section if (wasm.tables.length > 0) { if (debug) print('emitting tables @ ' + binary.length); @@ -1900,8 +1887,7 @@ class WasmModuleBuilder { } // If there are any passive data segments, add the DataCount section. - if (!this.early_data_count_section && - wasm.data_segments.some(seg => !seg.is_active)) { + if (wasm.data_segments.some(seg => !seg.is_active)) { binary.emit_section(kDataCountSectionCode, section => { section.emit_u32v(wasm.data_segments.length); });