Skip to content

Comments

Implement ISOSDacInterface::GetRegisterName in cDAC SOSDacImpl#124803

Open
Copilot wants to merge 5 commits intomainfrom
copilot/implement-get-register-name-api
Open

Implement ISOSDacInterface::GetRegisterName in cDAC SOSDacImpl#124803
Copilot wants to merge 5 commits intomainfrom
copilot/implement-get-register-name-api

Conversation

Copy link
Contributor

Copilot AI commented Feb 24, 2026

Description

Implements GetRegisterName in SOSDacImpl.cs using the RuntimeInfo contract instead of delegating to the legacy DAC. This mirrors the logic in ClrDataAccess::GetRegisterName (request.cpp) but uses IRuntimeInfo.GetTargetArchitecture() at runtime to select register names, rather than compile-time #ifdef guards.

Implementation (SOSDacImpl.cs)

  • Register name arrays for X64, X86, ARM, ARM64, LoongArch64, RiscV64 matching the C++ source
  • Caller frame encoding: negative regNum"caller.<reg>" via -(reg+1), using a separate regIndex local to avoid mutating the parameter
  • NullReferenceException thrown inside the try block when both buffer and pNeeded are null (HResult maps to E_POINTER), following the established pattern in the file (e.g., IsRCWDCOMProxy)
  • E_UNEXPECTED for out-of-range register index
  • S_FALSE when buffer is too small (partial copy via OutputBufferHelpers)
  • #if DEBUG legacy DAC comparison using the unmutated regName parameter directly

Tests (GetRegisterNameTests.cs)

  • 32 tests covering all 6 architectures, caller frame prefix, null/small buffer handling, out-of-range indices, and unsupported architecture (Wasm)

Fixes #124823


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 2 commits February 24, 2026 17:46
Replace the legacy-only delegation with a full contract-based implementation
that uses IRuntimeInfo.GetTargetArchitecture() to select the correct register
name arrays for each architecture (X64, X86, ARM, ARM64, LoongArch64, RiscV64).

Add comprehensive unit tests covering all architectures, caller frame prefix,
edge cases (null buffers, out-of-range, small buffer), and unsupported architectures.

Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
The regName parameter is mutated inside the try block for caller frame
decoding. Save the original value before mutation so the DEBUG assertion
correctly compares against the legacy DAC with the original parameter.
Also add missing blank line before GetStackLimits method.

Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Copilot AI changed the title [WIP] Add ISOSDacInterface::GetRegisterName implementation Implement ISOSDacInterface::GetRegisterName in cDAC SOSDacImpl Feb 24, 2026
@max-charlamb
Copy link
Member

@copilot, fix the failing CI run and address feedback

Instead of saving regName to originalRegName before mutation, use a
separate regIndex local for the decoded register index. This keeps
regName unchanged so it can be used directly in the DEBUG validation
block without needing a separate #if DEBUG section.

Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 24, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • https://api.github.com/graphql
    • Triggering command: /usr/bin/gh /usr/bin/gh pr checks 114465 --repo dotnet/runtime (http block)
    • Triggering command: /usr/bin/gh gh pr checks 114465 (http block)

If you need me to access, download, or install something from one of these locations, you can either:

@max-charlamb max-charlamb marked this pull request as ready for review February 24, 2026 18:58
Copilot AI review requested due to automatic review settings February 24, 2026 18:58
@dotnet-policy-service
Copy link
Contributor

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

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements the GetRegisterName method in the cDAC SOSDacImpl class, replacing the legacy DAC delegation with a contract-based implementation. The implementation uses the RuntimeInfo contract to dynamically determine the target architecture at runtime, eliminating the need for compile-time #ifdef guards used in the C++ version.

Changes:

  • Implements GetRegisterName using IRuntimeInfo.GetTargetArchitecture() to select architecture-specific register name arrays
  • Adds register name arrays for all six supported architectures (X64, X86, ARM, ARM64, LoongArch64, RiscV64)
  • Includes comprehensive test coverage with 32 tests across all architectures and edge cases

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs Implements GetRegisterName with architecture-specific register arrays, caller frame encoding, error handling, and DEBUG legacy comparison
src/native/managed/cdac/tests/GetRegisterNameTests.cs Adds comprehensive tests covering all architectures, caller frames, null/small buffers, out-of-range indices, and unsupported architecture scenarios

Copy link
Member

@max-charlamb max-charlamb left a comment

Choose a reason for hiding this comment

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

lgtm

Instead of returning E_POINTER before the try block, throw a
NullReferenceException inside it. The catch block converts ex.HResult
to E_POINTER (0x80004003) automatically, and the DEBUG comparison
block now runs for this error path as well.

Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Copy link
Contributor

@rcj1 rcj1 left a comment

Choose a reason for hiding this comment

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

Lgtm

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

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

[cDac] Implement ISOSDacInterface::GetRegisterName

4 participants