bytecodealliance / wasm-micro-runtime

WebAssembly Micro Runtime (WAMR)
Apache License 2.0
4.84k stars 618 forks source link

WAMR fails latest memory.wast spec test #3838

Open sjamesr opened 20 hours ago

sjamesr commented 20 hours ago

WAMR fails memory spec test at HEAD

Recently, the spec tests were updated to add a new test case asserting that __heap_base and other global exports do not affect memory accesses. WAMR fails this test and I'm trying to understand why.

Test case

Apply the following patch and run the spec tests:

diff --git a/tests/wamr-test-suites/spec-test-script/memory.patch b/tests/wamr-test-suites/spec-test-script/memory.patch
new file mode 100644
index 00000000..fc222f5e
--- /dev/null
+++ b/tests/wamr-test-suites/spec-test-script/memory.patch
@@ -0,0 +1,32 @@
+diff --git a/test/core/memory.wast b/test/core/memory.wast
+index 497b69f..053e678 100644
+--- a/test/core/memory.wast
++++ b/test/core/memory.wast
+@@ -240,3 +240,27 @@
+   "(import \"\" \"\" (memory $foo 1))"
+   "(import \"\" \"\" (memory $foo 1))")
+   "duplicate memory")
++
++(module
++  (memory (export "memory") 1 1)
++
++  ;; These should not change the behavior of memory accesses.
++  (global (export "__data_end") i32 (i32.const 10000))
++  (global (export "__stack_top") i32 (i32.const 10000))
++  (global (export "__heap_base") i32 (i32.const 10000))
++
++  (func (export "load") (param i32) (result i32)
++    (i32.load8_u (local.get 0))
++  )
++)
++
++;; None of these memory accesses should trap.
++(assert_return (invoke "load" (i32.const 0)) (i32.const 0))
++(assert_return (invoke "load" (i32.const 10000)) (i32.const 0))
++(assert_return (invoke "load" (i32.const 20000)) (i32.const 0))
++(assert_return (invoke "load" (i32.const 30000)) (i32.const 0))
++(assert_return (invoke "load" (i32.const 40000)) (i32.const 0))
++(assert_return (invoke "load" (i32.const 50000)) (i32.const 0))
++(assert_return (invoke "load" (i32.const 60000)) (i32.const 0))
++(assert_return (invoke "load" (i32.const 65535)) (i32.const 0))
++
diff --git a/tests/wamr-test-suites/test_wamr.sh b/tests/wamr-test-suites/test_wamr.sh
index 87e15686..198677f0 100755
--- a/tests/wamr-test-suites/test_wamr.sh
+++ b/tests/wamr-test-suites/test_wamr.sh
@@ -526,6 +526,7 @@ function spec_test()
         # Apr 3, 2024 [js-api] Integrate with the ResizableArrayBuffer proposal (#1300)
         git reset --hard bc76fd79cfe61033d7f4ad4a7e8fc4f996dc5ba8
         git apply ../../spec-test-script/ignore_cases.patch || exit 1
+        git apply ../../spec-test-script/memory.patch || exit 1
         if [[ ${ENABLE_SIMD} == 1 ]]; then
             git apply ../../spec-test-script/simd_ignore_cases.patch || exit 1
         fi

Run the test with ./test_wamr.sh -s spec -m x86_64 -t classic-interp or something similar.

Your environment

Expected behavior

The upstream patch indicates that engines should ignore the __data_end, __stack_top and __heap_base exports. I'm new to WASM, I could not find anything in the official spec to indicate what (if anything) the engine should do with these.

To me, either the WAMR engine implementation is wrong, or the spec tests are wrong.

Actual behavior

The tests fail with out-of-bounds memory access, because the WAMR loader uses these global module exports to set the heap size.

sjamesr commented 14 hours ago

Please see the discussion at https://github.com/WebAssembly/spec/pull/1753#issuecomment-2394262819.

Nick recommends removing the non-standard memory size handling and instead implementing the custom-page-sizes proposal.

Has this been discussed before? Is there any interest in incorporating this into WAMR?