[cDAC] WebAssembly support: stack walking and managed metadata resolution#130988
Merged
Conversation
WebAssembly has no native register context: DT_CONTEXT is an empty struct and REGDISPLAY is zeroed. Managed execution on wasm is fully interpreted, so a managed stack walk is a pointer-chase over the interpreter frame chain rather than a register-based unwind. Add a minimal WasmContext (IPlatformContext) so IPlatformAgnosticContext.GetContextForPlatform resolves for wasm targets instead of throwing. The IP/SP/FP slots are synthetic (populated by the interpreter frame-chain walker, not read from a native context blob, hence Size == 0), and register-unwind operations are intentionally unsupported. This unblocks the stack-walk entry point for wasm. The context-free contracts (RuntimeInfo, Object, RuntimeTypeSystem, Loader, Thread, GC) do not depend on this and are reachable via memory reads alone. Contributes to dotnet#120646. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 272b3faa-2ba5-4287-bc0a-753b59d2d3d2
The stub ContractDescriptor computed its flags with a bitwise-AND where
an OR was intended:
.flags = 0x1u & (sizeof(void*) == 4 ? 0x02u : 0x00u)
This evaluates to 0x00 in every case (0x1 & 0x02 == 0 on 32-bit,
0x1 & 0x00 == 0 on 64-bit), so the stub never set the mandatory low bit
(bit 0, always 1 per the contract-descriptor spec) and always masked
away the pointer-size bit (bit 1).
Use OR to match the real descriptor's computation in datadescriptor.cpp:
.flags = 0x1u | (sizeof(void*) == 4 ? 0x02u : 0x00u)
The stub is only linked when CDAC_BUILD_TOOL_BINARY_PATH is unset (builds
without a .NET SDK) and carries an empty descriptor, so impact is limited,
but a consumer inspecting the flags would previously see an invalid
descriptor and the wrong pointer size.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 272b3faa-2ba5-4287-bc0a-753b59d2d3d2
CoreCLR on WebAssembly has no native register context (empty DT_CONTEXT, zeroed REGDISPLAY), so the register-driven CreateStackWalk path cannot run. But managed execution on wasm is fully interpreted, and the interpreter maintains an explicit in-memory frame chain (InterpreterFrame.TopInterpMethodContextFrame -> InterpMethodContextFrame .pParent), so a managed stack walk is a pointer-chase rather than an unwind. Add IStackWalk.GetInterpretedFrames(threadPointer): it walks the Thread's explicit Frame chain and, for each InterpreterFrame, expands the InterpMethodContextFrame chain (reusing the existing WalkInterpreterFrameChain head-resolution), resolving each frame's MethodDesc via StartIp -> InterpByteCodeStart.Method -> InterpMethod.MethodDesc. It is context-free (pure memory reads) and makes no GetContextForPlatform / Unwind calls, so it works on interpreter-only targets such as wasm. The existing GetFrames (explicit Frame-chain enumeration, consumed by DacDbi internal-frame reporting) is intentionally left unchanged. Also adds the InterpretedFrameData record, a FrameHelpers helper for the StartIp -> MethodDesc resolution, mock infrastructure for the interpreter frame chain, and unit tests. Fixes an incorrect <paramref> name on MockFrameBuilder.LinkChain. Contributes to dotnet#120646. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 272b3faa-2ba5-4287-bc0a-753b59d2d3d2
Rubber-duck review noted WasmContext.Size returned 0 while ContextHolder.GetBytes() serializes the full 12-byte synthetic struct, so a consumer that allocates Size bytes and one that reads GetBytes() would disagree. Return the actual synthetic-context size (three 32-bit IP/SP/FP slots) so Size and GetBytes() are consistent. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 272b3faa-2ba5-4287-bc0a-753b59d2d3d2
CoreCLR on WASM will use ReadyToRun for acceptable perf, so a real stack is always a mix of R2R and interpreter frames. R2R frames are walked over the managed linear stack ($sp) via a frameSize-based virtual unwind with synthetic "virtual IPs" (there is no register context and code is not in linear memory). Add WasmUnwinder, the cDAC mirror of the native WASM R2R stack walk in src/coreclr/vm/wasm/helpers.cpp (and the ABI in clr-abi.md): - TryGetFramePointer: NormalizeFrameBase, incl. localloc indirection (STACK_WALK_INDIRECT_TO_FRAMEPOINTER) and the TERMINATE_R2R_STACK_WALK boundary where the walk hands off to the Frame / interpreter chain. - GetVirtualIP: base virtual IP + function-local virtual IP (stored /2). - GetEstablishingFramePointerFromTerminator: recovers the establishing frame pointer stored beside the terminator by CallFunclet* (funclet EH). - TryUnwindOneFrame: advance $sp by the ULEB128 fixed frame size and produce the caller's virtual IP (mirrors WasmUnwindStackFrameCore). The ExecutionManager-backed lookups (virtual-IP base, funclet check, per-function unwind data) are abstracted behind IWasmR2RInfo; they are implemented against the cDAC ExecutionManager once the WASM R2R virtual-IP data descriptors are emitted. Two-pass EH / funclet reporting is then inherited from the existing architecture-agnostic EH stackwalk (IsFunclet / FindParentStackFrame operate on CodeBlockHandle, not registers) once virtual IPs resolve to code blocks. Adds unit tests covering frame-base normalization (floor, localloc indirection, terminator), establishing-FP recovery, virtual IP resolution, and single/multi-byte ULEB128 frame-size unwinding. Contributes to dotnet#120646. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 272b3faa-2ba5-4287-bc0a-753b59d2d3d2
…dFrames)" This reverts commit e01be9e. On WASM, ReadyToRun is required for acceptable perf even in developer builds, so a real managed stack is always a mix of R2R and interpreter frames. A standalone, public "interpreter frames only" API therefore surfaces a partial stack that cannot position R2R frames in call order, and risks consumers building on a half-stack. The correct shape is a single interleaved walk (CreateStackWalk-style, driven by $sp), with the interpreter frame chain as one arm and the R2R virtual-IP unwind (see WasmUnwinder) as the other. The reusable interpreter primitives (FrameHelpers.WalkInterpreterFrameChain / ResolveTopInterpMethodContextFrame) remain available for that unified walk; the StartIp->MethodDesc helper and the public IStackWalk.GetInterpretedFrames surface are withdrawn until the interleaved walk lands so the contract is not committed prematurely. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 272b3faa-2ba5-4287-bc0a-753b59d2d3d2
Adds the descriptor data and reader needed to resolve a WASM R2R function
table index to its base virtual IP, funclet status, and unwind data -
backing the IWasmR2RInfo seam consumed by WasmUnwinder.
Descriptor emission (TARGET_WASM only):
- New FunctionTableIndexRangeSection type (MinFunctionTableIndex,
NumRuntimeFunctions, R2RModule, Next) + cdac_data in codeman.h.
- FunctionTableIndexRangeList global (ExecutionManager::
s_pFunctionTableIndexRangeList).
- MinVirtualIP field on ReadyToRunInfo (m_minVirtualIP).
Reader:
- WasmR2RInfo implements IWasmR2RInfo, mirroring ExecutionManager::
{FindFunctionTableIndexRangeSection, IsFuncletFunctionIndex,
GetWasmVirtualIPFromFunctionTableIndex} in codeman.cpp - including the
funclet backward-index-to-controlling-function walk and the
RUNTIME_FUNCTION funclet high-bit (0x80000000) - reusing the existing
RuntimeFunctionLookup / ReadyToRunInfo / Module infra.
Verified by a CoreCLR browser-wasm build: datadescriptor.cpp compiles
under TARGET_WASM and cdac-build-tool emits the type
(fields at offsets 0/4/8/12), the FunctionTableIndexRangeList pointer
global, and ReadyToRunInfo.MinVirtualIP into the contract descriptor -
matching the managed reader's field names and shapes.
Contributes to dotnet#120646.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 272b3faa-2ba5-4287-bc0a-753b59d2d3d2
WasmContext.Unwind previously threw. Now it advances the context by one ReadyToRun frame over the managed linear stack using WasmUnwinder + WasmR2RInfo: it reads StackPointer ($sp), unwinds one frame (nextSp + caller virtual IP), and updates StackPointer/InstructionPointer. When the R2R walk terminates (an interpreter transition or the stack top), StackPointer is set to null so the stack walker can fall back to the explicit Frame chain / interpreter frame chain. This makes WasmContext a real $sp-driven context (mirroring how X86Context.Unwind drives X86Unwinder) rather than a degenerate null-object. The full interleaved CreateStackWalk integration (per-thread $sp seed, virtual-IP -> method reverse mapping, interleaving with InterpreterVirtualUnwind) is the remaining step. Contributes to dotnet#120646. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 272b3faa-2ba5-4287-bc0a-753b59d2d3d2
WebAssembly has no native register context (DT_CONTEXT is empty and REGDISPLAY is zeroed), so the initial managed stack walk context is seeded from the explicit Frame chain rather than a captured thread context. The shared StackWalk_1 driver already walks the FrameChain to the innermost transition frame in GetContextFromFrames; wire the degenerate WasmContext into that path so seeding works on WASM: - Add WasmFrameHandler, routing ContextHolder<WasmContext> through the base frame handler. The base HandleInlinedCallFrame already reads InlinedCallFrame.CallSiteSP / CallerReturnAddress / CalleeSavedFP into the synthetic IP/SP/FP slots -- the common P/Invoke-boundary seeding path. Hijack frames (a debugger / GC-suspension concept) throw NotSupported. - Dispatch ContextHolder<WasmContext> to WasmFrameHandler in FrameHelpers.GetFrameHandler (previously threw for WASM). - Test the seam end-to-end through FrameHelpers.UpdateContextFromFrame, plus the mock-infra additions (InlinedCallFrame CallSiteSP / CalleeSavedFP) it needs. Everything flows through the shared StackWalk_1 driver and the existing IsInterpreterCode -> InterpreterVirtualUnwind path; no WASM-specific contract surface is added. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: afa9efc6-f596-41bd-9070-dc651db8b7b4
…on stash
The WASM WasmContext previously held only three synthetic IP/SP/FP slots,
which did not match the runtime's wasm T_CONTEXT (src/coreclr/pal/inc/pal.h,
HOST_WASM): { ContextFlags, InterpreterWalkFramePointer, InterpreterSP,
InterpreterFP, InterpreterIP } -- five 32-bit fields, 20 bytes. Because the
cDAC serializes this context to/from target memory (thread context and
*ExceptionFrame TargetContext blobs) and returns it to SOS, the managed
layout must match the native struct byte-for-byte.
- Rework WasmContext to mirror the native field order and size (Size is now
5 * sizeof(uint)), map StackPointer/InstructionPointer/FramePointer to the
Interpreter{SP,FP,IP} slots, and back RawContextFlags with ContextFlags.
- Expose the InterpreterWalkFramePointer slot as the synthetic first-argument
register the interpreter virtual unwind uses: WasmContext supports it in
Try{Set,Read}Register, GetFirstArgRegisterName returns it for WASM, and
WasmFrameHandler.HandleInlinedCallFrame stashes the InterpreterFrame address
there when a P/Invoke transition is directly below an InterpreterFrame --
matching native SetFirstArgReg (src/coreclr/vm/wasm/cgencpu.h) and the
per-architecture handlers.
- Unit-test the native layout / register round-trip and the
InlinedCallFrame-over-InterpreterFrame stash.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: afa9efc6-f596-41bd-9070-dc651db8b7b4
Add mock-target coverage for the interpreter side of a mixed R2R/interpreter WASM stack walk, exercising the real InterpreterVirtualUnwind path with a WasmContext: - Stepping a multi-node InterpMethodContextFrame chain: each unwind follows pParent and takes the parent frame's IP/SP/FP (mirrors native VirtualUnwindInterpreterCallFrame). - Graceful termination when the chain is exhausted and no owning InterpreterFrame is stashed in the synthetic first-argument register. This also guards the WASM first-argument-register wiring: the exhaustion path routes through GetFirstArgRegisterName, which returned NotSupported for WASM before it was mapped to InterpreterWalkFramePointer. Re-adds the MockInterpMethodContextFrame test descriptor and lets the shared CreateTarget register an IRuntimeInfo architecture for paths that consult it. The full IStackWalk.CreateStackWalk driver loop is validated end to end via dump/live targets (no architecture mock-tests it); these unit tests cover the WASM-specific interpreter virtual unwind that feeds it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: afa9efc6-f596-41bd-9070-dc651db8b7b4
On WASM a "code address" is a synthetic virtual IP (ExecutionManager::GetWasmVirtualIPFromStackPointer = base + function-local offset). R2R modules are registered in the RangeSectionMap by their virtual-IP range, so resolving a virtual IP to its MethodDesc uses the same generic RangeSection.Find -> ReadyToRunJitManager path as every other architecture -- there is no WASM-specific IP->MethodDesc code path. (MinVirtualIP and FunctionTableIndexRangeSection are consumed only by the unwinder's function-table-index -> base-virtual-IP mapping in WasmR2RInfo.) Add a test that resolves a wasm32 virtual IP to its R2R MethodDesc and confirms the ReadyToRun classification, mirroring GetMethodDesc_R2R_OneRuntimeFunction with explicit virtual-IP framing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: afa9efc6-f596-41bd-9070-dc651db8b7b4
On builds without code versioning (FEATURE_CODE_VERSIONING off, e.g. WASM), the runtime omits Module::MethodDefToILCodeVersioningStateMap from the emitted Module data descriptor (src/coreclr/vm/datadescriptor/datadescriptor.inc, guarded by FEATURE_CODE_VERSIONING). The DataContractReader Loader contract read that field unconditionally, so any managed-object -> type-name resolution on WASM threw "Field not found in any layout" the moment it needed the module lookup tables. Make the field optional, matching the existing EnCClassList pattern (also feature-gated): - Data.Module: declare MethodDefToILCodeVersioningStateMap as TargetPointer? so an absent field reads as null instead of throwing. - Loader.GetLookupTables: coalesce the absent map to TargetPointer.Null, which GetModuleLookupMapElement already treats as an empty table (no code-versioning lookups, the correct behavior when the feature is off). - Update Loader.md GetLookupTables pseudo-code to reflect the optional read. - Test: a Module layout without the field resolves the map to null rather than faulting; the mock Module layout gains an includeCodeVersioning toggle. Discovered via live browser-wasm cDAC validation: type resolution on a real frozen String.Empty failed here before this fix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: afa9efc6-f596-41bd-9070-dc651db8b7b4
EcmaMetadata resolution calls Loader.TryGetLoadedImageContents to locate a module's metadata. It only consulted PEImage.LoadedImageLayout (m_pLayouts[IMAGE_LOADED]), which is null for images that are never mapped/ loaded -- notably webcil ReadyToRun images on WASM, whose metadata lives in the flat layout (m_pLayouts[IMAGE_FLAT]). As a result any managed-object -> type name resolution on WASM failed with "Module is not loaded" once it reached the metadata step. - peimage.h / datadescriptor.inc: expose cdac_data<PEImage>::FlatImageLayout (m_pLayouts[IMAGE_FLAT]) as a new PEImage.FlatImageLayout descriptor field, alongside the existing LoadedImageLayout. - Data.PEImage: add FlatImageLayout (nullable; older descriptors without it read as null). - Loader.TryGetLoadedImageContents: when LoadedImageLayout is null, fall back to the flat layout. Its flags lack FLAG_MAPPED, so the metadata reader treats it as a flat (non-mapped) image, which is correct for webcil. - Update Loader.md and add a MockTarget test for the flat fallback. Surfaced by live browser-wasm cDAC validation: type resolution on a frozen String reached "Module is not loaded" before this fix. NOTE: the managed changes are unit-tested; the native datadescriptor change (peimage.h / datadescriptor.inc) requires a runtime build to validate. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: afa9efc6-f596-41bd-9070-dc651db8b7b4
Once the flat-layout fallback lets EcmaMetadata reach a WASM webcil ReadyToRun
image, it fed the flat bytes to System.Reflection.Metadata's PEReader, which
can't parse the stripped/rewrapped webcil header -> "BadImageFormatException:
Unknown file format." Make the read-only metadata path webcil-aware:
- EcmaMetadata.GetReadOnlyMetadataAddress: when the image begins with the webcil
magic ('WbIL'), locate the ECMA-335 metadata via the webcil header's
PeCliHeaderRva -> CLI (COR20) header -> metadata directory, resolving RVAs with
the loader's webcil-aware GetILAddr instead of PEReader. Non-webcil images keep
the existing PEReader path.
- Data.WebcilHeader: expose PeCliHeaderRva (RawOffset, no descriptor change).
- Loader: GetRvaData / GetILAddr had the same loaded-layout assumption as
TryGetLoadedImageContents and threw for webcil-on-WASM (LoadedImageLayout null).
Factor the loaded-or-flat layout selection into a shared helper so both fall
back to the flat layout.
- Update Loader.md / EcmaMetadata.md and add a GetILAddr flat-webcil-layout test
(the webcil header + RVA resolution the metadata path relies on).
Purely managed (WebcilHeader uses RawOffset; depends on the PEImage.FlatImageLayout
descriptor field from the previous change). Surfaced by live browser-wasm cDAC
validation, which advanced to "Unknown file format" after the flat-layout fix.
The EcmaMetadata webcil branch itself is validated end to end on a live wasm
target; the loader flat-fallback it relies on is unit-tested here.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: afa9efc6-f596-41bd-9070-dc651db8b7b4
|
Azure Pipelines: Successfully started running 5 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. |
Contributor
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Member
Author
|
cc @dotnet/wasm-contrib |
Contributor
There was a problem hiding this comment.
Pull request overview
Enables cDAC to operate against CoreCLR/WebAssembly targets by adding WASM-aware stack-walking support (context + R2R unwinding + interpreter transition seeding) and by fixing managed metadata resolution for WASM’s webcil/flat-image layout.
Changes:
- Add WASM stack-walk context and ReadyToRun linear-stack unwinder plumbing, including function-table-index range lookup.
- Make loader/metadata paths resilient to WASM specifics (optional code-versioning table, flat PEImage layout fallback, webcil-aware metadata location).
- Add/extend unit tests and update relevant contract design docs for Loader/EcmaMetadata.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/tests/UnitTests/WasmUnwinderTests.cs | New unit tests for WASM R2R unwinder behaviors (frame pointer, VIP, ULEB128 frame sizes). |
| src/native/managed/cdac/tests/UnitTests/StackWalkTests.cs | Adds WASM-specific stack-walk tests (frame-chain seeding, interpreter virtual unwind, context layout/register behavior). |
| src/native/managed/cdac/tests/UnitTests/MockDescriptors/MockDescriptors.Loader.cs | Allows mocking Module layouts with/without code versioning field. |
| src/native/managed/cdac/tests/UnitTests/MockDescriptors/MockDescriptors.Frame.cs | Extends frame mocks for WASM seeding and adds InterpMethodContextFrame mock layout/builder support. |
| src/native/managed/cdac/tests/UnitTests/LoaderTests.cs | Tests for missing code-versioning map and flat-layout/webcil RVA resolution + image contents fallback. |
| src/native/managed/cdac/tests/UnitTests/ExecutionManager/ExecutionManagerTests.cs | Adds WASM “virtual IP” ReadyToRun MethodDesc resolution coverage. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/DataType.cs | Adds FunctionTableIndexRangeSection data type id. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/WebcilHeader.cs | Adds PeCliHeaderRva field to support webcil metadata location. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ReadyToRunInfo.cs | Adds nullable MinVirtualIP field for WASM virtual-IP base computation. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/PEImage.cs | Exposes nullable FlatImageLayout to support non-mapped (flat) images. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Module.cs | Makes MethodDefToILCodeVersioningStateMap nullable for FEATURE_CODE_VERSIONING-off builds. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/FunctionTableIndexRangeSection.cs | New data model for WASM function-table-index range list nodes. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/FrameHandling/WasmFrameHandler.cs | New WASM frame handler for seeding/stashing interpreter walk frame pointer. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/FrameHandling/FrameHelpers.cs | Wires WasmFrameHandler and maps WASM “first arg register” name. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/WasmContext.cs | New WASM context struct mirroring native T_CONTEXT layout and providing unwind entrypoint. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/Wasm/WasmUnwinder.cs | New R2R linear-stack unwinder (frame pointer, virtual IP, ULEB128 frame size decode). |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/Wasm/WasmR2RInfo.cs | Implements function-table-index → R2R module/runtime function lookup via new descriptors. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/IPlatformAgnosticContext.cs | Adds WASM context selection based on RuntimeInfoArchitecture.Wasm. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs | Adds flat-layout fallback for image access and treats missing code-versioning map as empty. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/EcmaMetadata_1.cs | Adds webcil-aware metadata resolution path; keeps PEReader path for normal PEs. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs | Adds FunctionTableIndexRangeList global name. |
| src/coreclr/vm/readytoruninfo.h | Exposes MinVirtualIP in cdac_data under TARGET_WASM. |
| src/coreclr/vm/peimage.h | Adds FlatImageLayout descriptor field (m_pLayouts[IMAGE_FLAT]). |
| src/coreclr/vm/datadescriptor/datadescriptor.inc | Emits new PEImage/ReadyToRunInfo fields and WASM-only type/global for function table index ranges. |
| src/coreclr/vm/codeman.h | Adds cdac_data for FunctionTableIndexRangeSection and exposes list head address under TARGET_WASM. |
| src/coreclr/debug/datadescriptor-shared/contractdescriptorstub.c | Fixes stub ContractDescriptor flags initialization. |
| docs/design/datacontracts/Loader.md | Documents FlatImageLayout fallback and optional code-versioning table behavior. |
| docs/design/datacontracts/EcmaMetadata.md | Documents webcil magic/header-based metadata location approach. |
- WasmContext.UnsetSingleStepFlag: no-op instead of throwing. WASM has no hardware single-step flag (like ARM/LoongArch64/RISC-V), and callers such as Debugger_1.PrepareExceptionHijack invoke it unconditionally. - WasmUnwinder.TryGetFramePointer: re-apply the linear-stack floor after following the localloc frame-pointer indirection, so a null/invalid saved pointer returns false instead of reading at a bad address. - WasmUnwinder.TryUnwindOneFrame: terminate the R2R walk when the decoded frame size is 0 (no progress) or the caller's virtual IP is null (interpreter transition / stack top), rather than reporting a bogus advanced frame. - WasmUnwinder.DecodeULEB128: bound the decode to 5 bytes (uint32 max) and fail on a malformed, unterminated encoding. - Document the WASM ExecutionManager descriptors (ReadyToRunInfo.MinVirtualIP, FunctionTableIndexRangeSection, FunctionTableIndexRangeList) in ExecutionManager.md. - Fix an accidental single-line brace/declaration in a LoaderTests method. - Add unit tests for the new unwinder guards (below-floor localloc, zero frame size, malformed ULEB128). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: afa9efc6-f596-41bd-9070-dc651db8b7b4
Member
Author
|
@copilot review |
Open
3 tasks
max-charlamb
left a comment
max-charlamb
left a comment
Member
There was a problem hiding this comment.
The cDAC changes look good to me. Happy to merge this when you are ready.
Note that I didn't run this locally or verify that the WASM specifics (unwinder) work correctly. I would like to add this to our cDAC CI runs at some point.
…tadata dir Feedback from @max-charlamb on EcmaMetadata_1 webcil path: - Read the webcil 'WbIL' magic with ReadLittleEndian<uint> so detection is correct even on big-endian targets. - Read the COR20 metadata directory (RVA + size) through the existing ImageDataDirectory IData type instead of manual target.Read<uint> at fixed offsets, matching how the ReadyToRun JIT manager reads its data directories. Full cDAC suite: 2735 passed / 0 failed / 16 skipped. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: afa9efc6-f596-41bd-9070-dc651db8b7b4
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 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
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "9f9b36e95d22ee37b78be5cdb2aebf5990f61a77",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "f7dfaf6882e030191b6a70e6441c4141887d8d3b",
"last_reviewed_commit": "9f9b36e95d22ee37b78be5cdb2aebf5990f61a77",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "f7dfaf6882e030191b6a70e6441c4141887d8d3b",
"last_recorded_worker_run_id": "29772960653",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "9f9b36e95d22ee37b78be5cdb2aebf5990f61a77",
"review_id": 4738505655
}
]
} |
Main added an eager `[Field] MDImport` to Data.PEAssembly, and updated the existing LoaderTests mocks to match. The new TryGetLoadedImageContents_NoLoadedLayout_FallsBackToFlatLayout test hand-builds its own PEAssembly layout and was missing the field, so Data.PEAssembly's eager construction threw "Field not found" once merged with current main. Add the MDImport pointer field to the mock layout. Full cDAC suite: 2763 passed / 0 failed / 16 skipped. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: afa9efc6-f596-41bd-9070-dc651db8b7b4
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Holistic Review
Motivation: Justified and real. Bringing the managed cDAC up on CoreCLR/WebAssembly closes concrete gaps that block diagnostic type-name resolution and stack walking on a browser/WASI corerun.wasm target. Each gap (no native register context, feature-gated descriptor fields, flat webcil R2R images) is a genuine WASM-specific divergence, and the PR includes live end-to-end validation over CDP.
Approach: Sound and consistent with existing cDAC patterns. The stack walk flows through the existing StackWalk_1 driver with no new WASM-specific contract surface; nullable descriptor fields mirror the established EnCClassList pattern; the flat-layout fallback is factored into a shared TryGetUsableImageLayout helper; and the webcil metadata path reuses the loader's webcil-aware GetILAddr. The single native change (PEImage.FlatImageLayout descriptor field) is minimal and correct.
Summary: WasmContext against the native wasm T_CONTEXT — can only be confirmed against the runtime headers/live target the author validated. A human familiar with the WASM T_CONTEXT/SetFirstArgReg contract should confirm that invariant. Findings below are otherwise minor.
Detailed Findings
✅ Native descriptor + offsets — Verified correct
contractdescriptorstub.c:.flags = 0x1u | (sizeof(void*) == 4 ? 0x02u : 0x00u)is a genuine bug fix. Perdocs/design/datacontracts/contract-descriptor.md, bit 0 must always be 1 and bit 1 isptrSize. The prior&always produced0, leaving bit 0 (the required constant) clear;|is correct.peimage.h:FlatImageLayout = offsetof(PEImage, m_pLayouts)correctly targetsm_pLayouts[IMAGE_FLAT](index 0), andLoadedImageLayoutremains+ sizeof(PTR_PEImageLayout)(IMAGE_LOADED= 1). Matches theIMAGE_FLAT=0/IMAGE_LOADED=1enum in the same file.codeman.hcdac_data<FunctionTableIndexRangeSection>offsets (minFunctionTableIndex,numRuntimeFunctions,pR2RModule,pNext) match the native struct; the global correctly exposes&s_pFunctionTableIndexRangeList(a pointer-to-pointer, dereferenced once inWasmR2RInfo.FindSection).- Webcil header: new
PeCliHeaderRvaat[RawOffset(12)]matchesstruct WebcilHeaderinsrc/coreclr/inc/webcildecoder.h(Id[4], VersionMajor@4, VersionMinor@6, CoffSections@8, Reserved0@10, PeCliHeaderRva@12). The COR20 metadata-directory offset (RVA @ 8) used inGetWebcilReadOnlyMetadataAddressmatchesIMAGE_COR20_HEADER.
✅ Contract fixes + documentation — Consistent with conventions
- Nullable
Module.MethodDefToILCodeVersioningStateMap,PEImage.FlatImageLayout, andReadyToRunInfo.MinVirtualIPfollow the existing optional-descriptor-field pattern, andGetLookupTablesdegrades to an empty table via?? TargetPointer.Null. docs/design/datacontracts/{Loader,EcmaMetadata,ExecutionManager}.mdare all updated to match the descriptor/algorithm changes, satisfying the cDAC documentation requirement.
✅ Test quality — Good coverage
New WasmUnwinderTests, WasmR2RInfoTests, and additions to StackWalkTests/LoaderTests/ExecutionManagerTests exercise the WasmContext layout + InterpreterWalkFramePointer round-trip, the InlinedCallFrame-over-InterpreterFrame stash, virtual-IP → R2R MethodDesc resolution, the code-versioning-absent path, the flat-layout fallback, and GetILAddr through a flat webcil layout via MockTarget — matching the described scenarios.
💡 WasmUnwinder.GetEstablishingFramePointerFromTerminator — Currently test-only
This public helper (and its CallFuncletWith[out]Throwable terminator handling) is referenced only from WasmUnwinderTests, not from any production walk path yet — consistent with funclet/EH unwind being listed as an explicit follow-up. Not a blocker; just flagging that it is presently unexercised outside tests, so its contract will only be validated once the funclet path lands.
💡 Per-frame allocation in WasmContext.Unwind — Minor
Each Unwind() constructs a fresh WasmUnwinder + WasmR2RInfo (which builds a RuntimeFunctionLookup). This is fine for diagnostic use, but if deep WASM walks become common it may be worth hoisting. Non-blocking.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 155.7 AIC · ⌖ 10.5 AIC · ⊞ 10K
Contributor
There was a problem hiding this comment.
Holistic Review
Motivation: The problem is real and well-scoped: the cDAC contracts made assumptions (native register context, always-present code-versioning fields, mappable PE images) that don't hold on CoreCLR/WebAssembly, blocking live managed-state inspection. Each gap is concretely identified and independently justified.
Approach: Sound and consistent with the codebase. New behavior flows through the existing StackWalk_1 driver and frame-handling dispatch rather than adding a WASM-specific contract surface; the metadata fixes reuse established nullable-field patterns (mirroring EnCClassList) and the loader's existing webcil-aware RVA resolution. Native changes are minimal (one descriptor field plus WASM-gated types) and the managed WasmContext layout is deliberately mirrored byte-for-byte on the native T_CONTEXT. The contractdescriptorstub.c change is a genuine correctness fix: the previous 0x1 & (...) always evaluated to 0, never setting the mandatory bit 0 nor the 32-bit ptrSize bit that the reader consumes at ContractDescriptorTarget.cs:355.
Summary:
Detailed Findings
✅ Native descriptor / flags fix — correct
contractdescriptorstub.c now ORs the flag bits instead of ANDing them, so the stub descriptor correctly reports bit 0 and the 32-bit ptrSize bit. This is all-platforms but is a strict fix over the previously-always-zero value.
✅ Metadata resolution fixes — consistent with existing patterns
The three managed fixes (nullable MethodDefToILCodeVersioningStateMap, FlatImageLayout fallback factored into TryGetUsableImageLayout, and webcil-magic detection in GetReadOnlyMetadataAddress) each follow existing conventions and are wired uniformly through TryGetLoadedImageContents/GetRvaData/GetILAddr. The webcil metadata walk (PeCliHeaderRva -> COR20 header -> metadata directory at offset 8) matches the documented webcil header layout.
✅ Documentation — updated per cDAC guidance
Loader.md, EcmaMetadata.md, and ExecutionManager.md are updated to reflect the new descriptor fields, globals, and algorithm branches, satisfying the cDAC doc-sync requirement for descriptor and contract changes.
✅ Test quality — thorough
Unit tests cover the WASM context seeding, register round-trip, interpreter-frame stash, virtual-unwind chain step/exhaustion, malformed/zero ULEB128, localloc indirection, virtual-IP->MethodDesc resolution, code-versioning-absent path, and flat-webcil layout fallback. Good edge-case coverage given the MockTarget constraints.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 193.8 AIC · ⌖ 10.1 AIC · ⊞ 10K
Member
Author
Thanks, I can keep extending but I'm also fine taking this now and improving with follow ups, whatever is preferred. |
max-charlamb
approved these changes
Jul 21, 2026
This was referenced Jul 22, 2026
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.
Brings the managed data contract reader (cDAC) up on CoreCLR/WebAssembly, so diagnostic tools can inspect live managed state of a browser/WASI
corerun.wasmtarget the same way they do on other platforms. This covers two areas: the WASM stack walk, and the module/metadata resolution path that type-name lookup depends on.Background
WASM CoreCLR differs from other targets in ways the cDAC contracts did not yet account for:
T_CONTEXThas no real register file; execution is a mix of ReadyToRun frames unwound over the linear stack ($sp) with synthetic virtual IPs, and interpreter frames walked via the explicitInterpreterFrame/InterpMethodContextFramechain.System.Reflection.Metadata.PEReadercannot parse.Stack walking
Builds on the WASM
WasmContext/WasmUnwinder/WasmR2RInfofoundation and wires it into the sharedStackWalk_1driver — no WASM-specific contract surface is added (an earlier context-freeIStackWalk.GetInterpretedFramesexperiment is reverted); everything flows through the existing driver and theIsInterpreterCode → InterpreterVirtualUnwindpath.WasmContextnow mirrors the nativeT_CONTEXT(src/coreclr/pal/inc/pal.h,HOST_WASM):ContextFlags,InterpreterWalkFramePointer,InterpreterSP,InterpreterFP,InterpreterIP— five 32-bit fields, 20 bytes. The context is serialized to/from target memory and handed to SOS, so the managed layout must match byte-for-byte (it previously held only three IP/SP/FP slots).WasmFrameHandlerseeds the initial stack-walk context from the Frame chain (WASM has no capturedDT_CONTEXT): the innermostInlinedCallFrame'sCallSiteSPprovides$sp. On the P/Invoke-into-interpreter transition it stashes the owningInterpreterFrameaddress inInterpreterWalkFramePointer, matching nativeSetFirstArgReg(src/coreclr/vm/wasm/cgencpu.h);GetFirstArgRegisterNamereturns that slot for WASM.MethodDescresolution reuses the genericRangeSectionMappath (a virtual IP is just an address in a registered range); no WASM-specific IP→MethodDesc path is required.Managed metadata resolution (contract fixes)
Three feature/format gaps blocked managed-object → type-name resolution on WASM. Each is fixed following existing patterns:
Module.MethodDefToILCodeVersioningStateMapoptional — omitted from the descriptor whenFEATURE_CODE_VERSIONINGis off (WASM). Made nullable (matching theEnCClassListpattern) soLoader.GetLookupTablesreads it as an empty table instead of throwing "Field not found in any layout".PEImage.FlatImageLayout—cdac_data<PEImage>only exposed the loaded layout (m_pLayouts[IMAGE_LOADED]), which is null for images that are never mapped. AddFlatImageLayout(m_pLayouts[IMAGE_FLAT]) and fall back to it inLoader.TryGetLoadedImageContents(and, factored into a shared helper, inGetRvaData/GetILAddr), so webcil-on-WASM metadata is reachable instead of "Module is not loaded".EcmaMetadata.GetReadOnlyMetadataAddressfed the flat webcil bytes toPEReader(→ "BadImageFormatException: Unknown file format"). It now detects the webcil magic (WbIL) and locates the ECMA-335 metadata via the webcil header'sPeCliHeaderRva→ CLI (COR20) header → metadata directory, resolving RVAs with the loader's webcil-awareGetILAddr. Non-webcil images keep thePEReaderpath.The only native change is the
PEImage.FlatImageLayoutdescriptor field (peimage.h+datadescriptor.inc); the rest is managed.Testing
WasmContextlayout +InterpreterWalkFramePointerregister round-trip, the InlinedCallFrame-over-InterpreterFrame stash, the interpreter virtual unwind chain-step/exhaustion, virtual-IP → R2RMethodDescresolution, theModulecode-versioning-absent path, thePEImageflat-layout fallback, andGetILAddrresolving through a flat webcil layout. Full cDAC suite green.docs/design/datacontracts/{Loader,EcmaMetadata}.mdupdated to match the contract changes.clr.nativebuilds clean with the descriptor change.End-to-end validation
Validated against a live browser-
wasmCoreCLR target over CDP (rebuiltcorerun.wasmfor theFlatImageLayoutdescriptor field; managed reader hot-swapped for the rest). With all three fixes:managed_object(String.Empty)→full_type_name: "System.String",module: "System.Private.CoreLib",type_def_token: 0x0200007E,type_name_complete: true— the ECMA metadata is read from the webcil ReadyToRun image via theWbIL-header path and the TypeDef resolves.managed_threadsenumerates the managed thread (the trap thread'sLastThrownObjecttype resolves through the same metadata path).The full
read → walk → enumerate → type-namepath is green on live WASM managed state.Out of scope / follow-ups
Deferred (each with its own required trap/repro shape or larger effort): live
InlinedCallFrameCallSiteSPread on the P/Invoke seam, exception-handling/funclet unwind, GC stack-ref reporting during the walk,locallocindirect frames, wasm64, and DacDbi/debugger paths (disabled on WASM).Note
This pull request was authored with the assistance of GitHub Copilot.