Implement IXCLRDataExceptionState state comparison

[rcj1]

Implement a couple APIs that were found to be used internally

[azure-pipelines]

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.

[Copilot]

Pull request overview

Implements IXCLRDataExceptionState.IsSameState / IsSameState2 in the cDAC legacy shim and adds unit tests to validate the HRESULT behavior when comparing exception state based on exception address and “first-chance” semantics.

Changes:

• Implement IXCLRDataExceptionState.IsSameState and IsSameState2 in ClrDataExceptionState (including DEBUG-only validation against the legacy DAC implementation).
• Add unit tests for state comparison (IsSameState / IsSameState2) and flag validation behavior.

Show a summary per file

File	Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataExceptionState.cs	Adds cDAC implementation of exception state comparison logic for IsSameState/IsSameState2.
src/native/managed/cdac/tests/UnitTests/ClrDataExceptionStateTests.cs	Adds unit tests covering matching/different exception addresses and IsSameState2 flag handling.

Copilot's findings

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

[rcj1]

@copilot 1. please put both flags under CLRDATA_EXCEPTION_PARTIAL in an enum in IXCLRData.cs.
2. resolve #131011 (comment) by modifying the CLRDataExceptionState managed constructor to take the previous exception info as a parameter.

[Copilot]

Copilot's findings

• Files reviewed: 5/5 changed files
• Comments generated: 1

[Copilot]

@copilot 1. please put both flags under CLRDATA_EXCEPTION_PARTIAL in an enum in IXCLRData.cs.
2. resolve #131011 (comment) by modifying ...

Implemented in a554b966 with naming clarification in 48f1797d. The comparison flags now use CLRDataExceptionSameFlag, nested states retain their exception-info address through GetPrevious, and invalid flags throw ArgumentException within the HRESULT boundary.

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (1)

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

• This adds a new public enum type to the Legacy assembly. Unless this is intended to be part of the supported public surface, consider making it internal (tests can still access it via InternalsVisibleTo) to avoid expanding public API and triggering API-approval requirements.

[Flags]
public enum CLRDataExceptionSameFlag : uint
{
    CLRDATA_EXSAME_SECOND_CHANCE = 0,
    CLRDATA_EXSAME_FIRST_CHANCE = 0x1,
}

• Files reviewed: 5/5 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (1)

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

• This change introduces new public API surface (CLRDataExceptionSameFlag). The provided PR metadata doesn’t include any linked api-approved issue/proposal, which is required for new public APIs in dotnet/runtime. Either (1) link the approved API proposal/issue, or (2) make this enum internal if it’s only intended for internal/test use (the Legacy csproj already has InternalsVisibleTo for the tests).

[Flags]
public enum CLRDataExceptionSameFlag : uint
{
    CLRDATA_EXSAME_SECOND_CHANCE = 0,
    CLRDATA_EXSAME_FIRST_CHANCE = 0x1,
}

• Files reviewed: 5/5 changed files
• Comments generated: 1

[azure-pipelines]

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]

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

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "999caed0da5317795648effd23c91cb03eb7f7a7",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "3522269feadf7e6d9a5ed422ccd269335ebe62a9",
  "last_reviewed_commit": "999caed0da5317795648effd23c91cb03eb7f7a7",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "3522269feadf7e6d9a5ed422ccd269335ebe62a9",
  "last_recorded_worker_run_id": "29689456510",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "999caed0da5317795648effd23c91cb03eb7f7a7",
      "review_id": 4730877931
    }
  ]
}

[github-actions]

Holistic Review

Motivation: The cdac legacy IXCLRDataExceptionState implementation previously stubbed out IsSameState and IsSameState2 with E_NOTIMPL (falling back to the legacy DAC when available). This PR implements those two APIs natively in managed cdac code because they were found to be used internally, moving another slice of the legacy DAC surface onto the data-contract reader.

Approach: A new IsSameState2(uint flags, EXCEPTION_RECORD64*) private helper faithfully ports the logic from ClrDataExceptionState::IsSameState2 in src/coreclr/debug/daccess/task.cpp: it validates the flag mask (only SECOND_CHANCE/FIRST_CHANCE allowed, otherwise E_INVALIDARG), returns S_OK for a partial (last-exception) state only on a first-chance query, and otherwise compares the stored exception record's ExceptionAddress against the requested record. To reach the exception record the class now carries a new _exceptionInfoAddress field: when non-null it reads ExceptionInfo.ExceptionRecord via the data descriptor (used by the nested/previous state produced by GetPrevious), and when null it falls back to ThreadData.OSExceptionRecord. The constructor gains this parameter, and all four call sites are updated (Null for the current/last/notification cases, _previousExInfoAddress for the nested case). Both public methods delegate to the helper and, under DEBUG, cross-validate the HRESULT against the legacy implementation via Debug.ValidateHResult. A new CLRDataExceptionSameFlag enum mirrors the native constants. Tests exercise matching/non-matching addresses, the nested (previous) path, partial-state first/second chance, and invalid flags across the standard architectures.

Summary: This is a clean, well-scoped port that matches the native reference implementation closely, including the pointer-size-dependent exception-record offset (sizeof(uint)*2 + PointerSize for the ExceptionAddress field) and the 32-bit truncation of the requested address. The constructor signature change is threaded correctly through all call sites and the test file, and test coverage is thorough with good use of [Theory]/[ClassData]. I found no correctness or behavioral issues in the changed code. One minor, non-blocking style nit is noted inline (prefer nameof over a "ExceptionRecord" string literal for the field lookup). Verdict: LGTM.

Detailed Findings

No blocking issues. See the single inline comment for an optional nameof style suggestion consistent with the repository's C# conventions.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 187.9 AIC · ⌖ 10.5 AIC · ⊞ 10K