Add TraverseEHInfo cDAC API by rcj1 · Pull Request #125198 · dotnet/runtime

rcj1 · 2026-03-05T00:42:07Z

No description provided.

dotnet-policy-service · 2026-03-05T00:43:09Z

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

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.

...ractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.ReadyToRunJitManager.cs

Copilot

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.

...ed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IExecutionManager.cs

....Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.cs

...ractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.ReadyToRunJitManager.cs

src/coreclr/vm/datadescriptor/datadescriptor.inc

docs/design/datacontracts/ExecutionManager.md

Copilot

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.

docs/design/datacontracts/ExecutionManager.md

...ractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.ReadyToRunJitManager.cs

Copilot · 2026-03-06T18:36:51Z

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs

+            DACEHInfo expectedEhClause = expected.Elements[(int)clauseIndex];
+            Debug.Assert(pEHInfo->clauseType == expectedEhClause.clauseType, $"cDAC: {expectedEhClause.clauseType}, DAC: {pEHInfo->clauseType}");
+            Debug.Assert(pEHInfo->filterOffset == expectedEhClause.filterOffset, $"cDAC: {expectedEhClause.filterOffset:x}, DAC: {pEHInfo->filterOffset:x}");
+            Debug.Assert(pEHInfo->isCatchAllHandler == expectedEhClause.isCatchAllHandler, $"cDAC: {expectedEhClause.isCatchAllHandler}, DAC: {pEHInfo->isCatchAllHandler}");


The DEBUG cross-validation callback asserts isCatchAllHandler matches the legacy DAC output, but the legacy implementation sets this field using (&EHClause.TypeHandle == mdTypeRefNil) (see coreclr/debug/daccess/request.cpp), which will never be true in practice. As written, this can trigger Debug.Assert failures when enumerating a catch-all typed handler. Consider relaxing/removing this specific assertion (or otherwise matching legacy behavior) so DEBUG builds don't fail on valid inputs.

Suggested change

Debug.Assert(pEHInfo->isCatchAllHandler == expectedEhClause.isCatchAllHandler, $"cDAC: {expectedEhClause.isCatchAllHandler}, DAC: {pEHInfo->isCatchAllHandler}");

// The legacy DAC computes isCatchAllHandler using (&EHClause.TypeHandle == mdTypeRefNil),

// which will not be true for real catch-all typed handlers. Do not assert equality here to

// avoid spurious Debug.Assert failures when cross-validating against the legacy implementation.

Copilot

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 8 comments.

...ractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.ReadyToRunJitManager.cs

Copilot · 2026-03-09T23:49:06Z

...ractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.ReadyToRunJitManager.cs

 using System;
+using System.Collections;
+using System.Collections.Generic;


using System.Collections; appears unused in this file; removing it avoids unnecessary-usings style warnings (IDE0005) in the repo build.

Copilot · 2026-03-09T23:49:06Z

src/native/managed/cdac/tests/DumpTests/ExceptionHandlingInfoDumpTests.cs

+        ExceptionClauseInfo filterClause = clauses.First(c => c.ClauseType == ExceptionClauseInfo.ExceptionClauseFlags.Filter);
+        Assert.NotNull(filterClause.FilterOffset);
+    }


This test only asserts that the filter clause has a non-null FilterOffset. Consider also asserting that filter clauses do not report ClassToken/ModuleAddr (since the union field is a filter offset, not a token) so the test would catch regressions in clause classification/mapping.

...ractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.ReadyToRunJitManager.cs

Copilot · 2026-03-09T23:49:06Z

....Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.cs

