From c17d4c9f7cc4d4c585ee9b6008e9d78f97fe5344 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Sun, 7 Jul 2024 17:01:11 -0400 Subject: [PATCH 1/4] Report clauses in try region index order --- src/coreclr/jit/codegencommon.cpp | 57 ++++++++++++++++++++----------- 1 file changed, 38 insertions(+), 19 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 4a0d11282c5b3..a85cecbca3dc1 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -30,6 +30,8 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX #include "patchpointinfo.h" #include "optcse.h" // for cse metrics +#include "jitstd/algorithm.h" + /*****************************************************************************/ void CodeGenInterface::setFramePointerRequiredEH(bool value) @@ -2404,8 +2406,16 @@ void CodeGen::genReportEH() // Tell the VM how many EH clauses to expect. compiler->eeSetEHcount(EHCount); - XTnum = 0; // This is the index we pass to the VM + struct EHClauseInfo + { + CORINFO_EH_CLAUSE clause; + EHblkDsc* HBtab; + }; + + EHClauseInfo* clauses = new EHClauseInfo[compiler->compHndBBtabCount]; + // Set up EH clause table, but don't report anything to the VM, yet. + XTnum = 0; for (EHblkDsc* const HBtab : EHClauses(compiler)) { UNATIVE_OFFSET tryBeg, tryEnd, hndBeg, hndEnd, hndTyp; @@ -2427,7 +2437,31 @@ void CodeGen::genReportEH() hndTyp = HBtab->ebdTyp; } - CORINFO_EH_CLAUSE_FLAGS flags = ToCORINFO_EH_CLAUSE_FLAGS(HBtab->ebdHandlerType); + // Note that we reuse the CORINFO_EH_CLAUSE type, even though the names of + // the fields aren't accurate. + + CORINFO_EH_CLAUSE clause; + clause.ClassToken = hndTyp; /* filter offset is passed back here for filter-based exception handlers */ + clause.Flags = ToCORINFO_EH_CLAUSE_FLAGS(HBtab->ebdHandlerType); + clause.TryOffset = tryBeg; + clause.TryLength = tryEnd; + clause.HandlerOffset = hndBeg; + clause.HandlerLength = hndEnd; + clauses[XTnum++] = {clause, HBtab}; + } + + // The JIT's ordering of EH clauses does not guarantee that clauses covering the same try region are contiguous. + // We need this property to hold true so the CORINFO_EH_CLAUSE_SAMETRY flag is accurate. + jitstd::sort(clauses, clauses + compiler->compHndBBtabCount, + [](const EHClauseInfo& left, const EHClauseInfo& right) { + return left.HBtab->ebdTryBeg->bbTryIndex < right.HBtab->ebdTryBeg->bbTryIndex; + }); + + // Now, report EH clauses to the VM in order of increasing try region index. + for (XTnum = 0; XTnum < compiler->compHndBBtabCount; XTnum++) + { + CORINFO_EH_CLAUSE& clause = clauses[XTnum].clause; + EHblkDsc* const HBtab = clauses[XTnum].HBtab; if (XTnum > 0) { @@ -2436,33 +2470,18 @@ void CodeGen::genReportEH() // native code offsets because of different try blocks can have same offsets. Alternative // solution to this problem would be inserting extra nops to ensure that different try // blocks have different offsets. - if (EHblkDsc::ebdIsSameTry(HBtab, HBtab - 1)) + if (EHblkDsc::ebdIsSameTry(HBtab, clauses[XTnum - 1].HBtab)) { // The SAMETRY bit should only be set on catch clauses. This is ensured in IL, where only 'catch' is // allowed to be mutually-protect. E.g., the C# "try {} catch {} catch {} finally {}" actually exists in // IL as "try { try {} catch {} catch {} } finally {}". assert(HBtab->HasCatchHandler()); - flags = (CORINFO_EH_CLAUSE_FLAGS)(flags | CORINFO_EH_CLAUSE_SAMETRY); + clause.Flags = (CORINFO_EH_CLAUSE_FLAGS)(clause.Flags | CORINFO_EH_CLAUSE_SAMETRY); } } - // Note that we reuse the CORINFO_EH_CLAUSE type, even though the names of - // the fields aren't accurate. - - CORINFO_EH_CLAUSE clause; - clause.ClassToken = hndTyp; /* filter offset is passed back here for filter-based exception handlers */ - clause.Flags = flags; - clause.TryOffset = tryBeg; - clause.TryLength = tryEnd; - clause.HandlerOffset = hndBeg; - clause.HandlerLength = hndEnd; - assert(XTnum < EHCount); - - // Tell the VM about this EH clause. compiler->eeSetEHinfo(XTnum, &clause); - - ++XTnum; } // Now output duplicated clauses. From 15198f5545eda85450b630340415c895585cfe44 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Sun, 7 Jul 2024 18:59:32 -0400 Subject: [PATCH 2/4] Add tie-breaker logic --- src/coreclr/jit/codegencommon.cpp | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index a85cecbca3dc1..acf7eb016b155 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -2453,8 +2453,20 @@ void CodeGen::genReportEH() // The JIT's ordering of EH clauses does not guarantee that clauses covering the same try region are contiguous. // We need this property to hold true so the CORINFO_EH_CLAUSE_SAMETRY flag is accurate. jitstd::sort(clauses, clauses + compiler->compHndBBtabCount, - [](const EHClauseInfo& left, const EHClauseInfo& right) { - return left.HBtab->ebdTryBeg->bbTryIndex < right.HBtab->ebdTryBeg->bbTryIndex; + [this](const EHClauseInfo& left, const EHClauseInfo& right) { + const unsigned short leftTryIndex = left.HBtab->ebdTryBeg->bbTryIndex; + const unsigned short rightTryIndex = right.HBtab->ebdTryBeg->bbTryIndex; + + if (leftTryIndex == rightTryIndex) + { + // We have two clauses mapped to the same try region. + // Make sure we report the clause with the smaller index first. + const auto leftIndex = left.HBtab - this->compiler->compHndBBtab; + const auto rightIndex = right.HBtab - this->compiler->compHndBBtab; + return leftIndex < rightIndex; + } + + return leftTryIndex < rightTryIndex; }); // Now, report EH clauses to the VM in order of increasing try region index. From 0f07f780d2207393a5082f362287255a4e8357b2 Mon Sep 17 00:00:00 2001 From: Aman Khalid Date: Mon, 8 Jul 2024 09:05:36 -0400 Subject: [PATCH 3/4] Feedback Co-authored-by: Jakob Botsch Nielsen --- src/coreclr/jit/codegencommon.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index acf7eb016b155..4d77691182996 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -2412,7 +2412,7 @@ void CodeGen::genReportEH() EHblkDsc* HBtab; }; - EHClauseInfo* clauses = new EHClauseInfo[compiler->compHndBBtabCount]; + EHClauseInfo* clauses = new (compiler, CMK_Codegen) EHClauseInfo[compiler->compHndBBtabCount]; // Set up EH clause table, but don't report anything to the VM, yet. XTnum = 0; From ce30fd98a7dc4ca9d710dba48c7ac3c9dc407c23 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 8 Jul 2024 11:20:17 -0400 Subject: [PATCH 4/4] Use ptrdiff_t --- src/coreclr/jit/codegencommon.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 4d77691182996..8119a50baa7a0 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -2461,8 +2461,8 @@ void CodeGen::genReportEH() { // We have two clauses mapped to the same try region. // Make sure we report the clause with the smaller index first. - const auto leftIndex = left.HBtab - this->compiler->compHndBBtab; - const auto rightIndex = right.HBtab - this->compiler->compHndBBtab; + const ptrdiff_t leftIndex = left.HBtab - this->compiler->compHndBBtab; + const ptrdiff_t rightIndex = right.HBtab - this->compiler->compHndBBtab; return leftIndex < rightIndex; }