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.

Sign up for GitHub

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

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

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c17d4c9
Report clauses in try region index order
amanasifkhalid Jul 7, 2024
15198f5
Add tie-breaker logic
amanasifkhalid Jul 7, 2024
0f07f78
Feedback
amanasifkhalid Jul 8, 2024
ce30fd9
Use ptrdiff_t
amanasifkhalid Jul 8, 2024
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
View file
Open in desktop
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 auto leftIndex = left.HBtab - this->compiler->compHndBBtab;
const auto rightIndex = right.HBtab - this->compiler->compHndBBtab;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have used auto for anything but function types. Use ptrdiff_t?

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 •
edited
Loading

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