[PATCH] D130290: [MachineVerifier] add checks for INLINEASM_BR

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:904
+        report("INLINEASM_BR indirect target predecessor list missing parent",
+               &MO, i);
+    }
----------------
nickdesaulniers wrote:
> efriedma wrote:
> > nickdesaulniers wrote:
> > > efriedma wrote:
> > > > I don't think you need to check both isSuccessor and isPredecessor; we should have other checks that verify the predecessor lists are consistent with the successor lists.
> > > If you comment out these checks, then run `llc -run-pass=none -verify-machineinstrs /android0/llvm-project/llvm/test/MachineVerifier/verify-inlineasmbr.mir -o -`, you'll find that MachineVerify makes not such qualms about inconsistent predecessor or successor lists, despite BOTH being wrong.
> > All I'm trying to say is that if you try to construct a testcase where the isSuccessor check succeeds, but the isPredecessor check fails, you'll get some other error.
> I suspect the verifier will spot other things wrong later during its analysis, but if either list is corrupt, I feel strongly that we should eagerly diagnose that ASAP.  It makes for a significantly improved debugging experience if the verifier spots corrupted pred/succ lists ASAP.
I was thinking more about your point last night; if I may restate what I perceive to be the distinction another way.

Your latest point sounds like if ONE of either the predecessor list or successor list in incorrect, the MachineVerifier will catch that.

Granted.

But what happens if BOTH are wrong, simultaneously?

Because that is precisely what happened in D129997. The MachineVerifier did not catch that.

For MachineInstrs that imply control flow, we should verify that their parent MachineBasicBlock's successor list contains the MachineInstrs MachineBasicBlock operand AND that operand's predecessor list contains this MachineInstrs parent MachineBasicBlock.  This patch does such checking only for the rare cases of INLINEASM_BR, but I'd go so far as to suggest this should be done for ALL branching instructions (or maybe any instruction that has a MachineBasicBlock as an operand).

Stated a third way, if MBB A has a jump to MBB B, we should check that:
1. B is in A's successor list.
2. A is in B's predecessor list.

It _might_ be that MachineVerifier can catch imbalanced cases where ONE of the above does not hold, but currently, if BOTH don't hold MachineVerifier misses that.

Removing an edge between A and B without also removing the branching instruction in A (where that instruction branches to B) is a great way to get into the above anomalous conditions, which is what happened in D129997.


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:899-904
+      if (llvm::none_of(MBB->successors(),
+                        [IndirectTargetMBB](const MachineBasicBlock *Succ) {
+                          return Succ == IndirectTargetMBB;
+                        }))
+        report("INLINEASM_BR indirect target missing from successor list", &MO,
+               i);
----------------
nickdesaulniers wrote:
> arsenm wrote:
> > arsenm wrote:
> > > I'd expect CFG checks to go with the others in visitMachineBasicBlockBefore
> > This is !MBB->isSuccessor
> Perhaps, but consider that INLINEASM_BR are relatively rare constructs to see in the wild; I think it might be more efficient to do such scans only when they are encountered.
Further, `visitMachineBasicBlockBefore` walks the successor and predecessor lists and uses analyzeBranch, so it already assumes those lists are well formed.  For IR verification, that's a dangerous assumption to make. In D129997, those lists were fundamentally untrustworthy.

(I also don't think that analyzeBranch works with INLINEASM_BR).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130290/new/

https://reviews.llvm.org/D130290