Conversation
|
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. |
AndyAyersMS
left a comment
There was a problem hiding this comment.
Does this handle describing locals that are on the linear stack? Last I looked those SP/FP based encodings were wonky for Wasm.
Note the exact Wasm locals that hold SP/FP can vary and may even vary between main method and its funclets, which I doubt we can encode.
An easy test for this is to look at any GC-typed local, these are always on the linear stack.
|
@lewing, what value does this debugging data provide? Given my understanding that debugging is likely to only be supported on mostly interpreted scenarios, why should we make this correct for R2R scenarios. (I expect that nativeaot will make good use of this data, so I think overall its goodness, but I don't see why its particularly useful for R2RDump.) |
|
It came out of the DWARF discussion and I'm working on something in the nesm debugger and I'm trying to plumb out more info, it is isn't meant to be user facing currently. |
Preserve variable debug information produced by RyuJIT for ReadyToRun WebAssembly code, including scope ranges across relooper block ordering and packed wasm local register locations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The end-to-end variable-debug-info validation exposed the hidden wasm portable-entry-pointer argument as a source local. The argument is appended after user arguments, but unlike the wasm stack-pointer argument it was not recorded or excluded by compMap2ILvarNum. AddDoubles therefore reported the hidden i32 argument as source local 0, alongside the real f64 parameters. Record the argument's local number when it is created, map it to UNKNOWN_ILNUM, and account for it when mapping later internal locals back to IL variable numbers. Replace the count-only wasm R2R checks with complete exact records for the AddDoubles parameters and SumWithFinally local: variable identity, native range, location kind, packed wasm local, and frame-pointer-relative offset. Mutate the local-index bits of one packed register and prove the exact oracle rejects the corrupted record. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
8553c54 to
3979515
Compare
|
I opened #133890 (which includes these commits) with cDAC consumer side changes |
Define the packed WASM debug-register bit layout in ICorDebugInfo and have the JIT derive its register masks from that shared encoding contract. Assert that the JIT register representation and WasmValueType count remain compatible with the debug-info format. Document that the static ReadyToRun reader's compiled-in shift must move with a versioned R2R debug-info format change, since it has no live target descriptor from which to discover a different layout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Publish the shared WASM register type shift and value-type count through the target data descriptor so version-skewed readers can reject incompatible variable debug information. Document the producer-owned encoding and extend the static ReadyToRun reader coverage for reserved and unsupported value-type codes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add a no-opt object local that remains live across a GC call in a finally funclet. Assert its IL class type, complete ReadyToRun variable tuples, frame-relative GC slot, and safepoint coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add exact optimized tracked-variable coverage, frame-base ABI variations including localloc and funclets, and same-type GC slot identity with a legitimate null reference. Pin the current stack VarLoc base encoding and document that absolute frame reconstruction is independent of unstable wasm local indices. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 7 pipeline(s). 9 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.
Copilot review overview
🔵 Needs a closer look
scopeinfo.cpp has unresolved issues with BAD_IL_OFFSET handling and method-wide scope latching that can produce incorrect ranges.
Review tier: Lite
Findings: None
What changed in this PR
Preserves WebAssembly ReadyToRun variable debug information and defines register and stack-location contracts for diagnostic readers.
Changes:
- Emits optimized and unoptimized WASM variable locations while excluding hidden ABI arguments.
- Centralizes packed register encoding and publishes target metadata.
- Adds R2R validation for locals, GC roots, stack frames, funclets, and
localloc.
| File | Reviewed changes |
|---|---|
src/coreclr/vm/datadescriptor/datadescriptor.inc |
Publishes WASM encoding metadata. |
src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/DebugInfo.cs |
Decodes packed WASM registers. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCases/Webcil/WasmWebcilModule.cs |
Adds WASM debug-information test cases. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCases/R2RTestSuites.cs |
Validates exact debug-information records. |
src/coreclr/jit/scopeinfo.cpp |
Emits WASM variable ranges and locations; requires offset-sentinel guarding and per-block scope closure. |
src/coreclr/jit/registeropswasm.cpp |
Uses centralized register encoding constants. |
src/coreclr/jit/lclvars.cpp |
Excludes hidden portable-entry arguments. |
src/coreclr/jit/compiler.h |
Tracks the portable-entry argument index. |
src/coreclr/jit/compiler.cpp |
Enables WASM scope information. |
src/coreclr/jit/codegen.h |
Adds WASM scope-tracking state. |
src/coreclr/inc/cordebuginfo.h |
Defines WASM debug-register encoding constants. |
docs/design/datacontracts/DebugInfo.md |
Documents encoding and stack-base contracts. |
|
#134689 will remove this information by default |
…ish (#134689) ## Summary Pass `--strip-debug-info` to crossgen2 for CoreCLR browser-wasm ReadyToRun publish. This drops the R2R `DebugInfo` section (native-to-IL offset maps and variable locations) from shipped images. Opt out with `PublishReadyToRunStripDebugInfo=false` to keep the data for debugging R2R code (e.g. the cDAC work in #133086 / #133890). Also makes crossgen2 argument changes invalidate per-app R2R images. `_CreateR2RImages` only tracks file inputs, so toggling `PublishReadyToRunStripDebugInfo` (or any `PublishReadyToRunCrossgen2ExtraArgs` change) previously left stale images in `obj/R2R`. The arguments are now written to `obj/wasm-r2r-args.stamp` (only when different) and added to `_ReadyToRunCompilerInputs`, mirroring the existing P/Invoke manifest input. > [!IMPORTANT] > Stacked on #134618, which rewrites the same targets file. Retarget to `main` after it merges. ## Size impact Per-assembly R2R images compiled directly with crossgen2 using the SDK's browser-wasm arguments (`--obj-format:wasm --opt-cross-module:* --codegenopt:JitWasm*NyiToR2RUnsupported=1`), with and without `--strip-debug-info`. Total for System.Private.CoreLib, System.Text.Json, System.Linq, and System.Collections: | Compiler | Raw saved | gzip -9 saved | brotli -q 11 saved | | --- | --- | --- | --- | | Current (#134618 base) | 859,712 B (2.4%) | 624,690 B (7.1%) | 565,366 B (9.4%) | | With #133086 variable info | 2,105,520 B (5.6%) | 1,332,733 B (13.9%) | 1,121,574 B (17.0%) | <details> <summary>Per-assembly brotli sizes (bytes, keep → strip)</summary> | Assembly | Current | With #133086 | | --- | --- | --- | | System.Private.CoreLib | 4,822,231 → 4,347,541 | 5,331,799 → 4,403,849 | | System.Text.Json | 823,314 → 761,533 | 887,982 → 754,444 | | System.Linq | 219,177 → 200,471 | 238,853 → 200,441 | | System.Collections | 119,635 → 109,446 | 130,742 → 109,068 | The #133086 compiler is based on an older commit, so compare keep vs. strip within a column rather than across columns. </details> ## Validation - MSBuild evaluation: `--strip-debug-info` is present by default, absent with `PublishReadyToRunStripDebugInfo=false`, and absent when `PublishReadyToRun` is off. - Incremental harness: `_CreateR2RImages` runs on first build, skips when unchanged, reruns on opt-out, skips when repeated, and reruns when switching back. - Not yet run: an end-to-end browser-wasm publish loading stripped images in a browser (Wasm.Build.Tests in CI will cover this). Runtime-pack framework R2R images (used only by the dev-loop build) are unchanged; publish recompiles the whole closure through these targets. `--strip-inlining-info` is intentionally not included: it removes `CrossModuleInlineInfo`, and cross-module inlining is load-bearing on wasm, so it needs separate validation. > [!NOTE] > This pull request was created with assistance from GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
## Summary Fix cDAC live resolution of WebAssembly ReadyToRun virtual IPs by matching the runtime's existing lookup model: - expose `ExecutionManager::s_pVirtualIPRangeList` and `VirtualIPRangeSection` through the data descriptor; - resolve encoded virtual IPs through that intrusive list using cycle detection and a 65,536-node per-lookup reader resource budget, with no map fallback when an encoded VIP is absent; - mask the WebAssembly funclet flag from `RUNTIME_FUNCTION.BeginAddress` for ordering and address arithmetic while preserving funclet identity; - keep the virtual code base (`MinVirtualIP`) separate from the loaded-image base used for unwind, debug, GC, exception, and thunk RVA reads; - handle the actual WASM descriptor shape, where hot/cold metadata and delay-load thunk metadata are absent; - classify WASM filter funclets by mapping the executable filter entry to its containing runtime function. ## Root cause The model added in #130988 was already false when that PR merged. On `TARGET_WASM`, ReadyToRun modules are not added to `RangeSectionMap`; `ReadyToRunInfo::RegisterVirtualIPRange` registers them in `ExecutionManager::s_pVirtualIPRangeList`, and native `FindCodeRange` checks that list first. The prior unit test synthesized a `RangeSectionMap` entry with an address that did not satisfy native `IsVirtualIP`, so it validated a mock-only model rather than the live runtime layout. This is a test-model gap, not a reviewer fault. The prior review explicitly noted that the WebAssembly specifics had not been run locally and should be added to cDAC CI: #130988 (review). ## Blast radius and scope This affects ReadyToRun code on all CoreCLR WebAssembly hosts, including browser and WASI. Interpreter code is unaffected. The list lookup, descriptor feature gating, funclet masking, and image-base separation are inseparable: exposing the list alone would still throw while reading absent WASM fields, or could return the wrong method or read RVA data from the synthetic virtual address space. This PR is independent of #133086 and intentionally excludes variable producer/decoder work. #133890 depends on this PR for correct shared code lookup and function identity. On WASM, `FilterOffset` is the executable filter entry and can follow a synthetic funclet prolog. cDAC now mirrors the corrected native classification in #133932 by resolving that entry to its containing runtime function before comparing funclet starts. The PRs remain independent; #133917 does not depend on changing the producer offset. ## Validation - `./build.sh clr+libs+host` - `PATH="/opt/homebrew/bin:$PATH" ./build.sh -os browser -c Debug -subset clr+libs` - cDAC UnitTests: **3162 passed** - cDAC DataGeneratorTests: **46 passed** - cDAC UsageTests: **4 passed** - generated contract documentation check: **up to date** - focused ExecutionManager / RuntimeFunction / WasmR2R tests: **221 passed** The durable tests cover: - captured/live-shaped VIP `0x80010109`, exact `MethodDesc`, module, and runtime-function index; - the actual WASM descriptor shape: 8-byte `RUNTIME_FUNCTION` records with no `EndAddress`, and absent hot/cold and delay-load thunk fields; - start/end boundaries and adjacent ranges; - encoded VIP absent from the list with no `RangeSectionMap` fallback; - self-cycle, two-node cycle, inverted range, null module, and overlapping ambiguity; - unrelated partially registered nodes not blocking initialized ranges, while an uninitialized candidate fails closed; - valid 1,024/1,025-node lists, exact 65,536-node budget success, and budget+1 fail-closed behavior even when the head matches, with a read counter proving the extra node is never dereferenced; - root/funclet resolution with a flagged funclet entry that breaks raw ordering; - exact loaded-image debug, unwind, GC, and exception-clause reads while entrypoint lookup uses `MinVirtualIP`; - filter-funclet classification where `FilterOffset` follows the flagged funclet start but resolves to the same containing runtime function; - missing list capability and unchanged ordinary architecture behavior. Mutation proofs were applied, confirmed in source, run red, restored, and rerun green: 1. Removing the VIP-list branch fails the captured `0x80010109` test at the exact code-block assertion. 2. Using raw `BeginAddress` fails the funclet identity test. 3. Using `startVIP` as the loaded-image base fails the GC/unwind test with a read at `0x80010081` instead of the loaded image. 4. Raising the reader budget from 65,536 to 65,537 makes the budget+1 test fail at its read-boundary assertion (highest node index 65,536 instead of 65,535); restoring the budget returns the suite to green. 5. Replacing WASM filter-entry containing-function resolution with raw `FilterOffset == funcletStartOffset` comparison makes the filter regression fail with expected `true` and actual `false`. The finite list cutoff is an intentional diagnostic-reader resource policy, not a native registration limit or a claim that an over-budget list is corrupt. > [!NOTE] > This pull request description was generated with GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
Preserve variable debug information produced by RyuJIT for ReadyToRun WebAssembly code and define the producer-owned encoding and stack-base contracts used by downstream diagnostic readers.
ICorDebugInfoand advertise its shift/count through target data descriptors.VarLocbase encoding to2.This remains a draft. The producer changes are here; cDAC decoding and live consumer validation remain in dependent draft #133890.
Producer-owned encoding contract
WASM register locations pack a JIT-local index and
WasmValueTypeinto the 32-bitICorDebugInfo::RegNumpayload:Current values:
The JIT derives its masks from these constants and statically asserts the register-width and
WasmValueType::Countrelationships. The wasm target descriptor publishesWasmDebugRegisterTypeShift=29andWasmDebugValueTypeCount=7asuint8globals. Readers must reject missing or unsupported values rather than plausibly misdecode another local/type.The static ReadyToRun image reader has no live target descriptor, so a future width/count change requires a format-aware, versioned R2R debug-info update.
Stack base versus engine locals
Three distinct concepts must not be conflated:
DebugVarInfostack base literal2.The producer now statically asserts:
Therefore current
VLT_STK/VLT_STK2records are logical-frame-relative and do not encode a V8$varNlocal. If the producer changes away from base2, the reader needs a coordinated fail-loud update.Measured
WasmRegAllocrule:localloc, or funclets.localloc: FP aliases SP, including methods that make calls.localloc: FP is distinct so later SP movement does not move frame homes.localloc, the funclet receives the root's pre-adjustment frame base.Exact current-output matrix:
locallocSumStaticData)localloc, no funcletlocalloclocallocNo stable case was found where compiler-created locals precede the unique FP: current localloc roots allocate FP as the first declared i32 local after parameters, and funclets receive FP as a parameter before declared locals. Arity can still move the root localloc FP index, so these indices are deliberately not a contract.
C1: optimized tracked-variable gate
OptimizedTrackedVariables(int left, int right)is[NoInlining]only and exercises FullOpts/tracked liveness. Its IL declaressumanddifference;differenceis optimized away, whilesumhas two exact control-flow ranges.Complete output:
There is no record for optimized-away IL local 1. This proves the feature is active on the production optimized path, not only behind
OptimizationDisabled().Mutation proof: suppressing optimized non-parameter live-range starts was confirmed in
scopeinfo.cpp;WasmWebcilModulebecame red and crossgen2 detected invalid optimized debug ranges inOptimizedTrackedVariables. Restoring the path returned the full suite to green.C2: same-type slot identity and legitimate null
GcSlotIdentitycreates two distinct movableGcMarkerinstances with sentinel values 17 and 29, plus a separately stored legitimate-nullGcMarkerlocal. IL opcodes are asserted to bind 17 to local 0, 29 to local 1, andldnullto local 2.Complete output:
The two non-null offsets are distinct. All three reference offsets also exist as
GC_FRAMEREG_RELpinned/untracked GC slots. TheGC.Collectcall at native0x18Clies inside all three reference ranges.Mutation proof: swapping only the expected 17/29 offsets (
0x48and0x44) made the exact named test fail; restoring the identity mapping returned green.Current limitation: the minopts wasm
siWasmOpenedScopeslatch opens each synthesized source scope once and closes it at method end. Consequently these distinct IL locals share the exact[0x36,0x2B1)end range. Slot/value identity is covered here; disjoint lexical range ends require a focused producer follow-up rather than a semantic change hidden in this PR.Existing hard cases
Portable-entry-pointer exclusion
Stacked validation found hidden ABI argument
$3reported as source local 0. The fix recordslvaWasmPortableEntryPtrArg, maps it toUNKNOWN_ILNUM, and accounts for it in reverse local-number mapping.Frame-resident GC local across a funclet
GcLocalAcrossFinallyallocates a realGcMarker, enters afinally, callsGC.Collect, and keeps the marker alive afterward. IL proves local 0 isWebcil.WasmWebcilModule+GcMarker; debug info reportsVLT_STK base=2 offset=0x24; GC info independently reportsGC_FRAMEREG_REL+0x24; and the GC call at native0x161is inside its live range.The root frame currently accesses the slot through its SP/FP alias. The funclet uses its distinct parent-FP parameter to load offset
0x24. This remains a suitable #133890 live specimen, but cDAC stack-variable resolution uses reconstructed absolute FP rather than hardcoding either engine-local index.Existing exact tuples
AddDoublesretains its exact two-parameter register-to-stack transition oracle, andSumWithFinallyretains its exact parameter/local stack tuples and no-funclet-DebugInfoassertion.Stack status and ownership
#132650 merged as
09b4a18f8a6ce1f8eea9636e497a07bd7c50d887. This branch was rewritten ontomainat608580557f7c32e9787164ad78b889108d04faa5and contains six producer-side commits only.No
src/native/managed/cdac/**reader or implementation files are included. The cDAC consumer is dependent draft #133890.Validation
clr+libs+hostbaseline before this matrix: 0 warnings / 0 errors, 10m06s.clr+libsafter the stack-base invariants: 0 warnings / 0 errors, 11m21s.1to3): 1 failed with missing exact wasm instruction sequence, restored green.$1to$2, GC stack base2to3, and encoding value-count7to6all fail their respective gates.29, count7.git diff --checkare clean.Final kept artifacts:
Note
This pull request description was updated with the assistance of GitHub Copilot.