[LLVMdev] Should the MachineVerifier accept a MBB with a single (landing pad) successor?

[Ahmed Bougacha]

Ping!
- Ahmed


On Thu, Nov 6, 2014 at 11:26 AM, Ahmed Bougacha
<ahmed.bougacha at gmail.com> wrote:
> Hi all,
>
> I've been investigating a machine verifier failure on the attached
> testcase, and I'm tempted to say the verifier is wrong and should
> accept it.  Skip the description for the proposed change.
>
>
> On AArch64, the verifier complains with:
>
>     *** Bad machine code: MBB exits via unconditional branch but
> doesn't have exactly one CFG successor! ***
>     - function:    t4
>     - basic block: BB#5 invoke.cont41
>
> The freshly selected relevant blocks are:
>
>     BB#7: derived from LLVM BB %invoke.cont41
>             EH_LABEL <MCSym=Ltmp4>
>             B <BB#8>
>         Successors according to CFG: BB#8(1) BB#9(1)
>
>     BB#8: derived from LLVM BB %invoke.cont43
>         Predecessors according to CFG: BB#7
>
>     BB#9: derived from LLVM BB %lpad40, EH LANDING PAD
>         Predecessors according to CFG: BB#7
>             EH_LABEL <MCSym=Ltmp5>
>             ...
>
>
> The unreachable BB#8 gets removed, and we end up with:
>
>     BB#5: derived from LLVM BB %invoke.cont41
>             ...
>             B <BB#8>
>         Successors according to CFG: BB#8(2)
>     ...
>     BB#8: derived from LLVM BB %lpad40, EH LANDING PAD
>         Predecessors according to CFG: BB#5
>
>
> On other targets, the branch terminating BB#7 gets removed early, for
> various reasons (happenstance, really).  The edge to the landing pad
> is still there, but the verifier can't check anything because there's
> no branch to analyze.
>
> On AArch64, the branch remains, and the verifier hits this condition:
>
>       if (MBB->succ_size() != 1+LandingPadSuccs.size())
>
> So:
> - the problem exists elsewhere, but is hidden by branch folder optimizations
> - if the normal successor to an invoke BB is unreachable, it seems
> reasonable to only have 1 successor, the landing pad.
>
> Hence my simple change, making the verifier accept it:
>
> --- c/lib/CodeGen/MachineVerifier.cpp
> +++ w/lib/CodeGen/MachineVerifier.cpp
> @@ -590,7 +590,11 @@
> MachineVerifier::visitMachineBasicBlockBefore(const MachineBasicBlock
> *MBB) {
>        }
>      } else if (TBB && !FBB && Cond.empty()) {
>        // Block unconditionally branches somewhere.
> -      if (MBB->succ_size() != 1+LandingPadSuccs.size()) {
> +      // If the block has exactly one successor, that happens to be a
> +      // landingpad, accept it as valid control flow.
> +      if (MBB->succ_size() != 1+LandingPadSuccs.size() &&
> +          (MBB->succ_size() != 1 || LandingPadSuccs.size() != 1 ||
> +           *MBB->succ_begin() != *LandingPadSuccs.begin())) {
>          report("MBB exits via unconditional branch but doesn't have "
>                 "exactly one CFG successor!", MBB);
>        } else if (!MBB->isSuccessor(TBB)) {
>
>
> - Ahmed