[wasm] Fix caller identification across R2R prestub transitions - #133707
Merged
Merged
Conversation
Exclude completed non-FCall prestub activations from function-only stack walks while retaining their GC roots and physical unwind state. Replace the StackTrace-specific duplicate filter and extend caller, recursion, and GC regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @agocke |
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Add cold/warm caller-identity or stack-trace coverage for native and managed FCall cases.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
src/coreclr/vm/prestub.cpp — Cover the FCall prestub exception View comment |
What changed in this PR
Fixes Wasm CoreCLR caller identification across retained R2R prestub transitions.
Changes:
- Marks completed non-FCall prestub transitions as non-managed activations.
- Filters these frames from function-only stack walks.
- Expands regression coverage for caller identity, recursion, exceptions, and GC-sensitive arguments.
- Removes the obsolete debugger-specific duplicate-frame filter.
Review note: FCall-specific lifecycle behavior lacks direct regression coverage.
| File | Summary |
|---|---|
src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github60486.cs |
Expands caller-identity and GC-root regression coverage. |
src/coreclr/vm/stackwalk.cpp |
Centrally filters completed transition frames. |
src/coreclr/vm/prestub.cpp |
Marks completed non-FCall Wasm prestub transitions. |
src/coreclr/vm/frames.h |
Adds prestub lifecycle state and activation attributes. |
src/coreclr/vm/debugdebugger.cpp |
Removes the obsolete adjacency-based filter. |
davidwrighton
approved these changes
Sep 11, 2026
Member
Author
|
/ba-g failures are #133747 |
jkotas
reviewed
Sep 12, 2026
jkotas
reviewed
Sep 12, 2026
lewing
added a commit
to lewing/runtime
that referenced
this pull request
Sep 12, 2026
Integrate main at a7fda7e, including dotnet#133707, without rewriting the draft branch history. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
lewing
added a commit
to lewing/runtime
that referenced
this pull request
Sep 12, 2026
Remove the nine dotnet#133617 ActiveIssue annotations now that dotnet#133707 has landed. Preserve every original GetCurrentMethod call and assertion and retain unrelated platform and trimming exclusions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
lewing
pushed a commit
that referenced
this pull request
Sep 14, 2026
## Summary This fixes #133798 Fix the two AOT failures exposed by the expanded `github60486` regression in #133707 and observed in CI for #133774. - Preserve callers of the stack-walk implementation of `MethodBase.GetCurrentMethod` during Mono LLVM inlining. The LLVM-only intrinsic already prevents inlining; the stack-walk path needs the same protection. The test's inlineable helper remains unannotated. - Allow unrelated NativeAOT stack frames to lack reflection metadata. A local diagnostic run identified `TestEntryPoint` as the frame returning null from `GetMethod`; all four recursive frames returned valid method metadata. Keep the exact four-frame assertion and all existing DIM, exception-stack, caller-identity, and GC-root checks. Related caller-inlining report: #60334. Its iOS configuration was not tested here. ## Validation Built Checked CoreCLR/NativeAOT and Release Mono with LLVM from a clean baseline in the same worktree. | Windows x64 configuration | Before | After | |---|---|---| | NativeAOT Checked | Reported null-reference failure | Pass | | Mono LLVM AOT Release | Expected `GetCurrentMethodInlineable`, actual `ValidateCurrentMethod` | Pass | | CoreCLR Checked | Pass | Pass | | Mono MiniJIT Release | Pass | Pass | Each run executes the full standalone `github60486` regression. Mono LLVM AOT comparisons use identical test IL, and runtime logging confirms the fixed AOT image is loaded. LLVM IR adds `noinline` to the inlineable and generic wrappers; the optimized caller retains the helper call instead of inlining it. No performance claim is made. The Windows Mono LLVM AOT reproduction matches the Linux CI failure signature. Linux and browser Wasm were not run locally. The Windows x64 JIT formatting check completed successfully. > [!NOTE] > This change and pull request description were developed with GitHub Copilot assistance. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
lewing
added a commit
that referenced
this pull request
Sep 15, 2026
## Summary Follow up on review feedback received after #133707 merged: - Mark every completed portable prestub transition as no longer representing an additional managed activation, including FCalls. - Scope the lifecycle state and function-only stack-walk filtering to `FEATURE_PORTABLE_ENTRYPOINTS` instead of `TARGET_WASM`. - Add cold/warm coverage for a string constructor backed by a managed FCall. The test verifies that the first invocation does not expose an extra constructor frame that disappears after the entrypoint is published. The prestub frame remains active for argument rooting and physical unwinding while the resolved R2R body runs. ## Validation Built browser-wasm CoreCLR Checked with Release libraries. The focused `github60486` regression was compiled to ReadyToRun and run against identical test output: | Runtime | Cold constructor frames | Warm constructor frames | Result | |---|---:|---:|---| | Before | 1 | 0 | Failed | | After | 0 | 0 | Passed | The full regression also passes with `DOTNET_ReadyToRun=0`, and a native SDK-runtime smoke run passes. This intentionally does not address the separate Mono aggressive-inlining concern raised on #133775; that requires validation in both MiniJIT and LLVM configurations. > [!NOTE] > This change and pull request description were developed with GitHub Copilot assistance. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
lewing
added a commit
to lewing/runtime
that referenced
this pull request
Sep 17, 2026
Remove the nine dotnet#133617 ActiveIssue annotations now that dotnet#133707 has landed. Preserve every original GetCurrentMethod call and assertion and retain unrelated platform and trimming exclusions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
jtschuster
pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 18, 2026
) ## Summary This fixes dotnet#133798 Fix the two AOT failures exposed by the expanded `github60486` regression in dotnet#133707 and observed in CI for dotnet#133774. - Preserve callers of the stack-walk implementation of `MethodBase.GetCurrentMethod` during Mono LLVM inlining. The LLVM-only intrinsic already prevents inlining; the stack-walk path needs the same protection. The test's inlineable helper remains unannotated. - Allow unrelated NativeAOT stack frames to lack reflection metadata. A local diagnostic run identified `TestEntryPoint` as the frame returning null from `GetMethod`; all four recursive frames returned valid method metadata. Keep the exact four-frame assertion and all existing DIM, exception-stack, caller-identity, and GC-root checks. Related caller-inlining report: dotnet#60334. Its iOS configuration was not tested here. ## Validation Built Checked CoreCLR/NativeAOT and Release Mono with LLVM from a clean baseline in the same worktree. | Windows x64 configuration | Before | After | |---|---|---| | NativeAOT Checked | Reported null-reference failure | Pass | | Mono LLVM AOT Release | Expected `GetCurrentMethodInlineable`, actual `ValidateCurrentMethod` | Pass | | CoreCLR Checked | Pass | Pass | | Mono MiniJIT Release | Pass | Pass | Each run executes the full standalone `github60486` regression. Mono LLVM AOT comparisons use identical test IL, and runtime logging confirms the fixed AOT image is loaded. LLVM IR adds `noinline` to the inlineable and generic wrappers; the optimized caller retains the helper call instead of inlining it. No performance claim is made. The Windows Mono LLVM AOT reproduction matches the Linux CI failure signature. Linux and browser Wasm were not run locally. The Windows x64 JIT formatting check completed successfully. > [!NOTE] > This change and pull request description were developed with GitHub Copilot assistance. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
jtschuster
pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 18, 2026
## Summary Follow up on review feedback received after dotnet#133707 merged: - Mark every completed portable prestub transition as no longer representing an additional managed activation, including FCalls. - Scope the lifecycle state and function-only stack-walk filtering to `FEATURE_PORTABLE_ENTRYPOINTS` instead of `TARGET_WASM`. - Add cold/warm coverage for a string constructor backed by a managed FCall. The test verifies that the first invocation does not expose an extra constructor frame that disappears after the entrypoint is published. The prestub frame remains active for argument rooting and physical unwinding while the resolved R2R body runs. ## Validation Built browser-wasm CoreCLR Checked with Release libraries. The focused `github60486` regression was compiled to ReadyToRun and run against identical test output: | Runtime | Cold constructor frames | Warm constructor frames | Result | |---|---:|---:|---| | Before | 1 | 0 | Failed | | After | 0 | 0 | Passed | The full regression also passes with `DOTNET_ReadyToRun=0`, and a native SDK-runtime smoke run passes. This intentionally does not address the separate Mono aggressive-inlining concern raised on dotnet#133775; that requires validation in both MiniJIT and LLVM configurations. > [!NOTE] > This change and pull request description were developed with GitHub Copilot assistance. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #133617.
Problem
The Wasm portable-entrypoint slow path retains a
PrestubMethodFramewhile invoking a resolved R2R body. Function-only stack walks can report both the real managed activation and that transition frame.MethodBase.GetCurrentMethodcan therefore accept its own prestub frame as the caller.A Checked-runtime capture confirmed this: the stack marker was
0x4fe71c; the realGetCurrentMethodbody at SP0x4fe710did not pass the caller-marker comparison, but its explicitPrestubMethodFrameat SP0x4fe720did. The result wasGetCurrentMethodinstead of the calling method. This also reproduces without trimming.#133610 addressed the duplicate in
GetStackFramesCallback, but caller-sensitive reflection uses a different callback and did not benefit from that filter.Change
FUNCTIONSONLYwalks, covering both ordinary and skipped explicit frames.github60486regression with direct/inlineable/generic caller identity, assembly lookup, recursive frame counts, and compacting-GC object/byref/struct argument coverage. Its original DIM frame-sequence and exception-stack checks remain intact.Native FCalls retain their existing representation. Non-Wasm filtering behavior is unchanged.
Validation
CoreCLR runtime regression
Built browser-Wasm and macOS arm64 Checked runtimes with Release libraries. The expanded
Loader/classloader/DefaultInterfaceMethods/regressions/github60486passes with fixed Wasm R2R, the interpreter, and native arm64. The identical final regression assembly fails against the saved original Wasm runtime: expectedValidateCurrentMethod, actualGetCurrentMethod.These fixed runs include the original regression checks from #133610, with its
debugdebugger.cppfilter removed.Original browser library lane
Validated the exact patch in an isolated checkout of #133656 at
282d12dfe220a1f34d0bb81a85239cab60c779ce, which supplies the browser R2R publishing/test prerequisites. Built matched original/fixed CoreCLR Release products locally and ran Chromium 153. Temporarily enabled only the nine #133617-quarantined methods, preserving every original assertion and unrelated exclusion; all annotations were restored afterward. This PR contains no library-test quarantine changes.The affected-class fixed run also passed: 23 passed, 3 existing skips.
CrossAssembly2remains excluded when trimmed by dotnet/linker#2078 and passes untrimmed; it is not counted as a trimmed pass.Main test IL and main R2R images are byte-identical across each original/fixed pair; skip sets are identical. Served assets were verified against their respective rebuilt packs. This is a matched-product A/B, not a native-module-only swap: CoreLib R2R image bytes also differ.
Existing reflection compatibility controls
System.Reflection.Testscaller-assembly theory, real cross-assembly delegate invocation, static-constructor caller lookup, executing-assembly lookup, and reflected first/second invocation all pass: six cases on each original/fixed, trimmed/untrimmed browser R2R configuration. Fresh-process delegate and reflective-invocation probes also pass in all four configurations. All existing annotations remained intact. These are compatibility controls, not failing-before reproductions.Full browser reflection suites have identical original/fixed results:
Existing failures are
GetEntryAssembly(browser host expectation versus R2R expectation) and, trimmed only,AssemblyGetForwardedTypesLoadFailure(underlying trimming cause not isolated). Failure signatures and skip sets match before/after; neither was suppressed.Native arm64 Checked reflection control: six focused cases passed; full suite 1,776 passed, 2 platform skips, zero failures.
Remaining scope
Draft for review of the frame-lifecycle distinction and FCall exception. Dedicated FCall cold/warm stack-identity coverage, a broader CoreCLR GC-stress campaign, and other browsers have not been run. No performance claim is made.
Note
This change and pull request description were developed with GitHub Copilot assistance.