Skip unreachable blocks for CFIInstrInserter verify

[petarj]

Summary

Iterate only through reachable blocks. This finetunes r330706 and
resolves build issue reported by Craig Topper.

[craig.topper]

lib/CodeGen/CFIInstrInserter.cpp

295

pred_empty()?

295

Can't you just check CurrMBB == MF.front()? This applies to several other places in this pass.

[craig.topper]

This fix fixes the original case, but its easy to break it again

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

; Function Attrs: noinline nounwind optnone uwtable
define hidden void @foo() #0 {
bb:
  br label %bb1

bb1:                                              ; preds = %bb3, %bb
  %tmp = icmp ne i32 0, 0
  br i1 %tmp, label %bb2, label %bb3

bb2:                                              ; preds = %bb1
  br label %bb4

bb4:                                              ; preds = %bb2
  br label %bb3

bb3:                                              ; preds = %bb4, %bb1
  br label %bb1
}

attributes #0 = { noinline nounwind optnone uwtable "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" }

[petarj]

In D46399#1086801, @craig.topper wrote:

This fix fixes the original case, but its easy to break it again

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

; Function Attrs: noinline nounwind optnone uwtable
define hidden void @foo() #0 {
bb:
  br label %bb1

bb1:                                              ; preds = %bb3, %bb
  %tmp = icmp ne i32 0, 0
  br i1 %tmp, label %bb2, label %bb3

bb2:                                              ; preds = %bb1
  br label %bb4

bb4:                                              ; preds = %bb2
  br label %bb3

bb3:                                              ; preds = %bb4, %bb1
  br label %bb1
}

attributes #0 = { noinline nounwind optnone uwtable "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" }

Does this example come from a real test case? The issue here is that bb2 will not have predecessors after X86 DAG->DAG Instruction Selection, and this way we reach bb3 from either:

bb0->bb1->bb3 or
bb2->bb4->bb3.

What should be a correct offset for bb3 in this case?

[craig.topper]

It's not real. But bb4 is only reachable from bb2. I just wedged an empty block in the path from bb2 to bb3 in my previous case. So neither bb2 or bb4 are actually reachable from the start of the function.

[petarj]

In D46399#1087133, @craig.topper wrote:

It's not real. But bb4 is only reachable from bb2. I just wedged an empty block in the path from bb2 to bb3 in my previous case. So neither bb2 or bb4 are actually reachable from the start of the function.

I made a typo (bb3 vs bb4). I wanted to say:

and this way we reach bb3 from either:

bb0->bb1->bb3 or
bb2->bb4->bb3.

What should be a correct offset for bb3 in this case?

[petarj]

Updated the patch according to the comments.

[craig.topper]

Here is a C program rather than hacked IR that still crashes with this patch. I don't know anything about CFI so I don't know what the right answer is.

int __attribute((__always_inline__)) bar() {
  return 0;
}

void foo() {
  if (bar() != 0) {
    if (bar() != 0) {
    }
  }
}

[petarj]

Thanks Craig for the test example. The issues were related to unreachable blocks that exist with -O0, this patch now skips all of them and hopefully it will resolve issues you are seeing.

[thegameg]

lib/CodeGen/CFIInstrInserter.cpp

298

How about range for on depth_first(&MF)?

[craig.topper]

lib/CodeGen/CFIInstrInserter.cpp

297

Do you need an external set? You aren't accessing it are you?

[petarj]

Update the patch per comments.

lib/CodeGen/CFIInstrInserter.cpp

297

No, in this version of the patch I do not need it. I had some asserts in local copy.
Removing it.

[craig.topper]

LGTM

[violetav]

LGTM

[petarj]

Committed in r331628. Closing the issue.

https://reviews.llvm.org/rL331628

lib/CodeGen/CFIInstrInserter.cpp

 /// that outgoing information of predecessors matches incoming information of
 /// their successors. Then it checks if blocks have correct CFA calculation rule
 /// set and inserts additional CFI instruction at their beginnings if they
 /// don't. CFI instructions are inserted if basic blocks have incorrect offset
 /// or register set by previous blocks, as a result of a non-linear layout of
 /// blocks in a function.
 //===----------------------------------------------------------------------===//
 
+#include "llvm/ADT/DepthFirstIterator.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/CodeGen/TargetFrameLowering.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/Target/TargetMachine.h"
void CFIInstrInserter::report(const MBBCFAInfo &Pred,
   errs() << "Succ: " << Succ.MBB->getName()
          << " incoming CFA Reg:" << Succ.IncomingCFARegister << "\n";
   errs() << "Succ: " << Succ.MBB->getName()
          << " incoming CFA Offset:" << Succ.IncomingCFAOffset << "\n";
 }
 
 unsigned CFIInstrInserter::verify(MachineFunction &MF) {
   unsigned ErrorNum = 0;
-  for (MachineBasicBlock &CurrMBB : MF) {
-    const MBBCFAInfo &CurrMBBInfo = MBBVector[CurrMBB.getNumber()];
-    for (MachineBasicBlock *Succ : CurrMBB.successors()) {
+  for (auto *CurrMBB : depth_first(&MF)) {
+    const MBBCFAInfo &CurrMBBInfo = MBBVector[CurrMBB->getNumber()];
+    for (MachineBasicBlock *Succ : CurrMBB->successors()) {
       const MBBCFAInfo &SuccMBBInfo = MBBVector[Succ->getNumber()];
       // Check that incoming offset and register values of successors match the
       // outgoing offset and register values of CurrMBB
       if (SuccMBBInfo.IncomingCFAOffset != CurrMBBInfo.OutgoingCFAOffset ||
           SuccMBBInfo.IncomingCFARegister != CurrMBBInfo.OutgoingCFARegister) {
         report(CurrMBBInfo, SuccMBBInfo);
         ErrorNum++;
       }
     }
   }
   return ErrorNum;
 }