Repository navigation
Conversation
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.
There was a problem hiding this comment.
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
reviewed
Nov 1, 2026
tlively
left a comment
Member
There was a problem hiding this comment.
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.
Author
|
done! |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
--stack-checkwith astack-check-handlerpass-arg produces IR thatfails validation. For example, with this module:
The pass imports the handler with the stack pointer as its only param
and no results. But
stackBoundsCheckbuilt the call to the handlerwith 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
i64stack pointer. Without the fix, wasm-opt rejects theoutput 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-overflowsets the handler to__handle_stack_overflow. Emscripten passes that flag whenSTACK_OVERFLOW_CHECKis 2 or more. Finalize does not validate afterits 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 thefixed 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.pyandrunning
wasm-emscripten-finalizedirectly, not with a full emscriptenbuild.