[cDAC] Implement GetILCodeVersionNodeData in DacDbi by rcj1 · Pull Request #129448 · dotnet/runtime

rcj1 · 2026-06-15T23:48:50Z

Making InstrumentedILOffsetMapping m_cMap UINT, it already would fit in a UINT
Adding API TryGetInstrumentedILMap(ILCodeVersionHandle ilCodeVersionHandle, out uint mapEntryCount, out TargetPointer mapEntries)
Implementing GetILCodeVersionNodeData

… m_dwCodegenFlags Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>

dotnet-policy-service · 2026-06-15T23:53:57Z

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

Copilot

Pull request overview

Adds cDAC exposure for instrumented IL offset mappings and wires that into the legacy DacDbi GetILCodeVersionNodeData flow, including CoreCLR data-descriptor plumbing and unit test coverage.

Changes:

Add InstrumentedILOffsetMapping to the CoreCLR cDAC data descriptor set and contracts, and expose it off ILCodeVersionNode.
Introduce ICodeVersions.TryGetInstrumentedILMap(...) and implement it in CodeVersions_1.
Implement DacDbiImpl.GetILCodeVersionNodeData and add new unit tests/mocks for the new mapping data.

Show a summary per file

File	Description
src/native/managed/cdac/tests/UnitTests/MockDescriptors/MockDescriptors.CodeVersions.cs	Extends mock layouts/builders to represent instrumented IL map data on IL code version nodes.
src/native/managed/cdac/tests/UnitTests/DacDbiImplTests.cs	Adds a unit test validating `GetILCodeVersionNodeData` populates IL + instrumented map outputs.
src/native/managed/cdac/tests/UnitTests/CodeVersionsTests.cs	Adds contract-level tests for `TryGetInstrumentedILMap` (explicit + synthetic).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/IDacDbiInterface.cs	Updates `DacDbiSharedReJitInfo` managed struct layout to match native changes (removes unused fields).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements `GetILCodeVersionNodeData` using cDAC contracts and instrumented IL map query.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/DataType.cs	Adds `InstrumentedILOffsetMapping` to the contract `DataType` enum.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/InstrumentedILOffsetMapping.cs	New data contract type representing the embedded mapping (count + pointer).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ILCodeVersionNode.cs	Exposes the embedded mapping address via `[FieldAddress] InstrumentedILMap`.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/CodeVersions_1.cs	Implements `TryGetInstrumentedILMap` by reading the embedded mapping.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ICodeVersions.cs	Adds new contract API `TryGetInstrumentedILMap(...)`.
src/coreclr/vm/ilinstrumentation.h	Changes mapping count storage to `UINT` and adds `cdac_data<>` descriptor offsets.
src/coreclr/vm/ilinstrumentation.cpp	Updates `SetMappingInfo`/`GetCount` signatures to `UINT`.
src/coreclr/vm/datadescriptor/datadescriptor.inc	Adds cDAC type/fields for `InstrumentedILOffsetMapping` and embeds it in `ILCodeVersionNode`.
src/coreclr/vm/codeversion.h	Updates `SetInstrumentedILMap` signatures to `UINT` and adds cDAC offset for embedded mapping.
src/coreclr/vm/codeversion.cpp	Updates `SetInstrumentedILMap` implementations to use `UINT`.
src/coreclr/debug/inc/dacdbistructures.h	Removes unused fields from native `DacSharedReJitInfo` structure.
src/coreclr/debug/ee/functioninfo.cpp	Updates `DebuggerMethodInfo::SetInstrumentedILMap` count parameter to `UINT`.
src/coreclr/debug/ee/debugger.h	Updates `SetInstrumentedILMap` declaration to `UINT`.
src/coreclr/debug/daccess/dacdbiimpl.cpp	Stops populating removed `DacSharedReJitInfo` fields; keeps IL + instrumented map outputs.
docs/design/datacontracts/CodeVersions.md	Documents the new API and new data descriptors involved in the mapping.

Copilot's findings

Files reviewed: 20/20 changed files
Comments generated: 6

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Copilot's findings

Files reviewed: 20/20 changed files
Comments generated: 3

+        // The InstrumentedILOffsetMapping is embedded within the ILCodeVersionNode.
+        TargetPointer mappingAddress = AsNode(ilCodeVersionHandle).InstrumentedILMap;
+        Data.InstrumentedILOffsetMapping mapping = _target.ProcessedData.GetOrAdd<Data.InstrumentedILOffsetMapping>(mappingAddress);
+
+        mapEntryCount = mapping.Count;
+        mapEntries = mapping.Map;
+        return true;
+    }


+    TargetPointer mappingAddress = ilCodeVersionHandle.ILCodeVersionNode + /* ILCodeVersionNode::InstrumentedILMap offset */;
+    mapEntryCount = target.Read<uint>(mappingAddress + /* InstrumentedILOffsetMapping::Count offset */);
+    mapEntries = target.ReadPointer(mappingAddress + /* InstrumentedILOffsetMapping::Map offset */);
+    return true;


    public virtual TargetPointer GetIL(ILCodeVersionHandle ilCodeVersionHandle) => throw new NotImplementedException();
    public virtual bool HasDefaultIL(ILCodeVersionHandle ilCodeVersionHandle) => throw new NotImplementedException();
