Implement CLRDataMethodDefinition IL extent APIs in cDAC - #131384
Merged
Conversation
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 4 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Copilot
AI
changed the title
Implement CLRDataMethodDefinition IL extents in cDAC
Implement CLRDataMethodDefinition IL extent APIs in cDAC
Jul 26, 2026
Contributor
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/native/managed/cdac/tests/UnitTests/IXCLRDataProcessTests.cs:252
- The test currently writes the fat header
CodeSizeusing the target endianness (arch.IsLittleEndian), but IL headers are fixed little-endian (ECMA-335) and the implementation should be reading viaReadLittleEndian. Encoding the header this way can let big-endian tests pass even if the production code is incorrectly using target-endian reads.
secondHeader[codeSizeByteOffset] = FatCodeSize;
- Files reviewed: 4/4 changed files
- Comments generated: 1
rcj1
marked this pull request as ready for review
July 26, 2026 19:13
|
Azure Pipelines: Successfully started running 4 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Contributor
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs:422
- StartEnumExtents dereferences
handlebefore validating it. Other similar COM enumeration entrypoints (e.g., ClrDataMethodInstance.StartEnumExtents) explicitly check for null and throw ArgumentNullException to reliably returnE_POINTERwithout relying on null-pointer deref behavior.
try
{
*handle = 0;
TargetPointer code = GetILExtentStart(out uint codeSize);
if (code == TargetPointer.Null)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IXCLRData.cs:934
- This change introduces a new
publicenum and changes the public field type ofClrDataMethodDefinitionExtent.typefromuinttoCLRDataMethodDefinitionExtentType. That is a source-breaking public surface change for any consumers of the (packable) Legacy assembly. If this API is intended to be public, it should follow the repo’s public API approval process; otherwise consider keeping the field asuint(with a comment or helper cast) or making the enum/internal surface non-public.
public enum CLRDataMethodDefinitionExtentType : uint
{
CLRDATA_METHDEF_IL,
}
public struct ClrDataMethodDefinitionExtent
{
public ClrDataAddress startAddress;
public ClrDataAddress endAddress;
public uint enCVersion;
public CLRDataMethodDefinitionExtentType type;
}
- Files reviewed: 8/8 changed files
- Comments generated: 2
noahfalk
approved these changes
Jul 27, 2026
noahfalk
approved these changes
Jul 27, 2026
rcj1
enabled auto-merge (squash)
July 27, 2026 23:26
Contributor
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IXCLRData.cs:934
- This change introduces new public API surface in the Legacy COM projections (new public enum
CLRDataMethodDefinitionExtentTypeand changingClrDataMethodDefinitionExtent.typefromuintto that enum). The PR description doesn’t link anapi-approvedissue as required for new public APIs. If this is intended to avoid API review, consider keeping the struct field asuint(matching the prior surface) and using an internal enum/consts for readability, or link the approved API issue.
public enum CLRDataMethodDefinitionExtentType : uint
{
CLRDATA_METHDEF_IL,
}
public struct ClrDataMethodDefinitionExtent
{
public ClrDataAddress startAddress;
public ClrDataAddress endAddress;
public uint enCVersion;
public CLRDataMethodDefinitionExtentType type;
}
- Files reviewed: 8/8 changed files
- Comments generated: 1
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.
Implements IL extent enumeration and representative entry-address lookup for
CLRDataMethodDefinition, replacing legacy fallback behavior.Implementation
StartEnumExtents,EnumExtent,EndEnumExtents, andGetRepresentativeEntryAddress.Validation
Note
This description was generated by GitHub Copilot.