Fix EH clause sort to handle try-in-handler nesting

tracer/src/Datadog.Tracer.Native/il_rewriter.cpp

@@ -7,6 +7,7 @@
 #include "il_rewriter.h"
 
 #include <algorithm>
+#include <memory>
 
 #undef IfFailRet
 #define IfFailRet(EXPR)                                                                                                \
@@ -682,11 +683,11 @@ HRESULT ILRewriter::Export()
             //      handlers are nested, the most deeply nested try blocks shall come before the try blocks that enclose them."
             // If we will not follow that, we might face InvalidProgramException.
             // That's why we order the Exception Clauses right before applying them.
+            //
+            // Per ECMA-335 II.19, nesting includes a clause's try region being inside
+            // another clause's try region OR handler region.
 
-            std::sort(m_pEH, m_pEH + m_nEH, [](EHClause a, EHClause b) {
-                return a.m_pTryBegin->m_offset > b.m_pTryBegin->m_offset &&
-                       a.m_pTryEnd->m_offset < b.m_pTryEnd->m_offset;
-            });
+            SortEHClauses(m_pEH, m_nEH);
 
             for (unsigned iEH = 0; iEH < m_nEH; iEH++)
             {
@@ -819,3 +820,96 @@ bool ILRewriter::IsLoadConstantInstruction(unsigned opcode)
         default:return false;
     }
 }
+
+// Checks whether the range [innerBegin, innerEnd) is a proper subset of [outerBegin, outerEnd).
+// All values are IL offsets using exclusive ends (first offset past the region).
+static bool IsProperlyContained(unsigned innerBegin, unsigned innerEnd, unsigned outerBegin, unsigned outerEnd)
+{
+    bool contained = innerBegin >= outerBegin && innerEnd <= outerEnd;
+    bool identical = innerBegin == outerBegin && innerEnd == outerEnd;
+    return contained && !identical;
+}
+
+void ILRewriter::SortEHClauses(EHClause* pEH, unsigned nEH)
+{
+    if (nEH <= 1)
+    {
+        return;
+    }
+
+    // Per ECMA-335 I.12.4.2.5, the most deeply nested try blocks must come first
+    // in the EH table. Per II.19, nesting includes a clause's protected (try) block
+    // being inside another clause's protected block OR handler block.
+    //
+    // We compute a nesting depth for each clause: the number of other clauses whose
+    // try or handler region encloses this clause's try region. Then we sort by
+    // depth descending so that the most deeply nested clauses appear first.
+
+    std::unique_ptr<unsigned[]> depth(new unsigned[nEH]());  // zero-initialized
+
+    for (unsigned i = 0; i < nEH; i++)
+    {
+        unsigned iTryBegin = pEH[i].m_pTryBegin->m_offset;
+        unsigned iTryEnd = pEH[i].m_pTryEnd->m_offset;
+
+        for (unsigned j = 0; j < nEH; j++)
+        {
+            if (i == j)
+            {
+                continue;
+            }
+
+            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;
+
+            bool nestedInTry = IsProperlyContained(iTryBegin, iTryEnd, jTryBegin, jTryEnd);
+            bool nestedInHandler = IsProperlyContained(iTryBegin, iTryEnd, jHandlerBegin, jHandlerEnd);
+
+            if (nestedInTry || nestedInHandler)
+            {
+                depth[i]++;
+            }
+        }
+    }
+
+    // Build an index array and sort it. Using indices (rather than sorting clauses
+    // directly) lets us use the original position as a stability tiebreaker.
+    // This preserves the compiler's ordering of catch handlers that share the same
+    // try region -- their order determines which handler the runtime evaluates first.
+    std::unique_ptr<unsigned[]> indices(new unsigned[nEH]);
+    for (unsigned i = 0; i < nEH; i++)
+    {
+        indices[i] = i;
+    }
+
+    std::sort(indices.get(), indices.get() + nEH, [&](unsigned a, unsigned b) {
+        if (depth[a] != depth[b])
+        {
+            return depth[a] > depth[b];
+        }
+
+        unsigned offsetA = pEH[a].m_pTryBegin->m_offset;
+        unsigned offsetB = pEH[b].m_pTryBegin->m_offset;
+        if (offsetA != offsetB)
+        {
+            return offsetA < offsetB;
+        }
+
+        // Preserve original order for clauses with equal depth and try offset
+        // (e.g. multiple catch handlers for the same try block).
+        return a < b;
+    });
+
+    // Apply the sorted order back to the EH clause array.
+    std::unique_ptr<EHClause[]> sorted(new EHClause[nEH]);
+    for (unsigned i = 0; i < nEH; i++)
+    {
+        sorted[i] = pEH[indices[i]];
+    }
+    for (unsigned i = 0; i < nEH; i++)
+    {
+        pEH[i] = sorted[i];
+    }
+}

