Reduce raw global / typeinfo usage in cDAC Legacy interfaces by max-charlamb · Pull Request #129260 · dotnet/runtime

max-charlamb · 2026-06-10T20:27:03Z

Note

This PR description was AI-generated by GitHub Copilot.

Refactors the Microsoft.Diagnostics.DataContractReader.Legacy project to stop reading runtime globals and typeinfo by string key, routing them through typed contract accessors instead. Behavior-preserving: each refactor moves an existing read down one layer (from Legacy into the corresponding Contracts/*_1 impl) without changing what bytes get read.

Changes

1. ILoader.GetLookupTables owns the ModuleLookupMap.TableData offset

Adds a TableDataOffset field to the existing ModuleLookupTables record struct, populated by Loader_1.GetLookupTables. SOSDacImpl.GetModuleData no longer reads raw typeinfo via _target.GetTypeInfo(DataType.ModuleLookupMap) and instead consumes the offset from the struct, going through a small ReadMapBase helper that null-guards the table pointer.

This removes the only direct GetTypeInfo call in the Legacy project.

2. ILoader.GetAppDomain for the global AppDomain pointer

Adds a simple GetAppDomain() accessor on ILoader that reads and dereferences the AppDomain global pointer. All Legacy call sites that previously did ReadGlobalPointer + ReadPointer inline now call this single method.

RuntimeDomainInfo struct and GetDomainInfo() were removed per review feedback. DefaultADID was removed from the data contract -- Legacy call sites use a local const uint DefaultAppDomainId = 1 instead.

3. IRuntimeTypeSystem.GetWellKnownMethodTable for runtime singleton MTs

Introduces a WellKnownMethodTable enum (Object, String, Array, Exception, Free, Canon) and a GetWellKnownMethodTable(kind) accessor on IRuntimeTypeSystem. RuntimeTypeSystem_1 implements it with a safe TryReadGlobalPointer + TryReadPointer pair so callers (e.g. GetUsefulGlobals during early load notifications) get TargetPointer.Null when the runtime has not yet initialized a given global, matching the prior lazy-read behavior.

Refactors the Legacy project to use the new accessor:

SOSDacImpl: removes the _stringMethodTable / _objectMethodTable lazy fields; GetObjectData routes through the contract; GetUsefulGlobals reads all 5 well-known MTs via the contract.
DacDbiImpl: IsExceptionObject (Exception MT) and EnumerateTypeParameters (Canon MT).
Dbi/Helpers/HeapWalk: ctor (Free MT).

4. DAC: Remove SystemDomain special-casing

GetAppDomainStoreData now reports systemDomain = 0. The SystemDomain guard in GetAppDomainData, GetAssemblyList, and GetAppDomainName has been removed -- these methods now always treat the address as an AppDomain.

Important

Breaking behavior change: DacpAppDomainStoreData::systemDomain will now be NULL. SOS consumers that assume systemDomain is always valid need to be updated. The companion diagnostics PR handles this: dotnet/diagnostics#5878

Testing

All cDAC unit tests pass (2493 passed, 16 skipped). Test updates:

ClrDataTaskTests.GetCurrentAppDomain registers the ILoader contract.
DacDbiImplTests.IsExceptionObject now mocks IRuntimeTypeSystem.GetWellKnownMethodTable instead of setting up the raw ExceptionMethodTable global.

Out of scope

Several additional Legacy raw-global sites (e.g. DacNotificationFlags, StressLog, FeatureCOMInterop, SOSBreakingChangeVersion, TlsIndexBase/OffsetOfCurrentThreadInfo) remain and can be addressed in follow-up PRs where the contract-side surface is justified by callers beyond a single SOS API.

Copilot

Pull request overview

Refactors the cDAC Legacy implementation to reduce direct raw reads of runtime globals / typeinfo by routing those reads through typed contract APIs in the Abstractions/Contracts layer.

Changes:

Adds new contract accessors/types to centralize reading of domain globals (ILoader.GetDomainInfo) and well-known singleton MethodTables (IRuntimeTypeSystem.GetWellKnownMethodTable).
Moves ModuleLookupMap.TableData offset calculation into ILoader.GetLookupTables via a new TableDataOffset field on ModuleLookupTables.
Updates Legacy (SOS/IXCLRData*/DBI/heap walk) call sites and unit tests to consume the new contract-based APIs.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/native/managed/cdac/tests/UnitTests/DacDbiImplTests.cs	Updates tests to mock `GetWellKnownMethodTable` instead of setting up raw globals.
src/native/managed/cdac/tests/UnitTests/ClrDataTaskTests.cs	Updates test target setup to provide globals required by `ILoader.GetDomainInfo` and registers the Loader contract.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.IXCLRDataProcess.cs	Replaces raw AppDomain global reads with `Loader.GetDomainInfo().DefaultAppDomain`.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs	Removes remaining Legacy `GetTypeInfo(ModuleLookupMap)` usage and routes domain/MT reads via contracts.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/Helpers/HeapWalk.cs	Uses `GetWellKnownMethodTable(Free)` instead of reading `FreeObjectMethodTable` global directly.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Routes domain id / appdomain / exception+canon MT reads through the new contract accessors.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataTask.cs	Uses `Loader.GetDomainInfo().DefaultAppDomain` for `GetCurrentAppDomain`.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataFrame.cs	Uses `Loader.GetDomainInfo().DefaultAppDomain` for frame AppDomain/method-instance construction.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataAppDomain.cs	Uses `Loader.GetDomainInfo().DefaultAppDomainId` for AppDomain unique id.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs	Implements `GetWellKnownMethodTable` with safe `TryRead*` global dereferencing.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs	Adds `GetDomainInfo` and populates `ModuleLookupTables.TableDataOffset`.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeTypeSystem.cs	Introduces `WellKnownMethodTable` enum + `GetWellKnownMethodTable` API on the abstraction.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs	Extends `ModuleLookupTables` with `TableDataOffset` and adds `RuntimeDomainInfo` + `GetDomainInfo`.

