Fix EH clause sort to handle try-in-handler nesting#8428
Merged
Conversation
BenchmarksBenchmark execution time: 2026-04-17 14:16:34 Comparing candidate commit c5713b9 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 27 metrics, 0 unstable metrics, 87 known flaky benchmarks.
|
Collaborator
Author
|
Closing this as I just thought it would be easier to share this as a PR form than not, if this is valuable can always pick it back up but we don't know if this actually solves anything. |
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8428) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8428) - mean (75ms) : 70, 80
master - mean (73ms) : 70, 77
section Bailout
This PR (8428) - mean (78ms) : 74, 82
master - mean (80ms) : 75, 84
section CallTarget+Inlining+NGEN
This PR (8428) - mean (1,084ms) : 1027, 1142
master - mean (1,081ms) : 1009, 1154
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8428) - mean (116ms) : 109, 123
master - mean (116ms) : 110, 122
section Bailout
This PR (8428) - mean (114ms) : 111, 118
master - mean (117ms) : 111, 123
section CallTarget+Inlining+NGEN
This PR (8428) - mean (798ms) : 776, 820
master - mean (798ms) : 771, 824
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8428) - mean (101ms) : 95, 106
master - mean (103ms) : 97, 109
section Bailout
This PR (8428) - mean (101ms) : 99, 104
master - mean (105ms) : 100, 110
section CallTarget+Inlining+NGEN
This PR (8428) - mean (947ms) : 916, 979
master - mean (940ms) : 907, 972
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8428) - mean (103ms) : 97, 109
master - mean (102ms) : 96, 108
section Bailout
This PR (8428) - mean (102ms) : 99, 104
master - mean (101ms) : 99, 103
section CallTarget+Inlining+NGEN
This PR (8428) - mean (826ms) : 786, 865
master - mean (827ms) : 782, 872
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8428) - mean (210ms) : 206, 215
master - mean (207ms) : 202, 212
section Bailout
This PR (8428) - mean (215ms) : 211, 219
master - mean (211ms) : 207, 215
section CallTarget+Inlining+NGEN
This PR (8428) - mean (1,226ms) : 1176, 1275
master - mean (1,215ms) : 1159, 1271
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8428) - mean (304ms) : 297, 311
master - mean (299ms) : 292, 306
section Bailout
This PR (8428) - mean (304ms) : 297, 311
master - mean (298ms) : 291, 305
section CallTarget+Inlining+NGEN
This PR (8428) - mean (1,012ms) : 980, 1044
master - mean (998ms) : 960, 1035
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8428) - mean (299ms) : 293, 305
master - mean (296ms) : 288, 304
section Bailout
This PR (8428) - mean (298ms) : 293, 302
master - mean (294ms) : 287, 301
section CallTarget+Inlining+NGEN
This PR (8428) - mean (1,182ms) : 1146, 1217
master - mean (1,175ms) : 1140, 1210
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8428) - mean (297ms) : 289, 305
master - mean (292ms) : 286, 299
section Bailout
This PR (8428) - mean (297ms) : 292, 301
master - mean (293ms) : 288, 298
section CallTarget+Inlining+NGEN
This PR (8428) - mean (1,084ms) : 990, 1177
master - mean (1,073ms) : 992, 1154
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
anna-git
added
area:native-library
anna-git
added a commit
that referenced
this pull request
Apr 10, 2026
## Summary of changes This reproduces the crash [described in this README](https://github.com/DataDog/dd-trace-dotnet/blob/82cc4cf0e29478fd2fdc41afdf764a2abf662199/repro/APMS-19196/README.md) on linux x64 arch. Aim is to merge to #8428 First tested it's crashing on master <img width="2830" height="851" alt="image" src="https://github.com/user-attachments/assets/96e882fa-308e-44b6-8116-c93c7d5b7bb2" /> ## Reason for change ## Implementation details ## Test coverage ## Other details <!-- Fixes #{issue} --> <!--⚠️ Note: Where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. MergeQueue is NOT enabled in this repository. If you have write access to the repo, the PR has 1-2 approvals (see above), and all of the required checks have passed, you can use the Squash and Merge button to merge the PR. If you don't have write access, or you need help, reach out in the #apm-dotnet channel in Slack. -->
Contributor
Snapshots difference summaryThe following differences have been observed in committed snapshots. It is meant to help the reviewer. 3 occurrences of : + },
+ {
+ TraceId: Id_1,
+ SpanId: Id_62,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.DeepNestedExceptionHandlingSync,
+ Service: Samples.TraceAnnotations,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_63,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.SameTryMultipleCatch,
+ Service: Samples.TraceAnnotations,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_64,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.SameTryMultipleCatch,
+ Service: Samples.TraceAnnotations,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_65,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.SameTryMultipleCatch,
+ Service: Samples.TraceAnnotations,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_66,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.FilterAndTypedCatch,
+ Service: Samples.TraceAnnotations,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_67,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.FilterAndTypedCatch,
+ Service: Samples.TraceAnnotations,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_68,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.FilterAndTypedCatch,
+ Service: Samples.TraceAnnotations,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
3 occurrences of : + },
+ {
+ TraceId: Id_1,
+ SpanId: Id_62,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.DeepNestedExceptionHandlingSync,
+ Service: Samples.TraceAnnotations.VersionMismatch.AfterFeature,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_63,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.SameTryMultipleCatch,
+ Service: Samples.TraceAnnotations.VersionMismatch.AfterFeature,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_64,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.SameTryMultipleCatch,
+ Service: Samples.TraceAnnotations.VersionMismatch.AfterFeature,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_65,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.SameTryMultipleCatch,
+ Service: Samples.TraceAnnotations.VersionMismatch.AfterFeature,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_66,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.FilterAndTypedCatch,
+ Service: Samples.TraceAnnotations.VersionMismatch.AfterFeature,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_67,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.FilterAndTypedCatch,
+ Service: Samples.TraceAnnotations.VersionMismatch.AfterFeature,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_68,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.FilterAndTypedCatch,
+ Service: Samples.TraceAnnotations.VersionMismatch.AfterFeature,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
3 occurrences of : + },
+ {
+ TraceId: Id_1,
+ SpanId: Id_62,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.DeepNestedExceptionHandlingSync,
+ Service: Samples.TraceAnnotations.VersionMismatch.BeforeFeature,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_63,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.SameTryMultipleCatch,
+ Service: Samples.TraceAnnotations.VersionMismatch.BeforeFeature,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_64,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.SameTryMultipleCatch,
+ Service: Samples.TraceAnnotations.VersionMismatch.BeforeFeature,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_65,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.SameTryMultipleCatch,
+ Service: Samples.TraceAnnotations.VersionMismatch.BeforeFeature,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_66,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.FilterAndTypedCatch,
+ Service: Samples.TraceAnnotations.VersionMismatch.BeforeFeature,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_67,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.FilterAndTypedCatch,
+ Service: Samples.TraceAnnotations.VersionMismatch.BeforeFeature,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_1,
+ SpanId: Id_68,
+ Name: trace.annotation,
+ Resource: ExtremeExceptionHandling.FilterAndTypedCatch,
+ Service: Samples.TraceAnnotations.VersionMismatch.BeforeFeature,
+ ParentId: Id_2,
+ Tags: {
+ component: trace,
+ env: integration_tests,
+ language: dotnet,
+ version: 1.0.0
+ }
|
bouwkast
added
the
AI Generated
bouwkast
marked this pull request as ready for review
April 13, 2026 14:59
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b6b9a4b4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes EH clause ordering in ILRewriter::Export() to correctly handle clauses whose try region is nested inside another clause’s handler region, preventing CLR InvalidProgramException during JIT validation (APMS-19196).
Changes:
- Replaces the previous EH
std::sortcomparator withILRewriter::SortEHClauses, sorting by computed nesting depth (try-in-try and try-in-handler) with a strict weak ordering. - Adds native unit tests for EH sorting plus a managed “extreme EH” integration scenario to reproduce the crash.
- Updates integration test expectations/snapshots to account for the additional traced method.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tracer/src/Datadog.Tracer.Native/il_rewriter.cpp | Uses new SortEHClauses in Export() and implements depth-based EH sorting. |
| tracer/src/Datadog.Tracer.Native/il_rewriter.h | Declares ILRewriter::SortEHClauses for reuse/unit testing. |
| tracer/test/Datadog.Tracer.Native.Tests/il_rewriter_eh_sort_test.cpp | Adds gtest coverage for try-in-try and try-in-handler ordering, including a repro of the old comparator failure. |
| tracer/test/Datadog.Tracer.Native.Tests/Datadog.Tracer.Native.Tests.vcxproj | Includes the new native test source in the VS project. |
| tracer/test/Datadog.Tracer.Native.Tests/Datadog.Tracer.Native.Tests.vcxproj.filters | Adds the new native test source to filters for VS organization. |
| tracer/test/Datadog.Tracer.Native.Tests/CMakeLists.txt | Includes the new test source in the CMake test target. |
| tracer/test/test-applications/integrations/Samples.TraceAnnotations/ExtremeExceptionHandling.cs | Adds a managed method with deep EH nesting used as a regression repro. |
| tracer/test/test-applications/integrations/Samples.TraceAnnotations/ProgramHelpers.cs | Executes the new extreme EH scenario as part of the sample run. |
| tracer/test/test-applications/integrations/Samples.TraceAnnotations.VersionMismatch.BeforeFeature/Samples.TraceAnnotations.VersionMismatch.BeforeFeature.csproj | Ensures the new managed file is compiled into the version-mismatch sample. |
| tracer/test/test-applications/integrations/Samples.TraceAnnotations.VersionMismatch.AfterFeature/Samples.TraceAnnotations.VersionMismatch.AfterFeature.csproj | Ensures the new managed file is compiled into the version-mismatch sample. |
| tracer/test/Datadog.Trace.ClrProfiler.IntegrationTests/TraceAnnotationsTests.cs | Updates expected span count and sets DD_TRACE_METHODS to trace the new sync repro method. |
| tracer/test/snapshots/TraceAnnotationsAutomaticOnlyTests._.Net.verified.txt | Snapshot updated for added span from the extreme EH method. |
| tracer/test/snapshots/TraceAnnotationsAutomaticOnlyTests._.DotNet.verified.txt | Snapshot updated for added span from the extreme EH method. |
| tracer/test/snapshots/TraceAnnotationsAutomaticOnlyTests._.Core.verified.txt | Snapshot updated for added span from the extreme EH method. |
| tracer/test/snapshots/TraceAnnotationsVersionMismatchAfterFeatureTests._.Net.verified.txt | Snapshot updated for added span from the extreme EH method. |
| tracer/test/snapshots/TraceAnnotationsVersionMismatchAfterFeatureTests._.DotNet.verified.txt | Snapshot updated for added span from the extreme EH method. |
| tracer/test/snapshots/TraceAnnotationsVersionMismatchAfterFeatureTests._.Core.verified.txt | Snapshot updated for added span from the extreme EH method. |
| tracer/test/snapshots/TraceAnnotationsVersionMismatchBeforeFeatureTests._.Net.verified.txt | Snapshot updated for added span from the extreme EH method. |
| tracer/test/snapshots/TraceAnnotationsVersionMismatchBeforeFeatureTests._.DotNet.verified.txt | Snapshot updated for added span from the extreme EH method. |
| tracer/test/snapshots/TraceAnnotationsVersionMismatchBeforeFeatureTests._.Core.verified.txt | Snapshot updated for added span from the extreme EH method. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+14
| #include "pch.h" | ||
|
|
||
| #include "../../src/Datadog.Tracer.Native/il_rewriter.h" | ||
|
|
||
| // Helper to build a doubly-linked chain of ILInstr nodes with specified offsets. | ||
| // Returns a heap-allocated array; the caller must delete[]. | ||
| // Each node's m_pNext and m_pPrev are wired up in sequence. The last node's | ||
| // m_pNext points to a sentinel (the first node acts as its own sentinel for | ||
| // handler-end calculations via m_pHandlerEnd->m_pNext). | ||
| static ILInstr* MakeInstrChain(const std::vector<unsigned>& offsets) | ||
| { | ||
| auto n = offsets.size(); | ||
| auto* instrs = new ILInstr[n]; | ||
| memset(instrs, 0, n * sizeof(ILInstr)); |
There was a problem hiding this comment.
This test file uses std::vector, std::sort, and memset but does not include the corresponding standard headers. With the current pch.h (only gtest + CLR headers), this will not compile on a clean build. Add explicit includes for the headers you use (e.g., , , ).
Comment on lines
+5
to
+21
| // Helper to build a doubly-linked chain of ILInstr nodes with specified offsets. | ||
| // Returns a heap-allocated array; the caller must delete[]. | ||
| // Each node's m_pNext and m_pPrev are wired up in sequence. The last node's | ||
| // m_pNext points to a sentinel (the first node acts as its own sentinel for | ||
| // handler-end calculations via m_pHandlerEnd->m_pNext). | ||
| static ILInstr* MakeInstrChain(const std::vector<unsigned>& offsets) | ||
| { | ||
| auto n = offsets.size(); | ||
| auto* instrs = new ILInstr[n]; | ||
| memset(instrs, 0, n * sizeof(ILInstr)); | ||
| for (size_t i = 0; i < n; i++) | ||
| { | ||
| instrs[i].m_offset = offsets[i]; | ||
| instrs[i].m_pPrev = (i > 0) ? &instrs[i - 1] : &instrs[n - 1]; | ||
| instrs[i].m_pNext = (i < n - 1) ? &instrs[i + 1] : &instrs[0]; | ||
| } | ||
| return instrs; |
There was a problem hiding this comment.
MakeInstrChain builds a circular list (last->next points back to the first node), but the comment describes a sentinel node for handler-end calculations. This can be misleading, and it also doesn’t model ILRewriter’s real list where the last instruction’s m_pNext points to a dedicated sentinel with m_offset == code size (important when a handler ends at the end of the method). Consider either updating the comment to reflect the circular list, or creating an explicit sentinel node in the helper to better match production semantics.
anna-git
reviewed
Apr 14, 2026
anna-git
left a comment
anna-git
left a comment
Contributor
There was a problem hiding this comment.
Added a few comments, after trying to wrap my head around it 😅
From what I understand, the new algorithm does:
- calculate depth of each exception handling clause
- create a blank array the size of the clauses
- compare eh clause's depth 2 by 2 and reorder indices according to it
a. if they have different depths, sort the new indices array according to it, putting the deeper depth first (ecma rule)
b. if depths are the same, either sort by try begin offset
c. either the offsets are the same, then just keep the original order
| #include "pch.h" | ||
|
|
||
| #include "../../src/Datadog.Tracer.Native/il_rewriter.h" | ||
|
|
Contributor
There was a problem hiding this comment.
Copilot complaining about this:
Suggested change
| #include <cstring> | |
| #include <vector> |
Collaborator
Author
There was a problem hiding this comment.
I think it should be resolved now
Comment on lines
+7
to
+9
| // Each node's m_pNext and m_pPrev are wired up in sequence. The last node's | ||
| // m_pNext points to a sentinel (the first node acts as its own sentinel for | ||
| // handler-end calculations via m_pHandlerEnd->m_pNext). |
Contributor
There was a problem hiding this comment.
Suggested change
| // Each node's m_pNext and m_pPrev are wired up in sequence. The last node's | |
| // m_pNext points to a sentinel (the first node acts as its own sentinel for | |
| // handler-end calculations via m_pHandlerEnd->m_pNext). | |
| // For simplicity, nodes are linked in a ring (last points to first). This is not production IL layout; it avoids a separate sentinel while keeping every node with a valid successor. | |
| // SortEHClauses uses m_pHandlerEnd->m_pNext->m_offset as the exclusive end of a handler region (same convention as ILRewriter::ImportEH). |
bouwkast
force-pushed
the
steven/attempt-fix-invalid-program-exception
branch
from
April 15, 2026 11:40
50011b0 to
4a0d151
Compare
andrewlock
approved these changes
Apr 16, 2026
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
What I understand of it makes sense 😅 :blindfold:
Comment on lines
+866
to
+869
| unsigned jTryBegin = pEH[j].m_pTryBegin->m_offset; | ||
| unsigned jTryEnd = pEH[j].m_pTryEnd->m_offset; | ||
| unsigned jHandlerBegin = pEH[j].m_pHandlerBegin->m_offset; | ||
| unsigned jHandlerEnd = pEH[j].m_pHandlerEnd->m_pNext->m_offset; |
Member
There was a problem hiding this comment.
tiny nit: might be slightly easier to follow by extracting EHClause targetEH = pEH[j]
Comment on lines
+46
to
+51
| // clauses[0]: outer -- try [10,80), handler [80,90) | ||
| clauses[0].m_Flags = COR_ILEXCEPTION_CLAUSE_NONE; | ||
| clauses[0].m_pTryBegin = FindInstr(instrs, 8, 10); | ||
| clauses[0].m_pTryEnd = FindInstr(instrs, 8, 80); | ||
| clauses[0].m_pHandlerBegin = FindInstr(instrs, 8, 80); | ||
| clauses[0].m_pHandlerEnd = FindInstr(instrs, 8, 85); // last instr in handler |
Member
There was a problem hiding this comment.
Not gonna lie, I find this completely impenetrable 😅
Collaborator
Author
There was a problem hiding this comment.
😰
Should I remove these tests? I don't want someone thinking in the future that these were thoughtfully designed
Member
There was a problem hiding this comment.
Meh, keep them I guess, I just can't verify them 😅
| } | ||
|
|
||
| /// <summary> | ||
| /// Synchronous method with 9 levels of nested try/catch/finally. |
Member
There was a problem hiding this comment.
Does it really need 9 levels?! 😅 Also, should we try to exercise any of the other cases here, e.g. multiple catch clauses, try-catch inside a catch block etc?
Contributor
There was a problem hiding this comment.
It's very sensitive to levels 😅 In my tests, removing levels would stop the crash, adding complexity with await tasks and nesting wouldn't cause the crash to reappear.. but we could think of having another endpoint with other levels of complexity in other PRs🤔
Originally we had this 🤯 :
After many attempts, claude managed to get to a simplified version that would still crash:
If you remove just one level it stops crashing.
I don't know if we want to test the most complex version or the simplified one here. maybe we should take the most complex one
tonyredondo
approved these changes
Apr 16, 2026
tonyredondo
left a comment
tonyredondo
left a comment
Member
There was a problem hiding this comment.
I would an extra test before merging.
Reason:
- I do not see a concrete bug in the new sorting algorithm.
- The sorter now explicitly relies on preserving the original order when multiple handlers share the same try.
- That order is semantically important, because exception dispatch uses the first matching handler.
A minimal test will ensure us that the exception order is preserved inside the same try:
using System;
public static class EhOrderingDemo
{
public static int SameTryMultipleCatch(Exception ex)
{
try
{
throw ex;
}
catch (ArgumentNullException)
{
return 1;
}
catch (InvalidOperationException)
{
return 2;
}
catch (Exception)
{
return 3;
}
}
public static void Main()
{
Console.WriteLine(SameTryMultipleCatch(new ArgumentNullException())); // expected: 1
Console.WriteLine(SameTryMultipleCatch(new InvalidOperationException())); // expected: 2
Console.WriteLine(SameTryMultipleCatch(new Exception())); // expected: 3
}
}
Member
Also maybe we should add an exception filter test case as well |
bouwkast
enabled auto-merge (squash)
April 16, 2026 20:16
## Summary of changes This reproduces the crash [described in this README](https://github.com/DataDog/dd-trace-dotnet/blob/82cc4cf0e29478fd2fdc41afdf764a2abf662199/repro/APMS-19196/README.md) on linux x64 arch. Aim is to merge to #8428 First tested it's crashing on master <img width="2830" height="851" alt="image" src="https://github.com/user-attachments/assets/96e882fa-308e-44b6-8116-c93c7d5b7bb2" /> ## Reason for change ## Implementation details ## Test coverage ## Other details <!-- Fixes #{issue} --> <!--⚠️ Note: Where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. MergeQueue is NOT enabled in this repository. If you have write access to the repo, the PR has 1-2 approvals (see above), and all of the required checks have passed, you can use the Squash and Merge button to merge the PR. If you don't have write access, or you need help, reach out in the #apm-dotnet channel in Slack. -->
Add original index as final tiebreaker so std::sort does not reorder catch/filter handlers that share the same try region. Without this, multiple catch blocks for the same try can be silently swapped, changing which handler the runtime evaluates first.
Extract IsProperlyContained helper, use explicit types, add braces on all control flow, and use named variables in the sort comparator to match surrounding native code style.
Co-authored-by: Anna <[email protected]>
Co-authored-by: Anna <[email protected]>
Verify that when multiple catch handlers share the same try region, the sort preserves the compiler's original order. This matters because the CLR dispatches to the first matching handler. Requested by @tonyredondo in PR review.
Verify that filter clauses mixed with typed catches preserve original order when sharing the same try region. The runtime evaluates filters sequentially, so reordering would change exception dispatch semantics. Requested by @tonyredondo in PR comment.
Add end-to-end tests that verify multi-catch and filter clause ordering is preserved after instrumentation. The methods are instrumented via DD_TRACE_METHODS and self-validate by asserting each exception type dispatches to the correct handler. Covers both Tony's multi-catch scenario and the filter clause scenario.
bouwkast
force-pushed
the
steven/attempt-fix-invalid-program-exception
branch
from
April 17, 2026 13:07
20515be to
c5713b9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Fix incorrect EH clause ordering in
ILRewriter::Export()that causedInvalidProgramExceptionwhen instrumenting methods with try blocks nested inside handler regions.Reason for change
When we instrument a method, the IL rewriter may insert try/catch blocks inside the handler of an existing try/catch (e.g., debugger inserting
EndMethod(SetException)inside an async state machine's outer catch, or CallTarget instrumentation wrapping handler logic). ECMA-335 II.19 requires such nested clauses to appear before the clause whose region encloses them in the EH table.The existing sort comparator only checked try-in-try containment:
return a.m_pTryBegin->m_offset > b.m_pTryBegin->m_offset && a.m_pTryEnd->m_offset < b.m_pTryEnd->m_offset;Because the inserted clause's try region starts after the outer clause's try region ends, this check returns false. The clause stays in its appended position -- after the outer clause -- violating ECMA-335. The CLR validates EH table ordering on JIT and rejects the method with
InvalidProgramException.The comparator also violated strict weak ordering required by
std::sort(transitivity of incomparability fails for sibling clauses adjacent to a nesting relationship), making its behavior undefined.Implementation details
Replaced the inline
std::sortwithILRewriter::SortEHClauses, a new static method that:Computes a nesting depth for each clause: the number of other clauses whose try or handler region strictly contains this clause's try region. This correctly detects both try-in-try and try-in-handler nesting per ECMA-335 II.19.
Sorts by (depth descending, try offset ascending), which is a total order and therefore satisfies strict weak ordering. Deeper-nested clauses sort first, satisfying the ECMA-335 requirement.
The method is extracted as a
staticto make it directly unit-testable without requiringICorProfilerInfo. Internal allocations usestd::unique_ptrto prevent memory leaks if an allocation fails.Test coverage
Added
ILRewriterEHSortTestinDatadog.Tracer.Native.Testswith 8 Google Test cases covering:EndMethod(SetException)clause is ordered before the outer clause whose handler contains itThe
OldComparatorFailsMiddlewareScenariotest applies the original comparator inline against the middleware topology and asserts it produces the wrong ordering, confirming the bug is real and the new tests would catch a regression.Added integration test in
tracer/test/test-applications/integrations/Samples.TraceAnnotations/ExtremeExceptionHandling.csthat reproduces the crash. Without the fix, the test causes a CLR fatal exception.Other details
Symptom:
System.InvalidProgramException: Common Language Runtime detected an invalid programthrown on any instrumented method whose EH table contains try-in-handler nesting. Originally observed on async middlewareInvokeAsyncmethods with Exception Replay enabled, but affects any instrumentation path that goes throughILRewriter::Export().Linked to APMS-19196