Skip to content

Commit 48ccb2b

Browse files
aheejinaheejin
authored
Implement 0-len/drop spec changes in bulk memory (#2529)
This implements recent bulk memory spec changes (WebAssembly/bulk-memory-operations#126) in Binaryen. Now `data.drop` is equivalent to shrinking a segment size to 0, and dropping already dropped segments or active segments (which are thought to be dropped in the beginning) is treated as a no-op. And all bounds checking is performed in advance, so partial copying/filling/initializing does not occur. I tried to implement `visitDataDrop` in the interpreter as `segment.data.clear();`, which is exactly what the revised spec says. I didn't end up doing that because this also deletes all contents from active segments, and there are cases we shouldn't do that: - `wasm-ctor-eval` shouldn't delete active segments, because it will store the changed contents back into segments - When `--fuzz-exec` is given to `wasm-opt`, it runs the module and compare the execution call results before and after transformations. But if running a module will nullify all active segments, applying any transformation to the module or re-running it does not make any sense.
1 parent 2a972a9 commit 48ccb2b

5 files changed

Lines changed: 47 additions & 48 deletions

File tree

‎src/passes/MemoryPacking.cpp‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -142,12 +142,6 @@ struct MemoryPacking : public Pass {
142142
changed = true;
143143
}
144144
}
145-
void visitDataDrop(DataDrop* curr) {
146-
if (!getModule()->memory.segments[curr->segment].isPassive) {
147-
ExpressionManipulator::unreachable(curr);
148-
changed = true;
149-
}
150-
}
151145
void doWalkFunction(Function* func) {
152146
changed = false;
153147
super::doWalkFunction(func);

‎src/wasm-interpreter.h‎

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1901,18 +1901,22 @@ template<typename GlobalManager, typename SubType> class ModuleInstanceBase {
19011901
assert(curr->segment < instance.wasm.memory.segments.size());
19021902
Memory::Segment& segment = instance.wasm.memory.segments[curr->segment];
19031903

1904-
if (instance.droppedSegments.count(curr->segment)) {
1905-
trap("memory.init of dropped segment");
1906-
}
1907-
19081904
Address destVal(uint32_t(dest.value.geti32()));
19091905
Address offsetVal(uint32_t(offset.value.geti32()));
19101906
Address sizeVal(uint32_t(size.value.geti32()));
19111907

1908+
if (offsetVal + sizeVal > 0 &&
1909+
instance.droppedSegments.count(curr->segment)) {
1910+
trap("out of bounds segment access in memory.init");
1911+
}
1912+
if ((uint64_t)offsetVal + sizeVal > segment.data.size()) {
1913+
trap("out of bounds segment access in memory.init");
1914+
}
1915+
if ((uint64_t)destVal + sizeVal >
1916+
(uint64_t)instance.memorySize * Memory::kPageSize) {
1917+
trap("out of bounds memory access in memory.init");
1918+
}
19121919
for (size_t i = 0; i < sizeVal; ++i) {
1913-
if (offsetVal + i >= segment.data.size()) {
1914-
trap("out of bounds segment access in memory.init");
1915-
}
19161920
Literal addr(uint32_t(destVal + i));
19171921
instance.externalInterface->store8(instance.getFinalAddress(addr, 1),
19181922
segment.data[offsetVal + i]);
@@ -1921,9 +1925,6 @@ template<typename GlobalManager, typename SubType> class ModuleInstanceBase {
19211925
}
19221926
Flow visitDataDrop(DataDrop* curr) {
19231927
NOTE_ENTER("DataDrop");
1924-
if (instance.droppedSegments.count(curr->segment)) {
1925-
trap("data.drop of dropped segment");
1926-
}
19271928
instance.droppedSegments.insert(curr->segment);
19281929
return {};
19291930
}
@@ -1948,6 +1949,13 @@ template<typename GlobalManager, typename SubType> class ModuleInstanceBase {
19481949
Address sourceVal(uint32_t(source.value.geti32()));
19491950
Address sizeVal(uint32_t(size.value.geti32()));
19501951

1952+
if ((uint64_t)sourceVal + sizeVal >
1953+
(uint64_t)instance.memorySize * Memory::kPageSize ||
1954+
(uint64_t)destVal + sizeVal >
1955+
(uint64_t)instance.memorySize * Memory::kPageSize) {
1956+
trap("out of bounds segment access in memory.copy");
1957+
}
1958+
19511959
int64_t start = 0;
19521960
int64_t end = sizeVal;
19531961
int step = 1;
@@ -1958,9 +1966,6 @@ template<typename GlobalManager, typename SubType> class ModuleInstanceBase {
19581966
step = -1;
19591967
}
19601968
for (int64_t i = start; i != end; i += step) {
1961-
if (i + destVal >= std::numeric_limits<uint32_t>::max()) {
1962-
trap("Out of bounds memory access");
1963-
}
19641969
instance.externalInterface->store8(
19651970
instance.getFinalAddress(Literal(uint32_t(destVal + i)), 1),
19661971
instance.externalInterface->load8s(
@@ -1988,6 +1993,10 @@ template<typename GlobalManager, typename SubType> class ModuleInstanceBase {
19881993
Address destVal(uint32_t(dest.value.geti32()));
19891994
Address sizeVal(uint32_t(size.value.geti32()));
19901995

1996+
if ((uint64_t)destVal + sizeVal >
1997+
(uint64_t)instance.memorySize * Memory::kPageSize) {
1998+
trap("out of bounds memory access in memory.fill");
1999+
}
19912000
uint8_t val(value.value.geti32());
19922001
for (size_t i = 0; i < sizeVal; ++i) {
19932002
instance.externalInterface->store8(

‎test/passes/memory-packing_all-features.txt‎

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -22,24 +22,20 @@
2222
(type $none_=>_none (func))
2323
(memory $0 1 1)
2424
(func $foo (; 0 ;)
25-
(block
26-
(drop
27-
(i32.const 0)
28-
)
29-
(drop
30-
(i32.const 0)
31-
)
32-
(drop
33-
(i32.const 0)
34-
)
35-
(unreachable)
25+
(drop
26+
(i32.const 0)
27+
)
28+
(drop
29+
(i32.const 0)
30+
)
31+
(drop
32+
(i32.const 0)
3633
)
3734
(unreachable)
3835
)
3936
(func $bar (; 1 ;)
4037
(drop
4138
(loop $loop-in (result i32)
42-
(unreachable)
4339
(i32.const 42)
4440
)
4541
)

‎test/passes/memory-packing_all-features.wast‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,10 @@
2626
(i32.const 0)
2727
(i32.const 0)
2828
)
29-
(data.drop 0)
3029
)
3130
(func $bar
3231
(drop
3332
(loop (result i32)
34-
(data.drop 0)
3533
(i32.const 42)
3634
)
3735
)

‎test/spec/bulk-memory.wast‎

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,8 @@
3636
;; Succeed when writing 0 bytes at the end of the region.
3737
(invoke "fill" (i32.const 0x10000) (i32.const 0) (i32.const 0))
3838

39-
;; OK to write 0 bytes outside of memory.
40-
(invoke "fill" (i32.const 0x10001) (i32.const 0) (i32.const 0))
39+
;; Writing 0 bytes outside of memory limit is NOT allowed.
40+
(assert_trap (invoke "fill" (i32.const 0x10001) (i32.const 0) (i32.const 0)))
4141

4242
;; memory.copy
4343
(module
@@ -101,9 +101,9 @@
101101
(invoke "copy" (i32.const 0x10000) (i32.const 0) (i32.const 0))
102102
(invoke "copy" (i32.const 0) (i32.const 0x10000) (i32.const 0))
103103

104-
;; OK copying 0 bytes outside of memory.
105-
(invoke "copy" (i32.const 0x10001) (i32.const 0) (i32.const 0))
106-
(invoke "copy" (i32.const 0) (i32.const 0x10001) (i32.const 0))
104+
;; Copying 0 bytes outside of memory limit is NOT allowed.
105+
(assert_trap (invoke "copy" (i32.const 0x10001) (i32.const 0) (i32.const 0)))
106+
(assert_trap (invoke "copy" (i32.const 0) (i32.const 0x10001) (i32.const 0)))
107107

108108
;; memory.init
109109
(module
@@ -128,19 +128,22 @@
128128
;; Init ending at memory limit and segment limit is ok.
129129
(invoke "init" (i32.const 0xfffc) (i32.const 0) (i32.const 4))
130130

131-
;; Out-of-bounds writes trap, but all previous writes succeed.
131+
;; Out-of-bounds writes trap, and no partial writes has been made.
132132
(assert_trap (invoke "init" (i32.const 0xfffe) (i32.const 0) (i32.const 3))
133133
"out of bounds memory access")
134-
(assert_return (invoke "load8_u" (i32.const 0xfffe)) (i32.const 0xaa))
135-
(assert_return (invoke "load8_u" (i32.const 0xffff)) (i32.const 0xbb))
134+
(assert_return (invoke "load8_u" (i32.const 0xfffe)) (i32.const 0xcc))
135+
(assert_return (invoke "load8_u" (i32.const 0xffff)) (i32.const 0xdd))
136136

137137
;; Succeed when writing 0 bytes at the end of either region.
138138
(invoke "init" (i32.const 0x10000) (i32.const 0) (i32.const 0))
139139
(invoke "init" (i32.const 0) (i32.const 4) (i32.const 0))
140140

141-
;; OK writing 0 bytes outside of memory or segment.
142-
(invoke "init" (i32.const 0x10001) (i32.const 0) (i32.const 0))
143-
(invoke "init" (i32.const 0) (i32.const 5) (i32.const 0))
141+
;; Writing 0 bytes outside of memory / segment limit is NOT allowed.
142+
(assert_trap (invoke "init" (i32.const 0x10001) (i32.const 0) (i32.const 0)))
143+
(assert_trap (invoke "init" (i32.const 0) (i32.const 5) (i32.const 0)))
144+
145+
;; OK to access 0 bytes at offset 0 in a dropped segment.
146+
(invoke "init" (i32.const 0) (i32.const 0) (i32.const 0))
144147

145148
;; data.drop
146149
(module
@@ -157,9 +160,8 @@
157160
(memory.init 1 (i32.const 0) (i32.const 0) (i32.const 0)))
158161
)
159162

163+
;; OK to drop the same segment multiple times or drop an active segment.
160164
(invoke "init_passive")
161165
(invoke "drop_passive")
162-
(assert_trap (invoke "drop_passive") "data segment dropped")
163-
(assert_trap (invoke "init_passive") "data segment dropped")
164-
(assert_trap (invoke "drop_active") "data segment dropped")
165-
(assert_trap (invoke "init_active") "data segment dropped")
166+
(invoke "drop_passive")
167+
(invoke "drop_active")

0 commit comments

Comments
 (0)