Skip to content

Implement cDAC method definition enumeration by address#131338

Merged
rcj1 merged 11 commits into
mainfrom
copilot/implement-clrdata-process-methods
Jul 25, 2026
Merged

Implement cDAC method definition enumeration by address#131338
rcj1 merged 11 commits into
mainfrom
copilot/implement-clrdata-process-methods

Conversation

Copilot AI commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Implements StartEnumMethodDefinitionsByAddress, EnumMethodDefinitionByAddress, and EndEnumMethodDefinitionsByAddress for ClrDataProcess.

  • Enumeration
    • Resolves method definitions from module metadata and original IL addresses.
    • Matches legacy DAC behavior for tiny and fat method headers.
  • Interop conventions
    • Manages enumeration handles and child-object delegation.
    • Maps exceptions to HRESULTs and cross-validates results against the legacy DAC in debug builds.
  • Coverage
    • Covers valid, empty, invalid, boundary, tiny-header, and fat-header cases across target architectures.

Copilot AI and others added 5 commits July 24, 2026 17:46
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>
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:04

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.

@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.

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

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 19:48

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 24, 2026 22:07

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/tests/UnitTests/IXCLRDataProcessTests.cs:326

  • For tiny headers, the first address outside the IL stream is codeStart + codeSize (i.e., FirstHeaderAddress + 1 + TinyCodeSize). With TinyCodeSize = 3, FirstHeaderAddress + 3 is still inside the IL code range if you start from +1, so this boundary test should use FirstHeaderAddress + 4.
            hr = process.StartEnumMethodDefinitionsByAddress(FirstHeaderAddress + 3, &handle);
            Assert.Equal(HResults.S_OK, hr);
            try
            {
                DacComNullableByRef<IXCLRDataMethodDefinition> methodOut = new(isNullRef: false);
                Assert.Equal(HResults.S_FALSE, process.EnumMethodDefinitionByAddress(&handle, methodOut));
            }
  • Files reviewed: 2/2 changed files
  • Comments generated: 2

Comment thread src/native/managed/cdac/tests/UnitTests/IXCLRDataProcessTests.cs Outdated
Copilot AI review requested due to automatic review settings July 24, 2026 23:31

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/SOSDacImpl.IXCLRDataProcess.cs:1654

  • The tiny-header case uses a 0-byte adjustment for the address filter (vs headerSize). That matches the legacy DAC’s 4 * COR_ILMETHOD_DECODER::GetSize() behavior (where GetSize() is 0 for tiny), but it’s counterintuitive vs ECMA-335 and easy to “fix” incorrectly later. Add a short comment explaining the legacy parity requirement to preserve intent.
            bool isTinyHeader = headerSize == sizeof(byte);
            uint codeStartOffset = isTinyHeader ? 0 : (uint)headerSize;
            ClrDataAddress codeStart = (ilHeader + codeStartOffset).ToClrDataAddress(_target);
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new

Copilot AI review requested due to automatic review settings July 24, 2026 23:39
@rcj1
rcj1 marked this pull request as ready for review July 24, 2026 23:40
@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.

@rcj1
rcj1 requested review from max-charlamb and noahfalk July 24, 2026 23:41

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

  • Files reviewed: 3/3 changed files
  • Comments generated: 2

Comment thread src/native/managed/cdac/tests/UnitTests/IXCLRDataProcessTests.cs
Copilot AI review requested due to automatic review settings July 25, 2026 01:21

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/SOSDacImpl.IXCLRDataProcess.cs:1612

  • GCHandle.Target can throw InvalidOperationException when the handle value is not an allocated GCHandle (e.g., random/garbage non-zero handle), which would return COR_E_INVALIDOPERATION instead of E_INVALIDARG. Add an IsAllocated guard before accessing Target so invalid EndEnum handles consistently map to E_INVALIDARG.
            GCHandle gcHandle = GCHandle.FromIntPtr((IntPtr)handle);
            if (gcHandle.Target is not ProcessEnum<MethodDefinitionInfo> methodDefinitionsLocal)
                throw new ArgumentException("Invalid enumeration handle", nameof(handle));
  • Files reviewed: 3/3 changed files
  • Comments generated: 1

@rcj1
rcj1 enabled auto-merge (squash) July 25, 2026 04:07
@rcj1
rcj1 merged commit 2826b2d into main Jul 25, 2026
139 checks passed
@rcj1
rcj1 deleted the copilot/implement-clrdata-process-methods branch July 25, 2026 04:17
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.

5 participants