Repository navigation
Conversation
With `-sSTACK_OVERFLOW_CHECK=2`, `wasm-emscripten-finalize` runs binaryen's stack-check pass with `__handle_stack_overflow` as the handler. The pass builds the call to the handler with the stack pointer's type as its result type, but the handler returns nothing. Finalize does not validate after its passes, so these builds still work. `BINARYEN_PASS_DEBUG=1` makes binaryen validate after each pass, and then the build fails with "Last pass (stack-check) broke validation". The new test builds and runs `hello_world.c` with both settings. It fails until WebAssembly/binaryen#9335 rolls in.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new test test_stack_overflow_check_pass_debug to test/test_other.py. This test runs with the BINARYEN_PASS_DEBUG environment variable set to '1' and STACK_OVERFLOW_CHECK set to 2 to ensure that binaryen validates correctly after each pass and catches any invalid IR. There are no review comments, and the changes look correct.
| @with_env_modify({'BINARYEN_PASS_DEBUG': '1'}) | ||
| def test_stack_overflow_check_pass_debug(self): | ||
| self.set_setting('STACK_OVERFLOW_CHECK', 2) | ||
| self.do_runf('hello_world.c', 'Hello, world!\n') |
There was a problem hiding this comment.
I think the binaryen fix looks good, but I'm not sure we need to add tests like this (basically a test of binaryen) here in the emscripten repo? This seems like something that should be tested in binaryen. Having said that I guess it could make sense for emscripten to be tested with BINARYEN_PASS_DEBUG in some cases?
@kripken WDYT?
With
-sSTACK_OVERFLOW_CHECK=2,wasm-emscripten-finalizerunsbinaryen's stack-check pass with
__handle_stack_overflowas thehandler. The pass builds the call to the handler with the stack
pointer's type as its result type, but the handler returns nothing.
Finalize does not validate after its passes, so these builds still
work.
BINARYEN_PASS_DEBUG=1makes binaryen validate after each pass,and then the build fails:
The new test,
other.test_stack_overflow_check_pass_debug, setsBINARYEN_PASS_DEBUG=1. Then it builds and runshello_world.cwith-sSTACK_OVERFLOW_CHECK=2, and checks that the program prints"Hello, world!".
The test fails until WebAssembly/binaryen#9335 lands and rolls in. The
reviewer of that PR asked for this order:
fix reaches
test/emsdk_version.txt.On macOS arm64, with the emsdk build in
test/emsdk_version.txt(binaryenversion_133-147-ga6ed9adf2), the test fails as shown above. Withbinaryen built from the WebAssembly/binaryen#9335 branch, it passes. The
existing stack overflow tests in
core0,core2, andothergive thesame results with both builds.
An AI coding agent wrote this test and this description at my request,
and ran the checks above. It ran them only on macOS, not on Linux like
CI.