+    public virtual bool TryGetInstrumentedILMap(ILCodeVersionHandle ilCodeVersionHandle, out uint mapEntryCount, out TargetPointer mapEntries) => throw new NotImplementedException();
+


max-charlamb

lgtm

github-actions · 2026-06-16T03:13:23Z

Copilot Code Review

Holistic Assessment

Motivation: This PR implements GetILCodeVersionNodeData in the managed cDAC DacDbi layer, adds TryGetInstrumentedILMap to ICodeVersions, exposes InstrumentedILOffsetMapping as an embedded cDAC data type, and narrows SIZE_T to UINT for the map count. All justified as part of the ongoing cDAC migration.

Approach: Sound and well-structured. The latest commit (8cfa8fc) simplifies the implementation by using [Field] directly on the embedded InstrumentedILOffsetMapping type rather than the previous [FieldAddress] + manual GetOrAdd<T> approach. This matches established patterns (e.g., EEAllocContext.GCAllocationContext, Generation.AllocationContext).

Summary: ✅ LGTM. The implementation correctly mirrors native DAC behavior, the latest simplification follows idiomatic cDAC patterns, struct layout changes are consistent across native/managed boundaries, and tests provide good coverage.

Detailed Findings

✅ Latest commit simplification is correct

The change from [FieldAddress] public TargetPointer InstrumentedILMap + explicit GetOrAdd<T> to [Field] public InstrumentedILOffsetMapping InstrumentedILMap is the right pattern for embedded (non-pointer) struct fields. Per the FieldAttribute documentation: "Without [Pointer], FieldAttribute on an IData property does an in-place ReadDataField(T)." This matches how other embedded types like GCAllocContext are accessed.

✅ Native behavior fidelity

ILCodeVersionNode::GetInstrumentedILMap() returns &m_instrumentedILMap (always non-null for explicit versions). The cDAC TryGetInstrumentedILMap returns true for all explicit versions and false for synthetic versions (where native returns NULL). The DacDbi layer correctly maps this to the same struct fields the native DacDbiInterfaceImpl::GetILCodeVersionNodeData fills.

✅ Data descriptor registration

datadescriptor.inc correctly uses TYPE(InstrumentedILOffsetMapping) for the embedded field and declares the type with CDAC_TYPE_INDETERMINATE (appropriate since padding between T_UINT32 Count and T_POINTER Map varies by platform).

✅ `SIZE_T` → `UINT` narrowing

Safe. The profiler API (ICorProfilerInfo::SetILInstrumentedCodeMap) uses ULONG at the boundary, so counts are already bounded to 32 bits. The cdac_data<InstrumentedILOffsetMapping>::Count uses T_UINT32 consistently.

✅ Defensive initialization and struct field removal

*pData = default is set before the IReJIT contract gate. Removal of m_state and m_dwCodegenFlags from both DacSharedReJitInfo (native) and DacDbiSharedReJitInfo (managed) is consistent — the native GetILCodeVersionNodeData no longer writes those fields.

✅ Test coverage

Tests cover: explicit version with non-zero map data, synthetic version returning false, and the DacDbi layer integration with both hasMap=true and hasMap=false across both 32-bit and 64-bit architectures.

Note

This review was generated by Copilot.

Generated by Code Review for issue #129448 · ● 50.2M · ◷

Implement GetILCodeVersionNodeData in cDAC DacDbi; remove m_state and…

0ead291

… m_dwCodegenFlags Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>

Copilot AI review requested due to automatic review settings June 15, 2026 23:48

github-actions Bot added the area-Diagnostics-coreclr label Jun 15, 2026

Copilot started reviewing on behalf of rcj1 June 15, 2026 23:49 View session

dotnet-policy-service Bot assigned rcj1 Jun 15, 2026

rcj1 added area-Diagnostics-cdac and removed area-Diagnostics-coreclr labels Jun 15, 2026

Copilot AI reviewed Jun 15, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

rcj1 and others added 2 commits June 15, 2026 18:53

copilot comment

19f0752

Potential fix for pull request finding

08230a0

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings June 16, 2026 01:54

Copilot started reviewing on behalf of rcj1 June 16, 2026 01:54 View session

rcj1 requested review from barosiak, max-charlamb and noahfalk June 16, 2026 01:54

Copilot AI reviewed Jun 16, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

max-charlamb approved these changes Jun 16, 2026

View reviewed changes

Comment thread ...ve/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ILCodeVersionNode.cs Outdated

code review

8cfa8fc

max-charlamb approved these changes Jun 16, 2026

View reviewed changes

build-analysis Bot mentioned this pull request Jun 16, 2026

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

Conversation

rcj1 commented Jun 15, 2026

Uh oh!

dotnet-policy-service Bot commented Jun 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

This comment has been minimized.

max-charlamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jun 16, 2026

Copilot Code Review

Holistic Assessment

Detailed Findings

✅ Latest commit simplification is correct

✅ Native behavior fidelity

✅ Data descriptor registration

✅ SIZE_T → UINT narrowing

✅ Defensive initialization and struct field removal

✅ Test coverage

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

✅ `SIZE_T` → `UINT` narrowing