[wasm] Emit terminal end for EH funclets ending in a no-return call#131251
[wasm] Emit terminal end for EH funclets ending in a no-return call#131251lewing wants to merge 1 commit into
end for EH funclets ending in a no-return call#131251Conversation
crossgen2 emitted a malformed wasm function body for certain exception-handling funclets: the required terminal `end` opcode (0x0b) was dropped, so the module failed validation (V8 / `wasm-tools validate`: "function body must end with end opcode"). Observed on `System.Data.Common` (`DataColumn.set_Expression` funclet 4 and `DataTable.set_Locale` funclet 3) — both non-SIMD `BBJ_THROW` funclets whose last block ends in a no-return `call_indirect` throw-helper tail. The wasm end-emission guard `block->IsLast() || bbIsFuncletBeg(block->Next())` uses the GLOBAL block order (`bbNext`), in which a funclet's last block can be followed by an interspersed root throw-helper block (`fgIsThrowHlpBlk`, no handler index) that is not a funclet begin. Codegen iterates per-funclet, so the guard fails to recognize the funclet boundary and the terminal `end` is never emitted. This same unsound predicate was duplicated across four wasm end-emission sites (the `BBJ_THROW` and `BBJ_ALWAYS` arms of `genEmitEndBlock`, the retless `BBJ_CALLFINALLY` path in `genCallFinally`, and the `end`/`return` discriminator in `genFuncletEpilog`), which is what let this gap — and the incompleteness of the prior fix dotnet#129335 — arise. Consolidate the predicate into a single wasm helper `CodeGen::genIsLastBlockOfCurrentFunc` that also compares against the current func/funclet's authoritative last block (`funCurrentFunc()->GetLastBlock()`), and route all four sites through it. The added term is an additive OR, so each block still emits at most one terminal `end` (no double-`end` regression on the `BBJ_RETURN` / funclet-epilog paths). Validated by recrossgenning standalone `System.Data.Common` to wasm R2R: both funclets now emit their terminal `end` and the module passes `wasm-tools validate`, with no double-`end` regression across all functions. Related: dotnet#129335 (incomplete predecessor), dotnet#129449 (`genCallFinally` shares the predicate). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2e627b94-2658-41ce-a880-d9c27b7febd9
|
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. |
There was a problem hiding this comment.
Pull request overview
This PR fixes WebAssembly R2R codegen for CoreCLR by ensuring wasm function bodies always end with the required terminal end opcode, even when generating EH funclets whose last block ends in a no-return tail call and the global bbNext order doesn’t reflect the per-funclet boundary.
Changes:
- Introduce a shared helper (
CodeGen::genIsLastBlockOfCurrentFunc) to reliably detect the last block of the currently generated wasm function/funclet. - Route wasm end-emission decisions in
genFuncletEpilogand retlessgenCallFinallythrough the new helper. - Update wasm-specific end-emission checks in
genEmitEndBlock(incodegenlinear.cpp) to use the consolidated predicate.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/jit/codegenwasm.cpp | Adds genIsLastBlockOfCurrentFunc and uses it to decide end emission for funclet epilogs and retless BBJ_CALLFINALLY. |
| src/coreclr/jit/codegenlinear.cpp | Replaces wasm-specific “last block” checks with the consolidated helper when deciding to emit function terminators. |
| src/coreclr/jit/codegen.h | Declares the new wasm-only helper on CodeGen. |
|
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 'arch-wasm': @lewing, @pavelsavara |
If what's stated here is correct, this is the actual bug: root method blocks should not be interspersed with funclet blocks. Seems odd that this fix works. Maybe it's an unreachable throw helper block? |
I'll do a deeper pass and try to get the full trace, it is trickier when the code doesn't validate. The code does validate and run after the change. |
The "trick" we're relying on for layout is a specialized reverse post-order (RPO) traversal of the flow graph. In particular, all funclet and main method blocks should end up adjacent to one another with no interleaving. I thought I had added an assert checking that but looks like I didn't. There is a bunch of special casing in |
|
Is this vs main or picking up some of the in-progress PRs (in particular async?) Async adds in some new flow cases that makes layout more complicated (we can branch from method entry into the middle of various things like loops and trys). |
|
Superseded by #131279, which fixes the root cause in wasm block layout rather than the symptom here. This PR hardened the terminal- Closing in favor of #131279. Note This comment was authored with the assistance of GitHub Copilot. |
…#131279) [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 `end` opcode (0x0b) is dropped for an EH funclet, so `wasm-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 if ```cpp comp->bbInTryRegions(key.RegionIndex(), block) ``` is true. `bbInTryRegions` is 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 terminal `end` and corrupts its branch target depths. Concrete trace from `System.Data.DataColumn:set_Expression` (instrumented `VisitWasmSuccs`, deduped): ``` sideEntry BB50 (tryIdx=4 hndIdx=3) preds[async=0 catch=1 other=2] pulls dst BB76 (hndIdx=0) crossFunclet=1 sideEntry BB73 (tryIdx=4 hndIdx=3) preds[async=0 catch=1 other=0] pulls dst BB76 (hndIdx=0) crossFunclet=1 ``` 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, and `fgwasm.h` is byte-identical to `main`. It is latent on `main` only because R2R-wasm codegen isn't enabled there yet. ## Fix Gate the enclosing-try relaxation on same-function-region ownership. A new `funcRegionOf` lambda returns the funclet index that physically contains an arbitrary block (0 == main method); it mirrors `funGetFuncIdx` but 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: ```cpp if (!viaEnclosingTry || (funcRegionOf(block) == funcRegionOf(acd->acdDstBlk))) { RETURN_ON_ABORT(func(acd->acdDstBlk)); } ``` 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.Common` R2R wasm crossgen, release JIT, `wasm-tools validate` (only `fgwasm.h` differs between runs): | | `wasm-tools validate` | |---|---| | baseline (`origin/main` layout) | `func 597 failed to validate` ❌ | | with this fix | passes ✅ | ## Notes - Supersedes #131251, which hardened the terminal-`end` emission (the *effect*); this fixes the *cause* in layout. Closing #131251 in favor of this. - Related: #129335 (incomplete predecessor for this defect class, do not reopen); #130945 (introduced the lexical enclosing-try match); #131252 (miscomputed branch target depth — same corrupted layout, very likely subsumed by this fix). - Follow-up idea (not in this PR): a JIT-time assert that a funclet's blocks form a contiguous RPO range, so any future mis-layout traps at compile time instead of surfacing as an invalid module. > [!NOTE] > This change was authored with the assistance of GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2e627b94-2658-41ce-a880-d9c27b7febd9
Summary
When crossgen2 compiles managed methods to WebAssembly (wasm32) R2R, it emits a malformed wasm function body for certain exception-handling (EH) funclets: the required terminal
endopcode (0x0b) is dropped. The resulting module fails validation — both V8 andwasm-tools validatereject it withfunction body must end with end opcode. A wasm function body is structurally required to end inendregardless of whether the last instruction is reachable, so dropping it produces invalid wasm.The defect is in the shared
TARGET_WASMcodegen path (no OS-specific branching), so it reproduces identically for both--targetos:browserand--targetos:wasi, and the fix benefits both.Repro / observed
Compiling
System.Data.Common.dllto wasm R2R produces two malformed EH funclets, both property setters:System.Data.DataColumn.set_ExpressionSystem.Data.DataTable.set_LocaleBoth are EH funclets (they have an
exnreflocal —try_table/catch_refEH) whose last block ends in a no-return tail (call_indirect (no-return)+unreachable), after which the terminal0x0bis missing. This is not SIMD-related: the two funclets contain zero0xfd-prefixed opcodes, and the composite is byte-identical whether SIMD is forced on or bailed (--codegenopt:JitWasmSimdNyiToR2RUnsupported=0vs=1). It reproduces identically on browser and WASI (same funclet, same byte offset), confirming it is OS-agnostic.Root cause
The wasm end-emission guard
block->IsLast() || bbIsFuncletBeg(block->Next())uses the global block order (bbNext), in which a funclet's last block can be followed by an interspersed root throw-helper block (fgIsThrowHlpBlk, no handler index) that is not a funclet begin. Codegen iterates per funclet (genCodeForFuncletoverfuncInfo->Blocks()), so the guard fails to recognize the funclet boundary and the terminalendis never emitted.This same unsound predicate was duplicated across four wasm end-emission sites:
BBJ_THROWandBBJ_ALWAYSarms ofgenEmitEndBlock(codegenlinear.cpp),BBJ_CALLFINALLYpath ingenCallFinally(codegenwasm.cpp, added by Wasm R2R: emit trailing end after retless BBJ_CALLFINALLY #129449), andend/returndiscriminator ingenFuncletEpilog(codegenwasm.cpp).The copy-paste drift across these sites is what let this gap — and the incompleteness of the prior fix #129335 — arise.
Fix
Consolidate the predicate into a single wasm helper
CodeGen::genIsLastBlockOfCurrentFuncthat also compares against the current func/funclet's authoritative last block (funCurrentFunc()->GetLastBlock()), and route all four end-emission sites through it:Correct across every funclet kind, by construction.
FuncInfoDsc::GetLastBlockresolves tofgLastBBInMainFunction()forFUNC_ROOT,ehDsc->BBFilterLast()forFUNC_FILTER, andehDsc->ebdHndLastforFUNC_HANDLER(which covers catch/finally/fault handlers alike). Crucially, it is the exact upper bound of the per-funclet codegen iteration range —FuncInfoDsc::Blocks()is defined asBasicBlockRangeList(GetStartBlock(comp), GetLastBlock(comp))— so the new term recognizes precisely the last block codegen will process for the current func/funclet, for every kind, not just the twoBBJ_THROWcases the repro exercises.The added term is an additive OR, so each block still emits at most one terminal
end(no double-endregression on theBBJ_RETURN/ funclet-epilog paths), and because it is authoritative it cannot false-positive on mid-funclet blocks. Routing all four sites through one predicate removes the drift that caused this class of bug.This is not a mirror of the non-wasm
DOES_NOT_RETURNelse-clause (which emits anINS_BREAKPOINTfor GC liveness) — on wasm the correct behavior is to still emit the function body's terminalend.Validation
Recrossgenned standalone
System.Data.Common→ wasm R2R:end.wasm-tools validate: passes (was failing at func 597 pre-fix).endregression — validation rules out both a missingend("control frames remain") and an extraendacross all functions in the module.--targetos:browserand--targetos:wasi: unfixed crossgen2 fails validation at the same funclet (func 597, offset0xdb1ae) on both targets; the fixed build passeswasm-tools validateon both.System.Data.Commonfixture (same func 597).Related sibling defect (separate root cause, tracked separately)
The same wasm crossgen surfaces a second, independent EH-funclet codegen defect this PR does not address, tracked as #131252:
CodeGen::findTargetDepth(codegenwasm.cpp) asserts inDEBUGwhen a branch target isn't found in the control-flow stack, but release falls through toreturn ~0;, shipping0xFFFFFFFFas a branch depth (e.g. V8:invalid branch depth: 4294967295). Observed onSystem.Threading.Tasks.Parallel.Invoke_0funclet 1. Distinct root cause (branch-target resolution, not end-emission) and not fixable by the same predicate; both share the pattern of an EH-funclet codegen gap caught only by a checked-only assert that is compiled out in release.Related issues
genCallFinallysite that shares this predicate.Note
This PR was created with the help of GitHub Copilot.