tracer/src/Datadog.Tracer.Native/il_rewriter.h

@@ -150,6 +150,8 @@ class ILRewriter
     static uint32_t GetLocalIndexFromOpcode(const ILInstr* pInstr);
 
     static bool IsLoadConstantInstruction(unsigned opcode);
+
+    static void SortEHClauses(EHClause* pEH, unsigned nEH);
 };
 
 #endif // DD_CLR_PROFILER_IL_REWRITER_H_

tracer/test/Datadog.Trace.ClrProfiler.IntegrationTests/TraceAnnotationsTests.cs

@@ -78,7 +78,7 @@ protected TraceAnnotationsTests(string sampleAppName, bool enableTelemetry, ITes
         [SkippableFact]
         public async Task SubmitTraces()
         {
-            const int expectedSpanCount = 51;
+            const int expectedSpanCount = 52;
             var ddTraceMethodsString = string.Empty;
 
             foreach (var type in TestTypes)
@@ -88,6 +88,10 @@ public async Task SubmitTraces()
 
             ddTraceMethodsString += ";Samples.TraceAnnotations.ExtensionMethods[ExtensionMethodForTestType,ExtensionMethodForTestTypeGeneric,ExtensionMethodForTestTypeTypeStruct];System.Net.Http.HttpRequestMessage[set_Method]";
 
+            // Add method with extreme exception handling nesting (APMS-19196 regression test)
+            // Must target the sync method which has the EH directly in its body (not in an async state machine)
+            ddTraceMethodsString += ";Samples.TraceAnnotations.ExtremeExceptionHandling[DeepNestedExceptionHandlingSync]";
+
             SetEnvironmentVariable("DD_TRACE_METHODS", ddTraceMethodsString);
             // Don't bother with telemetry in version mismatch scenarios because older versions may only support V1 telemetry
             // which we no longer support in our mock telemetry agent

tracer/test/Datadog.Tracer.Native.Tests/CMakeLists.txt

@@ -43,6 +43,7 @@ add_executable(${TEST_EXECUTABLE_NAME}
     integration_test.cpp
     version_struct_test.cpp
     iast_util_test.cpp
+    il_rewriter_eh_sort_test.cpp
     string_test.cpp
 	util_test.cpp
 )

tracer/test/Datadog.Tracer.Native.Tests/Datadog.Tracer.Native.Tests.vcxproj

@@ -88,6 +88,7 @@
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="iast_util_test.cpp" />
+    <ClCompile Include="il_rewriter_eh_sort_test.cpp" />
     <ClCompile Include="integration_test.cpp" />
     <ClCompile Include="clr_helper_test.cpp" />
     <ClCompile Include="metadata_builder_test.cpp" />

tracer/test/Datadog.Tracer.Native.Tests/Datadog.Tracer.Native.Tests.vcxproj.filters

@@ -2,6 +2,7 @@
 <Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <ItemGroup>
     <ClCompile Include="iast_util_test.cpp" />
+    <ClCompile Include="il_rewriter_eh_sort_test.cpp" />
     <ClCompile Include="integration_test.cpp" />
     <ClCompile Include="clr_helper_test.cpp" />
     <ClCompile Include="metadata_builder_test.cpp" />