[PATCH] D156013: [BOLT] Fix jump table issue for fragmented functions

[Maksim Panchenko via Phabricator via llvm-commits]

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

Jump table for a split function may contain an entry matching the start
of another fragment. We used to ignore such entries while
post-processing jump tables. Fix that.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156013

Files:
  bolt/lib/Core/BinaryFunction.cpp
  bolt/test/X86/split-func-jump-table-fragment.s


Index: bolt/test/X86/split-func-jump-table-fragment.s
===================================================================
--- bolt/test/X86/split-func-jump-table-fragment.s
+++ bolt/test/X86/split-func-jump-table-fragment.s
@@ -1,12 +1,13 @@
-# This reproduces a bug with jump table identification where jump table has
-# entries pointing to code in function and its cold fragment.
+## This reproduces a bug with jump table identification where jump table has
+## entries pointing to code in function and its cold fragment.
 
 # REQUIRES: system-linux
 
 # RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s -o %t.o
 # RUN: llvm-strip --strip-unneeded %t.o
 # RUN: %clang %cflags %t.o -o %t.exe -Wl,-q
-# RUN: llvm-bolt %t.exe -o %t.out --lite=0 -v=1 2>&1 | FileCheck %s
+# RUN: llvm-bolt %t.exe -o %t.out --lite=0 -v=1 --print-cfg --print-only=main \
+# RUN:   2>&1 | FileCheck %s
 
 # CHECK-NOT: unclaimed PC-relative relocations left in data
 # CHECK: BOLT-INFO: marking main.cold.1 as a fragment of main
@@ -36,13 +37,14 @@
   ret
 .size main, .-main
 
+# Insert padding between functions, so that the next instruction cannot be
+# treated as __builtin_unreachable destination for the jump table.
+  .quad 0
+
   .globl main.cold.1
   .type main.cold.1, %function
   .p2align 2
 main.cold.1:
-  # load bearing nop: pad LBB4 so that it can't be treated
-  # as __builtin_unreachable by analyzeJumpTable
-  nop
 LBB4:
   callq abort
 .size main.cold.1, .-main.cold.1
@@ -55,3 +57,12 @@
   .long LBB3-JUMP_TABLE
   .long LBB4-JUMP_TABLE
   .long LBB3-JUMP_TABLE
+
+## Verify that the entry corresponding to the cold fragment was added to
+## the jump table.
+
+# CHECK:      PIC Jump table
+# CHECK-NEXT: 0x{{.*}} :
+# CHECK-NEXT: 0x{{.*}} :
+# CHECK-NEXT: 0x{{.*}} : main.cold.1
+# CHECK-NEXT: 0x{{.*}} :
Index: bolt/lib/Core/BinaryFunction.cpp
===================================================================
--- bolt/lib/Core/BinaryFunction.cpp
+++ bolt/lib/Core/BinaryFunction.cpp
@@ -1643,12 +1643,15 @@
       // Otherwise, secondary entry point to target function
       BinaryFunction *TargetBF =
           BC.getBinaryFunctionContainingAddress(EntryAddress);
-      if (uint64_t Offset = EntryAddress - TargetBF->getAddress()) {
-        MCSymbol *Label = (HasOneParent && TargetBF == this)
-                              ? getOrCreateLocalLabel(EntryAddress, true)
-                              : TargetBF->addEntryPointAtOffset(Offset);
-        JT.Entries.push_back(Label);
+      MCSymbol *Label;
+      if (HasOneParent && TargetBF == this) {
+        Label = getOrCreateLocalLabel(EntryAddress, true);
+      } else {
+        const uint64_t Offset = EntryAddress - TargetBF->getAddress();
+        Label = Offset ? TargetBF->addEntryPointAtOffset(Offset)
+                       : TargetBF->getSymbol();
       }
+      JT.Entries.push_back(Label);
     }
   }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D156013.543137.patch
Type: text/x-patch
Size: 2895 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230722/a050067b/attachment.bin>