Implement ISOSDacInterface::GetLoaderAllocatorHeapNames and GetLoaderAllocatorHeaps in cDAC SOSDacImpl#124818
Implement ISOSDacInterface::GetLoaderAllocatorHeapNames and GetLoaderAllocatorHeaps in cDAC SOSDacImpl#124818
Conversation
Co-authored-by: max-charlamb <[email protected]>
Co-authored-by: max-charlamb <[email protected]>
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs
Outdated
Show resolved
Hide resolved
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Co-authored-by: max-charlamb <[email protected]>
|
@copilot, try again |
…dd missing using Co-authored-by: max-charlamb <[email protected]>
Implementation reworked in commits up to 0554c43. Changes: Feature-conditional heap names: GetLoaderAllocatorHeaps: Fully implemented in SOSDacImpl.cs, reading heap pointers through the Native data descriptors: Added Tests: 32 new test cases (8 test methods × 4 architectures) covering both methods. All 864 cDAC tests pass. |
max-charlamb
left a comment
There was a problem hiding this comment.
@copilot , address feedback
...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs
Outdated
Show resolved
Hide resolved
…ps API Replace separate GetLoaderAllocatorHeapNames() and GetLoaderAllocatorHeaps(TargetPointer) with a single GetLoaderAllocatorHeaps(TargetPointer) returning IReadOnlyDictionary<string, TargetPointer>. SOSDacImpl.GetLoaderAllocatorHeapNames now queries the global loader allocator to get names from the dictionary. GetLoaderAllocatorHeaps iterates dictionary values. Tests updated to use dictionary-based mock and verify unordered name/address sets. Co-authored-by: max-charlamb <[email protected]>
max-charlamb
left a comment
There was a problem hiding this comment.
@copilot , address feedback
...native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs
Show resolved
Hide resolved
...native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs
Outdated
Show resolved
Hide resolved
...native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs
Outdated
Show resolved
Hide resolved
…tionary - Use nameof(Data.LoaderAllocator.X) instead of hardcoded strings for all dictionary keys in GetLoaderAllocatorHeaps - Make FixupPrecodeHeap and NewStubPrecodeHeap conditional on ContainsKey (exclude from dictionary when field absent from data descriptor) - Remove else branch that padded IndcellHeap/CacheEntryHeap with null when VirtualCallStubManager is null - only include them when VcsManager exists - All 864 cDAC tests pass Co-authored-by: max-charlamb <[email protected]>
Addressed all three review comments in e0e9cd5 —
Addressed all three review comments in e0e9cd5 — |
|
@copilot , please fix build break |
datadescriptor.h did not include virtualcallstub.h, so the cdac_data<VirtualCallStubManager> template specialization was not visible when datadescriptor.inc was compiled. This caused a build failure referencing cdac_data<VirtualCallStubManager>::IndcellHeap and cdac_data<VirtualCallStubManager>::CacheEntryHeap. Co-authored-by: max-charlamb <[email protected]>
Fixed in 3f63f2f. The issue was that Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs
Show resolved
Hide resolved
|
@copilot, address feedback from Noah. We should document any new datadescriptors and APIs in the relevant cDAC markdown file. |
Add documentation for the new ILoader.GetLoaderAllocatorHeaps API, including: - New API in the contract API section - New data descriptor fields for LoaderAllocator (StaticsHeap, ExecutableHeap, FixupPrecodeHeap, NewStubPrecodeHeap, DynamicHelpersStubHeap, VirtualCallStubManager) and VirtualCallStubManager (IndcellHeap, CacheEntryHeap) in the data descriptors table - Pseudocode implementation showing feature-conditional dictionary building Co-authored-by: max-charlamb <[email protected]>
…nager and PortableEntryPoint Co-authored-by: max-charlamb <[email protected]>
Documented in ab7f065. Added to
Also resolved a merge conflict in DataType.cs in a54d098 (both VirtualCallStubManager and PortableEntryPoint needed). All 900 cDAC tests pass. |
max-charlamb
left a comment
There was a problem hiding this comment.
@copilot , address feedback
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs
Show resolved
Hide resolved
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs
Show resolved
Hide resolved
…AllocatorHeaps Expand #if DEBUG validation blocks to compare actual name strings and heap pointer/kind values between cDAC and legacy DAC, following the pattern used by GetAssemblyList and GetMethodsWithProfilerModifiedIL. Co-authored-by: max-charlamb <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR implements ISOSDacInterface13.GetLoaderAllocatorHeapNames and ISOSDacInterface13.GetLoaderAllocatorHeaps in the managed cDAC, replacing legacy DAC delegation stubs. The implementation adds native data descriptors for loader allocator heaps and VirtualCallStubManager heaps, creates corresponding managed data classes, extends the ILoader contract, and includes comprehensive tests.
Changes:
- Added native data descriptors for LoaderAllocator heaps (StaticsHeap, ExecutableHeap, and feature-conditional heaps) and new VirtualCallStubManager type with heap fields
- Implemented
GetLoaderAllocatorHeapsin the ILoader contract returning a dictionary of heap names to pointers, with dynamic inclusion based on data descriptor field presence - Implemented SOSDacImpl methods to query heap names and addresses through the ILoader contract, with DEBUG validation against legacy DAC
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/loaderallocator.hpp | Added cdac_data fields for StaticsHeap, ExecutableHeap, and feature-conditional heap fields (FixupPrecodeHeap, NewStubPrecodeHeap, DynamicHelpersStubHeap, VirtualCallStubManager) |
| src/coreclr/vm/virtualcallstub.h | Created cdac_data specialization for VirtualCallStubManager with IndcellHeap and CacheEntryHeap fields |
| src/coreclr/vm/datadescriptor/datadescriptor.h | Added #include for virtualcallstub.h to expose VirtualCallStubManager data template |
| src/coreclr/vm/datadescriptor/datadescriptor.inc | Added LoaderAllocator heap fields and new VirtualCallStubManager type with matching feature guards |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/DataType.cs | Added VirtualCallStubManager enum value |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs | Added GetLoaderAllocatorHeaps method to ILoader interface |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/LoaderAllocator.cs | Added StaticsHeap, ExecutableHeap, and nullable feature-conditional heap properties, plus VirtualCallStubManager pointer |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/VirtualCallStubManager.cs | Created new data class with IndcellHeap and nullable CacheEntryHeap properties |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs | Implemented GetLoaderAllocatorHeaps with dictionary building logic that conditionally includes heaps based on data descriptor field presence |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs | Implemented GetLoaderAllocatorHeapNames and GetLoaderAllocatorHeaps with lazy initialization, caching, and DEBUG validation |
| src/native/managed/cdac/tests/LoaderTests.cs | Added 8 test methods covering count queries, full retrieval, insufficient buffers, and null parameters for both APIs |
| docs/design/datacontracts/Loader.md | Documented new API, data descriptor fields, and pseudocode implementation |
| private nint[]? _loaderAllocatorHeapNamePtrs; | ||
|
|
||
| private nint[] GetOrInitializeHeapNamePtrs() | ||
| { | ||
| if (_loaderAllocatorHeapNamePtrs is not null) | ||
| return _loaderAllocatorHeapNamePtrs; | ||
|
|
||
| Contracts.ILoader contract = _target.Contracts.Loader; | ||
| TargetPointer globalLoaderAllocator = contract.GetGlobalLoaderAllocator(); | ||
| IReadOnlyDictionary<string, TargetPointer> heaps = contract.GetLoaderAllocatorHeaps(globalLoaderAllocator); | ||
| nint[] ptrs = new nint[heaps.Count]; | ||
| int i = 0; | ||
| foreach (string name in heaps.Keys) | ||
| { | ||
| ptrs[i++] = Marshal.StringToHGlobalAnsi(name); | ||
| } | ||
| _loaderAllocatorHeapNamePtrs = ptrs; | ||
| return ptrs; | ||
| } |
There was a problem hiding this comment.
Memory allocated by Marshal.StringToHGlobalAnsi is never freed, causing a memory leak. The cached _loaderAllocatorHeapNamePtrs array holds unmanaged memory pointers that persist for the lifetime of the SOSDacImpl instance, but there's no cleanup mechanism (no finalizer, IDisposable implementation, or cleanup in Flush method). Consider adding cleanup logic to free the allocated memory, such as implementing IDisposable or adding cleanup to the Flush method.
| foreach (TargetPointer heap in heaps.Values) | ||
| { | ||
| pLoaderHeaps[i] = heap.ToClrDataAddress(_target); |
There was a problem hiding this comment.
Dictionary enumeration order is not guaranteed to be consistent, which causes a critical mismatch between GetLoaderAllocatorHeapNames and GetLoaderAllocatorHeaps. The native implementation uses a fixed-order static array (LoaderAllocatorLoaderHeapNames in request.cpp lines 3683-3699) that guarantees the heap names at index i correspond to the heap addresses at index i. The cDAC implementation iterates over heaps.Keys in GetLoaderAllocatorHeapNames (line 4475) and heaps.Values in GetLoaderAllocatorHeaps (line 4553), but Dictionary iteration order is not guaranteed to match between Keys and Values enumerations across different API calls or even within the same call in some runtime scenarios. This breaks the API contract where consumers expect names[i] to correspond to heaps[i]. Consider using an ordered collection (e.g., List of KeyValuePair or a custom ordered structure) or ensuring consistent ordering by sorting both Keys and Values by key name.
| foreach (TargetPointer heap in heaps.Values) | |
| { | |
| pLoaderHeaps[i] = heap.ToClrDataAddress(_target); | |
| foreach (KeyValuePair<string, TargetPointer> entry in heaps.OrderBy(entry => entry.Key)) | |
| { | |
| pLoaderHeaps[i] = entry.Value.ToClrDataAddress(_target); |
| public void GetLoaderAllocatorHeapNames_GetNames(MockTarget.Architecture arch) | ||
| { | ||
| ISOSDacInterface13 impl = CreateSOSDacImplForHeapTests(arch); | ||
|
|
||
| int needed; | ||
| int hr = impl.GetLoaderAllocatorHeapNames(0, null, &needed); | ||
| Assert.Equal(MockHeapDictionary.Count, needed); | ||
|
|
||
| char** names = stackalloc char*[needed]; | ||
| hr = impl.GetLoaderAllocatorHeapNames(needed, names, &needed); | ||
|
|
||
| Assert.Equal(HResults.S_OK, hr); | ||
| Assert.Equal(MockHeapDictionary.Count, needed); | ||
| HashSet<string> expectedNames = new(MockHeapDictionary.Keys); | ||
| for (int i = 0; i < needed; i++) | ||
| { | ||
| string actual = Marshal.PtrToStringAnsi((nint)names[i])!; | ||
| Assert.Contains(actual, expectedNames); | ||
| } | ||
| } |
There was a problem hiding this comment.
The tests verify that heap names exist in the expected set but don't verify the ordering correspondence between GetLoaderAllocatorHeapNames and GetLoaderAllocatorHeaps. The legacy DAC contract requires that names[i] corresponds to heaps[i] for the same index i. Consider adding a test that calls both APIs and verifies that the name at each index matches the expected name for the heap address at that index, ensuring the ordering contract is preserved.
Description
Implements
ISOSDacInterface13.GetLoaderAllocatorHeapNamesandISOSDacInterface13.GetLoaderAllocatorHeapsin the managed cDACSOSDacImpl.cs, replacing the legacy DAC delegation stubs. Based onClrDataAccess::GetLoaderAllocatorHeapNamesandClrDataAccess::GetLoaderAllocatorHeapsinrequest.cpp.Native Data Descriptors
cdac_data<LoaderAllocator>inloaderallocator.hpp: StaticsHeap, ExecutableHeap, FixupPrecodeHeap (#ifdef HAS_FIXUP_PRECODE), NewStubPrecodeHeap (#ifndef FEATURE_PORTABLE_ENTRYPOINTS), DynamicHelpersStubHeap (FEATURE_READYTORUN && FEATURE_STUBPRECODE_DYNAMIC_HELPERS), VirtualCallStubManagercdac_data<VirtualCallStubManager>invirtualcallstub.h: IndcellHeap (unconditional), CacheEntryHeap (#ifdef FEATURE_VIRTUAL_STUB_DISPATCH)datadescriptor.incwith matching feature guardsVirtualCallStubManagertoDataTypeenum#include "virtualcallstub.h"todatadescriptor.hsocdac_data<VirtualCallStubManager>is visible whendatadescriptor.incis compiledManaged Data Classes
Data/LoaderAllocator.cswith optional heap fields (FixupPrecodeHeap, NewStubPrecodeHeap, DynamicHelpersStubHeap as nullable) and always-present fields (StaticsHeap, ExecutableHeap, VirtualCallStubManager)Data/VirtualCallStubManager.cswith IndcellHeap and optional CacheEntryHeapILoader Contract
GetLoaderAllocatorHeaps(TargetPointer)method toILoaderinterface returningIReadOnlyDictionary<string, TargetPointer>— dictionary keys are heap names, values are heap pointersLoader_1.cswith dynamic dictionary building based on data descriptor field presence viaContainsKeychecks, matching the native#ifdefconditional compilationnameof(Data.LoaderAllocator.X)andnameof(Data.VirtualCallStubManager.X)instead of hardcoded stringsSOSDacImpl
GetLoaderAllocatorHeapNames: Queries global loader allocator viaILoader.GetLoaderAllocatorHeapsto get heap names from dictionary keys. Lazily initializes ANSI string pointers viaMarshal.StringToHGlobalAnsi, cached on the instance. Standard cDAC pattern with try/catch,S_FALSEwhen buffer is insufficientGetLoaderAllocatorHeaps: Reads heap pointers through the ILoader contract dictionary, fills LoaderHeapKindNormal for all heaps#if DEBUGvalidation blocks that compare cDAC results against legacy DAC output — heap name strings and heap pointer/kind values are validated to match, following the pattern used by other cDAC APIsContract Documentation
docs/design/datacontracts/Loader.mdwith:GetLoaderAllocatorHeapsAPI in the contract API sectionLoaderAllocator(StaticsHeap, ExecutableHeap, FixupPrecodeHeap, NewStubPrecodeHeap, DynamicHelpersStubHeap, VirtualCallStubManager) and newVirtualCallStubManagertype (IndcellHeap, CacheEntryHeap) in the data descriptors tableTests
LoaderTests.cs× 4 architectures = 32 new test casesGetLoaderAllocatorHeapNames: Count query, full name retrieval, insufficient buffer, null pNeededGetLoaderAllocatorHeaps: Count query, full heap retrieval, insufficient buffer, null address💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.