pcbjam/docs/features/hoist-cpp-catches-simplification/README.md
Viktor Vaczi 05127af431 docs(hoist): pass-simplification dossier; bump binaryen (FindAll/BranchUtils refactor)
Adds docs/features/hoist-cpp-catches-simplification/ (diagnosis + Layer 1 result) and bumps the binaryen submodule to 1d40cf5a8 (FindAll/BranchUtils refactor + describe-as-is comments of HoistCppCatches). Validated end-to-end: full pcbnew build through the refactored wasm-opt + Firefox pcbnew e2e 6/6.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-01 16:52:17 +02:00

12 KiB
Raw Permalink Blame History

Simplifying the --hoist-cpp-catches Binaryen pass

Status: research / refactor proposal (2026-06-30). Layer 1 LANDED + validated 2026-07-01 in worktree ../kicad-wasm-hoist-simplify (binaryen branch hoist-simplify): pass is 454 → 409 lines, all 6 run.sh checks byte-identical to baseline. Layers 23 still open. Subject: binaryen/src/passes/HoistCppCatches.cpp (was 454 lines), the pre---asyncify catch-arm-outlining pass. Companion to the wasm-exceptions/ dossier (why the pass exists) and 07-spike-results-and-opinion.md (the 7 KiCad shapes it covers). This doc asks a narrower question: keeping legacy-EH + Asyncify, can the pass itself be written smaller / cleaner / with fewer corner cases?

Verdict

Yes — realistically ~454 → ~300 lines with a lower bug surface — but the savings are almost entirely in boilerplate and defensive paths, not the core algorithm. The core (capture-flag-then-fall-through dispatch + the catch_all escape) is essentially irreducible for legacy-EH × Asyncify. There is exactly one structural idea that could simplify the hard part, and it is a prototype-behind-the-harness bet, not a sure win.

