Skip to content

Commit

Permalink
Fix iteration in redundant goto elimination
Browse files Browse the repository at this point in the history
This loop expected that, for a given exitTreeTop, the next tree
exitTreeTop->getNextTreeTop() would remain fixed even if the current
block is removed from the trees. However, we shouldn't assume anything
about the next/prev pointers of trees that have been deleted, since it's
highly unclear which trees are supposed to be before/after a deleted
tree. And indeed, the removal of the current block can sometimes update
the next pointer. In particular, this happens when removing an empty
block (in the following example, block_X) with two predecessors, one of
which falls through (block_F) and one of which branches (block_B):

    BBStart <block_B>
    ...
    if... --> block_X
    BBEnd </block_B>

    ...

    BBStart <block_F>
    ... (can fall through)
    BBEnd </block_F>

    BBStart <block_X>
    BBEnd </block_X>

    BBStart <block_N>
    ...
    BBEnd </block_N>

    BBStart <block_N'>
    ...
    BBEnd </block_N'>

When removing block_X, redundant goto elimination (indirectly) calls
TR::Block::insertBlockAsFallThrough() to make block_F fall through
directly to block_N. This effectively clips out block_N's trees and then
reinserts them immediately after block_F. As a result, before block_X is
deleted, the trees are temporarily in the order shown below, where the
tree immediately following the exit of block_X is the entry of block_N',
not block_N:

    BBStart <block_F>
    ... (can fall through)
    BBEnd </block_F>

    BBStart <block_N>
    ...
    BBEnd </block_N>

    BBStart <block_X>
    BBEnd </block_X>

    BBStart <block_N'>
    ...
    BBEnd </block_N'>

The typical consequence of the mistaken assumption that the next tree
remains stable would be just that processing skips block_N and goes
directly to block_N'.

However, this assumption can cause a crash when redundant goto
elimination is requested on block_X specifically. In that case, endTree
is the entry of block_N, and because block_N is skipped, the loop
doesn't terminate properly. Eventually it runs past the last block and
tries to do treeTop->getNode() when treeTop is null.

The crash went unnoticed because the code in dead trees elimination that
requests redundant goto elimination on particular blocks was instead
mistakenly requesting a pass over the entire method.

To prevent these issues, redundant goto elimination now gets the entry
of the next block before attempting to remove the current block.
  • Loading branch information
jdmpapin committed Jun 4, 2021
1 parent c086f16 commit e495805
Showing 1 changed file with 3 additions and 3 deletions.
6 changes: 3 additions & 3 deletions compiler/optimizer/LocalOpts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3338,16 +3338,16 @@ int32_t TR_EliminateRedundantGotos::process(TR::TreeTop *startTree, TR::TreeTop
bool gotosWereEliminated = false;


for (TR::TreeTop *treeTop = startTree, *exitTreeTop;
for (TR::TreeTop *treeTop = startTree, *nextBlockStartTree = NULL;
(treeTop != endTree);
treeTop = exitTreeTop->getNextTreeTop())
treeTop = nextBlockStartTree)
{
// Get information about this block
//
TR::Node *node = treeTop->getNode();
TR_ASSERT(node->getOpCodeValue() == TR::BBStart, "Local Opts, expected BBStart treetop");
TR::Block *block = node->getBlock();
exitTreeTop = block->getExit();
nextBlockStartTree = block->getExit()->getNextTreeTop();

if (block->hasExceptionPredecessors())
continue;
Expand Down

0 comments on commit e495805

Please sign in to comment.