Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: Ensure EH clauses with same try region are reported contiguously to the VM #104531

Merged
merged 4 commits into from
Jul 8, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 50 additions & 19 deletions src/coreclr/jit/codegencommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
#include "patchpointinfo.h"
#include "optcse.h" // for cse metrics

#include "jitstd/algorithm.h"

/*****************************************************************************/

void CodeGenInterface::setFramePointerRequiredEH(bool value)
Expand Down Expand Up @@ -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 (compiler, CMK_Codegen) 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;
Expand All @@ -2427,7 +2437,43 @@ 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,
[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 ptrdiff_t leftIndex = left.HBtab - this->compiler->compHndBBtab;
const ptrdiff_t rightIndex = right.HBtab - this->compiler->compHndBBtab;
return leftIndex < rightIndex;
}

return leftTryIndex < rightTryIndex;
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use any of the CORINFO_EH_CLAUSE data when sorting, so seems like it would be more efficient to just sort a table of EHblkDsc* and then report each clause like we did before?

Copy link
Member Author

@amanasifkhalid amanasifkhalid Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially tried doing this, and then iterating over the sorted table to build the clauses in the above loop, but changing the order in which we call Compiler::ehCodeOffset hit JIT asserts (in particular, assert(cookie != nullptr) in Compiler::ehEmitCookie) -- I think emitter::emitCodeOffset has some hard dependency on us calling ehCodeOffset in the order of the JIT's EH table. I can look into cleaning this up in a follow-up if you'd like, though the TP cost of the current approach doesn't seem too bad.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats fine. EH is relatively rare and lots of EH clauses even more so.

I don't see why there'd be such an order dependence in the emitter, so it is worth digging into more deeply.


// 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)
{
Expand All @@ -2436,33 +2482,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.
Expand Down
Loading