Fix #8785: StackEntry wasm stack type invariants and unreachable parser regressions - #8978
Open
ANAMASGARD wants to merge 7 commits into
Open
Fix #8785: StackEntry wasm stack type invariants and unreachable parser regressions#8978ANAMASGARD wants to merge 7 commits into
ANAMASGARD wants to merge 7 commits into
Conversation
Track Wasm operand-stack effects separately from Binaryen IR types via StackEntry::wasmStackType. Key fixes: - Sync wasmStackType to none when dropping unreachable predecessors - Guard finishScope tuple repackaging when hoist is empty - Mask deeper stack values under unreachable in hoistLastValue Adds validation tests for unreachable void-block drop, multivalue tuple parsing, and related edge cases.
Extend pushControlFlow/finishScope handling so void unreachable fallthrough control flow and return_call inside try remain parseable in concrete-result functions, while preserving WebAssembly#8785 rejection of (block (unreachable)) (drop). Allow void unreachable fallthrough blocks to satisfy concrete operand pops (e.g. if/br_if conditions). Skip struct.get field typing when the ref is unreachable. Add focused validation regressions for try/return_call and concrete block ends. Lit ledger: /tmp/binaryen-lit-ledger/report.md
Track void stack entries skipped in unreachable scopes so operand popping can match concrete values below (gufa struct.new) while still synthesizing unreachable operands when types mismatch (br_table index).
Treat wasm unreachable instructions (wasmStackType none but expr type unreachable) as polymorphic stack values in hoistLastValue, checkNeedsUnreachableFallback, and pop. Drop non-final concrete stack entries when building typed multi-expression blocks. Fixes array-multibyte parser failure and wasm-split validator errors. Adds unreachable-array-store-value-valid.wast regression test.
When wasmStackType is none but a void block retains expr->type unreachable, treat it as a poppable stack entry. Fixes -S pipe roundtrip on array-multibyte and drop(block) reparse paths.
The parser now distinguishes unreachable barriers from ordinary void entries, which changes block/drop/unreachable nesting in -S output across many tests. Also fixes scratch-local-roundtrip RTRIP checks and updates two validation tests to match baseline parse behavior.
Member
|
This is a large change, and I'm not sure if it makes sense to fix or not. That is, the motivation is good, but I'm not sure the complexity is worth the benefit in wasm-ir-builder. @tlively ? |
Member
|
If I understand correctly, this is adding proper Wasm validation to IRBuilder by tracking Wasm types alongside the parsed expressions in IRBuilder's expression stack. In general, this would be great to have. Users generally expect us to reject invalid WebAssembly modules, and this would help us run more of the spec tests to test correctness of other parts of our implementation. However, a few initial notes:
Also, it would be good to reduce functional changes to the parser to a bare minimum. Since the goal is only to add extra validation, I would not expect the shape of the parsed IR to change due to this PR. In other words, I would expect that no tests have to be updated except possibly to opt out of the extra validation. |
tlively
self-requested a review
August 13, 2026 01:32
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
3 participants
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.
Fixes parser/IRBuilder regressions from tracking Wasm operand-stack effects separately via
StackEntry::wasmStackType(#8785).wasmStackTypeon the IRBuilder expression stack and keep it in sync with Wasm stack semantics (unreachable barriers vs ordinary void instructions).-Spipe roundtrip, e.g.array-multibyte).-Sexpectations: output shape changes (extrablock/drop/unreachablenesting), not semantic IR changes.Test plan
build-clang/bin/binaryen-unittests— 379/379build-clang/bin/binaryen-lit test/lit -j 2— 982/986 (only 4 pre-existingd8/fuzz_shell*.wastfailures; needv8)test/lit/validation/unreachable-*.wastarray-multibyte,wat-kitchen-sink,remove-unused-brs_enable-multivalue,wasm-split/split-module-items