WAMR fails latest memory.wast spec test

[sjamesr]

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

• Linux x86_64
• WAMR at HEAD

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]

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?

[wenyongh]

@sjamesr thanks for reporting the issue, one of WAMR's goals is to reduce the footprint and the runtime may shrink the linear memory based on the global __data_end, __stack_top and __heap_base. For example, if there is no memory.grow opcode found, runtime may shrink the memory to the position that __heap_base points to, since the libc heap isn't used. That's really important for some embedded and IoT devices whose resources are limited, for example, if not truncated, at lease one page (64KB) of memory will be allocated, but if truncated, it may be shrunk into several KBs and a lot of memory is saved.

The custom-page-sizes proposal wasn't raised yet when WAMR did the optimization. We also pay a lot attention to it, I think we may try to support it once it is ready and is supported by the toolchain.

[lum1n0us]

#4006