[llvm-bugs] [Bug 42087] New: [MachineBlockPlacement] Erroneous assert

[via llvm-bugs]

https://bugs.llvm.org/show_bug.cgi?id=42087

            Bug ID: 42087
           Summary: [MachineBlockPlacement] Erroneous assert
           Product: libraries
           Version: trunk
          Hardware: PC
                OS: All
            Status: NEW
          Severity: enhancement
          Priority: P
         Component: Common Code Generator Code
          Assignee: unassignedbugs at nondot.org
          Reporter: james at jamesmolloy.co.uk
                CC: llvm-bugs at lists.llvm.org

In asserts mode, the following code is enabled in MachineBlockPlacement (line
2476):

#ifndef NDEBUG
    if (!BlocksWithUnanalyzableExits.count(PrevBB)) {
      // Given the exact block placement we chose, we may actually not _need_
to
      // be able to edit PrevBB's terminator sequence, but not being _able_ to
      // do that at this point is a bug.
      assert((!TII->analyzeBranch(*PrevBB, TBB, FBB, Cond) ||
              !PrevBB->canFallThrough()) &&
             "Unexpected block with un-analyzable fallthrough!");
      Cond.clear();
      TBB = FBB = nullptr;
    }
#endif

This assertion is incorrect, or at least, can trigger on well-formed code.

BlocksWithUnanalyzableExits is populated with all blocks for which
analyzeBranch returns failure (true) or !BB->canFallThrough().

Consider an unanalyzable block that before layout is the last block in its
function. BB->canFallThrough() will *always* return false. Now consider the
above assert occurs *after* some layout has been changed. Now BB is no longer
the last BB in the block, canFallThrough() no longer returns false (because
it's unanalyzable and there's a succeessor block in layout order). We fail the
assert.

The comment below the assert appears to notice this can happen, but I don't
think it's got this particular case (the comment assumes the branch was
analyzable!):

    // The "PrevBB" is not yet updated to reflect current code layout, so,
    //   o. it may fall-through to a block without explicit "goto" instruction
    //      before layout, and no longer fall-through it after layout; or
    //   o. just opposite.
    //
    // analyzeBranch() may return erroneous value for FBB when these two
    // situations take place. For the first scenario FBB is mistakenly set
NULL;
    // for the 2nd scenario, the FBB, which is expected to be NULL, is
    // mistakenly pointing to "*BI".
    // Thus, if the future change needs to use FBB before the layout is set, it
    // has to correct FBB first by using the code similar to the following:
    //
    // if (!Cond.empty() && (!FBB || FBB == ChainBB)) {
    //   PrevBB->updateTerminator();
    //   Cond.clear();
    //   TBB = FBB = nullptr;
    //   if (TII->analyzeBranch(*PrevBB, TBB, FBB, Cond)) {
    //     // FIXME: This should never take place.
    //     TBB = FBB = nullptr;
    //   }
    // }
    if (!TII->analyzeBranch(*PrevBB, TBB, FBB, Cond))
      PrevBB->updateTerminator();

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-bugs/attachments/20190531/ae04068a/attachment.html>