Skip to content

Commit

Permalink
Introduce BBJ_EHFAULTRET (#84467)
Browse files Browse the repository at this point in the history 
The IL `endfinally` and `endfault` instructions are aliases,
imported in the JIT IR as `BBJ_EHFINALLYRET`.

Introduce `BBJ_EHFAULTRET` for cases within `fault` clauses.

This simplifies some code, and makes it more clear when writing
code that `fault` clauses need to be considered, separately from
`finally` clauses.

To do this, after importing EH clauses, convert any `BBJ_EHFINALLYRET`
as necessary. Also, try/finally cloning, which (sometimes) converts
`finally` clauses to `fault` clauses, needs to update the corresponding
`BBJ_EHFINALLYRET`. When creating new try/fault clauses for synchronized
functions, use `BBJ_EHFAULTRET` now.

Fixes #84307
  • Loading branch information
@BruceForstall
BruceForstall authored Apr 7, 2023
1 parent 35f5a56 commit f402190
Show file tree
Hide file tree
Showing 18 changed files with 177 additions and 75 deletions.
34 changes: 9 additions & 25 deletions src/coreclr/jit/block.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -609,6 +609,10 @@ void BasicBlock::dspJumpKind()
printf(" (finret)");
break;

case BBJ_EHFAULTRET:
printf(" (falret)");
break;

case BBJ_EHFILTERRET:
printf(" (fltret)");
break;
Expand Down Expand Up @@ -1023,6 +1027,7 @@ bool BasicBlock::bbFallsThrough() const
{
case BBJ_THROW:
case BBJ_EHFINALLYRET:
case BBJ_EHFAULTRET:
case BBJ_EHFILTERRET:
case BBJ_EHCATCHRET:
case BBJ_RETURN:
Expand Down Expand Up @@ -1060,6 +1065,7 @@ unsigned BasicBlock::NumSucc() const
case BBJ_THROW:
case BBJ_RETURN:
case BBJ_EHFINALLYRET:
case BBJ_EHFAULTRET:
case BBJ_EHFILTERRET:
return 0;

Expand Down Expand Up @@ -1147,30 +1153,17 @@ unsigned BasicBlock::NumSucc(Compiler* comp)
{
case BBJ_THROW:
case BBJ_RETURN:
case BBJ_EHFAULTRET:
return 0;

case BBJ_EHFINALLYRET:
{
// We may call this method before we realize we have invalid IL. Tolerate.
//
if (!hasHndIndex())
{
return 0;
}

// The first block of the handler is labelled with the catch type.
BasicBlock* hndBeg = comp->fgFirstBlockOfHandler(this);
if (hndBeg->bbCatchTyp == BBCT_FINALLY)
{
return comp->fgNSuccsOfFinallyRet(this);
}
else
{
assert(hndBeg->bbCatchTyp == BBCT_FAULT); // We can only BBJ_EHFINALLYRET from FINALLY and FAULT.
// A FAULT block has no successors.
return 0;
}
}
return comp->fgNSuccsOfFinallyRet(this);

case BBJ_CALLFINALLY:
case BBJ_ALWAYS:
Expand Down Expand Up @@ -1615,16 +1608,7 @@ bool BasicBlock::hasEHBoundaryIn() const
//
bool BasicBlock::hasEHBoundaryOut() const
{
bool returnVal = false;
if (bbJumpKind == BBJ_EHFILTERRET)
{
returnVal = true;
}

if (bbJumpKind == BBJ_EHFINALLYRET)
{
returnVal = true;
}
bool returnVal = KindIs(BBJ_EHFILTERRET, BBJ_EHFINALLYRET, BBJ_EHFAULTRET);

#if FEATURE_EH_FUNCLETS
if (bbJumpKind == BBJ_EHCATCHRET)
Expand Down
24 changes: 22 additions & 2 deletions src/coreclr/jit/block.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ typedef BitVec_ValRet_T ASSERT_VALRET_TP;

enum BBjumpKinds : BYTE
{
BBJ_EHFINALLYRET,// block ends with 'endfinally' (for finally or fault)
BBJ_EHFINALLYRET,// block ends with 'endfinally' (for finally)
BBJ_EHFAULTRET, // block ends with 'endfinally' (IL alias for 'endfault') (for fault)
BBJ_EHFILTERRET, // block ends with 'endfilter'
BBJ_EHCATCHRET, // block ends with a leave out of a catch (only #if defined(FEATURE_EH_FUNCLETS))
BBJ_THROW, // block ends with 'throw'
Expand All @@ -74,6 +75,24 @@ enum BBjumpKinds : BYTE
BBJ_COUNT
};

#ifdef DEBUG
const char* const BBjumpKindNames[] = {
"BBJ_EHFINALLYRET",
"BBJ_EHFAULTRET",
"BBJ_EHFILTERRET",
"BBJ_EHCATCHRET",
"BBJ_THROW",
"BBJ_RETURN",
"BBJ_NONE",
"BBJ_ALWAYS",
"BBJ_LEAVE",
"BBJ_CALLFINALLY",
"BBJ_COND",
"BBJ_SWITCH",
"BBJ_COUNT"
};
#endif // DEBUG

// clang-format on

struct GenTree;
Expand Down Expand Up @@ -829,7 +848,7 @@ struct BasicBlock : private LIR::Range
// GetSucc() without a Compiler*.
//
// The behavior of NumSucc()/GetSucc() is different when passed a Compiler* for blocks that end in:
// (1) BBJ_EHFINALLYRET (a return from a finally or fault block)
// (1) BBJ_EHFINALLYRET (a return from a finally block)
// (2) BBJ_EHFILTERRET (a return from EH filter block)
// (3) BBJ_SWITCH
//
Expand Down Expand Up @@ -1656,6 +1675,7 @@ inline BasicBlock::BBSuccList::BBSuccList(const BasicBlock* block)
case BBJ_THROW:
case BBJ_RETURN:
case BBJ_EHFINALLYRET:
case BBJ_EHFAULTRET:
case BBJ_EHFILTERRET:
// We don't need m_succs.
m_begin = nullptr;
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/jit/codegencommon.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,7 @@ void CodeGen::genMarkLabelsForCodegen()
break;

case BBJ_EHFINALLYRET:
case BBJ_EHFAULTRET:
case BBJ_EHFILTERRET:
case BBJ_RETURN:
case BBJ_THROW:
Expand Down Expand Up @@ -7857,7 +7858,7 @@ void CodeGen::genReturn(GenTree* treeNode)
#else // !FEATURE_EH_FUNCLETS
// Don't generate stack checks for x86 finally/filter EH returns: these are not invoked
// with the same SP as the main function. See also CodeGen::genEHFinallyOrFilterRet().
if ((compiler->compCurBB->bbJumpKind == BBJ_EHFINALLYRET) || (compiler->compCurBB->bbJumpKind == BBJ_EHFILTERRET))
if (compiler->compCurBB->KindIs(BBJ_EHFINALLYRET, BBJ_EHFAULTRET, BBJ_EHFILTERRET))
{
doStackPointerCheck = false;
}
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/jit/codegenlinear.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,7 @@ void CodeGen::genCodeForBBlist()

case BBJ_RETURN:
case BBJ_EHFINALLYRET:
case BBJ_EHFAULTRET:
case BBJ_EHFILTERRET:
// These are the "epilog follows" case, handled in the emitter.

Expand Down Expand Up @@ -715,6 +716,7 @@ void CodeGen::genCodeForBBlist()
FALLTHROUGH;

case BBJ_EHFINALLYRET:
case BBJ_EHFAULTRET:
case BBJ_EHFILTERRET:
genReserveFuncletEpilog(block);
break;
Expand All @@ -726,6 +728,7 @@ void CodeGen::genCodeForBBlist()
break;

case BBJ_EHFINALLYRET:
case BBJ_EHFAULTRET:
case BBJ_EHFILTERRET:
genEHFinallyOrFilterRet(block);
break;
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/codegenxarch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ void CodeGen::genEHFinallyOrFilterRet(BasicBlock* block)
assert(block->lastNode() != nullptr);
assert(block->lastNode()->OperGet() == GT_RETFILT);

if (block->bbJumpKind == BBJ_EHFINALLYRET)
if (block->KindIs(BBJ_EHFINALLYRET, BBJ_EHFAULTRET))
{
assert(block->lastNode()->AsOp()->gtOp1 == nullptr); // op1 == nullptr means endfinally

Expand Down
38 changes: 29 additions & 9 deletions src/coreclr/jit/fgbasic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2802,6 +2802,7 @@ void Compiler::fgLinkBasicBlocks()
// We do it in impFixPredLists.
break;

case BBJ_EHFAULTRET:
case BBJ_THROW:
case BBJ_RETURN:
break;
Expand Down Expand Up @@ -3064,6 +3065,7 @@ unsigned Compiler::fgMakeBasicBlocks(const BYTE* codeAddr, IL_OFFSET codeSize, F
break;

case CEE_ENDFINALLY:
// Start with BBJ_EHFINALLYRET; change to BBJ_EHFAULTRET later if it's in a 'fault' clause.
jmpKind = BBJ_EHFINALLYRET;
break;

Expand Down Expand Up @@ -3782,6 +3784,13 @@ void Compiler::fgFindBasicBlocks()
if (!block->hasHndIndex())
{
block->setHndIndex(XTnum);

// If the most nested EH handler region of this block is a 'fault' region, then change any
// BBJ_EHFINALLYRET that were imported to BBJ_EHFAULTRET.
if ((hndBegBB->bbCatchTyp == BBCT_FAULT) && block->KindIs(BBJ_EHFINALLYRET))
{
block->bbJumpKind = BBJ_EHFAULTRET;
}
}

// All blocks in a catch handler or filter are rarely run, except the entry
Expand Down Expand Up @@ -4060,6 +4069,7 @@ void Compiler::fgCheckBasicBlockControlFlow()
break;

case BBJ_EHFINALLYRET:
case BBJ_EHFAULTRET:
case BBJ_EHFILTERRET:

if (!blk->hasHndIndex()) // must be part of a handler
Expand All @@ -4077,17 +4087,28 @@ void Compiler::fgCheckBasicBlockControlFlow()
BADCODE("Unexpected endfilter");
}
}
// endfinally allowed only in a finally/fault block
else if (!HBtab->HasFinallyOrFaultHandler())
else if (blk->bbJumpKind == BBJ_EHFILTERRET)
{
BADCODE("Unexpected endfinally");
// endfinally allowed only in a finally block
if (!HBtab->HasFinallyHandler())
{
BADCODE("Unexpected endfinally");
}
}
else if (blk->bbJumpKind == BBJ_EHFAULTRET)
{
// 'endfault' (alias of IL 'endfinally') allowed only in a fault block
if (!HBtab->HasFaultHandler())
{
BADCODE("Unexpected endfault");
}
}

// The handler block should be the innermost block
// Exception blocks are listed, innermost first.
if (blk->hasTryIndex() && (blk->getTryIndex() < blk->getHndIndex()))
{
BADCODE("endfinally / endfilter in nested try block");
BADCODE("endfinally / endfault / endfilter in nested try block");
}

break;
Expand Down Expand Up @@ -6298,8 +6319,7 @@ BasicBlock* Compiler::fgFindInsertPoint(unsigned regionIndex,
#endif // DEBUG

JITDUMP("fgFindInsertPoint(regionIndex=%u, putInTryRegion=%s, startBlk=" FMT_BB ", endBlk=" FMT_BB
", nearBlk=" FMT_BB ", "
"jumpBlk=" FMT_BB ", runRarely=%s)\n",
", nearBlk=" FMT_BB ", jumpBlk=" FMT_BB ", runRarely=%s)\n",
regionIndex, dspBool(putInTryRegion), startBlk->bbNum, (endBlk == nullptr) ? 0 : endBlk->bbNum,
(nearBlk == nullptr) ? 0 : nearBlk->bbNum, (jumpBlk == nullptr) ? 0 : jumpBlk->bbNum, dspBool(runRarely));

Expand Down Expand Up @@ -6697,10 +6717,10 @@ _FoundAfterBlk:;
/* We have decided to insert the block after 'afterBlk'. */
noway_assert(afterBlk != nullptr);

JITDUMP("fgNewBBinRegion(jumpKind=%u, tryIndex=%u, hndIndex=%u, putInFilter=%s, runRarely=%s, insertAtEnd=%s): "
JITDUMP("fgNewBBinRegion(jumpKind=%s, tryIndex=%u, hndIndex=%u, putInFilter=%s, runRarely=%s, insertAtEnd=%s): "
"inserting after " FMT_BB "\n",
jumpKind, tryIndex, hndIndex, dspBool(putInFilter), dspBool(runRarely), dspBool(insertAtEnd),
afterBlk->bbNum);
BBjumpKindNames[jumpKind], tryIndex, hndIndex, dspBool(putInFilter), dspBool(runRarely),
dspBool(insertAtEnd), afterBlk->bbNum);

return fgNewBBinRegionWorker(jumpKind, afterBlk, regionIndex, putInTryRegion);
}
Expand Down
8 changes: 7 additions & 1 deletion src/coreclr/jit/fgdiagnostic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ void Compiler::fgDebugCheckUpdate()
{
case BBJ_CALLFINALLY:
case BBJ_EHFINALLYRET:
case BBJ_EHFAULTRET:
case BBJ_EHFILTERRET:
case BBJ_RETURN:
/* for BBJ_ALWAYS is probably just a GOTO, but will have to be treated */
Expand Down Expand Up @@ -1994,6 +1995,10 @@ void Compiler::fgTableDispBasicBlock(BasicBlock* block, int ibcColWidth /* = 0 *
printf("%*s (finret)", maxBlockNumWidth - 2, "");
break;

case BBJ_EHFAULTRET:
printf("%*s (falret)", maxBlockNumWidth - 2, "");
break;

case BBJ_EHFILTERRET:
printf("%*s (fltret)", maxBlockNumWidth - 2, "");
break;
Expand Down Expand Up @@ -2633,9 +2638,10 @@ bool BBPredsChecker::CheckJump(BasicBlock* blockPred, BasicBlock* block)
assert(CheckEHFinallyRet(blockPred, block));
return true;

case BBJ_EHFAULTRET:
case BBJ_THROW:
case BBJ_RETURN:
assert(!"THROW and RETURN block cannot be in the predecessor list!");
assert(!"EHFAULTRET, THROW, and RETURN block cannot be in the predecessor list!");
break;

case BBJ_SWITCH:
Expand Down
25 changes: 20 additions & 5 deletions src/coreclr/jit/fgehopt.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1035,7 +1035,7 @@ PhaseStatus Compiler::fgCloneFinally()
// statement after the finally, so they can share the clone.
//
// Clone the finally body, and splice it into the flow graph
// within in the parent region of the try.
// within the parent region of the try.
//
const unsigned finallyTryIndex = firstBlock->bbTryIndex;
BasicBlock* insertAfter = nullptr;
Expand All @@ -1048,14 +1048,18 @@ PhaseStatus Compiler::fgCloneFinally()
{
BasicBlock* newBlock;

// Avoid asserts when `fgNewBBinRegion` verifies the handler table, by mapping any cloned finally
// return blocks to BBJ_ALWAYS (which we would do below if we didn't do it here).
BBjumpKinds bbNewJumpKind = (block->bbJumpKind == BBJ_EHFINALLYRET) ? BBJ_ALWAYS : block->bbJumpKind;

if (block == firstBlock)
{
// Put first cloned finally block into the appropriate
// region, somewhere within or after the range of
// callfinallys, depending on the EH implementation.
const unsigned hndIndex = 0;
BasicBlock* const nearBlk = cloneInsertAfter;
newBlock = fgNewBBinRegion(block->bbJumpKind, finallyTryIndex, hndIndex, nearBlk);
newBlock = fgNewBBinRegion(bbNewJumpKind, finallyTryIndex, hndIndex, nearBlk);

// If the clone ends up just after the finally, adjust
// the stopping point for finally traversal.
Expand All @@ -1069,7 +1073,7 @@ PhaseStatus Compiler::fgCloneFinally()
{
// Put subsequent blocks in the same region...
const bool extendRegion = true;
newBlock = fgNewBBafter(block->bbJumpKind, insertAfter, extendRegion);
newBlock = fgNewBBafter(bbNewJumpKind, insertAfter, extendRegion);
}

cloneBBCount++;
Expand Down Expand Up @@ -1121,7 +1125,7 @@ PhaseStatus Compiler::fgCloneFinally()
JITDUMP("Cloned finally blocks are: " FMT_BB " ... " FMT_BB "\n", blockMap[firstBlock]->bbNum,
blockMap[lastBlock]->bbNum);

// Redirect redirect any branches within the newly-cloned
// Redirect any branches within the newly-cloned
// finally, and any finally returns to jump to the return
// point.
for (BasicBlock* block = firstBlock; block != nextBlock; block = block->bbNext)
Expand All @@ -1134,7 +1138,7 @@ PhaseStatus Compiler::fgCloneFinally()
GenTree* finallyRetExpr = finallyRet->GetRootNode();
assert(finallyRetExpr->gtOper == GT_RETFILT);
fgRemoveStmt(newBlock, finallyRet);
newBlock->bbJumpKind = BBJ_ALWAYS;
assert(newBlock->bbJumpKind == BBJ_ALWAYS); // we mapped this above already
newBlock->bbJumpDest = normalCallFinallyReturn;

fgAddRefPred(normalCallFinallyReturn, newBlock);
Expand Down Expand Up @@ -1231,6 +1235,17 @@ PhaseStatus Compiler::fgCloneFinally()
JITDUMP("All callfinallys retargeted; changing finally to fault.\n");
HBtab->ebdHandlerType = EH_HANDLER_FAULT_WAS_FINALLY;
firstBlock->bbCatchTyp = BBCT_FAULT;

// Change all BBJ_EHFINALLYRET to BBJ_EHFAULTRET in the now-fault region.
BasicBlock* const hndBegIter = HBtab->ebdHndBeg;
BasicBlock* const hndEndIter = HBtab->ebdHndLast->bbNext;
for (BasicBlock* block = hndBegIter; block != hndEndIter; block = block->bbNext)
{
if (block->bbJumpKind == BBJ_EHFINALLYRET)
{
block->bbJumpKind = BBJ_EHFAULTRET;
}
}
}
else
{
Expand Down
Loading

0 comments on commit f402190

Please # to comment.