Skip to content

Commit fd4621c

Browse files
Fix removal of unreachable BBJ_CALLFINALLY/BBJ_ALWAYS on Linux/arm (#94751)
In the test case, with extreme amounts of unreachable code, we could remove a BBJ_CALLFINALLY that was not marked as `BBF_DONT_REMOVE`. If so, it would not go down the code path that handles removing the BBJ_ALWAYS pair of the BBJ_CALLFINALLY, which handles removing the `BBF_FINALLY_TARGET` flag on Linux/arm. Refactor the code so unreachable BBJ_CALLFINALLY/BBJ_ALWAYS pairs are always handled. Fixes #94680
1 parent 0883c0d commit fd4621c

File tree

1 file changed

+38
-27
lines changed

1 file changed

+38
-27
lines changed

src/coreclr/jit/fgopt.cpp

Lines changed: 38 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -453,14 +453,14 @@ bool Compiler::fgRemoveUnreachableBlocks(CanRemoveBlockBody canRemoveBlock)
453453
// Make sure that the block was marked as removed */
454454
noway_assert(block->bbFlags & BBF_REMOVED);
455455

456-
// Some blocks mark the end of trys and catches
457-
// and can't be removed. We convert these into
458-
// empty blocks of type BBJ_THROW
456+
// Some blocks mark the end of trys and catches and can't be removed. We convert these into
457+
// empty blocks of type BBJ_THROW.
458+
459+
const bool bIsBBCallAlwaysPair = block->isBBCallAlwaysPair();
460+
BasicBlock* leaveBlk = bIsBBCallAlwaysPair ? block->Next() : nullptr;
459461

460462
if (block->bbFlags & BBF_DONT_REMOVE)
461463
{
462-
const bool bIsBBCallAlwaysPair = block->isBBCallAlwaysPair();
463-
464464
// Unmark the block as removed, clear BBF_INTERNAL, and set BBJ_IMPORTED
465465

466466
JITDUMP("Converting BBF_DONT_REMOVE block " FMT_BB " to BBJ_THROW\n", block->bbNum);
@@ -472,36 +472,47 @@ bool Compiler::fgRemoveUnreachableBlocks(CanRemoveBlockBody canRemoveBlock)
472472
block->bbFlags |= BBF_IMPORTED;
473473
block->SetJumpKindAndTarget(BBJ_THROW DEBUG_ARG(this));
474474
block->bbSetRunRarely();
475+
}
476+
else
477+
{
478+
/* We have to call fgRemoveBlock next */
479+
hasUnreachableBlocks = true;
480+
changed = true;
481+
}
475482

476-
// If this is a <BBJ_CALLFINALLY, BBJ_ALWAYS> pair, we just converted it to a BBJ_THROW.
477-
// Get rid of the BBJ_ALWAYS block which is now dead.
478-
if (bIsBBCallAlwaysPair)
483+
// If this is a <BBJ_CALLFINALLY, BBJ_ALWAYS> pair, get rid of the BBJ_ALWAYS block which is now dead.
484+
if (bIsBBCallAlwaysPair)
485+
{
486+
assert(leaveBlk->KindIs(BBJ_ALWAYS));
487+
488+
if (!block->KindIs(BBJ_THROW))
479489
{
480-
BasicBlock* leaveBlk = block->Next();
481-
noway_assert(leaveBlk->KindIs(BBJ_ALWAYS));
490+
// We didn't convert the BBJ_CALLFINALLY to a throw, above. Since we already marked it as removed,
491+
// change the kind to something else. Otherwise, we can hit asserts below in fgRemoveBlock that
492+
// the leaveBlk BBJ_ALWAYS is not allowed to be a CallAlwaysPairTail.
493+
assert(block->KindIs(BBJ_CALLFINALLY));
494+
block->SetJumpKind(BBJ_NONE);
495+
}
482496

483-
leaveBlk->bbFlags &= ~BBF_DONT_REMOVE;
497+
leaveBlk->bbFlags &= ~BBF_DONT_REMOVE;
484498

485-
for (BasicBlock* const leavePredBlock : leaveBlk->PredBlocks())
486-
{
487-
fgRemoveEhfSuccessor(leavePredBlock, leaveBlk);
488-
}
489-
assert(leaveBlk->bbRefs == 0);
490-
assert(leaveBlk->bbPreds == nullptr);
499+
for (BasicBlock* const leavePredBlock : leaveBlk->PredBlocks())
500+
{
501+
fgRemoveEhfSuccessor(leavePredBlock, leaveBlk);
502+
}
503+
assert(leaveBlk->bbRefs == 0);
504+
assert(leaveBlk->bbPreds == nullptr);
491505

492-
fgRemoveBlock(leaveBlk, /* unreachable */ true);
506+
fgRemoveBlock(leaveBlk, /* unreachable */ true);
493507

494508
#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
495-
// We have to clear BBF_FINALLY_TARGET flag on the target node (of BBJ_ALWAYS).
496-
fgClearFinallyTargetBit(leaveBlk->GetJumpDest());
509+
// We have to clear BBF_FINALLY_TARGET flag on the target node (of BBJ_ALWAYS).
510+
fgClearFinallyTargetBit(leaveBlk->GetJumpDest());
497511
#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
498-
}
499-
}
500-
else
501-
{
502-
/* We have to call fgRemoveBlock next */
503-
hasUnreachableBlocks = true;
504-
changed = true;
512+
513+
// Note: `changed` will already have been set to true by processing the BBJ_CALLFINALLY.
514+
// `hasUnreachableBlocks` doesn't need to be set for the leaveBlk itself because we've already called
515+
// `fgRemoveBlock` on it.
505516
}
506517
}
507518

0 commit comments

Comments
 (0)