[BOLT] Fix jump table issue for split functions

[maksfb]

Summary

A jump table in a split function may contain an entry matching a start
address of another fragment of the function. While converting addresses
to labels, we used to ignore such entries resulting in underpopulated
jump table. Change that, so we always create one label per address.

[Amir]

Thanks. I hope it won't break our jump table detection in unexpected ways.
As a suggestion to minimize fallout – we can restrict this new treatment to fragment starts only (ie. to *.cold functions only)

[maksfb]

Update comment.

[maksfb]

In D156013#4524579, @Amir wrote:

Thanks. I hope it won't break our jump table detection in unexpected ways.
As a suggestion to minimize fallout – we can restrict this new treatment to fragment starts only (ie. to *.cold functions only)

In postProcessJumpTables() we are not making decisions on what entries belong to the table. The population of the table is happening in BinaryContext.
The code that I'm changing is supposed to convert addresses to labels, but was skipping the corner case.

bolt/lib/Core/BinaryFunction.cpp

for (uint64_t EntryAddress : JT.EntriesAsAddress) {
           llvm::any_of(JT.Parents, [&](const BinaryFunction *Parent) {
             return EntryAddress == Parent->getAddress() + Parent->getSize();
           });
       if (IsBuiltinUnreachable) {
         MCSymbol *Label = getOrCreateLocalLabel(EntryAddress, true);
         JT.Entries.push_back(Label);
         continue;
       }
-      // Create local label for targets cannot be reached by other fragments
-      // Otherwise, secondary entry point to target function
+      // Create a local label for targets that cannot be reached by other
+      // fragments. Otherwise, create a secondary entry point in the 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);
     }
   }
 
   // Add TakenBranches from JumpTables.
   //
   // We want to do it after initial processing since we don't know jump tables'
   // boundaries until we process them all.
   for (auto &JTSite : JTSites) {

bolt/test/X86/split-func-jump-table-fragment.s

-# 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
   .text
   .globl main
   .type main, %function
   .p2align 2
 main:

 
 LBB2:
   xorq %rax, %rax
 LBB3:
   addq $0x8, %rsp
   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
 
   .rodata
 # jmp table, entries must be R_X86_64_PC32 relocs
   .globl JUMP_TABLE
 JUMP_TABLE:
   .long LBB2-JUMP_TABLE
   .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{{.*}} :