[BOLT] Remove original relocs when moving JTs in relocation mode

[Amir]

Summary

Erase original jump table relocations in relocation mode with -jump-tables=move
(or higher). Otherwise we end up updating the original data relocations which
may no longer reference existing labels, as demonstrated by the test case.
Zero out the original jump table entries to trigger a crash if we end up using them.

[Amir]

The plan is to explicitly zero out the original jump table entries to trigger a crash if we end up using them.

[rafauler]

Nice solution. Can @maksfb sign off on this too? I'm not sure if he's aware.

[rafauler]

In D147150#4232008, @Amir wrote:

The plan is to explicitly zero out the original jump table entries to trigger a crash if we end up using them.

Oh, ok

[Amir]

Zero out the original jump table entries

[Amir]

In D147150#4232011, @rafauler wrote:

Nice solution. Can @maksfb sign off on this too? I'm not sure if he's aware.

We discussed the solution. @maksfb – please check if it looks good. I didn't refactor the creation of a zero symbol, we still create them ad-hoc in a couple of places.

[Amir]

Can't proceed with this change as is.
In one internal test we end up erasing a PIC jump table entry that is added accidentally because it spuriously evaluates to a valid address/instruction/basic block inside the current function, even though it is a start of a jump table in another function, causing a runtime crash.

bolt/include/bolt/Core/JumpTable.h

public:
   bool isJumpTable() const override { return true; }
 
   /// Change all entries of the jump table in \p JTAddress pointing to
   /// \p OldDest to \p NewDest. Return false if unsuccessful.
   bool replaceDestination(uint64_t JTAddress, const MCSymbol *OldDest,
                           MCSymbol *NewDest);
 
   /// Update jump table at its original location.
-  void updateOriginal();
+  void updateOriginal(bool Zero);
 
   /// Print for debugging purposes.
   void print(raw_ostream &OS) const override;
 };
 
 } // namespace bolt
 } // namespace llvm
 
 #endif

bolt/lib/Core/BinaryEmitter.cpp

void BinaryEmitter::emitJumpTables(const BinaryFunction &BF) {
   for (auto &JTI : BF.jumpTables()) {
     JumpTable &JT = *JTI.second;
     // Only emit shared jump tables once, when processing the first parent
     if (JT.Parents.size() > 1 && JT.Parents[0] != &BF)
       continue;
     if (opts::PrintJumpTables)
       JT.print(outs());
     if (opts::JumpTables == JTS_BASIC && BC.HasRelocations) {
-      JT.updateOriginal();
+      JT.updateOriginal(/*Zero=*/false);
     } else {
       MCSection *HotSection, *ColdSection;
       if (opts::JumpTables == JTS_BASIC) {
         // In non-relocation mode we have to emit jump tables in local sections.
         // This way we only overwrite them when the corresponding function is
         // overwritten.
         std::string Name = ".local." + JT.Labels[0]->getName().str();
         std::replace(Name.begin(), Name.end(), '/', '.');
         BinarySection &Section =
             BC.registerOrUpdateSection(Name, ELF::SHT_PROGBITS, ELF::SHF_ALLOC);
         Section.setAnonymous(true);
         JT.setOutputSection(Section);
         HotSection = BC.getDataSection(Name);
         ColdSection = HotSection;
       } else {
         if (BF.isSimple()) {
           HotSection = ReadOnlySection;
           ColdSection = ReadOnlyColdSection;
         } else {
           HotSection = BF.hasProfile() ? ReadOnlySection : ReadOnlyColdSection;
           ColdSection = HotSection;
         }
+        JT.updateOriginal(/*Zero=*/true);
       }
       emitJumpTable(JT, HotSection, ColdSection);
     }
   }
 }
 
 void BinaryEmitter::emitJumpTable(const JumpTable &JT, MCSection *HotSection,
                                   MCSection *ColdSection) {

bolt/lib/Core/JumpTable.cpp

for (auto I = Range.first; I != Range.second; ++I) {
     if (Entries[I] == OldDest) {
       Patched = true;
       Entries[I] = NewDest;
     }
   }
   return Patched;
 }
 
-void bolt::JumpTable::updateOriginal() {
+void bolt::JumpTable::updateOriginal(bool Zero) {
   BinaryContext &BC = getSection().getBinaryContext();
   const uint64_t BaseOffset = getAddress() - getSection().getAddress();
   uint64_t EntryOffset = BaseOffset;
   for (MCSymbol *Entry : Entries) {
     const uint64_t RelType =
         Type == JTT_NORMAL ? ELF::R_X86_64_64 : ELF::R_X86_64_PC32;
     const uint64_t RelAddend =
         Type == JTT_NORMAL ? 0 : EntryOffset - BaseOffset;
     // Replace existing relocation with the new one to allow any modifications
     // to the original jump table.
     if (BC.HasRelocations)
       getOutputSection().removeRelocationAt(EntryOffset);
+    if (Zero)
+      Entry = BC.registerNameAtAddress("Zero", 0, 0, 0);
     getOutputSection().addRelocation(EntryOffset, Entry, RelType, RelAddend);
     EntryOffset += EntrySize;
   }
 }
 
 void bolt::JumpTable::print(raw_ostream &OS) const {
   uint64_t Offset = 0;
   if (Type == JTT_PIC)

bolt/test/X86/normalize-jump-table-target.s

+# This test reproduces an issue with removing a basic block which is referenced
+# from rodata section ("externally referenced offset"/jump table target).
+# If the block is removed (by remove-nops + NormalizeCFG), and BOLT updates the
+# original jump table (with jump-tables=move), this causes an emission error
+# (undefined temporary symbol).
+
+# REQUIRES: system-linux,bolt-runtime
+
+# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s -o %t.o
+# RUN: %clang -no-pie %t.o -o %t.exe -Wl,-q
+# RUN: llvm-bolt %t.exe -o %t.out --instrument --instrumentation-file=%t.tmp \
+# RUN:   2>&1 | FileCheck %s --check-prefix=CHECK-BOLT
+
+# CHECK-BOLT-NOT:  undefined temporary symbol
+
+# Grab the jump table address in original text section (.bolt.org.text)
+# RUN: llvm-objdump -dj.bolt.org.text %t.out > %t.log
+# Inspect jump table entries, should be two zero values
+# RUN: llvm-objdump -sj.rodata %t.out >> %t.log
+# RUN: FileCheck %s --input-file %t.log --check-prefix=CHECK-OBJDUMP
+
+# CHECK-OBJDUMP: jmpq *0x[[#%x,JTADDR:]](,%rax,8)
+# CHECK-OBJDUMP: [[#JTADDR]] 00000000 00000000 00000000 00000000
+
+  .globl main
+main:
+  .cfi_startproc
+  jmp main
+  jmpq  *JT(,%rax,8)
+a:
+  shlq  %rax
+.Ltmp:
+  nop
+c:
+  ret
+  .cfi_endproc
+
+.rodata
+JT:
+  .quad .Ltmp
+  .quad a