Copilot

Pull request overview

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

Copilot

Pull request overview

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

> [!NOTE] > This PR was created with assistance from GitHub Copilot. The runtime will begin reporting `systemDomain` as NULL (0) from `GetAppDomainStoreData` (see dotnet/runtime#129260). This PR guards all SOS code that previously assumed `systemDomain` was always valid, matching the existing pattern used for `SharedDomain`. **Affected call sites:** - **`DumpDomain`** -- wraps SystemDomain info section in null guard; guards the single-domain label path - **`FindAppDomain`** -- guards the "System Domain" label comparison - **`GCHandleStatsForDomains::Init`** -- conditionally counts and adds SystemDomain to the domain array - **`ModuleFromName` (util.cpp)** -- conditionally adds SystemDomain to domain array - **`GetDomainList` (util.cpp)** -- conditionally adds SystemDomain to domain list Two existing call sites were already safe: `SaveModules` (has `!= 0` check) and `EEHeapCommand.cs` (`PrintAppDomain` already null-checks). --------- Co-authored-by: Max Charlamb <maxcharlamb@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

- Add ILoader.GetDomainInfo and route domain global reads through it - Move ModuleLookupMap TableData offset into ILoader.GetLookupTables - Add IRuntimeTypeSystem.GetWellKnownMethodTable for runtime singleton MTs - Document RuntimeDomainInfo, WellKnownMethodTable, and TableDataOffset - Address PR feedback on GetDomainInfo and naming

max-charlamb · 2026-06-16T15:55:17Z

/ba-g known CI infra issues. SOSTests same baseline across DAC and cDAC due to known issues

PR dotnet#5878 added null guards in SOS for the SystemDomain (which dotnet/runtime#129260 makes nullable on non-framework runtimes), but did not update the corresponding test scripts. The 'System Domain:' / 'LowFrequencyHeap' / 'HighFrequencyHeap' / 'Total size:' lines from the SystemDomain section are no longer printed on non-framework, so wrap them in IFDEF:DESKTOP. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

PR dotnet#5878 added null guards in SOS for the SystemDomain (which dotnet/runtime#129260 makes nullable on non-framework runtimes), but did not update the corresponding test scripts. The 'System Domain:' line from the SystemDomain section is no longer printed on non-framework, so: - GCTests.script (EEHeap) and OtherCommands.script (DumpDomain): wrap the 'System Domain:' VERIFY in IFDEF:DESKTOP. Other heap lines (LowFrequencyHeap/HighFrequencyHeap/Total size) still match against Domain 1 so they stay outside. - GCPOH.script (EEHeap): delete the 'System Domain:' VERIFY entirely; GCPOHTests is already skipped on Desktop (POH didn't exist before .NET 5). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

Address Jan's review feedback: - Remove RuntimeDomainInfo struct and GetDomainInfo(), replace with simple ILoader.GetAppDomain() returning TargetPointer - Remove DefaultADID from data contract globals, use local constant (value 1) in Legacy call sites - Remove SystemDomain special-casing from DAC request.cpp (GetAppDomainStoreData, GetAppDomainData, GetAssemblyList, GetAppDomainName) - Report systemDomain as NULL from GetAppDomainStoreData - Fix unused arrayTypeInfo variable in GetObjectData - Update Loader.md documentation to match Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

github-actions · 2026-06-16T19:48:51Z

Copilot Code Review

Holistic Assessment

Motivation: Well-justified refactoring that eliminates raw ReadGlobalPointer/GetTypeInfo usage from the Legacy project, routing them through typed contract accessors (ILoader.GetAppDomain(), IRuntimeTypeSystem.GetWellKnownMethodTable(), ModuleLookupTables.TableDataOffset). This reduces code duplication and establishes a cleaner boundary between Legacy consumers and the data contract layer.

Approach: Sound approach. Each refactoring moves one existing raw read into the contract layer without changing the semantics. The WellKnownMethodTable enum + TryRead pattern is a good fit for the early-init scenarios where globals may not yet be populated. The SystemDomain removal is coordinated with a companion diagnostics PR.

Summary: ✅ LGTM. The refactoring is behavior-preserving (with the documented breaking change for systemDomain = 0 handled by dotnet/diagnostics#5878). Code is clean, tests are updated, and the remaining open review comments about GetAppDomainData ignoring addr are inaccurate — addr IS used for module enumeration at line 128.

Detailed Findings

✅ Contract Accessor Design — Correct and defensive

RuntimeTypeSystem_1.GetWellKnownMethodTable correctly uses TryReadGlobalPointer + TryReadPointer with TargetPointer.Null fallback, matching the prior lazy-read behavior where globals might not be initialized during early load notifications. The switch expression with ArgumentOutOfRangeException for unknown enum values is idiomatic.

✅ `ILoader.GetAppDomain()` — Clean extraction

The extraction of the ReadGlobalPointer + ReadPointer pair into a single GetAppDomain() accessor eliminates repetition across 10+ call sites in the Legacy project. The internal reuse via ((ILoader)this).GetAppDomain() in GetRootAssembly and GetAppDomainFriendlyName keeps the contract consistent.

✅ `ReadMapBase` Helper — Good null-guard pattern

The static local ReadMapBase helper in GetModuleData properly null-guards the table pointer before adding the offset, returning default (0) for null tables. This is correct and matches the C++ DAC behavior where null map pointers would similarly resolve to 0.

✅ SystemDomain Removal — Consistent across C++ and cDAC

The removal of SystemDomain guards in request.cpp (GetAppDomainData, GetAssemblyList, GetAppDomainName) matches the cDAC changes. GetAppDomainStoreData now reports systemDomain = 0 in both paths. The breaking change is properly documented and the companion diagnostics PR handles SOS consumers.

✅ `GetUsefulGlobals` Behavior — Improved resilience

Previously, if any one global was missing, ALL values would be zeroed and an error HResult returned. Now, each GetWellKnownMethodTable call independently returns TargetPointer.Null (→ 0) for missing globals while returning S_OK for the overall call. This is actually better behavior and still consistent with the DEBUG validation logic (lines 4526–4533) which accepts either hr or hrLocal being S_OK.

✅ Test Updates — Adequate

ClrDataTaskTests.GetCurrentAppDomain correctly adds the ILoader contract registration. DacDbiImplTests.IsExceptionObject correctly switches to mocking IRuntimeTypeSystem.GetWellKnownMethodTable rather than setting up raw globals, with the now-unused exceptionMT parameter properly removed from CreateDacDbiWithExceptionMT.

💡 `GetObjectData` — Repeated uncached MT lookups

Lines 3333 and 3340 call GetWellKnownMethodTable(String) and GetWellKnownMethodTable(Object) on every GetObjectData invocation. Previously these were cached in Lazy<TargetPointer> fields. In a heap walk scenario calling GetObjectData for thousands of objects, this adds two TryReadGlobalPointer + TryReadPointer round-trips per object. In diagnostics context this is unlikely to be a meaningful regression, but noting it as a potential follow-up optimization if profiling shows overhead.

Note

This review was generated by GitHub Copilot.

Generated by Code Review for issue #129260 · ● 52.5M · ◷

Follow-up to #5878. dotnet/runtime#129260 makes `GetAppDomainStoreData::systemDomain` nullable (NULL on non-framework runtimes). #5878 added the null guards in SOS so the SystemDomain section is no longer emitted in that case, but the test scripts were not updated to match. This PR removes/guards the now-conditional `System Domain:` VERIFY in the three affected scripts: - `OtherCommands.script` (`DumpDomain`) and `GCTests.script` (`EEHeap`): wrap the `System Domain:` VERIFY in `IFDEF:DESKTOP`. The other heap lines (`LowFrequencyHeap`/`HighFrequencyHeap`/`Total size:`) still match against `Domain 1`'s heaps so they stay outside. - `GCPOH.script` (`EEHeap`): delete the `System Domain:` VERIFY entirely. `GCPOHTests` is already skipped on Desktop (`config.IsDesktop` short-circuit; POH was introduced in .NET 5), so an `IFDEF:DESKTOP` block would be dead code. Edit: See the following [runtime-diagnostics](https://dev.azure.com/dnceng-public/public/_build/results?buildId=1466895&view=results) run showing success Co-authored-by: Max Charlamb <maxcharlamb@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

max-charlamb · 2026-06-16T19:56:41Z

/ba-g previous successful pipeline run: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1466895&view=results

Copilot AI review requested due to automatic review settings June 10, 2026 20:27

Copilot started reviewing on behalf of max-charlamb June 10, 2026 20:27 View session

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

max-charlamb added area-Diagnostics-cdac and removed area-Diagnostics-coreclr labels Jun 10, 2026

max-charlamb requested review from barosiak, noahfalk and rcj1 June 10, 2026 20:28

This comment has been minimized.

Sign in to view

Copilot AI reviewed Jun 10, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

Copilot AI review requested due to automatic review settings June 10, 2026 23:26

Copilot started reviewing on behalf of max-charlamb June 10, 2026 23:26 View session

This comment has been minimized.

Sign in to view

Copilot AI reviewed Jun 10, 2026

View reviewed changes

jkotas reviewed Jun 11, 2026

View reviewed changes

Comment thread docs/design/datacontracts/Loader.md Outdated

jkotas reviewed Jun 11, 2026

View reviewed changes

Comment thread docs/design/datacontracts/Loader.md Outdated

max-charlamb mentioned this pull request Jun 12, 2026

Handle null SystemDomain in SOS commands dotnet/diagnostics#5878

Merged

Copilot AI review requested due to automatic review settings June 12, 2026 17:39

max-charlamb force-pushed the removeGlobalUsageInLegacyInterfaces branch from 4b91585 to f8464a3 Compare June 12, 2026 17:39

Copilot started reviewing on behalf of max-charlamb June 12, 2026 17:39 View session

This comment has been minimized.

Sign in to view

Copilot AI reviewed Jun 12, 2026

View reviewed changes

Comment thread ...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs

Comment thread src/coreclr/debug/daccess/request.cpp

Comment thread src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs Outdated

This was referenced Jun 12, 2026

Multiple Helix work items fail on maccatalyst/tvos CoreCLR Release #126460

Open

Arm/Arm64: Segfault in System.Security.Cryptography.Tests #129339

Closed

max-charlamb force-pushed the removeGlobalUsageInLegacyInterfaces branch from f8464a3 to d602046 Compare June 15, 2026 20:47

This comment has been minimized.

Sign in to view

This was referenced Jun 16, 2026

Unable to pull image from mcr.microsoft.com #117164

Open

CI build failure: error CS8032: An instance of analyzer Microsoft.NetCore.CSharp.Analyzers.... cannot be created #129031

Open

steveisok self-requested a review June 16, 2026 13:22

steveisok approved these changes Jun 16, 2026

View reviewed changes

max-charlamb added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 16, 2026

max-charlamb mentioned this pull request Jun 16, 2026

Update SOS test expectations for null SystemDomain dotnet/diagnostics#5881

Merged

Copilot AI review requested due to automatic review settings June 16, 2026 18:15

max-charlamb force-pushed the removeGlobalUsageInLegacyInterfaces branch from d602046 to 8595521 Compare June 16, 2026 18:15

Copilot started reviewing on behalf of max-charlamb June 16, 2026 18:16 View session

Copilot AI reviewed Jun 16, 2026

View reviewed changes

Comment thread src/coreclr/debug/daccess/request.cpp

Comment thread src/coreclr/debug/daccess/request.cpp

Comment thread src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs

This comment has been minimized.

Sign in to view

max-charlamb force-pushed the removeGlobalUsageInLegacyInterfaces branch from 8595521 to 4a8e236 Compare June 16, 2026 19:39

max-charlamb removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 16, 2026

max-charlamb merged commit 45d015d into main Jun 16, 2026
28 of 108 checks passed

max-charlamb deleted the removeGlobalUsageInLegacyInterfaces branch June 16, 2026 19:58

max-charlamb mentioned this pull request Jun 16, 2026

Ignore AppDomain address parameters in ISOSDacInterface #129476

Draft

This was referenced Jun 16, 2026

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

Unable to checkout dotnet/dnceng#4115

Open

Conversation

max-charlamb commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Testing

Out of scope

Uh oh!

This comment has been minimized.

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

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

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

max-charlamb commented Jun 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

github-actions Bot commented Jun 16, 2026

Copilot Code Review

Holistic Assessment

Detailed Findings

✅ Contract Accessor Design — Correct and defensive

✅ ILoader.GetAppDomain() — Clean extraction

✅ ReadMapBase Helper — Good null-guard pattern

✅ SystemDomain Removal — Consistent across C++ and cDAC

✅ GetUsefulGlobals Behavior — Improved resilience

✅ Test Updates — Adequate

💡 GetObjectData — Repeated uncached MT lookups

Uh oh!

max-charlamb commented Jun 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

max-charlamb commented Jun 10, 2026 •

edited

Loading

✅ `ILoader.GetAppDomain()` — Clean extraction

✅ `ReadMapBase` Helper — Good null-guard pattern

✅ `GetUsefulGlobals` Behavior — Improved resilience

💡 `GetObjectData` — Repeated uncached MT lookups