[cDAC] Implement simple DacDbi APIs with Loader contract

[rcj1]

Implement cDAC DacDbi APIs: GetModuleData, IsModuleMapped

[Copilot]

Pull request overview

Implements additional cDAC-backed IDacDbiInterface loader-related APIs by wiring them to the ILoader contract, and extends the Loader contract/data model to support DomainAssembly↔Module navigation and “is mapped” queries.

Changes:

• Implemented GetModuleData, GetModuleForDomainAssembly, GetDomainAssemblyFromModule, IsModuleMapped, and basic enumeration APIs in DacDbiImpl using the ILoader contract.
• Extended Loader contract + data descriptors with Module.DomainAssembly, plus new ILoader APIs for DomainAssembly↔Module mapping and module mapping status.
• Added/updated mock descriptors and unit tests for the new Loader contract behaviors.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
src/native/managed/cdac/tests/MockDescriptors/MockDescriptors.Loader.cs	Enhances mock module creation with Assembly.Module back-pointer and optional Module.DomainAssembly; adds helper to allocate a mock DomainAssembly.
src/native/managed/cdac/tests/MockDescriptors/MockDescriptors.cs	Adds DomainAssembly to mock Module field layout.
src/native/managed/cdac/tests/LoaderTests.cs	Adds unit tests for GetModuleForDomainAssembly and GetDomainAssemblyFromModule.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/IDacDbiInterface.cs	Updates enumeration callbacks to function pointer types and fixes IsModuleMapped out-parameter type to BOOL*.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements the new/updated DacDbi APIs via Loader contract and adds DEBUG cross-validation with legacy DAC.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Module.cs	Adds DomainAssembly field parsing to the Module data model.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs	Adds GetModuleForDomainAssembly, GetDomainAssemblyFromModule, and IsModuleMapped; refactors PE image retrieval.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs	Adds new Loader contract surface for DomainAssembly↔Module mapping and module mapping status.
src/coreclr/vm/datadescriptor/datadescriptor.inc	Publishes Module.DomainAssembly in the cDAC type descriptor.
src/coreclr/vm/ceeload.h	Adds cdac_data<Module>::DomainAssembly offset definition.
docs/design/datacontracts/Loader.md	Documents the new Loader APIs/field and updates pseudocode.

