[PATCH] D148055: [BOLT] Fix creation of invalid CFG in presence of dead code

[Rafael Auler via Phabricator via llvm-commits]

rafauler created this revision.
rafauler added reviewers: bolt, yota9.
Herald added subscribers: treapster, ayermolo.
Herald added a reviewer: Amir.
Herald added a reviewer: maksfb.
Herald added a project: All.
rafauler requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

When there is a direct jump right after an indirect one, in
the absence of code jumpting to this direct jump, this is obviously
dead code. However, BOLT was failing to recognize that by mistakenly
placing both jmp instructions in the same basic block, and creating
wrong successor edges. Fix that, so we can safely run UCE on
that. This bug also causes validateCFG to fail and BOLT to crash if it
is running ICP on that function.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148055

Files:
  bolt/lib/Core/BinaryFunction.cpp
  bolt/test/X86/unreachable-jmp.s


Index: bolt/test/X86/unreachable-jmp.s
===================================================================
--- /dev/null
+++ bolt/test/X86/unreachable-jmp.s
@@ -0,0 +1,64 @@
+# This checks that we don't create an invalid CFG when there is an
+# unreachable direct jump right after an indirect one.
+
+# REQUIRES: system-linux
+
+# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown \
+# RUN:   %s -o %t.o
+# RUN: link_fdata %s %t.o %t.fdata
+# RUN: llvm-strip --strip-unneeded %t.o
+# RUN: %clang %cflags -no-pie %t.o -o %t.exe -Wl,-q -nostdlib
+# RUN: llvm-bolt %t.exe -o %t.out --data %t.fdata \
+# RUN:     -print-cfg | FileCheck %s
+
+  .globl _start
+  .type _start, %function
+_start:
+  .cfi_startproc
+# FDATA: 0 [unknown] 0 1 _start 0 0 1
+  push  %rbp
+  mov   %rsp, %rbp
+  push  %rbx
+  push  %r14
+  subq  $0x20, %rsp
+  movq  %rdi, %rcx
+b:
+  jmpq  *JUMP_TABLE(,%rcx,8)
+# FDATA: 1 _start #b# 1 _start #hotpath# 0 20
+# Unreachable direct jump here. Our CFG should still make sense and properly
+# place this instruction in a new basic block.
+  jmp  .lbb2
+.lbb1:  je  .lexit
+.lbb2:
+  xorq  %rax, %rax
+  addq  $0x20, %rsp
+  pop %r14
+  pop %rbx
+  pop %rbp
+  ret
+hotpath:
+  movq  $2, %rax
+  addq  $0x20, %rsp
+  pop %r14
+  pop %rbx
+  pop %rbp
+  ret
+.lexit:
+  movq  $1, %rax
+  addq  $0x20, %rsp
+  pop %r14
+  pop %rbx
+  pop %rbp
+  ret
+  .cfi_endproc
+  .size _start, .-_start
+
+  .rodata
+  .globl JUMP_TABLE
+JUMP_TABLE:
+  .quad .lbb1
+  .quad .lbb2
+  .quad hotpath
+
+# No basic blocks above should have 4 successors! That is a bug.
+# CHECK-NOT:   Successors: {{.*}} (mispreds: 0, count: 20), {{.*}} (mispreds: 0, count: 0), {{.*}} (mispreds: 0, count: 0), {{.*}} (mispreds: 0, count: 0)
Index: bolt/lib/Core/BinaryFunction.cpp
===================================================================
--- bolt/lib/Core/BinaryFunction.cpp
+++ bolt/lib/Core/BinaryFunction.cpp
@@ -2038,6 +2038,7 @@
       MCInst *PrevInstr = PrevBB->getLastNonPseudoInstr();
       assert(PrevInstr && "no previous instruction for a fall through");
       if (MIB->isUnconditionalBranch(Instr) &&
+          !MIB->isIndirectBranch(*PrevInstr) &&
           !MIB->isUnconditionalBranch(*PrevInstr) &&
           !MIB->getConditionalTailCall(*PrevInstr) &&
           !MIB->isReturn(*PrevInstr)) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D148055.512587.patch
Type: text/x-patch
Size: 2319 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230411/7b8f9941/attachment.bin>