Skip to content

Implement CLRDataMethodInstance APIs in cDAC - #131340

Open
rcj1 with Copilot wants to merge 20 commits into
mainfrom
copilot/implement-getaddressrangesbyiloffset-and-getdefini
Open

Implement CLRDataMethodInstance APIs in cDAC#131340
rcj1 with Copilot wants to merge 20 commits into
mainfrom
copilot/implement-getaddressrangesbyiloffset-and-getdefini

Conversation

Copilot AI commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Implements CLRDataMethodInstance.GetDefinition and GetAddressRangesByILOffset using cDAC contracts.

Changes

  • Method definitions

    • Resolves the defining module and metadata token from the MethodDesc.
    • Preserves legacy child-object fallback and debug HRESULT validation.
  • IL address ranges

    • Produces native ranges from debug-info IL mappings.
    • Matches native DAC buffer, epilog, and E_NOINTERFACE semantics.
    • Cross-validates results against the legacy DAC in debug builds.
  • Coverage

    • Adds dump tests for metadata resolution, matching address ranges, and missing IL offsets.

Copilot AI and others added 4 commits July 24, 2026 17:56
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>
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Copilot AI self-assigned this Jul 24, 2026
Copilot AI review requested due to automatic review settings July 24, 2026 18:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because there is no eligible user to bill. To allow Copilot reviews on bot-authored pull requests, enable direct organization billing in your organization's Copilot settings.

@azure-pipelines

Copy link
Copy Markdown
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 review requested due to automatic review settings July 24, 2026 19:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because there is no eligible user to bill. To allow Copilot reviews on bot-authored pull requests, enable direct organization billing in your organization's Copilot settings.

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 24, 2026 23:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because there is no eligible user to bill. To allow Copilot reviews on bot-authored pull requests, enable direct organization billing in your organization's Copilot settings.

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 24, 2026 23:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because there is no eligible user to bill. To allow Copilot reviews on bot-authored pull requests, enable direct organization billing in your organization's Copilot settings.

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 26, 2026 01:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because there is no eligible user to bill. To allow Copilot reviews on bot-authored pull requests, enable direct organization billing in your organization's Copilot settings.

Copilot AI review requested due to automatic review settings July 26, 2026 01:08
@rcj1
rcj1 marked this pull request as ready for review July 26, 2026 01:09
@azure-pipelines

Copy link
Copy Markdown
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.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

Copilot finished work on behalf of rcj1 July 26, 2026 01:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Comments suppressed due to low confidence (1)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodInstance.cs:475

  • GetILAddressMap can throw before writing mapNeeded (e.g., when there’s no debug info). Even though this PR now throws InvalidCastException for the empty-map case, mapNeeded should still be set to 0 on all failure paths to avoid leaving an uninitialized out parameter.
            if (map.Count == 0)
                throw new InvalidCastException();
  • Files reviewed: 3/3 changed files
  • Comments generated: 5

CLRDATA_SOURCE_TYPE_INVALID = 0,
}

public enum CLRDataILOffsetMarker : uint
Comment on lines +358 to +365
if (addressRanges is not null && hits < addressRanges.Length)
{
uint nativeEndOffset = i == map.Count - 1 ? 0 : map[i + 1].NativeOffset;
addressRanges[hits].startAddress = new TargetPointer(codeStart + entry.NativeOffset).ToClrDataAddress(_target);
addressRanges[hits].endAddress = entry.ILOffset == (uint)CLRDataILOffsetMarker.CLRDATA_IL_OFFSET_EPILOG && nativeEndOffset == 0
? default
: new TargetPointer(codeStart + nativeEndOffset).ToClrDataAddress(_target);
}
Comment thread src/native/managed/cdac/tests/DumpTests/StackWalkDumpTests.cs
Copilot AI review requested due to automatic review settings July 26, 2026 01:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because there is no eligible user to bill. To allow Copilot reviews on bot-authored pull requests, enable direct organization billing in your organization's Copilot settings.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 26, 2026 01:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Comments suppressed due to low confidence (1)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IXCLRData.cs:552

  • Adding a new public enum here expands the surface area of the (packable) Legacy assembly, but the PR description doesn’t link an api-approved issue. If these marker values are only an implementation detail for interpreting ilOffset values, make the enum internal (or replace with private const uint values) to avoid introducing a new public API type.
public enum CLRDataILOffsetMarker : uint
  • Files reviewed: 3/3 changed files
  • Comments generated: 2

Comment on lines 617 to +621
int GetAddressRangesByILOffset(
uint ilOffset,
uint rangesLen,
uint* rangesNeeded,
/*CLRDATA_ADDRESS_RANGE* */ void* addressRanges);
[In, Out, MarshalUsing(CountElementName = nameof(rangesLen))] ClrDataAddressRange[]? addressRanges);
Comment on lines +190 to +199
IXCLRDataMethodDefinition methodDefinition = Assert.IsAssignableFrom<IXCLRDataMethodDefinition>(definitionOut.Interface);
DacComNullableByRef<IXCLRDataModule> nullModule = new(isNullRef: true);
uint instanceToken;
hr = methodInstance.GetTokenAndScope(&instanceToken, nullModule);
AssertHResult(HResults.S_OK, hr);

uint definitionToken;
hr = methodDefinition.GetTokenAndScope(&definitionToken, nullModule);
AssertHResult(HResults.S_OK, hr);
Assert.Equal(instanceToken, definitionToken);

try
{
if (canFallback && _legacyImpl is not null)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can't fallback shouldn't we return E_NOTIMPL? Right now it looks like we return a ClrMethodDefinition that has a NULL legacyMethodDefinition embedded inside it or maybe that constructor throws some other error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants