Fix #8785: StackEntry wasm stack type invariants and unreachable parser regressions - #8978
Fix #8785: StackEntry wasm stack type invariants and unreachable parser regressions#8978ANAMASGARD wants to merge 8 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.
|
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 ? |
|
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. |
…ng parsed IR. Track wasmStackType as a shadow operand stack for parse-time validation only; IR packaging still uses expr->type as on main. Reject Wasm stack underflow when validation is enabled and reuse --no-validation as the opt-out for tests that parse valid Binaryen IR but invalid Wasm. Restore lit -S expectations from main and add focused unreachable validation regressions. Signed-off-by: Gaurav Chaudhary <chaudharygaurav2004@gmail.com>
|
@kripken @tlively Thanks for the review = 999c1cd separates validation from IR generation: wasmStackType is only for Wasm stack checks, IR still uses expr->type like main, and I reverted the lit -S diffs. Opt-out is --no-validation / -n. ~21% parse overhead on a small test module. PR description updated. Happy to trim further if this still feels too heavy for IRBuilder. Please review again and give feedback |
3 participants
Fixes #8785: reject Wasm operand-stack underflow at parse time (e.g.
(block (unreachable)) (drop)), matching V8, without changing Binaryen IR for valid Wasm.Approach: Track
StackEntry::wasmStackTypeas a shadow Wasm stack (likeStackIRGenerator::makeStackInst). IR packaging still usesexpr->typeas onmain. Validation runs only whenPassOptions.validateis true;--no-validation/-n(andwasm-as --validate=none) opt out.Outliningdisables stack validation when reconstructing IR.Changes vs earlier commits: Reverted ~91 lit
-Sexpectation diffs. No IR shape changes for valid modules — only newunreachable-*.wasttests and a few--no-validationRUN:lines where tests parse Binaryen IR that isn’t valid Wasm.Perf: ~21% parse overhead on a small kitchen-sink module (~7.2 ms vs ~5.7 ms per parse, 100×,
-all).Test plan
binaryen-unittests— 379/379binaryen-lit test/lit/validation/unreachable-*.wast— 15/15wat-kitchen-sink,array-multibyte,remove-unused-brs_enable-multivalue,wasm-split/split-module-items