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.

5 files changed, 47 insertions, 48 deletions

diff --git a/src/passes/MemoryPacking.cpp b/src/passes/MemoryPacking.cpp
index a831c0efc..d5a8a96dd 100644
--- a/src/passes/MemoryPacking.cpp
+++ b/src/passes/MemoryPacking.cpp
@@ -142,12 +142,6 @@ struct MemoryPacking : public Pass {
           changed = true;
         }
       }
-      void visitDataDrop(DataDrop* curr) {
-        if (!getModule()->memory.segments[curr->segment].isPassive) {
-          ExpressionManipulator::unreachable(curr);
-          changed = true;
-        }
-      }
       void doWalkFunction(Function* func) {
         changed = false;
         super::doWalkFunction(func);
diff --git a/src/wasm-interpreter.h b/src/wasm-interpreter.h
index caf8a9a3d..5f8296f0d 100644
--- a/src/wasm-interpreter.h
+++ b/src/wasm-interpreter.h
@@ -1901,18 +1901,22 @@ private:
       assert(curr->segment < instance.wasm.memory.segments.size());
       Memory::Segment& segment = instance.wasm.memory.segments[curr->segment];
 
-      if (instance.droppedSegments.count(curr->segment)) {
-        trap("memory.init of dropped segment");
-      }
-
       Address destVal(uint32_t(dest.value.geti32()));
       Address offsetVal(uint32_t(offset.value.geti32()));
       Address sizeVal(uint32_t(size.value.geti32()));
 
+      if (offsetVal + sizeVal > 0 &&
+          instance.droppedSegments.count(curr->segment)) {
+        trap("out of bounds segment access in memory.init");
+      }
+      if ((uint64_t)offsetVal + sizeVal > segment.data.size()) {
+        trap("out of bounds segment access in memory.init");
+      }
+      if ((uint64_t)destVal + sizeVal >
+          (uint64_t)instance.memorySize * Memory::kPageSize) {
+        trap("out of bounds memory access in memory.init");
+      }
       for (size_t i = 0; i < sizeVal; ++i) {
-        if (offsetVal + i >= segment.data.size()) {
-          trap("out of bounds segment access in memory.init");
-        }
         Literal addr(uint32_t(destVal + i));
         instance.externalInterface->store8(instance.getFinalAddress(addr, 1),
                                            segment.data[offsetVal + i]);
@@ -1921,9 +1925,6 @@ private:
     }
     Flow visitDataDrop(DataDrop* curr) {
       NOTE_ENTER("DataDrop");
-      if (instance.droppedSegments.count(curr->segment)) {
-        trap("data.drop of dropped segment");
-      }
       instance.droppedSegments.insert(curr->segment);
       return {};
     }
@@ -1948,6 +1949,13 @@ private:
       Address sourceVal(uint32_t(source.value.geti32()));
       Address sizeVal(uint32_t(size.value.geti32()));
 
+      if ((uint64_t)sourceVal + sizeVal >
+            (uint64_t)instance.memorySize * Memory::kPageSize ||
+          (uint64_t)destVal + sizeVal >
+            (uint64_t)instance.memorySize * Memory::kPageSize) {
+        trap("out of bounds segment access in memory.copy");
+      }
+
       int64_t start = 0;
       int64_t end = sizeVal;
       int step = 1;
@@ -1958,9 +1966,6 @@ private:
         step = -1;
       }
       for (int64_t i = start; i != end; i += step) {
-        if (i + destVal >= std::numeric_limits<uint32_t>::max()) {
-          trap("Out of bounds memory access");
-        }
         instance.externalInterface->store8(
           instance.getFinalAddress(Literal(uint32_t(destVal + i)), 1),
           instance.externalInterface->load8s(
@@ -1988,6 +1993,10 @@ private:
       Address destVal(uint32_t(dest.value.geti32()));
       Address sizeVal(uint32_t(size.value.geti32()));
 
+      if ((uint64_t)destVal + sizeVal >
+          (uint64_t)instance.memorySize * Memory::kPageSize) {
+        trap("out of bounds memory access in memory.fill");
+      }
       uint8_t val(value.value.geti32());
       for (size_t i = 0; i < sizeVal; ++i) {
         instance.externalInterface->store8(
diff --git a/test/passes/memory-packing_all-features.txt b/test/passes/memory-packing_all-features.txt
index 1666c23a1..4d5b40fcf 100644
--- a/test/passes/memory-packing_all-features.txt
+++ b/test/passes/memory-packing_all-features.txt
@@ -22,24 +22,20 @@
  (type $none_=>_none (func))
  (memory $0 1 1)
  (func $foo (; 0 ;)
-  (block
-   (drop
-    (i32.const 0)
-   )
-   (drop
-    (i32.const 0)
-   )
-   (drop
-    (i32.const 0)
-   )
-   (unreachable)
+  (drop
+   (i32.const 0)
+  )
+  (drop
+   (i32.const 0)
+  )
+  (drop
+   (i32.const 0)
   )
   (unreachable)
  )
  (func $bar (; 1 ;)
   (drop
    (loop $loop-in (result i32)
-    (unreachable)
     (i32.const 42)
    )
   )
diff --git a/test/passes/memory-packing_all-features.wast b/test/passes/memory-packing_all-features.wast
index e43bd0314..6cadf6b04 100644
--- a/test/passes/memory-packing_all-features.wast
+++ b/test/passes/memory-packing_all-features.wast
@@ -26,12 +26,10 @@
    (i32.const 0)
    (i32.const 0)
   )
-  (data.drop 0)
  )
  (func $bar
   (drop
    (loop (result i32)
-    (data.drop 0)
     (i32.const 42)
    )
   )
diff --git a/test/spec/bulk-memory.wast b/test/spec/bulk-memory.wast
index 1e04e02c3..99485e66b 100644
--- a/test/spec/bulk-memory.wast
+++ b/test/spec/bulk-memory.wast
@@ -36,8 +36,8 @@
 ;; Succeed when writing 0 bytes at the end of the region.
 (invoke "fill" (i32.const 0x10000) (i32.const 0) (i32.const 0))
 
-;; OK to write 0 bytes outside of memory.
-(invoke "fill" (i32.const 0x10001) (i32.const 0) (i32.const 0))
+;; Writing 0 bytes outside of memory limit is NOT allowed.
+(assert_trap (invoke "fill" (i32.const 0x10001) (i32.const 0) (i32.const 0)))
 
 ;; memory.copy
 (module
@@ -101,9 +101,9 @@
 (invoke "copy" (i32.const 0x10000) (i32.const 0) (i32.const 0))
 (invoke "copy" (i32.const 0) (i32.const 0x10000) (i32.const 0))
 
-;; OK copying 0 bytes outside of memory.
-(invoke "copy" (i32.const 0x10001) (i32.const 0) (i32.const 0))
-(invoke "copy" (i32.const 0) (i32.const 0x10001) (i32.const 0))
+;; Copying 0 bytes outside of memory limit is NOT allowed.
+(assert_trap (invoke "copy" (i32.const 0x10001) (i32.const 0) (i32.const 0)))
+(assert_trap (invoke "copy" (i32.const 0) (i32.const 0x10001) (i32.const 0)))
 
 ;; memory.init
 (module
@@ -128,19 +128,22 @@
 ;; Init ending at memory limit and segment limit is ok.
 (invoke "init" (i32.const 0xfffc) (i32.const 0) (i32.const 4))
 
-;; Out-of-bounds writes trap, but all previous writes succeed.
+;; Out-of-bounds writes trap, and no partial writes has been made.
 (assert_trap (invoke "init" (i32.const 0xfffe) (i32.const 0) (i32.const 3))
     "out of bounds memory access")
-(assert_return (invoke "load8_u" (i32.const 0xfffe)) (i32.const 0xaa))
-(assert_return (invoke "load8_u" (i32.const 0xffff)) (i32.const 0xbb))
+(assert_return (invoke "load8_u" (i32.const 0xfffe)) (i32.const 0xcc))
+(assert_return (invoke "load8_u" (i32.const 0xffff)) (i32.const 0xdd))
 
 ;; Succeed when writing 0 bytes at the end of either region.
 (invoke "init" (i32.const 0x10000) (i32.const 0) (i32.const 0))
 (invoke "init" (i32.const 0) (i32.const 4) (i32.const 0))
 
-;; OK writing 0 bytes outside of memory or segment.
-(invoke "init" (i32.const 0x10001) (i32.const 0) (i32.const 0))
-(invoke "init" (i32.const 0) (i32.const 5) (i32.const 0))
+;; Writing 0 bytes outside of memory / segment limit is NOT allowed.
+(assert_trap (invoke "init" (i32.const 0x10001) (i32.const 0) (i32.const 0)))
+(assert_trap (invoke "init" (i32.const 0) (i32.const 5) (i32.const 0)))
+
+;; OK to access 0 bytes at offset 0 in a dropped segment.
+(invoke "init" (i32.const 0) (i32.const 0) (i32.const 0))
 
 ;; data.drop
 (module
@@ -157,9 +160,8 @@
     (memory.init 1 (i32.const 0) (i32.const 0) (i32.const 0)))
 )
 
+;; OK to drop the same segment multiple times or drop an active segment.
 (invoke "init_passive")
 (invoke "drop_passive")
-(assert_trap (invoke "drop_passive") "data segment dropped")
-(assert_trap (invoke "init_passive") "data segment dropped")
-(assert_trap (invoke "drop_active") "data segment dropped")
-(assert_trap (invoke "init_active") "data segment dropped")
+(invoke "drop_passive")
+(invoke "drop_active")