[cDAC] DacDbi code-version node APIs for ReJIT/SOS

[Copilot]

Description

Implements the cDAC DacDbiImpl APIs GetActiveRejitILCodeVersionNode, GetNativeCodeVersionNode, and GetILCodeVersionNode using existing CodeVersions and ReJIT contracts.

[dotnet-policy-service]

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

Implements cDAC equivalents of legacy DAC DBI code-version node lookup APIs used by SOS/ICorDebug, mapping through existing CodeVersions and ReJIT contracts while preserving HRESULT-style behavior.

Changes:

• Implemented GetActiveRejitILCodeVersionNode by resolving (Module, mdMethodDef) → MethodDesc and returning the explicit active ReJIT IL node (or 0).
• Implemented GetNativeCodeVersionNode by matching (MethodDesc, codeStartAddress) against contract-enumerated native code versions and returning an explicit native node (or 0 for synthetic/absent).
• Implemented GetILCodeVersionNode by mapping an explicit native node → explicit IL node (or 0 for default/synthetic), with DEBUG cross-validation against legacy DAC.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

[github-actions]

🤖 Copilot Code Review — PR #126980

Note

This review was AI/Copilot-generated. Multi-model review: Claude Opus 4.6 (primary) + Claude Sonnet 4.5 + GPT-5.3-Codex (sub-agents).

Holistic Assessment

Motivation: Justified. These three DacDbi APIs (GetActiveRejitILCodeVersionNode, GetNativeCodeVersionNode, GetILCodeVersionNode) are part of the ongoing effort to replace legacy C++ DAC fallbacks with native cDAC contract implementations. The callers (in debug/di) use these to discover ReJIT IL code versions and native code version nodes during debugging.

Approach: Sound. The implementations correctly use the cDAC contracts (ICodeVersions, ILoader, IReJIT) and follow the established pattern in DacDbiImpl.cs (try/catch → HRESULT, #if DEBUG validation against legacy). I verified each method against the C++ DAC reference in dacdbiimpl.cpp:7634-7702 and confirmed behavioral equivalence.

Summary: ✅ LGTM. All three implementations faithfully reproduce the C++ DAC behavior. The code is correct, the debug validation is properly structured, and the edge cases align with the reference. Minor suggestions below are non-blocking.

---

Detailed Findings

✅ Correctness — Faithful C++ DAC translation

I verified each method against the C++ reference implementations:

• GetActiveRejitILCodeVersionNode: The C++ version calls GetActiveILCodeVersion(pModule, methodTk) directly, while the cDAC resolves Module → LookupTables → MethodDesc first, then calls GetActiveILCodeVersion(methodDesc). This is necessary because the ICodeVersions contract only exposes the MethodDesc-based overload. The filtering logic (IsValid && IsExplicit ↔ !IsNull() && !IsDefaultVersion()) and rejit state check are equivalent. The TryGetContract<IReJIT> is the correct runtime equivalent of the C++ #ifdef FEATURE_REJIT — when ReJIT is unavailable, the condition short-circuits and output stays 0, matching the #else branch.

• GetNativeCodeVersionNode: The C++ version uses CodeVersionManager::GetNativeCodeVersion(pMD, codeStartAddress) which internally iterates native code versions with a linear scan. The cDAC version iterates IL versions → native versions per IL version, covering the same set. The IsExplicit check correctly maps to C++ AsNode() returning NULL for synthetic versions.

• GetILCodeVersionNode: Correctly creates an explicit NativeCodeVersionHandle from the input pointer, calls GetILCodeVersion, and filters with IsValid && IsExplicit (equivalent to C++ !IsDefaultVersion()).

✅ Debug validation — Correct pattern

All three #if DEBUG blocks correctly:

• Call the legacy implementation with a stack-local output variable
• Use Debug.ValidateHResult(hr, hrLocal) with the default AllowDivergentFailures mode
• Only compare output values when hr == HResults.S_OK
• Include diagnostic format strings in Debug.Assert messages

This matches the established pattern used throughout the file (e.g., GetMDStructuresVersion, GetTypeHandle, GetAppDomainId).

💡 Suggestion — Minor inconsistency in null-pointer check pattern

The new methods check for null output pointers inside the try block and throw ArgumentException:

if (pVmILCodeVersionNode is null)
    throw new ArgumentException("Output pointer cannot be null.", ...);
*pVmILCodeVersionNode = 0;

Existing neighboring methods (e.g., GetMDStructuresVersion at line 1174, GetAppDomainId at line 87) initialize the output before the try block without a null check:

*pMDStructuresVersion = 0;  // before try, no null check
int hr = HResults.S_OK;
try { ... }

The new approach is actually safer and matches the C++ DAC's explicit null check (if (pVmILCodeVersionNode == NULL) return E_INVALIDARG), so it's arguably better. Just noting the inconsistency with sibling methods — not a blocker. The ArgumentException.HResult (0x80070057) correctly maps to E_INVALIDARG.

💡 Suggestion — Long compound condition could benefit from a clarifying comment

Line 1227 has a dense four-part condition. The C++ source at dacdbiimpl.cpp:7643-7647 includes a helpful comment explaining the two definitions of "active":

"Be careful, there are two different definitions of 'active' being used here. For the CodeVersionManager, the active IL version is whatever one should be used in the next invocation. 'Rejit active' narrows that to only include rejit IL bodies where the profiler has already provided the definition."

Consider adding a similar comment to the cDAC implementation for maintainability.

Generated by Code Review for issue #126980 · ◷

[noahfalk]

You could replace this double loop with codeVersions.GetNativeCodeVersionForIP(). Exposing GetSpecificNativeCodeVersion might perform a little better so you aren't re-finding the MethodDesc.

[max-charlamb]

I'd prefer to not expose this separate API. It breaks when a method desc is not versionable and duplicates the functionality of GetNativeCodeVersionForIP.

I see @noahfalk comment below about this, but calling GetNativeCodeVersionForIP should have little extra overhead as the MethodDesc and CodeBlockHandle are both cached and just dictionary lookups.

[rcj1]

Ok, I’m fine with it either way

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.