[BOLT][NFC] Move addRelocation{X86,AArch64} into MCPlusBuilder

[Amir]

Summary

The two methods don't belong in BinaryFunction methods.
Move the dispatch tables into target-specific MCPlusBuilder methods.

[rafauler]

LGTM

[Amir]

Plan to break addRelocation into the target-specific check in MCPlusBuilder and actually adding the relocation to the container in BinaryFunction.

[Amir]

Move the switch tables into MCPlusBuilder

[Amir]

Bugfix

[rafauler]

bolt/include/bolt/Core/MCPlusBuilder.h

399

I'm a bit confused with the name, for example, what does it mean to have a supported relocation? Why is ELF::R_AARCH64_CALL26 unsupported? I think either the comments here need to be expanded a bit or the function get a better name, because this "support" is very specific to the needs of BinaryFunction.

[Amir]

bolt/include/bolt/Core/MCPlusBuilder.h

399

What name and comment would you suggest? These are relocations we store for function splitting and reordering.

[rafauler]

bolt/include/bolt/Core/MCPlusBuilder.h

399

I don't have good names that give the necessary context. I guess something like "shouldRecordCodeReloc/shouldStoreReloc" or "shouldRetainCodeReloc". The context might go in a comment maybe... the context is that these relocations are essentially used by our disassembly step to better understand code (by using these code relocs). For ARM, they help us decode instruction operands unambiguously, but sometimes we might discard them because we already have the necessary information in the instruction itself (for example, we don't need to record CALL relocs in ARM because we can fully decode the target from the call operand). For X86, they might be used in scanExternalRefs when we want to skip a function but still patch references inside it.

[Amir]

Rename isSupportedRelocation to shouldRecordCodeRelocation, add comment.

[rafauler]

LGTM

bolt/include/bolt/Core/BinaryFunction.h

public:
 
   /// Return mapping of input to output addresses. Most users should call
   /// translateInputToOutputAddress() for address translation.
   InputOffsetToAddressMapTy &getInputOffsetToAddressMap() {
     assert(isEmitted() && "cannot use address mapping before code emission");
     return InputOffsetToAddressMap;
   }
 
-  void addRelocationAArch64(uint64_t Offset, MCSymbol *Symbol, uint64_t RelType,
-                            uint64_t Addend, uint64_t Value, bool IsCI) {
-    std::map<uint64_t, Relocation> &Rels =
-        (IsCI) ? Islands->Relocations : Relocations;
-    switch (RelType) {
-    case ELF::R_AARCH64_ABS64:
-    case ELF::R_AARCH64_ABS32:
-    case ELF::R_AARCH64_ABS16:
-    case ELF::R_AARCH64_ADD_ABS_LO12_NC:
-    case ELF::R_AARCH64_ADR_GOT_PAGE:
-    case ELF::R_AARCH64_ADR_PREL_LO21:
-    case ELF::R_AARCH64_ADR_PREL_PG_HI21:
-    case ELF::R_AARCH64_ADR_PREL_PG_HI21_NC:
-    case ELF::R_AARCH64_LD64_GOT_LO12_NC:
-    case ELF::R_AARCH64_LDST8_ABS_LO12_NC:
-    case ELF::R_AARCH64_LDST16_ABS_LO12_NC:
-    case ELF::R_AARCH64_LDST32_ABS_LO12_NC:
-    case ELF::R_AARCH64_LDST64_ABS_LO12_NC:
-    case ELF::R_AARCH64_LDST128_ABS_LO12_NC:
-    case ELF::R_AARCH64_TLSDESC_ADD_LO12:
-    case ELF::R_AARCH64_TLSDESC_ADR_PAGE21:
-    case ELF::R_AARCH64_TLSDESC_ADR_PREL21:
-    case ELF::R_AARCH64_TLSDESC_LD64_LO12:
-    case ELF::R_AARCH64_TLSIE_ADR_GOTTPREL_PAGE21:
-    case ELF::R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC:
-    case ELF::R_AARCH64_MOVW_UABS_G0:
-    case ELF::R_AARCH64_MOVW_UABS_G0_NC:
-    case ELF::R_AARCH64_MOVW_UABS_G1:
-    case ELF::R_AARCH64_MOVW_UABS_G1_NC:
-    case ELF::R_AARCH64_MOVW_UABS_G2:
-    case ELF::R_AARCH64_MOVW_UABS_G2_NC:
-    case ELF::R_AARCH64_MOVW_UABS_G3:
-    case ELF::R_AARCH64_PREL16:
-    case ELF::R_AARCH64_PREL32:
-    case ELF::R_AARCH64_PREL64:
-      Rels[Offset] = Relocation{Offset, Symbol, RelType, Addend, Value};
-      return;
-    case ELF::R_AARCH64_CALL26:
-    case ELF::R_AARCH64_JUMP26:
-    case ELF::R_AARCH64_TSTBR14:
-    case ELF::R_AARCH64_CONDBR19:
-    case ELF::R_AARCH64_TLSDESC_CALL:
-    case ELF::R_AARCH64_TLSLE_ADD_TPREL_HI12:
-    case ELF::R_AARCH64_TLSLE_ADD_TPREL_LO12_NC:
-      return;
-    default:
-      llvm_unreachable("Unexpected AArch64 relocation type in code");
-    }
-  }
-
-  void addRelocationX86(uint64_t Offset, MCSymbol *Symbol, uint64_t RelType,
-                        uint64_t Addend, uint64_t Value) {
-    switch (RelType) {
-    case ELF::R_X86_64_8:
-    case ELF::R_X86_64_16:
-    case ELF::R_X86_64_32:
-    case ELF::R_X86_64_32S:
-    case ELF::R_X86_64_64:
-    case ELF::R_X86_64_PC8:
-    case ELF::R_X86_64_PC32:
-    case ELF::R_X86_64_PC64:
-    case ELF::R_X86_64_GOTPCRELX:
-    case ELF::R_X86_64_REX_GOTPCRELX:
-      Relocations[Offset] = Relocation{Offset, Symbol, RelType, Addend, Value};
-      return;
-    case ELF::R_X86_64_PLT32:
-    case ELF::R_X86_64_GOTPCREL:
-    case ELF::R_X86_64_TPOFF32:
-    case ELF::R_X86_64_GOTTPOFF:
-      return;
-    default:
-      llvm_unreachable("Unexpected x86 relocation type in code");
-    }
-  }
-
   /// Register relocation type \p RelType at a given \p Address in the function
   /// against \p Symbol.
   /// Assert if the \p Address is not inside this function.
   void addRelocation(uint64_t Address, MCSymbol *Symbol, uint64_t RelType,
-                     uint64_t Addend, uint64_t Value) {
-    assert(Address >= getAddress() && Address < getAddress() + getMaxSize() &&
-           "address is outside of the function");
-    uint64_t Offset = Address - getAddress();
-    if (BC.isAArch64()) {
-      return addRelocationAArch64(Offset, Symbol, RelType, Addend, Value,
-                                  isInConstantIsland(Address));
-    }
-
-    return addRelocationX86(Offset, Symbol, RelType, Addend, Value);
-  }
+                     uint64_t Addend, uint64_t Value);
 
   /// Return the name of the section this function originated from.
   std::optional<StringRef> getOriginSectionName() const {
     if (!OriginSection)
       return std::nullopt;
     return OriginSection->getName();
   }
 

