Add cDAC dump collection provider - #132584
Conversation
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Adds a Windows NativeAOT mscordaccore.dll provider backed by cDAC contracts for dump collection and metadata, integrated with CoreCLR.
Changes:
- Adds COM entrypoints and cDAC-based memory enumeration.
- Collects managed objects, methods, types, modules, and mini metadata.
- Integrates provider builds, resources, installation, and solution configuration.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Summary / final review note |
|---|---|
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.ICLRDataEnumMemoryRegions.cs |
Forwards legacy memory-region enumeration calls. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ICLRData.cs |
Adds callback declarations. Critical: the typed callback may break legacy COM marshalling. Nit: the new public API lacks approval. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/CdacErrorLogger.cs |
Adds legacy diagnostics logging. Nit: appears unused. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/ObjectCollector.cs |
Collects objects and type names. Moderate: cancellation HRESULTs are swallowed. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MiniMetadataWriter.cs |
Writes DacStreams mini metadata. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/Microsoft.Diagnostics.DataContractReader.DumpCollect.csproj |
Defines the NativeAOT provider project. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MethodCollector.cs |
Collects method dependencies and names. Moderate: cancellation HRESULTs are swallowed. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MemoryRegionEnumerator.cs |
Bridges cDAC reads and callbacks. Critical: collection mode is incorrectly derived, causing mini and triage dumps to run heap phases. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/GenerateWindowsVersionResource.ps1 |
Generates the Windows version resource. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/Entrypoints.cs |
Exposes the COM entrypoint. Critical: IXCLRDataProcess initialization fails. Nit: integration coverage is missing. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/DumpCreator.cs |
Coordinates dump collection. Critical: mini dumps omit managed code regions and module PE/debug data. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/DumpCollectLogger.cs |
Provides provider diagnostics. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/ContractDescriptorLocator.cs |
Locates the runtime contract descriptor. |
src/native/managed/cdac/cdac.slnx |
Adds the provider to the cDAC solution. |
src/coreclr/runtime.proj |
Publishes and configures the provider. Critical: Windows debug-subset builds publish neither the managed replacement nor an installed native DAC. |
src/coreclr/dlls/mscoree/coreclr/GenClrDebugResource.ps1 |
Generates the CLR debug resource. |
src/coreclr/dlls/mscoree/coreclr/CMakeLists.txt |
Embeds provider resource identity. |
src/coreclr/dlls/mscordac/CMakeLists.txt |
Adjusts native DAC installation behavior. |
Suppressed comments (6)
src/coreclr/dlls/mscoree/coreclr/CMakeLists.txt:250
- When
CLR_DUMP_COLLECT_PROVIDER_PATHis set, the command consumes that external provider at line 249, but this dependency list only tracks the nativemscordaccoretarget. Republishing/replacing the managed provider without rebuilding that target can leaveclr_debug_resource.binwith stale DAC timestamp/image-size identity, so incremental builds may embed an invalid provider identity. Add the selected provider path toDEPENDS(while retaining the target dependency for the fallback case).
DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/GenClrDebugResource.ps1" mscordaccore mscordbi
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/DumpCreator.cs:60
- The
trycovers an entire phase, so one unreadable module, thread, or stack frame exits the phase and skips every later item. That undermines the stated best-effort behavior and can omit most regions from a dump because of one corrupt target structure. Catch and log failures at the individual-item loops (while preserving cancellation), leaving this wrapper for phase-level failures.
try
{
enumerate();
DumpCollectLogger.Log($"Completed {phase} enumeration.");
}
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MethodCollector.cs:27
- Both dependency enumeration calls run before the only guarded naming operation. If one frame points at unreadable/corrupt MethodDesc data, the exception escapes
CaptureMethod, aborts the entirethreadsphase throughDumpCreator.TryEnumerate, and prevents later threads from being collected. Handle failures per frame/dependency (while propagating cancellation) so one bad frame does not discard the rest of a best-effort dump.
EnumerateMethodDependencies(methodDesc);
EnumerateMethodDescDataDependencies(methodDesc);
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MethodCollector.cs:83
- The existing native DacStreams producer truncates a method name at the first
(and appends()before callingDacMdCacheAddEEName(seeMethodDesc::EnumMemoryRegions), but this path stores the full signature. The replacement provider therefore produces different names for!clrstackand other consumers than the established mini-metadata format; apply the same truncation before adding the method name.
TypeNameBuilder.AppendMethodInternal(
_target,
name,
method,
TypeNameFormat.FormatSignature |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MiniMetadataWriter.cs:19
- The new NativeAOT provider and its COM callback paths have no automated tests. Existing
DacStreamsTestsexercise the reader, but not the generated COM ABI, callback HRESULT/cancellation behavior, callback2 updates, pointer-size serialization, or the end-to-end mini/heap collection. Add focused tests (or a Windows end-to-end test) before relying on the manual dump check for this low-level provider.
public static void Write(
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/ObjectCollector.cs:79
- Because the type and parent pointers come from target memory, this loop has no termination guard. A corrupt or cyclic MethodTable parent chain will spin forever, and the surrounding best-effort catch cannot recover from a hang; track visited MethodTable addresses or cap the traversal, as the thread walk does.
while (type.Address != TargetPointer.Null)
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
Suppressed comments (2)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MemoryRegionEnumerator.cs:33
clrFlagsis documented as reserved/ignored by the native implementation, and createdump always passesCLRDATA_ENUM_MEM_HEAP2even for--normaland--triage(crashinfo.cpp:378-383). As written, every non-full createdump request therefore runsEnumerateGC, loader heaps, sync blocks, and stress-log collection, turning mini/triage dumps into heap-sized dumps. Select heap enumeration fromMiniDumpWithPrivateReadWriteMemory, as the existing DAC does.
bool includeHeap =
clrFlags is CLRDataEnumMemoryFlags.CLRDATA_ENUM_MEM_HEAP or CLRDataEnumMemoryFlags.CLRDATA_ENUM_MEM_HEAP2
|| (miniDumpFlags & MiniDumpWithPrivateReadWriteMemory) != 0;
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/ObjectCollector.cs:68
- This catch also consumes
COR_E_OPERATIONCANCELEDthrown by_emitter.Addabove. Cancellation while collecting an exception object is therefore ignored and dump generation continues, unlike everyTryEnumeratepath. Filter cancellation out so it reachesEnumMemoryRegionsand is returned to the caller.
catch (System.Exception)
{
}
|
Azure Pipelines: Successfully started running 6 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/coreclr/pal/prebuilt/inc/clrdata.h:155
- This file identifies itself as MIDL-generated at line 4, while the source
src/coreclr/inc/clrdata.idlis unchanged. Revert this whitespace-only generated-output churn rather than modifying the prebuilt artifact directly.
src/coreclr/pal/prebuilt/idl/clrdata_i.cpp:26 - This file identifies itself as MIDL-generated at line 4, while the source
src/coreclr/inc/clrdata.idlis unchanged. Revert this whitespace-only generated-output churn rather than modifying the prebuilt artifact directly.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MemoryRegionEnumerator.cs:35
- This mode selection collapses every non-heap request into the regular mini path.
CLRDATA_ENUM_MEM_TRIAGE/MiniDumpFilterTriageare valid triage modes, andMiniDumpWithFullAuxiliaryStatehas separate native behavior (enummem.cpp:2044-2061). Since this DLL replaces the auxiliary provider, those requests silently receive the wrong region set. Dispatch supported modes explicitly, or reject/fallback for unsupported modes rather than treating them as mini.
bool includeHeap =
clrFlags is CLRDataEnumMemoryFlags.CLRDATA_ENUM_MEM_HEAP or CLRDataEnumMemoryFlags.CLRDATA_ENUM_MEM_HEAP2
|| (miniDumpFlags & MiniDumpWithPrivateReadWriteMemory) != 0;
new DumpCreator(target, runtimeModule, includeHeap, emitter).EnumerateMemoryRegions();
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Suppressed comments (10)
Previously missed (7) — in code that hasn't changed since the last review.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/ObjectCollector.cs:68
- This catch also swallows
COR_E_OPERATIONCANCELEDraised by_emitter.Addor an implicit target read. The caller's cancellation is then ignored and dump collection continues; the surroundingDumpCreator.TryEnumerateexplicitly preserves cancellation, so this catch must do the same.
catch (System.Exception)
{
}
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/ObjectCollector.cs:160
- Target reads performed while formatting the type name can invoke the emitter and throw
COR_E_OPERATIONCANCELED; this blanket catch suppresses that cancellation and lets collection continue. Preserve cancellation here just asDumpCreator.TryEnumeratedoes.
catch (System.Exception)
{
}
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MethodCollector.cs:91
- Formatting a method performs target reads, so this catch can consume a
COR_E_OPERATIONCANCELEDfrom the enumeration callback. That prevents cancellation from reachingEnumMemoryRegionsand causes more callbacks after the caller requested termination.
catch (System.Exception)
{
}
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MemoryRegionEnumerator.cs:126
- This passes zero-extended
TargetPointer.Valueaddresses to a callback whose address type isCLRDATA_ADDRESS. On 32-bit targets, that ABI requires sign extension (TO_CDADDRindacimpl.h:42;ConversionExtensions.ToClrDataAddressfollows the same rule), and this provider is enabled for x86. Regions above0x7fffffffwill therefore be reported under the wrong 64-bit address. Normalize callback addresses using the target pointer size before invoking the callback.
This issue also appears on line 174 of the same file.
int hr = _enumMemoryRegion(callback, address, chunkSize);
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MethodCollector.cs:87
- Mini-metadata method names must match the native DAC's compatibility shape. The native implementation formats the signature but then truncates at the first
(and appends)because!analyzeassumes that argument-list form (method.cpp:3965-3975). Writing the full signature here changes fallback symbols; truncate it before adding the name.
TypeNameFormat.FormatSignature |
TypeNameFormat.FormatNamespace |
TypeNameFormat.FormatFullInst);
src/coreclr/pal/prebuilt/inc/clrdata.h:155
- This file identifies itself as “ALWAYS GENERATED” at line 4, and the PR only removes generator-produced whitespace without changing
clrdata.idl. Revert this unrelated generated-file edit rather than hand-modifying prebuilt output.
src/coreclr/pal/prebuilt/idl/clrdata_i.cpp:26 - This is an “ALWAYS GENERATED” MIDL output file (line 4), and these whitespace-only removals have no corresponding source IDL change. Revert the generated-file churn.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MemoryRegionEnumerator.cs:33
clrFlagsis intentionally ignored by the existing DAC;ClrDataAccess::EnumMemoryRegionsselects heap mode only fromMiniDumpWithPrivateReadWriteMemory(enummem.cpp:1991-2062). Callers such as createdump passCLRDATA_ENUM_MEM_HEAP2for every non-full dump, so consulting it here can turn a normal/triage dump into a heap dump. Derive this solely fromminiDumpFlagsto remain a drop-in provider.
bool includeHeap =
clrFlags is CLRDataEnumMemoryFlags.CLRDATA_ENUM_MEM_HEAP or CLRDataEnumMemoryFlags.CLRDATA_ENUM_MEM_HEAP2
|| (miniDumpFlags & MiniDumpWithPrivateReadWriteMemory) != 0;
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MemoryRegionEnumerator.cs:35
- The provider never distinguishes
MiniDumpFilterTriage, so triage requests run the normal path. That path emitsExceptionData.Message, both stack-trace strings, Watson buckets, and module paths; the native triage worker deliberately omits the message and rewrites stack traces to remove file information (enummem.cpp:524-539, 638-653) because triage dumps must be PII-free. Pass a dump mode intoDumpCreatorand implement the triage-specific omissions/updates before enabling this provider.
new DumpCreator(target, runtimeModule, includeHeap, emitter).EnumerateMemoryRegions();
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MemoryRegionEnumerator.cs:174
UpdateMemoryRegionalso takes aCLRDATA_ADDRESS, butbufferAddress.Valueis zero-extended on x86. If the mini-metadata buffer is above 2 GB, the update targets a different 64-bit address and the dump retains invalid/stale stream data. Apply the same target-width sign extension used for enumeration callbacks.
hr = updateMemoryRegion(callback2, address, (uint)buffer.Length, bufferPointer);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MiniMetadataWriter.cs:46
- Stopping at the first name that does not fit drops every later entry, even though later names may be shorter and fit in the remaining mini-metadata buffer. The native writer rejects only the individual oversized entry and continues accepting subsequent names (
daccess.cpp:2412-2421). Continue here instead so one long generic name does not discard unrelated method/type names.
if (offset > buffer.Length - entrySize)
break;
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/DumpCreator.cs:245
- This cleanup-list traversal has no cycle detection. Since dump collection is explicitly best-effort for partially unreadable or corrupt targets, a repeated
LinkNextvalue makes dump creation loop forever instead of returning a partial dump. Track visited cleanup blocks, as the thread traversal already does, and stop when an address repeats.
TargetPointer cleanup = syncBlock.GetSyncBlockFromCleanupList();
while (cleanup != TargetPointer.Null)
cleanup = syncBlock.GetNextSyncBlock(cleanup);
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MemoryRegionEnumerator.cs:35
- The non-heap provider path is not validated with an actual mini/triage dump. The existing dump harness only generates Heap and Full dumps (
DumpTests.targets:223-228), and Full dumps already contain process memory, so they cannot detect omitted regions in this path. Add a Mini (and ideally Triage) dump case that verifies stack walking and mini-metadata names before making this provider the default.
bool includeHeap =
clrFlags is CLRDataEnumMemoryFlags.CLRDATA_ENUM_MEM_HEAP or CLRDataEnumMemoryFlags.CLRDATA_ENUM_MEM_HEAP2
|| (miniDumpFlags & MiniDumpWithPrivateReadWriteMemory) != 0;
new DumpCreator(target, runtimeModule, includeHeap, emitter).EnumerateMemoryRegions();
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d56e1d60-7e1a-4353-8d3f-27deac700b4f
3619b59 to
4f3c6f7
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d56e1d60-7e1a-4353-8d3f-27deac700b4f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d56e1d60-7e1a-4353-8d3f-27deac700b4f
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MemoryRegionEnumerator.cs:35
- Mode selection ignores
MiniDumpFilterTriage(0x00100000), although the native DAC derives triage mode fromminiDumpFlagsand treats theclrFlagsargument as reserved. A triage request therefore follows the normal mini path here, andDumpCreatoremits full module paths and exception message/stack-trace strings instead of applying the native triage PII filtering. Decode the triage flag and propagate that mode so sensitive regions are skipped or poisoned consistently withEnumMemoryRegionsWorkerMicroTriage; add triage dump coverage as well.
bool includeHeap =
clrFlags is CLRDataEnumMemoryFlags.CLRDATA_ENUM_MEM_HEAP or CLRDataEnumMemoryFlags.CLRDATA_ENUM_MEM_HEAP2
|| (miniDumpFlags & MiniDumpWithPrivateReadWriteMemory) != 0;
new DumpCreator(target, runtimeModule, includeHeap, emitter).EnumerateMemoryRegions();
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ICLRData.cs:13
- This introduces a new public enum and changes the public COM interface parameter type, but the PR does not link an issue with the
api-approvedlabel. New public managed surface needs an approved API shape; otherwise keep the existingintsignature and use an internal enum inside the provider.
public enum CLRDataEnumMemoryFlags
Add runtime contract descriptor discovery and statically link the PAL into createdump so the managed dump collector can replace the legacy DAC across supported platforms. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d56e1d60-7e1a-4353-8d3f-27deac700b4f
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 3 comments.
Suppressed comments (2)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/MemoryRegionEnumerator.cs:33
clrFlagscannot determine heap mode here: createdump passesCLRDATA_ENUM_MEM_HEAP2for every non-full dump (src/coreclr/debug/createdump/crashinfo.cpp:381-386), while the legacy DAC derives MINI/HEAP2/TRIAGE fromminiDumpFlags(src/coreclr/debug/daccess/enummem.cpp:2044-2061). Consequently, normal mini and triage dumps enter the heap branch; triage can include the managed heap despite its size/privacy filtering. Derive the complete mode fromminiDumpFlags, propagate triage semantics intoDumpCreator, and add triage dump coverage.
bool includeHeap =
clrFlags is CLRDataEnumMemoryFlags.CLRDATA_ENUM_MEM_HEAP or CLRDataEnumMemoryFlags.CLRDATA_ENUM_MEM_HEAP2
|| (miniDumpFlags & MiniDumpWithPrivateReadWriteMemory) != 0;
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DumpCollect/DumpCreator.cs:245
- The cleanup list is target-controlled and may contain a cycle when the process state is corrupt. This loop then never terminates (and cached contract reads can keep returning the same nodes), contradicting the collector's best-effort handling of corrupt targets. Track visited addresses, as the thread traversal already does.
TargetPointer cleanup = syncBlock.GetSyncBlockFromCleanupList();
while (cleanup != TargetPointer.Null)
cleanup = syncBlock.GetNextSyncBlock(cleanup);
Avoid duplicate ARM64 PAL symbols, bound corrupt SyncBlock cleanup traversal, and sign-extend 32-bit addresses passed to dump callbacks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d56e1d60-7e1a-4353-8d3f-27deac700b4f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d56e1d60-7e1a-4353-8d3f-27deac700b4f
Order the DAC PAL before the PAL archive so GNU ld resolves the bundled libunwind symbols, and document the remaining expected CI failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d56e1d60-7e1a-4353-8d3f-27deac700b4f
Mach-O symbol lookup returns a relocated process address, so use it directly instead of adding the module base a second time. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d56e1d60-7e1a-4353-8d3f-27deac700b4f
| # legs that run on top of it: cDAC, cDAC_verify, DAC. | ||
| # Each leg sets testInterpreter: true so interpreter coverage | ||
| # is exercised inline (no separate Interpreter leg). | ||
| # -rc release -lc release -clrinterpreter) consumed by the |
There was a problem hiding this comment.
Did you have a plan in mind for how this work would eventually get checked in? Right now it looks like we are disabling DAC and cDAC_verify testing which I'd be very wary of doing until we are confident that the fragile implementation of DAC/DBI will not ship in the next version of .NET and we are no longer getting value from cDAC_verify style comparison testing. I do think we are getting there, but now seemed too soon.
A few options I thought of:
- treat it as a proof of concept, test it manually, but then leave it disabled for now until the rest of work to remove DAC/DBI is ready.
- make use of a feature flag (UseCdacDumpCollectProvider) and have test legs that run both with the flag enabled and disabled. To keep CI costs down we'd probably only want it opt-in or on a low scheduled cadence.
There was a problem hiding this comment.
If we get a good enough signal, feature flag it would be my vote.
| goto exit; | ||
| } | ||
| #ifndef CDAC_DUMP_COLLECTOR | ||
| hr = pfnCLRDataCreateInstance(__uuidof(IXCLRDataProcess), dataTarget, (void**)&m_pClrDataProcess); |
There was a problem hiding this comment.
There is some functionality loss around not implementing this right? Is it a TODO for later or there is an alternative plan for how this info gets populated?
There was a problem hiding this comment.
Added TODO: [cdac] for:
CrashInfo::EnumerateManagedModules()CrashInfo::UnwindAllThreads()
which require theIXCLRDataProcessinterface.
| AddRef(); | ||
| return S_OK; | ||
| } | ||
| else if (InterfaceId == IID_ICLRContractLocator) |
There was a problem hiding this comment.
I'd recommend we try to phase out this interface and instead define cDAC API exports that take the contract address directly as a parameter in the method signature. This means on Windows we'd need two entrypoints (one that implements the MAP extensibility point and one that createdump calls) but I think that is still clearer sometimes passing in this interface and sometimes not.
There was a problem hiding this comment.
Changed to match the pattern of the new dbgshim api which passes in the descriptoraddress directly.
| add_dependencies(coreclr clr_debug_resources) | ||
| add_dependencies(coreclr_static clr_debug_resources mscordaccore) | ||
| add_dependencies(coreclr_static clr_debug_resources) | ||
| if(NOT CLR_DUMP_COLLECT_PROVIDER_PATH) |
There was a problem hiding this comment.
Nit: I missed this at first - moving this right after the clr_debug_resource target was defined would help it be visible.
| if (flags.HasFlag(ModuleFlags.ReflectionEmit)) | ||
| _emitter.RegisterMetadataRange(ecmaMetadata.GetReadWriteSavedMetadataAddress(module)); | ||
|
|
||
| if (loader.TryGetSymbolStream(module, out TargetPointer symbolBuffer, out uint symbolSize)) |
There was a problem hiding this comment.
did the original DAC code capture dynamic/in-memory symbol streams? I would not have expected that.
There was a problem hiding this comment.
The DAC does not explicitly enumerate the in-memory symbol buffer, but it is included in heap dumps.
For Windows heap dumps, createdump requests MiniDumpWithPrivateReadWriteMemory and passes it to MiniDumpWriteDump. DbgHelp documents that this flag scans the virtual address space for PAGE_READWRITE memory.
On Unix, createdump directly adds every read/write native mapping for a heap dump. Since CGrowableStream allocates its buffer using new char[], the buffer is captured.
I updated the collector, so the symbol buffer is explicitly emitted only when heap collection is enabled. Mini dumps should no longer include it. While this isn't explicitly required, I like having more explicit enumeration.
| loader.GetFileName(module); | ||
|
|
||
| if (loader.TryGetLoadedImageContents(module, out _, out _, out _)) | ||
| _emitter.RegisterMetadataRange(ecmaMetadata.GetReadOnlyMetadataAddress(module)); |
There was a problem hiding this comment.
Do we do similar masking in the DAC implementation of the dump reader? Does something differ so the masking matters here and didn't matter before?
There was a problem hiding this comment.
Yes, the legacy DAC does essentially the same thing.
When it creates the host-side metadata importer, it reads the target metadata using DacInstantiateTypeByAddressNoReport. That creates a normal DAC cache entry but marks it with the noReport bit. When the DAC reports memory that was implicitly touched during enumeration, DumpAllInstances skips entries with noReport set. The comment there notes that metadata is the only memory currently excluded this way.
| if (bytesRead != (uint)buffer.Length) | ||
| return HResults.E_FAIL; | ||
|
|
||
| if (emitter.ShouldEmitTargetRead(address, bytesRead)) |
There was a problem hiding this comment.
I know DAC does this implicit recording of every piece of memory that got touched while doing the exploration and I think thats a fine starting point for a port. I'm hoping that we'd be able to move away from this eventually (in a future PR) and more precisely document exactly what memory goes into the dump. Right now its unclear to me how much memory gets swept in through this mechanism and of that how much is actually needed.
There was a problem hiding this comment.
I think this is a pretty reasonable way of doing the enumeration. It directly connects what is expected to work to the collection. If we change the commands to require more memory, the enumeration is automatically updated.
Agreed we can discuss other approaches moving forwards.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d56e1d60-7e1a-4353-8d3f-27deac700b4f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d56e1d60-7e1a-4353-8d3f-27deac700b4f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d56e1d60-7e1a-4353-8d3f-27deac700b4f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d56e1d60-7e1a-4353-8d3f-27deac700b4f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d56e1d60-7e1a-4353-8d3f-27deac700b4f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d56e1d60-7e1a-4353-8d3f-27deac700b4f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d56e1d60-7e1a-4353-8d3f-27deac700b4f
Add an explicit descriptor-address entrypoint while retaining the existing discovery path for compatibility. Remove createdump's ICLRContractLocator implementation and the corresponding native IDL surface. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d56e1d60-7e1a-4353-8d3f-27deac700b4f
Summary
EnumMemoryprovider backed by the managed cDAC contractsmscordaccorebinary whenUseCdacDumpCollectProvider=trueMemory policy
Runtime integration
CoreCLR publishes the provider under the existing
mscordaccorename and omits the native DAC only when the opt-in property is enabled. Default builds continue to use the legacy DAC.createdumplocatesDotNetRuntimeContractDescriptorin regular, single-file, and NativeAOT layouts.Createdump now probes
IXCLRDataProcessas an optional provider capability. The EnumMemory provider returnsE_NOINTERFACE, allowing dump creation to continue without a build-timeCDAC_DUMP_COLLECTORdistinction. Until smaller targeted APIs are added, createdump does not:These limitations affect createdump's native bookkeeping and optional crash-report JSON. The collector independently captures the contract memory needed for subsequent SOS, ClrMD, and cDAC dump analysis.
Diagnostics CI
The existing
cDAC,cDAC_verify, andDACSOS legs remain unchanged and consume the standard runtime build. A separateAllSubsets_CoreCLR_EnumMemorybuild publishes uniquely suffixed_enum_memoryartifacts for the newcDAC_EnumMemoryleg.