Ignore AppDomain address parameters in ISOSDacInterface

[max-charlamb]

Note

This PR was created with assistance from GitHub Copilot.

CoreCLR only has a single AppDomain. The addr/domain parameters passed to GetAppDomainData, GetAssemblyList, and GetAppDomainName are historical artifacts from the multi-AppDomain era (.NET Framework). These methods now use AppDomain::GetCurrentDomain() (DAC) or ILoader.GetAppDomain() (cDAC) directly instead of trusting the caller-provided address.

Changes

DAC (request.cpp):

• GetAppDomainData -- uses AppDomain::GetCurrentDomain() instead of casting addr
• GetAssemblyList -- uses AppDomain::GetCurrentDomain() instead of casting addr
• GetAppDomainName -- uses AppDomain::GetCurrentDomain() instead of casting addr

cDAC (SOSDacImpl.cs):

• Same 3 methods use loader.GetAppDomain() instead of the input address

All addr == 0 precondition checks are removed since the parameter is no longer used.

Testing

All cDAC unit tests pass (2509 passed, 0 failed).

Follow-up from

#129260

[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

This PR updates the SOS DAC (ISOSDacInterface) AppDomain-related APIs to stop trusting the caller-provided AppDomain address and instead query the current/default AppDomain directly (native DAC: AppDomain::GetCurrentDomain(), cDAC: ILoader.GetAppDomain()), reflecting CoreCLR’s single-AppDomain model.

Changes:

• Native DAC (request.cpp): GetAppDomainData, GetAssemblyList, and GetAppDomainName now use AppDomain::GetCurrentDomain() and no longer validate addr != 0.
• cDAC (SOSDacImpl.cs): the same methods now use loader.GetAppDomain() and no longer validate addr != 0.
• Removes addr == 0 precondition checks since addr is no longer used.

Reviewed changes

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

File	Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs	Switches AppDomain-dependent methods to use ILoader.GetAppDomain() instead of the input address.
src/coreclr/debug/daccess/request.cpp	Switches AppDomain-dependent methods to use AppDomain::GetCurrentDomain() instead of the input address.

[github-actions]

Copilot Code Review

Holistic Assessment

Motivation: Well-justified follow-up to #129260. CoreCLR has exactly one AppDomain, so the addr parameter in these three ISOSDacInterface methods was dead weight from the .NET Framework era. Removing its usage eliminates a class of potential bugs where callers could pass stale/wrong addresses.

Approach: Correct and minimal — replaces PTR_AppDomain(TO_TADDR(addr)) with AppDomain::GetCurrentDomain() in native DAC and loader.GetAppDomain() in cDAC. The null-check on the result (returning E_FAIL / throwing) is a sensible defensive guard for the extreme edge case of diagnostics attaching before AppDomain initialization.

Summary: ✅ LGTM. Clean, focused change that correctly eliminates dead parameter usage in GetAppDomainData, GetAssemblyList, and GetAppDomainName across both DAC and cDAC. One minor observation below (non-blocking).

---

Detailed Findings

Detailed Findings

✅ Correctness — Safe behavioral change

The removal of addr == 0 → E_INVALIDARG checks is correct because addr is now completely unused. The new GetCurrentDomain() == NULL → E_FAIL path is a safer guard — it protects against the (unlikely) scenario of diagnostics attaching before the AppDomain is constructed, rather than checking for a meaningless caller error.

In GetAssemblyList, moving the validation inside SOSDacEnter() / SOSDacLeave() (instead of the old early-return before SOSDacEnter) is actually a consistency improvement, matching the pattern in GetAppDomainData.

✅ DAC/cDAC alignment — Consistent approach

Both native DAC and managed cDAC make the same logical change for all three methods. The cDAC GetAppDomainName already ignored addr (calling loader.GetAppDomainFriendlyName() with no address argument); this PR correctly adds only the documenting comment there.

💡 Minor — HRESULT discrepancy in null-AppDomain error path (non-blocking)

When GetAppDomain() returns null in the cDAC path (GetAppDomainData, GetAssemblyList), the code throws InvalidOperationException, whose default HResult is COR_E_INVALIDOPERATION (0x80131509). The native DAC returns E_FAIL (0x80004005) in the same scenario. The #if DEBUG validation blocks would flag this mismatch if it were ever hit.

This is non-blocking because:

1. The AppDomain is initialized extremely early — this path is practically unreachable when diagnostics tools are attached.
2. Both return "failure" to the caller; the specific HRESULT distinction is unlikely to matter.

If you wanted perfect parity, you could use Marshal.ThrowExceptionForHR(HResults.E_FAIL) or a COMException with E_FAIL, but this is strictly optional.

✅ Scope — Appropriately focused

GetFailedAssemblyList (line 2594 in request.cpp) still uses the caller-provided address, which is correctly documented as out-of-scope for this PR.

Note

This review was created by GitHub Copilot.

Generated by Code Review for issue #129476 · ● 16.6M · ◷