[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.

[jkotas]

What is pulling in DomainAssembly into the data contract? What would it take to avoid it?

For historic reasons, the loader implementation was split into number of types. This split does not make sense anymore. @elinor-fung has been cleaning it up by merging and deleting the split types. I expect DomainAssembly is going to be deleted once we get to it.

[rcj1]

It is GetModuleForDomainAssembly; there's lots of places in the DBI that pass a DomainAssembly into GetModuleForDomainAssembly.

[rcj1]

In fact, there are pointers to DomainAssembly embedded in the IPC event structures that get sent between DBI and runtime. I had run into these structures when attempting to implement other APIs; the issue is that the structures are currently shared between DBI and runtime and would have to be stabilized and separated to prepare for the splitting off of the DBI. Removing references to DomainAssembly would be another thing to keep in mind for the new API design.

[jkotas]

Hmm, we have #104590 tracking this, and there is a long discussion in #108618 about what needs to happen to clean it up in DBI.

We should make sure to put #104590 to the contract cleanup/simplification backlog.

[github-actions]

Note

This review was generated by Copilot (Claude Opus 4.6).

🤖 Copilot Code Review — PR #126774

Holistic Assessment

Motivation: This PR implements several cDAC DacDbi APIs (EnumerateAppDomains, EnumerateAssembliesInAppDomain, EnumerateModulesInAssembly, GetModuleForDomainAssembly, GetDomainAssemblyFromModule, GetModuleData, IsModuleMapped) that were previously delegating to the legacy DAC. Migrating these APIs to the cDAC path is a well-understood incremental goal for the diagnostics infrastructure.

Approach: The approach is sound — each API follows the established cDAC pattern (implement via ILoader contracts, wrap in try/catch with HRESULT conversion, and add #if DEBUG validation against legacy DAC). The refactoring of TryGetPEImage into a shared helper is a nice deduplication, and the type safety improvement of callback signatures from nint to delegate* unmanaged<ulong, nint, void> is a positive change. The CollectThreadCallback → CollectEnumerationCallback rename and reuse is clean.

Summary: ⚠️ Needs Changes. There is one regression bug (missing null check for LoadedImageLayout dropped during the TryGetPEImage refactoring) and several minor issues worth addressing.

---

Detailed Findings

❌ Missing null check for LoadedImageLayout in TryGetLoadedImageContents — Regression bug

File: Loader_1.cs, line 216

The refactoring to extract TryGetPEImage accidentally dropped the check for peImage.LoadedImageLayout == TargetPointer.Null from TryGetLoadedImageContents. The original code had three guard checks:

if (module.PEAssembly == TargetPointer.Null) return false;
if (peAssembly.PEImage == TargetPointer.Null) return false;
if (peImage.LoadedImageLayout == TargetPointer.Null) return false; // ← REMOVED

The new code only checks the first two (via TryGetPEImage) and then unconditionally calls _target.ProcessedData.GetOrAdd<Data.PEImageLayout>(peImage.LoadedImageLayout) — which will attempt to read from a null address if LoadedImageLayout is null.

The native equivalent HasLoadedPEImage() checks both HasPEImage() && GetPEImage()->HasLoadedLayout().

Notably, the new IsModuleMapped correctly includes this check:

if (peImage.LoadedImageLayout == TargetPointer.Null)
    return false;

Fix: Add the null check back to TryGetLoadedImageContents after the TryGetPEImage call:

if (!TryGetPEImage(handle, out Data.PEImage? peImage))
    return false;

if (peImage.LoadedImageLayout == TargetPointer.Null)
    return false; // no loaded image layout

This is merge-blocking — it is a behavioral regression from the pre-PR code.

---

⚠️ Output pointer dereference before null check — GetModuleForDomainAssembly, IsModuleMapped, GetDomainAssemblyFromModule

File: DacDbiImpl.cs, lines 320, 586, 626

Three methods write a default value to the output pointer before checking if that pointer is null:

// GetModuleForDomainAssembly (line 320)
*pModule = 0;                          // ← dereference
// ...
if (vmDomainAssembly == 0 || pModule == null)  // ← null check AFTER
    throw new ArgumentNullException(...);

Same pattern for *isModuleMapped = Interop.BOOL.FALSE (line 586) and *pVmDomainAssembly = 0 (line 626).

The native implementations check for null first (e.g., GetDomainAssemblyFromModule in dacdbiimpl.cpp checks pVmDomainAssembly == NULL before any write). If the callers ever pass a null output pointer, the cDAC would crash with an access violation before reaching the null check. While callers are unlikely to pass null in practice, the native code is defensive here and the cDAC should match.

Fix: Move the default-value assignment inside the try block, after the null check — as GetModuleData already correctly does at line 278 (*pData = default after the null guard).

Advisory — callers don't pass null, but the inconsistency with GetModuleData and the native code is worth fixing.

---

⚠️ GetModuleData — ArgumentNullException parameter name mismatch

File: DacDbiImpl.cs, line 272–273

if (vmModule == 0 || pData == null)
    throw new ArgumentNullException(nameof(pData));

When vmModule == 0, the exception incorrectly names pData as the null argument. The native equivalent simply asserts pData != NULL and relies on DAC pointer resolution to fail for invalid module pointers.

Advisory — the HRESULT is what matters for COM callers, not the exception message.

---

✅ IsMapped Webcil handling — Correct

File: Loader_1.cs, lines 225–231

The addition of if (peImageLayout.Format == (uint)ImageFormat.Webcil) return false; correctly matches the native PE_OR_WEBCIL(IsMapped(), FALSE) macro behavior. Webcil images are never considered mapped.

---

✅ DomainAssembly layout assumption — Verified correct

The cDAC GetModuleForDomainAssembly reads the first pointer at the DomainAssembly address to get the Assembly pointer. This is correct: DomainAssembly is a final class with no virtual methods (no vtable), and PTR_Assembly m_pAssembly is its sole member field. Verified in src/coreclr/vm/domainassembly.h.

---

✅ EnumerateModulesInAssembly semantics — Correct

Despite the confusing parameter name vmAssembly, the native signature is VMPTR_DomainAssembly vmAssembly — the parameter IS a DomainAssembly pointer (verified in dacdbiimpl.cpp:4712 and callers in rsappdomain.cpp). The cDAC correctly treats it as such by calling GetModuleForDomainAssembly. The native IsVisibleToDebugger() check is correctly skipped since it always returns TRUE (ceeload.cpp:2076-2082).

---

✅ Callback type safety improvements — Clean

Changing callback parameters from nint to delegate* unmanaged<ulong, nint, void> in both IDacDbiInterface and DacDbiImpl is a good type safety improvement. The CollectThreadCallback → CollectEnumerationCallback rename and reuse across all enumeration methods is clean and consistent.

---

✅ Debug validation pattern — Thorough

All new implementations include #if DEBUG validation against the legacy DAC with Debug.ValidateHResult and field-level assertions. The enumeration methods use SequenceEqual for list comparison, which verifies both content and order.

---

✅ Tests — Well-structured

The two new Loader contract tests (GetModuleForDomainAssembly_ReturnsCorrectModule and GetDomainAssemblyFromModule_ReturnsCorrectDomainAssembly) correctly validate the happy path with proper mock setup including the Assembly-Module back-pointer and DomainAssembly layout.

---

💡 GetModuleForDomainAssembly could reuse GetModuleHandleFromAssemblyPtr

File: Loader_1.cs, lines 65–79

After reading the assembly pointer from the DomainAssembly, the method could delegate to the existing GetModuleHandleFromAssemblyPtr rather than duplicating the Assembly-Module lookup:

return ((ILoader)this).GetModuleHandleFromAssemblyPtr(assemblyPointer);

Minor follow-up suggestion, not blocking.

Generated by Code Review for issue #126774 · ◷

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs:200

• TryGetLoadedImageContents no longer checks peImage.LoadedImageLayout for null before calling GetOrAdd on it. If the PE image exists but hasn’t been laid out yet, this will attempt to read a descriptor at address 0 and throw instead of returning false (as the method name/contract implies). Please restore the LoadedImageLayout == TargetPointer.Null check and return false in that case.

        if (!TryGetPEImage(handle, out Data.PEImage? peImage))
            return false; // no PE image

        Data.PEImageLayout peImageLayout = _target.ProcessedData.GetOrAdd<Data.PEImageLayout>(peImage.LoadedImageLayout);

        baseAddress = peImageLayout.Base;
        size = peImageLayout.Size;
        imageFlags = peImageLayout.Flags;

        return true;

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

[Copilot]

Argument validation conflates an invalid module pointer (pModule == 0) with a null output pointer, and throws ArgumentNullException(nameof(isModuleMapped)) in both cases. Split the checks so pModule==0 yields an invalid-argument HRESULT, and only treat isModuleMapped==null as a null-pointer case.

Suggested change

	try
	if (pModule == 0)
	throw new ArgumentException("Module pointer must not be zero.", nameof(pModule));
	if (isModuleMapped == null)

[Copilot]

GetModuleData is newly implemented using the Loader contract, but there are no tests exercising it (unlike other DacDbiImpl methods that have dump-based coverage). Consider adding a dump-based integration test that calls GetModuleData for at least one module and validates key fields (e.g., vmAssembly, vmPEAssembly, fIsDynamic/fInMemory) against Loader contract values.

[Copilot]

IsModuleMapped is newly implemented via Loader contract behavior (including S_FALSE when no loaded image), but there is no test coverage validating the HRESULT and BOOL output across common cases (mapped vs not mapped, and Webcil). Consider adding a dump-based test that locates a real module from the dump and verifies the result against Loader.IsModuleMapped/TryGetLoadedImageContents.