Allow inconsistent offsets for 'noreturn' basic blocks when '-verify-cfiinstrs'

[vstefanovic]

Summary

With r295105, some 'noreturn' blocks (those that don't return and have no
successors) may be merged.
If such blocks' predecessors have different outgoing offset or register, don't
report an error in CFIInstrInserter verify().

Thanks to Vlad Tsyrklevich for reporting the issue.

[vstefanovic]

C code to reproduce the issue, built with

'-O2 -mllvm -tail-merge-size=1 -mllvm -verify-cfiinstrs':

void foo1(int v) __attribute__((noreturn)) {
  if (v == 1) {
    __builtin_trap();
  }
  if (foo2(v)) {
    __builtin_trap();
  }
}

Machine IR before branch-folder:

bb.0.entry:
  CMP32ri8 $edi, 1
  JNE_1 %bb.2
  JMP_1 %bb.1

bb.1.if.then:
  TRAP

bb.2.if.end:
  PUSH64r $rax
  CFI_INSTRUCTION def_cfa_offset 16
  $eax = MOV32r0
  CALL64pcrel32 @foo2
  TRAP

(No stack adjustment after foo() because it's noreturn block.)

Then branch-folder merges TRAPs:

bb.0.entry:
  CMP32ri8 $edi, 1
  JE_1 %bb.2

bb.1.if.end:
  PUSH64r $rax
  CFI_INSTRUCTION def_cfa_offset 16
  $eax = MOV32r0
  CALL64pcrel32 @foo2

bb.2.if.then:
  TRAP

So bb.0 and bb.1 become direct predecessors of bb.2 with different outgoing offsets.

[thegameg]

LGTM, thanks.

Could you also add what you said in the previous comment to the test case?

C code to reproduce the issue, built with

'-O2 -mllvm -tail-merge-size=1 -mllvm -verify-cfiinstrs':

void foo1(int v) __attribute__((noreturn)) {
  if (v == 1) {
    __builtin_trap();
  }
  if (foo2(v)) {
    __builtin_trap();
  }
}

Thanks!

lib/CodeGen/CFIInstrInserter.cpp

320

Do you think we should add because we don't generate epilogues inside noreturn blocks to the comment? It might make it clearer on why the inconsistency is ok.

test/CodeGen/X86/cfi-inserter-noreturnblock.mir

8

You should be able to remove the LLVM IR completely here.

[vstefanovic]

lib/CodeGen/CFIInstrInserter.cpp

320

Makes sense.

[thegameg]

Looks good, thanks!

lib/CodeGen/CFIInstrInserter.cpp

unsigned CFIInstrInserter::verify(MachineFunction &MF) {
   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) {
+        // Inconsistent offsets/registers are ok for 'noreturn' blocks because
+        // we don't generate epilogues inside such blocks.
+        if (SuccMBBInfo.MBB->succ_empty() && !SuccMBBInfo.MBB->isReturnBlock())
+          continue;
         report(CurrMBBInfo, SuccMBBInfo);
         ErrorNum++;
       }
     }
   }
   return ErrorNum;
 }

test/CodeGen/X86/cfi-inserter-noreturnblock.mir

+# RUN: llc -o - %s -mtriple=x86_64-- -verify-cfiinstrs \
+# RUN:     -run-pass=cfi-instr-inserter
+
+# Test that CFI verifier does not report inconsistent offset for the
+# 'noreturn' block.
+---
+name: testNoreturnBlock
+body: |
+  bb.0:
+    CFI_INSTRUCTION def_cfa_offset 24
+    JNE_1 %bb.2, implicit undef $eflags
+
+  bb.1:
+    CFI_INSTRUCTION def_cfa_offset 32
+
+  bb.2:
+    TRAP
+...
+
+# C code to reproduce the issue, built with
+# '-O2 -mllvm -tail-merge-size=1 -mllvm -verify-cfiinstrs':
+#
+# void foo1(int v) __attribute__((noreturn)) {
+#   if (v == 1) {
+#     __builtin_trap();
+#   }
+#   if (foo2(v)) {
+#     __builtin_trap();
+#   }
+# }