[PATCH] D78234: [BranchFolding] assert when removing INLINEASM_BR indirect targets

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers created this revision.
nickdesaulniers added reviewers: efriedma, void, arsenm.
Herald added subscribers: llvm-commits, hiraditya, wdng.
Herald added a project: LLVM.

Twice now, we've had to debug pretty awful codegen bugs where the
indirect target of an INLINEASM_BR was removed by BranchFolding.

1. https://reviews.llvm.org/D76961
2. https://reviews.llvm.org/D77849

In both cases, this was a cascaded failure caused earlier, and not
necessarily BranchFolding's fault.  For that reason, I don't think it's
appropriate to always guard against removing MachineBasicBlocks that
have their address taken; this is indicative of something else going
wrong earlier.

Rather than proceed to generate jumps to blocks that have been removed,
which is quite hard to debug (the generated code), add an assertion to
help catch and hint at similar failures in the future.

The first conditional added is false for all tests currently in tree,
but this exceptional case is true for the test case in D77849 <https://reviews.llvm.org/D77849>. When
D77849 <https://reviews.llvm.org/D77849> is resolved this exceptional cases should hopefully never be
true, but the assertion is still useful to catch regressions or help
find if there's more bugs lying in wait.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78234

Files:
  llvm/lib/CodeGen/BranchFolding.cpp


Index: llvm/lib/CodeGen/BranchFolding.cpp
===================================================================
--- llvm/lib/CodeGen/BranchFolding.cpp
+++ llvm/lib/CodeGen/BranchFolding.cpp
@@ -159,6 +159,19 @@
 void BranchFolder::RemoveDeadBlock(MachineBasicBlock *MBB) {
   assert(MBB->pred_empty() && "MBB must be dead!");
   LLVM_DEBUG(dbgs() << "\nRemoving MBB: " << *MBB);
+#ifndef NDEBUG
+  if (MBB->hasAddressTaken())
+    for (const MachineBasicBlock &Pred : *MBB->getParent())
+      for (const MachineInstr &I : Pred)
+        if (I.getOpcode() == TargetOpcode::INLINEASM_BR)
+          // Iterating I.terminators() may be unreliable in this case.
+          // https://reviews.llvm.org/D78166#1984487
+          for (const MachineOperand &Op : I.operands())
+            if (Op.isBlockAddress() &&
+                Op.getBlockAddress()->getBasicBlock() == MBB->getBasicBlock())
+              assert(false &&
+                     "Removing MBB that's an indirect target of INLINEASM_BR");
+#endif
 
   MachineFunction *MF = MBB->getParent();
   // drop all successors.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D78234.257806.patch
Type: text/x-patch
Size: 1080 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200415/4402f14a/attachment.bin>