[wasm][R2R] Map state machine MoveNext method ids to virtual IPs - #134754
Conversation
RuntimeMethodHandle_GetNativeCode returned the portable entry point for Wasm R2R methods, which holds a function-table index that EECodeInfo cannot resolve. Share the EventPipe mapping to the synthetic virtual IP via a new GetDiagnosticCodeStartFromEntryPoint helper, and re-enable the V1 async profiler callstack tests. Fixes #134145 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
GetActualCode only reads a field, but declared STANDARD_VM_CONTRACT (THROWS/GC_TRIGGERS/MODE_PREEMPTIVE). Its callers include the NOTHROW/GC_NOTRIGGER GetDiagnosticCodeStartFromEntryPoint and the cooperative-mode interpreter, so use LIMITED_METHOD_CONTRACT to match HasNativeEntryPoint and SetActualCode. Also drop a redundant null check. 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. |
|
Tagging subscribers to this area: @agocke |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The changes affect CoreCLR runtime, Wasm ReadyToRun diagnostics, and contract-sensitive paths requiring final human review.
Review effort: Lite
Findings: None
What changed in this PR
Maps Wasm ReadyToRun portable entry points to diagnostic virtual IPs, restoring async profiler frame resolution.
Changes:
- Shares diagnostic mapping between EventPipe and runtime method handles.
- Adjusts portable entry-point contract usage.
- Re-enables three Wasm ReadyToRun profiler tests.
| File | Description |
|---|---|
src/libraries/System.Runtime/tests/System.Threading.Tasks.Tests/System.Runtime.CompilerServices/AsyncProfilerV1Tests.cs |
Removes obsolete issue suppressions. |
src/coreclr/vm/runtimehandles.cpp |
Applies diagnostic address mapping. |
src/coreclr/vm/precode.h |
Declares the shared helper. |
src/coreclr/vm/precode.cpp |
Implements Wasm virtual-IP mapping. |
src/coreclr/vm/precode_portable.cpp |
Relaxes the accessor contract. |
src/coreclr/vm/eventtrace.cpp |
Reuses the shared mapping helper. |
|
cc @dotnet/wasm-contrib |
davidwrighton
left a comment
There was a problem hiding this comment.
This change looks like it would work correctly, but leaves a bad name in the BCL. Please fix the naming of the functions which have now changed in meaning with this PR.
The value returned is only meaningful for diagnostic reporting (on Wasm it may be interpreter bytecode or a synthetic virtual IP), so name it accordingly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The Mono fallback in AsyncProfilerTests looks up the wrapper by name via reflection. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…rtual IPs (#134756) > [!IMPORTANT] > Stacked on #134754 (targets its branch). Only the commits above #134754's head belong to this PR; retarget to `main` once #134754 merges. ## Problem On `FEATURE_PORTABLE_ENTRYPOINTS` targets (WebAssembly), a method's entry point slot holds a `PortableEntryPoint` address. Native maps it before handing it to diagnostics (`GetInterpreterCodeFromEntryPointIfPresent` / `GetDiagnosticCodeStartFromEntryPoint` in `precode.cpp`), but the cDAC returned the raw address. SOS values such as `DacpMethodDescData.NativeCodeAddr` and rejit `NativeCodeAddr`, plus the DBI/`ClrDataMethodInstance` paths, therefore reported an address that doesn't resolve through `ExecutionManager` for both interpreted and R2R methods. ## Change Adds `IExecutionManager.GetDiagnosticCodeStartFromEntryPoint`, which mirrors the native `GetDiagnosticCodeStartFromEntryPoint`: - **Without portable entrypoints:** delegates to `PrecodeStubs.GetInterpreterCodeFromInterpreterPrecodeIfPresent`. Behavior is unchanged. - **With portable entrypoints:** 1. Returns the address unchanged if it lies in a code range. This is the native `FindCodeRange` check and includes Wasm R2R virtual-IP ranges from #133917. 2. Maps interpreted methods to `MethodDesc::m_interpreterCode`. 3. Maps native R2R methods from the function-table index in `PortableEntryPoint._pActualCode` to the synthetic virtual IP, stepping back past funclet entries. This matches `ExecutionManager::GetWasmVirtualIPFromFunctionTableIndex`. As in native code, it applies only to the method's own (temporary) entry point, and only when the entry point doesn't prefer the interpreter. The mapping lives in ExecutionManager rather than PrecodeStubs because ExecutionManager owns the virtual-IP ranges and R2R lookup. ExecutionManager already depends on PrecodeStubs, so this adds no contract cycle. Function-table-index resolution moves into `ExecutionManagerHelpers.WasmFunctionTableIndexLookup`, which now bounds the list walk and detects cycles, like the virtual-IP list walk. The stack walk's `WasmR2RInfo` becomes a thin wrapper over it. The Legacy SOS/DBI callers (`SOSDacImpl`, `ClrDataMethodInstance`, `DacDbiImpl`) now call the new API. `DacDbiImpl.EnumerateAsyncLocals` also maps its code address before querying async debug info. The native DAC's `EnumerateAsyncLocals` gets the matching `GetInterpreterCodeFromEntryPointIfPresent` mapping (as `GetMethodVarInfo` already does), so the debug-build cDAC/DAC cross-check stays consistent. ### Data descriptors - `PortableEntryPoint.ActualCode` and `PortableEntryPoint.Flags` (only under `FEATURE_PORTABLE_ENTRYPOINTS`). - `MethodDesc.InterpreterCode` (only under `FEATURE_INTERPRETER`). - The contract relies on `kPrefersInterpreterEntryPoint` and `INTERPRETER_CODE_POISON`; the native side now has `[cDAC]` comments marking that dependency. ### Interaction with #133890 #133890 adds `TryGetFunctionIdentity` and `TryIsFunclet` to `WasmR2RInfo`. Whichever PR lands second should add those two methods to `WasmFunctionTableIndexLookup` and have `WasmR2RInfo` forward to them. The `FunctionTableIndexRange*` descriptor meanings here already use #133890's exact wording, so that JSON should merge cleanly. ## Validation - cDAC: `./build.sh -s tools.cdac+tools.cdactests -c Debug -test` passes: 3196 unit tests (17 new), usage tests (contract cycles and generated docs up to date), and generator tests. - New ExecutionManager tests cover: - interpreted, R2R, funclet, poison, prefers-interpreter, not-own-entry-point and unknown-index cases; - an end-to-end check that the resolved virtual IP maps back to the MethodDesc through `GetCodeBlockHandle`; - readable `PortableEntryPoint`-shaped bytes inside a registered code range staying unchanged. This test fails if the range check is removed. - a cyclic function-table range list; - the non-portable delegation path. - CoreCLR `clr.runtime` builds for osx-arm64 Debug (with `FEATURE_INTERPRETER`) and browser-wasm Debug. The generated wasm descriptor has `PortableEntryPoint {ActualCode@0, MethodDesc@4, Flags@12}`. - A new `DacDbiImplTests` test covers `EnumerateAsyncLocals` mapping for both the code-address and MethodDesc paths. It fails without the fix. - cDAC dump tests (osx-arm64, local runtime): 260 passed, 0 failed. The 746 skips are net10.0 configurations plus by-design skips (Windows-only COM debuggees, and dump types a debuggee doesn't produce). - CoreCLR `clr.runtime` Release (osx-arm64) builds with the `dacdbiimpl.cpp` change. - Not run: any check against a live wasm target. Fixes #134753 > [!NOTE] > This PR description was generated with GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…rtual IPs (#134827) This replaces #134756, which was merged into #134754's branch by mistake and reverted there. The change is otherwise identical (cherry-picked onto `main`). ## Problem On `FEATURE_PORTABLE_ENTRYPOINTS` targets (WebAssembly), a method's entry point slot holds a `PortableEntryPoint` address. Native maps it before handing it to diagnostics (`GetInterpreterCodeFromEntryPointIfPresent` / `GetDiagnosticCodeStartFromEntryPoint` in `precode.cpp`), but the cDAC returned the raw address. SOS values such as `DacpMethodDescData.NativeCodeAddr` and rejit `NativeCodeAddr`, plus the DBI/`ClrDataMethodInstance` paths, therefore reported an address that doesn't resolve through `ExecutionManager` for both interpreted and R2R methods. ## Change Adds `IExecutionManager.GetDiagnosticCodeStartFromEntryPoint`, which mirrors the native `GetDiagnosticCodeStartFromEntryPoint`: - **Without portable entrypoints:** delegates to `PrecodeStubs.GetInterpreterCodeFromInterpreterPrecodeIfPresent`. Behavior is unchanged. - **With portable entrypoints:** 1. Returns the address unchanged if it lies in a code range. This is the native `FindCodeRange` check and includes Wasm R2R virtual-IP ranges from #133917. 2. Maps interpreted methods to `MethodDesc::m_interpreterCode`. 3. Maps native R2R methods from the function-table index in `PortableEntryPoint._pActualCode` to the synthetic virtual IP, stepping back past funclet entries. This matches `ExecutionManager::GetWasmVirtualIPFromFunctionTableIndex`. As in native code, it applies only to the method's own (temporary) entry point, and only when the entry point doesn't prefer the interpreter. The mapping lives in ExecutionManager rather than PrecodeStubs because ExecutionManager owns the virtual-IP ranges and R2R lookup. ExecutionManager already depends on PrecodeStubs, so this adds no contract cycle. Function-table-index resolution moves into `ExecutionManagerHelpers.WasmFunctionTableIndexLookup`, which now bounds the list walk and detects cycles, like the virtual-IP list walk. The stack walk's `WasmR2RInfo` becomes a thin wrapper over it. The Legacy SOS/DBI callers (`SOSDacImpl`, `ClrDataMethodInstance`, `DacDbiImpl`) now call the new API. `DacDbiImpl.EnumerateAsyncLocals` also maps its code address before querying async debug info. The native DAC's `EnumerateAsyncLocals` gets the matching `GetInterpreterCodeFromEntryPointIfPresent` mapping (as `GetMethodVarInfo` already does), so the debug-build cDAC/DAC cross-check stays consistent. ### Data descriptors - `PortableEntryPoint.ActualCode` and `PortableEntryPoint.Flags` (only under `FEATURE_PORTABLE_ENTRYPOINTS`). - `MethodDesc.InterpreterCode` (only under `FEATURE_INTERPRETER`). - The contract relies on `kPrefersInterpreterEntryPoint` and `INTERPRETER_CODE_POISON`; the native side now has `[cDAC]` comments marking that dependency. ### Interaction with #133890 #133890 adds `TryGetFunctionIdentity` and `TryIsFunclet` to `WasmR2RInfo`. Whichever PR lands second should add those two methods to `WasmFunctionTableIndexLookup` and have `WasmR2RInfo` forward to them. The `FunctionTableIndexRange*` descriptor meanings here already use #133890's exact wording, so that JSON should merge cleanly. ## Validation - cDAC: `./build.sh -s tools.cdac+tools.cdactests -c Debug -test` passes: 3196 unit tests (17 new), usage tests (contract cycles and generated docs up to date), and generator tests. - New ExecutionManager tests cover: - interpreted, R2R, funclet, poison, prefers-interpreter, not-own-entry-point and unknown-index cases; - an end-to-end check that the resolved virtual IP maps back to the MethodDesc through `GetCodeBlockHandle`; - readable `PortableEntryPoint`-shaped bytes inside a registered code range staying unchanged. This test fails if the range check is removed. - a cyclic function-table range list; - the non-portable delegation path. - CoreCLR `clr.runtime` builds for osx-arm64 Debug (with `FEATURE_INTERPRETER`) and browser-wasm Debug. The generated wasm descriptor has `PortableEntryPoint {ActualCode@0, MethodDesc@4, Flags@12}`. - A new `DacDbiImplTests` test covers `EnumerateAsyncLocals` mapping for both the code-address and MethodDesc paths. It fails without the fix. - cDAC dump tests (osx-arm64, local runtime): 260 passed, 0 failed. The 746 skips are net10.0 configurations plus by-design skips (Windows-only COM debuggees, and dump types a debuggee doesn't produce). - CoreCLR `clr.runtime` Release (osx-arm64) builds with the `dacdbiimpl.cpp` change. - Re-validated after cherry-picking onto `main`: `./build.sh clr -c Debug` (osx-arm64) builds, and `./build.sh -s tools.cdac+tools.cdactests -c Debug -test` passes (3196 unit tests, 46 generator tests, 4 usage tests; 0 failed). - Not run: any check against a live wasm target. Resolves #134753 > [!NOTE] > This PR description was generated with GitHub Copilot. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
State-machine (V1) async profiler callstack frames identify methods by
AsyncStateMachineDiagnostics<T>.MethodId, which comes fromRuntimeMethodHandle_GetNativeCodeonMoveNext. On Wasm ReadyToRun, a method's entry point is aPortableEntryPointwhose actual code is a function-table index.EECodeInfocan't resolve that index, soStackFrame.GetMethodFromNativeIPreturned nothing and the frame names were lost.EventPipe method events already mapped these entry points to the synthetic virtual IP registered through
ExecutionManager::AddVirtualIPRange(#134026). This PR moves that mapping into a sharedGetDiagnosticCodeStartFromEntryPointhelper inprecode.cpp, used by both EventPipe andRuntimeMethodHandle_GetNativeCode. The only managed caller of that QCall isAsyncStateMachineDiagnostics.TARGET_WASMandFEATURE_PORTABLE_ENTRYPOINTS. Treating the actual code as a function-table index is a Wasm fact, andGetWasmVirtualIPFromFunctionTableIndexis declared only underTARGET_WASM. Other portable-entrypoint targets would store a real code address thatEECodeInfoalready resolves.PortableEntryPoint::GetActualCodegoes fromSTANDARD_VM_CONTRACTtoLIMITED_METHOD_CONTRACT. It only reads a field, and its callers include thisNOTHROW/GC_NOTRIGGERhelper and the cooperative-mode interpreter. That matches its siblingsHasNativeEntryPointandSetActualCode.ActiveIssueannotations for [wasm][R2R] StateMachineAsync profiler callstack identities do not resolve #134145 are removed.Validation
Browser CoreCLR, Release,
TestWasmReadyToRun=true,EnableAggressiveTrimming=true, Chrome:StateMachineAsync_*_ChainEventsAndCallstacktests: 3/3 pass. With only theruntimehandles.cppchange reverted and the runtime rebuilt: 0/3.AsyncProfilerTestsclass: 136 run, 61 passed, 0 failed, 75 skipped by existing platform conditions.Not validated: a Debug/Checked browser build, where contracts are enforced. Non-Wasm builds compile the new mapping out.
Follow-up
The cDAC has the same gap for SOS native code addresses on portable-entrypoint targets: #134753.
Resolves #134145
Note
This pull request description was generated with GitHub Copilot.