This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by vstefanovic on Aug 23 2018, 6:42 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

vstefanovic created this revision.Aug 23 2018, 6:42 AM

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 accepted this revision.Aug 30 2018, 8:09 AM

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.

This revision is now accepted and ready to land.Aug 30 2018, 8:09 AM
vstefanovic marked 2 inline comments as done.
vstefanovic added inline comments.
lib/CodeGen/CFIInstrInserter.cpp
320

Makes sense.

Looks good, thanks!

This revision was automatically updated to reflect the committed changes.
vstefanovic marked an inline comment as done.