bolt/include/bolt/Core/MCPlusBuilder.h

public:
   /// Check whether we support inverting this branch
   virtual bool isUnsupportedBranch(unsigned Opcode) const { return false; }
 
   /// Return true of the instruction is of pseudo kind.
   bool isPseudo(const MCInst &Inst) const {
     return Info->get(Inst.getOpcode()).isPseudo();
   }
 
+  /// Return true if the relocation type is supported
+  virtual bool isSupportedRelocation(uint64_t RelType) const {
+    llvm_unreachable("not implemented");
+    return false;
+  }
+
   /// Creates x86 pause instruction.
   virtual void createPause(MCInst &Inst) const {
     llvm_unreachable("not implemented");
   }
 
   virtual void createLfence(MCInst &Inst) const {
     llvm_unreachable("not implemented");
   }

bolt/lib/Core/BinaryFunction.cpp

for (auto I = BasicBlocks.begin() + 1, E = BasicBlocks.end(); I != E; ++I) {
     for (MCInst &Inst : **I)
       if (!BC.MIB->isNoop(Inst))
         return false;
   }
 
   return true;
 }
 
+void BinaryFunction::addRelocation(uint64_t Address, MCSymbol *Symbol,
+                                   uint64_t RelType, uint64_t Addend,
+                                   uint64_t Value) {
+  assert(Address >= getAddress() && Address < getAddress() + getMaxSize() &&
+         "address is outside of the function");
+  uint64_t Offset = Address - getAddress();
+  LLVM_DEBUG(dbgs() << "BOLT-DEBUG: addRelocation in "
+                    << formatv("{0}@{1:x} against {2}\n", this, Offset,
+                               Symbol->getName()));
+  bool IsCI = BC.isAArch64() && isInConstantIsland(Address);
+  std::map<uint64_t, Relocation> &Rels =
+      IsCI ? Islands->Relocations : Relocations;
+  if (BC.MIB->isSupportedRelocation(RelType))
+    Rels[Offset] = Relocation{Offset, Symbol, RelType, Addend, Value};
+}
+
 } // namespace bolt
 } // namespace llvm

bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp

   bool isMoveMem2Reg(const MCInst &Inst) const override { return false; }
 
   bool isLeave(const MCInst &Inst) const override { return false; }
 
   bool isPop(const MCInst &Inst) const override { return false; }
 
   bool isPrefix(const MCInst &Inst) const override { return false; }
 