+            else if (!IsFaultOrFinally(flags))
+            {
+                if (IsTypedHandler(flags))


The !IsFaultOrFinally(flags) branch also runs for Filter clauses, which means filter clauses will incorrectly get ModuleAddr/ClassToken populated even though the union field is a filter offset. This later causes TraverseEHInfo to report a non-zero tokCatch for filter clauses and can mismatch legacy DAC behavior. Restrict this block to typed clauses only (and leave ClassToken/ModuleAddr null for filter clauses).

Copilot · 2026-03-09T23:49:06Z

....Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.cs

+                TargetPointer mtPtr = rts.GetMethodTable(mdHandle);
+                TypeHandle th = rts.GetTypeHandle(mtPtr);
+                moduleAddr = rts.GetModule(th);
+                classToken = entry.ClassToken;


classToken = entry.ClassToken; uses the union field for all non-fault/finally clauses, but for Filter clauses this value is a filter offset rather than a metadata token. Ensure ClassToken is only set for typed clauses (and preferably only when HasCachedTypeHandle is false).

Copilot · 2026-03-09T23:49:07Z

....Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.cs

+            if (HasCachedTypeHandle(entry))
+            {
+                typeHandle = ((EEExceptionClause)entry).TypeHandle;
+            }


HasCachedTypeHandle(entry) can be true for any IExceptionClauseData; if this ever occurs for an R2R clause, the cast to EEExceptionClause will throw. Consider guarding this with !isR2R (or otherwise ensuring the flag cannot be set for R2R clauses) before casting.

Copilot · 2026-03-09T23:49:07Z

....Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.cs

+                TargetPointer methodDescPtr = ((IExecutionManager)this).GetMethodDesc(codeInfoHandle);
+                IRuntimeTypeSystem rts = _target.Contracts.RuntimeTypeSystem;
+                MethodDescHandle mdHandle = rts.GetMethodDescHandle(methodDescPtr);
+                TargetPointer mtPtr = rts.GetMethodTable(mdHandle);
+                TypeHandle th = rts.GetTypeHandle(mtPtr);


The RuntimeTypeSystem lookups for MethodDesc -> Module are repeated for every clause in the loop. Since the module is per-method (not per-clause), consider hoisting the method/module resolution outside the clause iteration (or caching it the first time it is needed) to avoid unnecessary contract calls and target reads.

rcj1 · 2026-03-09T23:51:19Z

@janvorli

runtime/src/coreclr/vm/codeman.cpp

Line 6138 in e3b528c

// The last entry in the lookup table (end-1) points to a sentinel entry.

janvorli · 2026-03-09T23:59:04Z

@rcj1 ah, sorry, I have not realized that's the R2R case.

noahfalk

This looks good to me. It would probably be good for someone maintaining the EH typesystem/R2R code (@janvorli or @davidwrighton maybe?) to confirm the contract surface area we are binding to looks good.

...ed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IExecutionManager.cs

add TraverseEHInfo cDAC API

7679a3f

Copilot AI review requested due to automatic review settings March 5, 2026 00:42

github-actions bot added the area-Diagnostics-coreclr label Mar 5, 2026

dotnet-policy-service bot assigned rcj1 Mar 5, 2026

Merge branch 'main' into TraverseEhInfo

b216b60

Copilot started reviewing on behalf of rcj1 March 5, 2026 00:43 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

e

a9f929f

rcj1 requested a review from Copilot March 5, 2026 01:44

Copilot started reviewing on behalf of rcj1 March 5, 2026 01:45 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

...ractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.ReadyToRunJitManager.cs Show resolved Hide resolved

rcj1 added 2 commits March 4, 2026 18:26

Remove duplicate declaration of lastIndex variable

61ddb38

mocks

77caf6e

Copilot AI review requested due to automatic review settings March 5, 2026 17:44

Copilot AI reviewed Mar 5, 2026

View reviewed changes

rcj1 added 2 commits March 6, 2026 10:26

Adjust high index calculation in binary search

14d4a49

update docs

a2298ec

Copilot AI review requested due to automatic review settings March 6, 2026 18:28

Copilot started reviewing on behalf of rcj1 March 6, 2026 18:29 View session

Merge branch 'main' into TraverseEhInfo

8508fe3

rcj1 requested review from barosiak, janvorli, max-charlamb and noahfalk March 6, 2026 18:32

rcj1 removed the request for review from janvorli March 6, 2026 18:33

rcj1 marked this pull request as ready for review March 6, 2026 18:33

Copilot AI reviewed Mar 6, 2026

View reviewed changes

rcj1 removed request for barosiak, max-charlamb and noahfalk March 9, 2026 03:30

rcj1 marked this pull request as draft March 9, 2026 03:31

rcj1 added 2 commits March 9, 2026 16:38

touch up

c755dc2

check for invalid num of exception clauses

e1371b8

Copilot AI review requested due to automatic review settings March 9, 2026 23:41

rcj1 marked this pull request as ready for review March 9, 2026 23:42

Copilot started reviewing on behalf of rcj1 March 9, 2026 23:42 View session

rcj1 requested review from barosiak, janvorli, max-charlamb and noahfalk March 9, 2026 23:42

Copilot AI reviewed Mar 9, 2026

View reviewed changes

noahfalk approved these changes Mar 10, 2026

View reviewed changes

max-charlamb reviewed Mar 10, 2026

View reviewed changes

...ed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IExecutionManager.cs Show resolved Hide resolved

-            Debug.Assert(pEHInfo->isCatchAllHandler == expectedEhClause.isCatchAllHandler, $"cDAC: {expectedEhClause.isCatchAllHandler}, DAC: {pEHInfo->isCatchAllHandler}");
+            // The legacy DAC computes isCatchAllHandler using (&EHClause.TypeHandle == mdTypeRefNil),
+            // which will not be true for real catch-all typed handlers. Do not assert equality here to
+            // avoid spurious Debug.Assert failures when cross-validating against the legacy implementation.

Conversation

rcj1 commented Mar 5, 2026

Uh oh!

dotnet-policy-service bot commented Mar 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

rcj1 commented Mar 9, 2026

Uh oh!

janvorli commented Mar 9, 2026

Uh oh!

noahfalk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

noahfalk left a comment •

edited

Loading