[wasm] Restrict enclosing-try throw-helper attach to the same funclet#131279
Conversation
VisitWasmSuccs (added in dotnet#130945) makes a throw helper a successor of a try side-entry whenever the helper's keyed try region lexically encloses the side-entry (bbInTryRegions). bbInTryRegions is a purely lexical try-nesting test: it does not check that the helper and the side-entry live in the same function region (funclet). As a result a main-method throw helper for an outer try can be attached to a catch-resumption side-entry that merely nests inside that try but lives in a handler funclet. RPO layout then places the main-method helper inside the funclet, interleaving it with funclet blocks. That corrupts the emitted wasm: the funclet's terminal `end` is dropped (module fails "function body must end with end opcode") and branch target depths are miscomputed. Gate the enclosing-try relaxation on same-function-region ownership: only attach the helper when funcRegionOf(block) == funcRegionOf(acd->acdDstBlk). funcRegionOf mirrors funGetFuncIdx but works for an arbitrary block and distinguishes a filter funclet from its filter-handler. The exact-match path (the block's own region) is unchanged, and dotnet#130945's intended case (an inner-try side-entry pulling its enclosing try's helper within the same funclet) still matches. Verified on a standalone System.Data.Common R2R wasm crossgen (release JIT, without the end-emission change from dotnet#131251): baseline emits an invalid module (func 597 fails to validate); with this fix the module validates. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2e627b94-2658-41ce-a880-d9c27b7febd9
…ix-wasm-funclet-layout
…ix-wasm-funclet-layout
|
Azure Pipelines: Successfully started running 6 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @agocke |
There was a problem hiding this comment.
Pull request overview
This PR adjusts WebAssembly (wasm32) flow-graph successor enumeration to avoid introducing cross-funclet edges when attaching throw-helper “ACD” blocks for enclosing try regions. The change is aimed at preventing block layout interleaving across funclet boundaries, which can corrupt wasm function bodies.
Changes:
- Add a local
funcRegionOfhelper to compute the physical function-region (funclet) index for an arbitrary block, including distinguishing filter vs. filter-handler regions. - Track whether an ACD match came specifically from the enclosing-try relaxation (
viaEnclosingTry). - Gate the enclosing-try successor attachment so the throw helper is only attached when it belongs to the same function region as the side-entry.
Validation: full input-set surveyRan the entire 194-assembly input set as standalone crossgen2 R2R-wasm under the failing SIMD-forced config (
Note This comment was authored with the assistance of GitHub Copilot. |
…ix-wasm-funclet-layout
…ix-wasm-funclet-layout
…ix-wasm-funclet-layout
…nclosingTry Andy Ayers' review on dotnet#131279: 1. Extract the local funcRegionOf lambda in VisitWasmSuccs into a reusable Compiler::bbIsInSameFunclet(BasicBlock*, BasicBlock*) predicate (declared in compiler.h beside funGetFuncIdx, inline definition in compiler.hpp). It mirrors funGetFuncIdx but is valid for an arbitrary (non-entry) block and distinguishes a filter funclet from its filter-handler. 2. Drop the viaEnclosingTry guard and gate every match on bbIsInSameFunclet unconditionally. viaEnclosingTry restricted the same-funclet check to the enclosing-try relaxation only, out of blast-radius conservatism. A 194-assembly R2R-wasm crossgen A/B (guarded vs. unconditional) produced byte-identical output on all 193 emitted modules with zero new invalid modules or crossgen failures, so the guard never changed a direct-match outcome. Gating direct matches on same-funclet too is strictly more correct: a direct match naming a cross-funclet helper is exactly the mis-layout this fix prevents. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2e627b94-2658-41ce-a880-d9c27b7febd9
The helper is documented as valid only after funclets are created; assert it like funGetFunc/funGetFuncIdx do, to catch misuse early instead of silently returning region 0 for blocks with unset funclet metadata. The sole caller (VisitWasmSuccs, in PHASE_WASM_CONTROL_FLOW) runs long after PHASE_CREATE_FUNCLETS, so the invariant always holds. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2e627b94-2658-41ce-a880-d9c27b7febd9
…on branch (#131287) Fixes #131285 ## Problem On WebAssembly R2R (crossgen), an `async` method with a `try { ...; return; } catch { }` where the `try` completes normally by branching out early (a `return` after an `await`) generates a **valid** wasm module that **traps at runtime** with `RuntimeError: unreachable` on the normal-completion path. This is distinct from #131279 (dropped terminal `end` → *invalid* module) and is not a reopening of #129335 — here the module is well-formed and validates; it simply traps at runtime. ## Root cause In `Compiler::fgWasmControlFlow()`, a non-contiguous forward branch that exits a try/catch region started its enclosing wasm `Block` at the **innermost** try's begin. When the branch also escapes an **outer** catch-try that shares the same end cursor — the wasm catch-resumption / resume-after-catch shape, where the paired `ExnRefWrapper` is stretched past the try-end to cover the resumption dispatcher — the `Block` ends up nested *inside* that outer try. The normal-completion branch then resolves to depth 0 and lands on the outer try's trailing validation `unreachable` (emitted right after its `end` by `genEmitStartBlock`) instead of the continuation. ## Fix Walk the enclosing try regions outward and start the `Block` at the **outermost** escaped catch-try's begin, so the `Block` encloses those trys' ends and the branch resolves to the continuation (`br 0` → `br 1`). For a single-level `try/catch` the outermost escaped region is also the innermost, so behavior there is unchanged. ## Test Adds `src/tests/JIT/Regression/JitBlue/Runtime_131285`, a sync `[Fact]` that drives an `async Task` `MoveNext` through the normal-completion `return` path. The csproj sets `<AlwaysUseCrossGen2 Condition="'$(TargetOS)' == 'browser'">true</AlwaysUseCrossGen2>` so the browser leg is actually compiled to wasm R2R and run under node — without it a plain browser leg runs the test as ordinary JIT and silently false-passes. ## Validation - Runtime A/B on the exact CLRTest node command: unfixed JIT traps (`exit=1`), fixed JIT passes (`exit=100`). - Composite-mode crossgen A/B: unfixed emits trapping `br 0`, fixed emits `br 1`. - 194-assembly sweep: 0 new invalid modules, 185 byte-identical; single-level try/catch provably unaffected. > [!NOTE] > This pull request was generated with the assistance of GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 964b4f25-1c41-44d6-81fb-a6a75ee7d3e4
|
/ba-g osx checked build completed successfully but infra is stuck |
[wasm] Restrict enclosing-try throw-helper attach to the same funclet
Problem
crossgen2 emits an invalid WASM (wasm32) R2R module for methods with certain try/catch shapes: the terminal
endopcode (0x0b) is dropped for an EH funclet, sowasm-tools validate(and V8) reject the module with "function body must end with end opcode". A related symptom is a miscomputed branch target depth in the same funclet (tracked as #131252).Root cause
The defect is in wasm block layout, in
FgWasm::VisitWasmSuccs(src/coreclr/jit/fgwasm.h).#130945 added an "enclosing try" relaxation: when a block is a try side-entry and the throw-helper ACD key is
KD_TRY, the helper is also attached as a successor ifcomp->bbInTryRegions(key.RegionIndex(), block)is true.
bbInTryRegionsis a purely lexical try-nesting test — it does not check that the throw helper and the side-entry live in the same function region (funclet).So a throw helper for an outer try region that belongs to the main method can be attached to a catch-resumption side-entry (
BBF_CATCH_RESUMPTION) that merely nests inside that try but physically lives in a handler funclet. RPO layout then lays the main-method helper inside the funclet, interleaving it with funclet blocks. Because a funclet is a distinct wasm function body, that interleaving drops the funclet's terminalendand corrupts its branch target depths.Concrete trace from
System.Data.DataColumn:set_Expression(instrumentedVisitWasmSuccs, deduped):BB76 is the throw helper for root try region #4 (
KD_TRY, no handler index → main method); BB50/BB73 are catch-resumption side-entries in handler funclet #3 (which lexically nests in try #4), so the enclosing-try rule pulls the main-method helper into funclet #3.This is an ordinary catch-resumption EH shape (
async=0); it is independent of runtime-async, andfgwasm.his byte-identical tomain. It is latent onmainonly because R2R-wasm codegen isn't enabled there yet.Fix
Gate the enclosing-try relaxation on same-function-region ownership. A new
funcRegionOflambda returns the funclet index that physically contains an arbitrary block (0 == main method); it mirrorsfunGetFuncIdxbut works for non-entry blocks and distinguishes a filter funclet from its filter-handler. The helper is attached only when it shares the side-entry's function region:The exact-match path (a helper keyed to the block's own region) is unchanged, and #130945's intended case (an inner-try side-entry pulling its enclosing try's helper within the same funclet) still matches — so this only removes the cross-funclet edge.
Validation
Standalone
System.Data.CommonR2R wasm crossgen, release JIT,wasm-tools validate(onlyfgwasm.hdiffers between runs):wasm-tools validateorigin/mainlayout)func 597 failed to validate❌Notes
endfor EH funclets ending in a no-return call #131251, which hardened the terminal-endemission (the effect); this fixes the cause in layout. Closing [wasm] Emit terminalendfor EH funclets ending in a no-return call #131251 in favor of this.Note
This change was authored with the assistance of GitHub Copilot.