From 05127af4317bffa7df64c8a85d7265d51cf9f400 Mon Sep 17 00:00:00 2001 From: Viktor Vaczi Date: Wed, 1 Jul 2026 16:52:17 +0200 Subject: [PATCH] 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) --- binaryen | 2 +- .../README.md | 214 ++++++++++++++++++ 2 files changed, 215 insertions(+), 1 deletion(-) create mode 100644 docs/features/hoist-cpp-catches-simplification/README.md diff --git a/binaryen b/binaryen index a6c26e3..1d40cf5 160000 --- a/binaryen +++ b/binaryen @@ -1 +1 @@ -Subproject commit a6c26e3c606e381603b2763a1b2f10f3ddb2ca07 +Subproject commit 1d40cf5a8fc87f2c412b4b6081b1054d0b1cab22 diff --git a/docs/features/hoist-cpp-catches-simplification/README.md b/docs/features/hoist-cpp-catches-simplification/README.md new file mode 100644 index 0000000..7d49eac --- /dev/null +++ b/docs/features/hoist-cpp-catches-simplification/README.md @@ -0,0 +1,214 @@ +# 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 2–3 still open. +> Subject: `binaryen/src/passes/HoistCppCatches.cpp` (was 454 lines), the pre-`--asyncify` +> catch-arm-outlining pass. Companion to the +> [`wasm-exceptions/`](../wasm-exceptions/) dossier (why the pass exists) and +> [`07-spike-results-and-opinion.md`](../wasm-exceptions/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` | 72–86 | `FindAll` + filter | or delete entirely with Layer 2 | +| `CppArmCollector` | 89–99 | `FindAll` + `isCppTag` filter | | +| `TryFinder` / `tryWithin` | 132–148 | `FindAll` + `std::find` | | +| `LabelCollector` | 151–168 | `BranchUtils::getBranchTargets` | one call (`branch-utils.h:247`) | +| `OrphanRetargeter` | 181–204 | 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 +`rethrow`→`throw`, the only `rethrow`s 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 (199–203) 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, ~40–50 lines.** Three pieces handle things that don't occur in production +or are mere optimizations. + +### 2a. The entire value-typed `$result` path (256–263, 330–332, 383, 429–445) + +Per our own [`07-spike-results`](../wasm-exceptions/07-spike-results-and-opinion.md): **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 256–263; 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](../wasm-exceptions/README.md)). 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, 389–402) + +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` (72–86, 307–321) + +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`](../wasm-exceptions/07-spike-results-and-opinion.md)): + +- **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`** (411–414, 422–431) 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 `.wat`s are this shape** + (`nested-catchall-*`, `delegate-orphan-*`). +- **The nested-try pop guard** (`ArmRewriter::insideNestedTry`, 108–115) — don't clobber a + nested catch's payload `pop`. +- **Deferral / escape-target selection** (`insideAncestorCatch`, 235–246) gated to `catch_all` + cleanup pads only — this *was* the over-eager-deferral `null function` bug; the gate is + correct and necessary. +- **`ReFinalize`** (221–227) — 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 (235–246) + collect-own-plus-nested +(266–280) + outermost-filter (287–305) — 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). + +## Recommended sequencing + +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.