This is not about switching abstractions. JSPI / new-EH (try_table) were evaluated separately and rejected for now (JSPI = a coroutine-backend rewrite gated on Safari shipping; new-EH = worse, because Binaryen's Asyncify Fatal()s on any exnref local, #3739, open since 2021). We stay on legacy EH + Asyncify; this is purely "write the pass better."

Why the pass is complex (one root cause, not seven)

The "7 shapes" and every piece of corner-case machinery exist for one reason: Asyncify rewinds by falling forward through code and cannot fall into a catch/catch_all arm (only the engine's exception dispatch enters one), and legacy EH nests handlers inside the try with an implicit handler stack Asyncify can't spill. The pass flattens that nesting into straight-line, fall-through-reachable code. The complexity is essential to that pairing — so "simpler" means better engineering of the same flattening, and most of the wins are in code that isn't the flattening itself.

The file splits cleanly into three layers. Two are removable.


Layer 1 — accidental boilerplate: hand-rolled walkers → Binaryen idioms

Clear win: ~80 lines, lower risk, and it closes a latent correctness gap. The pass hand-rolls five walkers that Binaryen already ships, and the hand-rolled versions are less exhaustive than the library.

In the pass Lines Replace with Notes
SuspendCallFinder 7286 FindAll<Call> + filter or delete entirely with Layer 2
CppArmCollector 8999 FindAll<Try> + isCppTag filter
TryFinder / tryWithin 132148 FindAll<Try> + std::find
LabelCollector 151168 BranchUtils::getBranchTargets one call (branch-utils.h:247)
OrphanRetargeter 181204 per-orphan BranchUtils::replacePossibleTarget + keep bespoke delegate→caller; see below

(FindAll lives in binaryen/src/ir/find_all.h:26.)

The OrphanRetargeter swap is a correctness fix, not just fewer lines

The hand-rolled OrphanRetargeter only visited Break, Switch, and Try-delegate. The refactor collapses those into a single UnifiedExpressionVisitor whose branch arm calls BranchUtils::operateOnScopeNameUses (branch-utils.h:48, generated from wasm-delegations-fields.def) — so the ordinary-branch retarget to $done now covers br/br_if/br_table and BrOn exhaustively (and any future branch kind), instead of a hand-listed subset.

A caveat corrected from the original proposal: I claimed this "closes the latent rethrow-retarget gap for free." It does not, and the landed code deliberately leaves rethrow untouched — because a rethrow (like a delegate) can only target a try or the caller, never the $done block, and there is no valid "rethrow to caller" sentinel to retarget an orphaned one to. So the refactor is strictly behavior-preserving: delegate→caller stays explicit, rethrow is skipped exactly as before. The orphaned-rethrow case remains theoretically unhandled, but it does not arise — after ArmRewriter rewrites the owning try's rethrowthrow, the only rethrows left in a hoisted arm target nested tries internal to the arm, which are never orphaned. The real, banked win here is fewer bespoke walkers (four removed) and exhaustive branch coverage, not a bug fix.

The one piece that stays bespoke: a delegate can only target a try or the caller — not a block — so the delegate→DELEGATE_CALLER_TARGET special case (199203) must remain; you can't fold it into a generic →$done retarget. That's correct domain logic, not boilerplate.

Risk: low. Pure mechanical substitution, fully covered by run.sh (fuzz-exec + real unwind/rewind) and eh-spike.spec.ts.


Layer 2 — defensive generality you can shed

Win, small risk, ~4050 lines. Three pieces handle things that don't occur in production or are mere optimizations.

2a. The entire value-typed $result path (256263, 330332, 383, 429445)

Per our own 07-spike-results: LLVM keeps C++ catch values in locals → cpp tries are always void/unreachable. The value-typed routing is exercised only by hand-written .wat (value-typed-cpp-catch.wat, value-typed-suspend.wat). And --hoist-cpp-catches runs first in the wasm-opt invocation (run.sh: one binary does --hoist-cpp-catches then --asyncify/-O2), so its input is raw LLVM EH lowering — a value-typed cpp try can't arise.

→ Replace ~30 lines of $result routing + the valueTyped / blockType conditionals woven through visitTry with a single early skip of any concrete-result escape target (the code already skips non-defaultable result types at 256263; generalize that to all concrete results). Delete the two synthetic tests.

Safety gate: only do this with -sASYNCIFY_ASSERTIONS on in CI (already on the migration checklist). The asserts tripwire (AsyncifyUnwindWalker) turns a hypothetical value-typed-cpp-try into a loud trap at test time instead of silent corruption in production. Without that gate, keep the routing.

2b. bareSingle (348, 389402)

A single-arm optimization (emit a bare trailing handler instead of a br_if-skip block). Drop it — always emit the uniform br_if-skip dispatch and let the -O2 that runs immediately afterward coalesce the single-arm case. Removes a code path and a branch of reasoning.

2c. HOIST_ONLY_SUSPEND / SuspendCallFinder (7286, 307321)

Debug-only since we ship hoist-all. Judgment call: keep as a blast-radius-narrowing lever while debugging, or cut for simplicity. If cut, SuspendCallFinder goes with it.


Layer 3 — the essential core (do not try to shrink)

Stated explicitly so nobody burns time here. These are each pinned to a real failure already debugged (see 07):

  • Capture-flag-then-fall-through dispatch is load-bearing. Handlers must be reached by fall-through so Asyncify's forward-motion rewind re-enters them; the flag is what skips them on the no-exception path. A direct br-to-handler-block would validate but not rewind. Not negotiable.
  • The block $esc escape + br (411414, 422431) exists because the catch_all-nested shape has trailing (unreachable)/cleanup after the nested try; you must branch past the outer try rather than complete into it. All three hard .wats are this shape (nested-catchall-*, delegate-orphan-*).
  • The nested-try pop guard (ArmRewriter::insideNestedTry, 108115) — don't clobber a nested catch's payload pop.
  • Deferral / escape-target selection (insideAncestorCatch, 235246) gated to catch_all cleanup pads only — this was the over-eager-deferral null function bug; the gate is correct and necessary.
  • ReFinalize (221227) — rewriting Try nodes in place leaves stale cached type; Asyncify keys off types, so a bottom-up re-finalize is mandatory.

The line count overstates the logical size: ~40% of the file is comments, and they encode hard-won corner-case knowledge. Stripping them would make the pass look simpler and be more dangerous. Keep them.


The one structural lever for the hard part (prototype, don't assume)

The intricate trio — insideAncestorCatch deferral (235246) + collect-own-plus-nested (266280) + outermost-filter (287305) — could collapse into one explicit computation:

For each cpp catch arm, walk up the expressionStack while you remain on the catch side of each enclosing Try; the topmost such Try is the arm's escape root. Group arms by escape root; emit one dispatch per root.

This replaces the asymmetric defer-vs-self-process dance (and the catch_all-vs-regular special case) with a single uniform rule: a catch_all arm and a regular cpp arm both just mean "a catch you must ascend past." It computes each handler's final position directly (one hoist) instead of via composition of multiple hoists — which is exactly where the null function / unreachable bugs lived.

Honest caveats — why this is a bet, not a slam-dunk:

  • The final hoist destination is the same as today (past the outermost catch-reached try), so the orphan set does not shrink — orphan retargeting stays.
  • Pulling handlers past intermediate cleanup pads in one step may surface rethrow-of-intermediate-try cases the current compositional form never reaches (the Layer-1 BranchUtils switch mitigates this, but it's new surface to test).
  • It rewrites the most bug-prone region of a pass that is currently green in all three engines. The corner cases may relocate rather than disappear.

Recommendation: do Layers 1 + 2 first. Only attempt the escape-root reformulation if the core still feels too subtle afterward, and only behind the full harness.


What "done" requires (verification)

The pass shipped in the last commit and is green in Firefox + Chrome + WebKit. Any rewrite must re-clear, in all three engines (standing project policy):

  1. scripts/binaryen-hoist-pass/tests/run.sh--fuzz-exec value-preservation and the real Asyncify unwind/rewind tests (*-suspend.wat yield 50).
  2. tests/asyncify/eh-spike.spec.ts — the 3-variant ablation (JS-EH green / wasm-EH red / wasm-EH+hoist green).
  3. The wx + KiCad e2e suites (npm run test:e2e / test:kicad), ideally under -sASYNCIFY_ASSERTIONS for the Layer-2a change.

Rebuild the fork wasm-opt via scripts/binaryen-hoist-pass/build-wasm-opt.sh (the submodule is version_130 + the pass).

  1. Layer 1 (FindAll/BranchUtils) — safe, high-value, fixes the latent rethrow-retarget gap. Land first; it's mechanical and the harness covers it.
  2. Layer 2b/2c (drop bareSingle; decide on HOIST_ONLY_SUSPEND) — small, low-risk.
  3. Layer 2a (drop value-typed routing) — only with the -sASYNCIFY_ASSERTIONS CI gate.
  4. Escape-root experiment — optional, behind the harness, only if the core still warrants it. Treat a "no net win" outcome as a valid result and revert.

Net

The pass is well-structured for what it does; the 454 is inflated by (good) comments and by boilerplate Binaryen already solves. The realistic, safe simplification — adopt FindAll/BranchUtils + shed the value-typed/bareSingle defensive paths — yields a leaner, more robust pass with the same proven core, and it even closes a latent rethrow-retarget bug. The genuinely hard part (flatten legacy-EH nesting for a fall-forward rewinder) cannot get materially smaller without changing the abstraction, which we've ruled out.