dbgshim: activate cDAC via contract descriptor#5941
Open
hoyosjs wants to merge 2 commits into
Open
Conversation
When servicing a data-access interface (`IXCLRDataProcess` / `ISOSDacInterface`), dbgshim now resolves the target runtime module's `DotNetRuntimeContractDescriptor` export itself and activates the cDAC through the universal binary's new `DbgShimCreateInstanceFromContractDescriptor` entry point (dotnet/runtime#131357), passing the descriptor address directly. This is needed because dbgshim's data targets don't always implement `ICLRContractLocator`, so the existing `CLRDataCreateInstance` path can't source the descriptor on them. Behavior: - Descriptor not resolvable -> cDAC can't succeed, so it's skipped and we fall through to the legacy DAC. - Descriptor resolvable but the cDAC binary predates the new export -> falls back to `CLRDataCreateInstance` on the same binary (temporary while cDAC gets updated in the tools). - `LegacyDacOnly` policy and the public dbgshim API are unchanged; this is purely additive. - On Windows-host targets the descriptor is read via a small PE export-directory walk (`TryGetPEExportSymbol`) that we already use in runtime; ELF/Mach-O use the existing `TryGetSymbol`. ## Testing `DbgShim.UnitTests` 104/104 pass. The contract path itself can't be exercised until a `mscordaccore_universal` carrying the new export flows in; it degrades gracefully until then.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates dbgshim’s cDAC activation path to resolve the runtime’s DotNetRuntimeContractDescriptor export directly (including via a Windows-host PE export-directory walk) and, when available, activate cDAC using the universal binary’s new DbgShimCreateInstanceFromContractDescriptor entry point. This improves compatibility with data targets that don’t implement ICLRContractLocator, while preserving the existing fallback behavior to the legacy DAC.
Changes:
- Introduces contract-descriptor resolution in dbgshim and routes cDAC activation through
DbgShimCreateInstanceFromContractDescriptorwith a fallback toCLRDataCreateInstance. - Adds a Windows-host helper (
TryGetPEExportSymbol) to resolve PE exports via the target data target (for Windows targets). - Centralizes the contract-descriptor symbol name as
CONTRACT_DESCRIPTOR_SYMBOLfor reuse.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/SOS/Strike/platform/runtimeimpl.cpp | Switches contract descriptor lookup to use shared CONTRACT_DESCRIPTOR_SYMBOL. |
| src/shared/debug/inc/runtimeinfo.h | Defines CONTRACT_DESCRIPTOR_SYMBOL constant for cross-component use. |
| src/shared/debug/inc/dbgutil.h | Declares TryGetPEExportSymbol (Windows-host PE export resolution). |
| src/shared/debug/dbgutil/dbgutil.cpp | Implements TryGetPEExportSymbol by walking the PE export directory via the target. |
| src/dbgshim/debugshim.cpp | Adds contract-descriptor resolution + new cDAC activation path using the descriptor-address entry point. |
Comments suppressed due to low confidence (1)
src/shared/debug/dbgutil/dbgutil.cpp:462
- The ordinal read from the name-ordinal table is untrusted data. Before using it to index into the export address table, validate it is within NumberOfFunctions; otherwise the code may read from an arbitrary RVA and return an invalid symbol address.
// If the name matches, we should be able to get the ordinal
uint16_t ordinalForNamedExport = 0;
if (FAILED(ReadFromDataTarget(dataTarget, baseAddress + ordinalTableRVA + sizeof(uint16_t) * nameIndex, (BYTE*)&ordinalForNamedExport, sizeof(ordinalForNamedExport))))
{
return false;
}
// If the name matches, we should be able to get the export
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Guard a zero export-directory RVA (a module with no export directory would otherwise parse its PE headers as an IMAGE_EXPORT_DIRECTORY), and bounds-check the export ordinal against NumberOfFunctions before indexing the export address table. Also zero the out-param on entry so early failure paths do not leave it uninitialized.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When servicing a data-access interface (
IXCLRDataProcess/ISOSDacInterface), dbgshim now resolves the target runtime module'sDotNetRuntimeContractDescriptorexport itself and activates the cDAC through the universal binary's newDbgShimCreateInstanceFromContractDescriptorentry point (dotnet/runtime#131357), passing the descriptor address directly.This is needed because dbgshim's data targets don't always implement
ICLRContractLocator, so the existingCLRDataCreateInstancepath can't source the descriptor on them.Behavior:
CLRDataCreateInstanceon the same binary (temporary while cDAC gets updated in the tools).LegacyDacOnlypolicy and the public dbgshim API are unchanged; this is purely additive.TryGetPEExportSymbol) that we already use in runtime; ELF/Mach-O use the existingTryGetSymbol.