Skip to content

[StackCheck] Fix handler call result type - #9335

Merged
tlively merged 2 commits into
WebAssembly:mainfrom
nexxeln:fix-stack-check-handler-type
Nov 2, 2026
Merged

tlively merged 2 commits into
WebAssembly:mainfrom
nexxeln:fix-stack-check-handler-type

Conversation

@nexxeln

@nexxeln nexxeln commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

--stack-check with a stack-check-handler pass-arg produces IR that
fails validation. For example, with this module:

(module
 (memory 1)
 (global $__stack_pointer (mut i32) (i32.const 1024))
 (func $f (global.set $__stack_pointer (i32.sub (global.get $__stack_pointer) (i32.const 16)))))
$ wasm-opt m.wat --stack-check --pass-arg=stack-check-handler@__handle_stack_overflow -o out.wasm
[wasm-validator error in function f] i32 != none: call* type must match callee return type, on
(call $__handle_stack_overflow
 (local.get $0)
)
[wasm-validator error in function f] unexpected true: if without else must not return a value in body, on
...
Fatal: error after opts

The pass imports the handler with the stack pointer as its only param
and no results. But stackBoundsCheck built the call to the handler
with the stack pointer's type as its result type. The wrong type dates
back to #4471, which added the param. This PR builds the call with
Type::none. The other code the pass builds, including
__set_stack_limits, already has the right types.

The handler case is a second RUN line in stack-check-memory64.wast,
which uses an i64 stack pointer. Without the fix, wasm-opt rejects the
output and the test fails. The text format does not print a call's type,
so the test relies on validation for that part.

Emscripten runs this pass with a handler. In non-standalone builds,
wasm-emscripten-finalize --check-stack-overflow sets the handler to
__handle_stack_overflow. Emscripten passes that flag when
STACK_OVERFLOW_CHECK is 2 or more. Finalize does not validate after
its passes, so those builds still work. With BINARYEN_PASS_DEBUG=1,
finalize stops with "Last pass (stack-check) broke validation".

The emitted binary was always fine. A call in the binary format holds
only a function index, and readers take its result type from the
callee. With --no-validation, version 133 writes the same bytes as the
fixed build, and V8 compiles them. Only binaryen's IR was wrong, so only
pipelines that validate after the pass fail.

I used an AI coding agent to help reproduce the bug, write the fix and
the test, and draft this description. I reviewed the change and can
answer questions about it. One area could use a closer look. I checked
the emscripten notes by reading emscripten's tools/emscripten.py and
running wasm-emscripten-finalize directly, not with a full emscripten
build.

Until the next commit, wasm-opt rejects the output of the new RUN line.
The pass gives the handler call the stack pointer's type, but the
handler has no results.
With stack-check-handler set, the pass imports the handler with no
results but built the call to it with the stack pointer's type, so the
output failed validation. Build the call with Type::none.
@nexxeln
nexxeln requested a review from a team as a code owner November 1, 2026 06:58
@nexxeln
nexxeln requested review from tlively and removed request for a team November 1, 2026 06:58

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request corrects the return type of the stack overflow handler call in StackCheck.cpp from stackPointer->type to Type::none. It also adds a test case in stack-check-memory64.wast to verify the behavior when a custom stack check handler is provided. I have no additional feedback to provide as there are no review comments to evaluate.

@tlively tlively left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. Can you create a corresponding Emscripten PR adding a test that shows the current failure with BINARYEN_PASS_DEBUG=1? Then we can land this to fix the test in the Emscripten PR, then finally land the Emscripten PR.

@nexxeln

nexxeln commented Nov 1, 2026

Copy link
Copy Markdown
Contributor Author

done!

@tlively
tlively enabled auto-merge (squash) November 2, 2026 18:08
@tlively
tlively merged commit c00f653 into WebAssembly:main Nov 2, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants