Reduce raw global / typeinfo usage in cDAC Legacy interfaces#129260
Reduce raw global / typeinfo usage in cDAC Legacy interfaces#129260max-charlamb wants to merge 2 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
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.TableDataoffset calculation intoILoader.GetLookupTablesvia a newTableDataOffsetfield onModuleLookupTables. - 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. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| Target.TypeInfo arrayTypeInfo = _target.GetTypeInfo(DataType.Array); | ||
| ulong numComponentsOffset = (ulong)_target.GetTypeInfo(DataType.Array).Fields[Constants.FieldNames.Array.NumComponents].Offset; | ||
| data->Size += _target.Read<uint>(objAddr + numComponentsOffset) * data->dwComponentSize; | ||
| } | ||
| else if (mt == _stringMethodTable.Value) | ||
| else if (mt == runtimeTypeSystemContract.GetWellKnownMethodTable(Contracts.WellKnownMethodTable.String)) |
| @@ -131,6 +141,9 @@ public interface IRuntimeTypeSystem : IContract | |||
| // True if the MethodTable is the System.Object MethodTable (g_pObjectClass) | |||
| bool IsObject(TypeHandle typeHandle) => throw new NotImplementedException(); | |||
| bool IsString(TypeHandle typeHandle) => throw new NotImplementedException(); | |||
| // Returns the address of one of the runtime's well-known singleton MethodTables, | |||
| // or TargetPointer.Null if the runtime has not yet initialized that global. | |||
| TargetPointer GetWellKnownMethodTable(WellKnownMethodTable kind) => throw new NotImplementedException(); | |||
| bool IsObjRef(TypeHandle typeHandle) => throw new NotImplementedException(); | |||
| public record struct ModuleLookupTables( | ||
| TargetPointer FieldDefToDesc, | ||
| TargetPointer ManifestModuleReferences, | ||
| TargetPointer MemberRefToDesc, | ||
| TargetPointer MethodDefToDesc, | ||
| TargetPointer TypeDefToMethodTable, | ||
| TargetPointer TypeRefToMethodTable, | ||
| TargetPointer MethodDefToILCodeVersioningState); | ||
| TargetPointer MethodDefToILCodeVersioningState, | ||
| uint TableDataOffset); | ||
|
|
||
| public readonly struct RuntimeDomainInfo | ||
| { | ||
| public TargetPointer SystemDomain { get; init; } | ||
| public TargetPointer DefaultAppDomain { get; init; } | ||
| public uint DefaultAppDomainId { get; init; } | ||
| } | ||
|
|
||
| public readonly struct LoaderHeapBlockData | ||
| { | ||
| public TargetPointer Address { get; init; } | ||
| public TargetNUInt Size { get; init; } | ||
| public TargetPointer NextBlock { get; init; } | ||
| } | ||
|
|
||
| public interface ILoader : IContract | ||
| { | ||
| static string IContract.Name => nameof(Loader); | ||
|
|
||
| ModuleHandle GetModuleHandleFromModulePtr(TargetPointer modulePointer) => throw new NotImplementedException(); | ||
| ModuleHandle GetModuleHandleFromAssemblyPtr(TargetPointer assemblyPointer) => throw new NotImplementedException(); | ||
| IEnumerable<ModuleHandle> GetModuleHandles(TargetPointer appDomain, AssemblyIterationFlags iterationFlags) => throw new NotImplementedException(); | ||
| TargetPointer GetRootAssembly() => throw new NotImplementedException(); | ||
| string GetAppDomainFriendlyName() => throw new NotImplementedException(); | ||
| RuntimeDomainInfo GetDomainInfo() => throw new NotImplementedException(); | ||
| TargetPointer GetModule(ModuleHandle handle) => throw new NotImplementedException(); |
| | --- | --- | --- | | ||
| | `AppDomain` | TargetPointer | Pointer to the global AppDomain | | ||
| | `SystemDomain` | TargetPointer | Pointer to the global SystemDomain | | ||
| | `DefaultADID` | uint | Id of the default AppDomain | |
There was a problem hiding this comment.
| | `DefaultADID` | uint | Id of the default AppDomain | | |
| | `DefaultADID` | uint | Id of the global AppDomain | |
This is the same thing as what AppDomain points to, so we should call it the same.
(I am not sure whether we need ADID in the data contract. It exists just to pass a dummy value around a few APIs. Having a local constant for that as needed would be fine.)
| // Aggregate view of the runtime's domain singletons. | ||
| readonly struct RuntimeDomainInfo | ||
| { | ||
| TargetPointer SystemDomain { get; init; } |
There was a problem hiding this comment.
SystemDomain is a grab bag of some statics. It does not make sense as a concept. It may be better to convert the few fields that we need to read from it to regular statics and read them directly.
There was a problem hiding this comment.
If you incorporate this and my other feedback, RuntimeDomainInfo will be unnecessary since there is going to be just the DefaultAppDomain left.
There was a problem hiding this comment.
Heads up - As-is reporting SystemDomain = NULL to SOS is going to give errors in some cases but I think it will be easy to make an SOS change to fix them.
There was a problem hiding this comment.
Made changes in diagnostics to handle a NULL systemdomain gracefully. dotnet/diagnostics#5878
- 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
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 - Update Loader.md documentation to match Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
max-charlamb
force-pushed
the
removeGlobalUsageInLegacyInterfaces
branch
from
4b91585 to
f8464a3
Compare
🤖 Copilot Code Review — PR #129260Note This review was generated by GitHub Copilot. Holistic AssessmentMotivation: Well-justified layering improvement. The Legacy project had 22+ raw Approach: Clean and mechanical. Each refactored read moves exactly one layer down into the contract implementation without changing semantics. The latest revision correctly incorporates maintainer feedback (jkotas) by simplifying Summary: Detailed Findings✅ Correctness — Managed refactoring is behavior-preservingAll managed call sites maintain identical semantics:
|
| TargetPointer MethodDefToDesc, | ||
| TargetPointer TypeDefToMethodTable, | ||
| TargetPointer TypeRefToMethodTable, | ||
| TargetPointer MethodDefToILCodeVersioningState); | ||
| TargetPointer MethodDefToILCodeVersioningState, | ||
| uint TableDataOffset); |
| ClrDataAccess::GetAssemblyList(CLRDATA_ADDRESS addr, int count, CLRDATA_ADDRESS values[], int *pNeeded) | ||
| { | ||
| if (addr == (CLRDATA_ADDRESS)NULL) | ||
| return E_INVALIDARG; | ||
|
|
| Target.TypeInfo arrayTypeInfo = _target.GetTypeInfo(DataType.Array); | ||
| ulong numComponentsOffset = (ulong)_target.GetTypeInfo(DataType.Array).Fields[Constants.FieldNames.Array.NumComponents].Offset; |
Note
This PR description was AI-generated by GitHub Copilot.
Refactors the
Microsoft.Diagnostics.DataContractReader.Legacyproject 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 correspondingContracts/*_1impl) without changing what bytes get read.Changes
1.
ILoader.GetLookupTablesowns theModuleLookupMap.TableDataoffsetAdds a
TableDataOffsetfield to the existingModuleLookupTablesrecord struct, populated byLoader_1.GetLookupTables.SOSDacImpl.GetModuleDatano longer reads raw typeinfo via_target.GetTypeInfo(DataType.ModuleLookupMap)and instead consumes the offset from the struct, going through a smallReadMapBasehelper that null-guards the table pointer.This removes the only direct
GetTypeInfocall in the Legacy project.2.
ILoader.GetAppDomainfor the global AppDomain pointerAdds a simple
GetAppDomain()accessor onILoaderthat reads and dereferences theAppDomainglobal pointer. All Legacy call sites that previously didReadGlobalPointer+ReadPointerinline now call this single method.RuntimeDomainInfostruct andGetDomainInfo()were removed per review feedback.DefaultADIDwas removed from the data contract -- Legacy call sites use a localconst uint DefaultAppDomainId = 1instead.3.
IRuntimeTypeSystem.GetWellKnownMethodTablefor runtime singleton MTsIntroduces a
WellKnownMethodTableenum (Object,String,Array,Exception,Free,Canon) and aGetWellKnownMethodTable(kind)accessor onIRuntimeTypeSystem.RuntimeTypeSystem_1implements it with a safeTryReadGlobalPointer+TryReadPointerpair so callers (e.g.GetUsefulGlobalsduring early load notifications) getTargetPointer.Nullwhen 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/_objectMethodTablelazy fields;GetObjectDataroutes through the contract;GetUsefulGlobalsreads all 5 well-known MTs via the contract.DacDbiImpl:IsExceptionObject(Exception MT) andEnumerateTypeParameters(Canon MT).Dbi/Helpers/HeapWalk: ctor (Free MT).4. DAC: Remove SystemDomain special-casing
GetAppDomainStoreDatanow reportssystemDomain = 0. The SystemDomain guard inGetAppDomainData,GetAssemblyList, andGetAppDomainNamehas been removed -- these methods now always treat the address as an AppDomain.Important
Breaking behavior change:
DacpAppDomainStoreData::systemDomainwill now beNULL. SOS consumers that assumesystemDomainis always valid need to be updated. The companion diagnostics PR handles this: dotnet/diagnostics#5878Testing
All cDAC unit tests pass (2493 passed, 16 skipped). Test updates:
ClrDataTaskTests.GetCurrentAppDomainregisters theILoadercontract.DacDbiImplTests.IsExceptionObjectnow mocksIRuntimeTypeSystem.GetWellKnownMethodTableinstead of setting up the rawExceptionMethodTableglobal.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.