+  bool isSupportedRelocation(uint64_t RelType) const override {
+    switch (RelType) {
+    case ELF::R_AARCH64_ABS64:
+    case ELF::R_AARCH64_ABS32:
+    case ELF::R_AARCH64_ABS16:
+    case ELF::R_AARCH64_ADD_ABS_LO12_NC:
+    case ELF::R_AARCH64_ADR_GOT_PAGE:
+    case ELF::R_AARCH64_ADR_PREL_LO21:
+    case ELF::R_AARCH64_ADR_PREL_PG_HI21:
+    case ELF::R_AARCH64_ADR_PREL_PG_HI21_NC:
+    case ELF::R_AARCH64_LD64_GOT_LO12_NC:
+    case ELF::R_AARCH64_LDST8_ABS_LO12_NC:
+    case ELF::R_AARCH64_LDST16_ABS_LO12_NC:
+    case ELF::R_AARCH64_LDST32_ABS_LO12_NC:
+    case ELF::R_AARCH64_LDST64_ABS_LO12_NC:
+    case ELF::R_AARCH64_LDST128_ABS_LO12_NC:
+    case ELF::R_AARCH64_TLSDESC_ADD_LO12:
+    case ELF::R_AARCH64_TLSDESC_ADR_PAGE21:
+    case ELF::R_AARCH64_TLSDESC_ADR_PREL21:
+    case ELF::R_AARCH64_TLSDESC_LD64_LO12:
+    case ELF::R_AARCH64_TLSIE_ADR_GOTTPREL_PAGE21:
+    case ELF::R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC:
+    case ELF::R_AARCH64_MOVW_UABS_G0:
+    case ELF::R_AARCH64_MOVW_UABS_G0_NC:
+    case ELF::R_AARCH64_MOVW_UABS_G1:
+    case ELF::R_AARCH64_MOVW_UABS_G1_NC:
+    case ELF::R_AARCH64_MOVW_UABS_G2:
+    case ELF::R_AARCH64_MOVW_UABS_G2_NC:
+    case ELF::R_AARCH64_MOVW_UABS_G3:
+    case ELF::R_AARCH64_PREL16:
+    case ELF::R_AARCH64_PREL32:
+    case ELF::R_AARCH64_PREL64:
+      return true;
+    case ELF::R_AARCH64_CALL26:
+    case ELF::R_AARCH64_JUMP26:
+    case ELF::R_AARCH64_TSTBR14:
+    case ELF::R_AARCH64_CONDBR19:
+    case ELF::R_AARCH64_TLSDESC_CALL:
+    case ELF::R_AARCH64_TLSLE_ADD_TPREL_HI12:
+    case ELF::R_AARCH64_TLSLE_ADD_TPREL_LO12_NC:
+      return false;
+    default:
+      llvm_unreachable("Unexpected AArch64 relocation type in code");
+    }
+  }
+
   bool createReturn(MCInst &Inst) const override {
     Inst.setOpcode(AArch64::RET);
     Inst.clear();
     Inst.addOperand(MCOperand::createReg(AArch64::LR));
     return true;
   }
 
   InstructionListType materializeAddress(const MCSymbol *Target, MCContext *Ctx,

bolt/lib/Target/X86/X86MCPlusBuilder.cpp

bool isCleanRegXOR(const MCInst &Inst) const override {
     return (Inst.getOperand(0).getReg() == Inst.getOperand(2).getReg());
   }
 
   bool isPacked(const MCInst &Inst) const override {
     const MCInstrDesc &Desc = Info->get(Inst.getOpcode());
     return (Desc.TSFlags & X86II::OpPrefixMask) == X86II::PD;
   }
 
+  bool isSupportedRelocation(uint64_t RelType) const override {
+    switch (RelType) {
+    case ELF::R_X86_64_8:
+    case ELF::R_X86_64_16:
+    case ELF::R_X86_64_32:
+    case ELF::R_X86_64_32S:
+    case ELF::R_X86_64_64:
+    case ELF::R_X86_64_PC8:
+    case ELF::R_X86_64_PC32:
+    case ELF::R_X86_64_PC64:
+    case ELF::R_X86_64_GOTPCRELX:
+    case ELF::R_X86_64_REX_GOTPCRELX:
+      return true;
+    case ELF::R_X86_64_PLT32:
+    case ELF::R_X86_64_GOTPCREL:
+    case ELF::R_X86_64_TPOFF32:
+    case ELF::R_X86_64_GOTTPOFF:
+      return false;
+    default:
+      llvm_unreachable("Unexpected x86 relocation type in code");
+    }
+  }
+
   unsigned getTrapFillValue() const override { return 0xCC; }
 
   struct IndJmpMatcherFrag1 : MCInstMatcher {
     std::unique_ptr<MCInstMatcher> Base;
     std::unique_ptr<MCInstMatcher> Scale;
     std::unique_ptr<MCInstMatcher> Index;
     std::unique_ptr<MCInstMatcher> Offset;