Implement GetIlCodeAndSig DacDbi API in cDAC#128354
Open
Copilot wants to merge 3 commits into
Open
Conversation
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Contributor
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Contributor
There was a problem hiding this comment.
Pull request overview
Implements the managed cDAC path for GetILCodeAndSig, replacing an E_NOTIMPL fallback with metadata/loader-based IL lookup and signature-token extraction.
Changes:
- Adds IL header size/code-size helpers.
- Implements
DacDbiImpl.GetILCodeAndSig. - Adds metadata token and HRESULT constants needed by the implementation.
Show a summary per file
| File | Description |
|---|---|
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/HeaderReaderHelpers.cs |
Adds helpers for IL header size and code size decoding. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs |
Implements cDAC GetILCodeAndSig behavior. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/EcmaMetadataUtils.cs |
Adds mdtSignature token type. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/CorDbHResults.cs |
Adds CORDBG_E_FUNCTION_NOT_IL. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 3
Contributor
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (3)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:1361
- pLocalSigToken is pre-initialized to TokenType.mdtSignature (0x11000000). Native DAC initializes this out param to mdSignatureNil (0) and only sets a non-zero token when a LocalVarSigTok exists. Initializing to the token type mask yields an invalid token on success for methods without locals (and will mismatch the legacy DAC behavior).
if (dwExactGenericArgsTokenIndex == 0)
{
// In a rare case of VS4Mac debugging VS4Mac ARM64 optimized code we get a null
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:1371
- GetILCodeAndSig doesn’t validate that functionToken is actually a MethodDef token before using it with MetadataReader and the MethodDefToDesc lookup. Other methods in this type explicitly validate token type (e.g., GetActiveRejitILCodeVersionNode throws ArgumentException when the token type isn’t mdtMethodDef). Adding the same check here should help keep HRESULT behavior aligned with the native DAC (E_INVALIDARG for invalid tokens) and avoid BadImageFormat/other accidental failures.
{
*pRetVal = rawToken;
}
else
{
// The real generics type token is the MethodTable of the "this" object.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:1365
- GetILCodeAndSig now has a non-trivial managed implementation (metadata validation, MethodDesc mapping, IL header parsing) but there are no unit/integration tests exercising the new behavior (success path, non-IL methods -> CORDBG_E_FUNCTION_NOT_IL, methods without locals -> mdSignatureNil, etc.). This file already has targeted tests for other DacDbiImpl entry points, so adding coverage here would help prevent regressions and ensure behavior matches the native DAC.
try
{
if (dwExactGenericArgsTokenIndex == 0)
{
// In a rare case of VS4Mac debugging VS4Mac ARM64 optimized code we get a null
// generics argument token. We aren't sure why the token is null, it may be a bug
// or it may be by design in the runtime. In the interest of time we are working
// around the issue rather than investigating the root cause.
if (rawToken == 0)
- Files reviewed: 4/4 changed files
- Comments generated: 3
Comment on lines
+20
to
+28
| // see ECMA-335 II.25.4 | ||
| ushort sizeAndFlags = target.Read<ushort>(ilHeader); | ||
| CorILMethodFlags flags = (CorILMethodFlags)(sizeAndFlags & (int)CorILMethodFlags.CorILMethod_FormatMask); | ||
|
|
||
| return flags switch | ||
| { | ||
| CorILMethodFlags.CorILMethod_TinyFormat => 1, | ||
| CorILMethodFlags.CorILMethod_FatFormat => 12, | ||
| _ => throw new BadImageFormatException("Invalid IL method header."), |
| public const int CORDBG_E_READVIRTUAL_FAILURE = unchecked((int)0x80131c49); | ||
| public const int ERROR_BUFFER_OVERFLOW = unchecked((int)0x8007006F); // HRESULT_FROM_WIN32(ERROR_BUFFER_OVERFLOW) | ||
| public const int CORDBG_E_CLASS_NOT_LOADED = unchecked((int)0x80131303); | ||
| public const int CORDBG_E_FUNCTION_NOT_IL = unchecked((int)0x8013130a); |
Comment on lines
17
to
26
| // ECMA-335 II.22 | ||
| // Metadata table index is the most significant byte of the 4-byte token | ||
| public enum TokenType : uint | ||
| { | ||
| mdtTypeRef = 0x01 << 24, | ||
| mdtTypeDef = 0x02 << 24, | ||
| mdtFieldDef = 0x04 << 24, | ||
| mdtMethodDef = 0x06 << 24, | ||
| mdtSignature = 0x11 << 24, | ||
| } |
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.
Port the
GetILCodeAndSigDacDbi entry point from the native DAC to the managed cDAC, returning the target-address/size of a method's IL and its local-variable signature token.