changed to using BinarySections, cleaned up code that is no longer necessary, etc

Author: Alexander Yermolovich

bolt/include/bolt/Core/BinarySection.h

@@ -340,6 +340,8 @@ class BinarySection {
   /// Does this section have any non-pending relocations?
   bool hasRelocations() const { return !Relocations.empty(); }
 
+  bool hasDynamicRelocations() const { return !DynamicRelocations.empty(); }
+
   /// Does this section have any pending relocations?
   bool hasPendingRelocations() const { return !PendingRelocations.empty(); }
 

bolt/include/bolt/Core/Relocation.h

@@ -180,11 +180,6 @@ inline raw_ostream &operator<<(raw_ostream &OS, const Relocation &Rel) {
   return OS;
 }
 
-uint32_t getRelocationSymbol(const object::ELFObjectFileBase *Obj,
-                             const object::RelocationRef &Rel);
-
-int64_t getRelocationAddend(const object::ELFObjectFileBase *Obj,
-                            const object::RelocationRef &Rel);
 } // namespace bolt
 } // namespace llvm
 

bolt/include/bolt/Passes/IdenticalCodeFolding.h

@@ -39,30 +39,35 @@ class IdenticalCodeFolding : public BinaryFunctionPass {
     All,  // Aggressive ICF for code.
   };
   explicit IdenticalCodeFolding(const cl::opt<bool> &PrintPass)
-      : BinaryFunctionPass(PrintPass) {
-    VtableBitVector.resize((((uint64_t)1) << 32) / 8);
-  }
+      : BinaryFunctionPass(PrintPass) {}
 
   const char *getName() const override { return "identical-code-folding"; }
   Error runOnFunctions(BinaryContext &BC) override;
 
 private:
   /// Bit vector of memory addresses of vtables.
   llvm::BitVector VtableBitVector;
-  bool isInVTable(uint64_t Address) const {
+  /// Returns true if the memory address is in the bit vector of vtables.
+  bool isAddressInVTable(uint64_t Address) const {
     return VtableBitVector.test(Address / 8);
   }
+  /// Initialize bit vector of memory addresses of vtables.
+  void initVtable() {VtableBitVector.resize((((uint64_t)1) << 32) / 8);}
+  /// Mark memory address of vtable as used.
+  void setAddressUsedInVTable(uint64_t Address) { VtableBitVector.set(Address / 8); }
   /// Scans symbol table and creates a bit vector of memory addresses of
   /// vtables.
-  void processSymbolTable(const BinaryContext &BC);
+  void initVTableReferences(const BinaryContext &BC);
   /// Analyze .text section and relocations and mark functions that are not
   /// safe to fold.
-  Error markFunctionsUnsafeToFold(BinaryContext &BC);
-  /// Process relocations in the .data section to identify function
-  /// references.
-  Error processDataRelocations(BinaryContext &BC,
-                               const SectionRef &SecRefRelData,
-                               const bool HasAddressTaken);
+  void markFunctionsUnsafeToFold(BinaryContext &BC);
+  /// Process static and dynamic relocations in the data sections to identify
+  /// function references, and marks them as unsafe to fold. It filters out
+  /// symbol references that are in vtables.
+  void analyzeDataRelocations(BinaryContext &BC);
+  /// Process functions that have CFG created and mark functions unsafe to fold
+  /// that are used in non-control flow instructions.
+  void analyzeFunctions(BinaryContext &BC);
 };
 
 } // namespace bolt

bolt/lib/Core/Relocation.cpp

@@ -1115,65 +1115,3 @@ void Relocation::print(raw_ostream &OS) const {
     OS << ", 0x" << Twine::utohexstr(Addend);
   OS << ", 0x" << Twine::utohexstr(Value);
 }
-
-namespace {
-template <typename ELFT>
-int64_t getRelocationAddend(const llvm::object::ELFObjectFile<ELFT> *Obj,
-                            const llvm::object::RelocationRef &RelRef) {
-  using ELFShdrTy = typename ELFT::Shdr;
-  using Elf_Rela = typename ELFT::Rela;
-  int64_t Addend = 0;
-  const llvm::object::ELFFile<ELFT> &EF = Obj->getELFFile();
-  llvm::object::DataRefImpl Rel = RelRef.getRawDataRefImpl();
-  const ELFShdrTy *RelocationSection = cantFail(EF.getSection(Rel.d.a));
-  switch (RelocationSection->sh_type) {
-  default:
-    llvm_unreachable("unexpected relocation section type");
-  case ELF::SHT_REL:
-    break;
-  case ELF::SHT_RELA: {
-    const Elf_Rela *RelA = Obj->getRela(Rel);
-    Addend = RelA->r_addend;
-    break;
-  }
-  }
-
-  return Addend;
-}
-
-template <typename ELFT>
-uint32_t getRelocationSymbol(const llvm::object::ELFObjectFile<ELFT> *Obj,
-                             const llvm::object::RelocationRef &RelRef) {
-  using ELFShdrTy = typename ELFT::Shdr;
-  uint32_t Symbol = 0;
-  const llvm::object::ELFFile<ELFT> &EF = Obj->getELFFile();
-  llvm::object::DataRefImpl Rel = RelRef.getRawDataRefImpl();
-  const ELFShdrTy *RelocationSection = cantFail(EF.getSection(Rel.d.a));
-  switch (RelocationSection->sh_type) {
-  default:
-    llvm_unreachable("unexpected relocation section type");
-  case ELF::SHT_REL:
-    Symbol = Obj->getRel(Rel)->getSymbol(EF.isMips64EL());
-    break;
-  case ELF::SHT_RELA:
-    Symbol = Obj->getRela(Rel)->getSymbol(EF.isMips64EL());
-    break;
-  }
-
-  return Symbol;
-}
-} // namespace
-
-namespace llvm {
-namespace bolt {
-uint32_t getRelocationSymbol(const llvm::object::ELFObjectFileBase *Obj,
-                             const llvm::object::RelocationRef &Rel) {
-  return ::getRelocationSymbol(cast<llvm::object::ELF64LEObjectFile>(Obj), Rel);
-}
-
-int64_t getRelocationAddend(const llvm::object::ELFObjectFileBase *Obj,
-                            const llvm::object::RelocationRef &Rel) {
-  return ::getRelocationAddend(cast<llvm::object::ELF64LEObjectFile>(Obj), Rel);
-}
-} // namespace bolt
-} // namespace llvm

bolt/lib/Passes/IdenticalCodeFolding.cpp

@@ -14,6 +14,8 @@
 #include "bolt/Core/HashUtilities.h"
 #include "bolt/Core/ParallelUtilities.h"
 #include "llvm/ADT/BitVector.h"
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ThreadPool.h"
@@ -354,94 +356,51 @@ typedef std::unordered_map<BinaryFunction *, std::vector<BinaryFunction *>,
 
 namespace llvm {
 namespace bolt {
-/// Scans symbol table and creates a bit vector of memory addresses of vtables.
-void IdenticalCodeFolding::processSymbolTable(const BinaryContext &BC) {
-  for (auto &[Address, Data] : BC.getBinaryData()) {
+/// Scans symbol table and creates a bit vector of memory addresses of vtable symbols.
+void IdenticalCodeFolding::initVTableReferences(const BinaryContext &BC) {
+  initVtable();
+  for (const auto &[Address, Data] : BC.getBinaryData()) {
     // Filter out all symbols that are not vtables.
     if (!Data->getName().starts_with("_ZTV"))
       continue;
-    for (uint64_t I = Address / 8, End = I + (Data->getSize() / 8); I < End;
-         ++I)
-      VtableBitVector.set(I);
+    for (uint64_t I = Address, End = I + Data->getSize(); I < End; I += 8)
+      setAddressUsedInVTable(I);
   }
 }
-Error IdenticalCodeFolding::processDataRelocations(
-    BinaryContext &BC, const SectionRef &SecRefRelData,
-    const bool HasAddressTaken) {
-  for (const RelocationRef &Rel : SecRefRelData.relocations()) {
-    if (isInVTable(Rel.getOffset()))
+void IdenticalCodeFolding::analyzeDataRelocations(BinaryContext &BC) {
+  initVTableReferences(BC);
+  for (const BinarySection &Sec : BC.sections()) {
+    if (!Sec.hasSectionRef() || !Sec.isData())
       continue;
-    symbol_iterator SymbolIter = Rel.getSymbol();
-    const ObjectFile *OwningObj = Rel.getObject();
-    assert(SymbolIter != OwningObj->symbol_end() &&
-           "relocation Symbol expected");
-    const SymbolRef &Symbol = *SymbolIter;
-    const uint64_t SymbolAddress = cantFail(Symbol.getAddress());
-    const ELFObjectFileBase *ELFObj = dyn_cast<ELFObjectFileBase>(OwningObj);
-    if (!ELFObj)
-      return createFatalBOLTError(
-          Twine("BOLT-ERROR: Only ELFObjectFileBase is supported"));
-    const int64_t Addend = getRelocationAddend(ELFObj, Rel);
-    BinaryFunction *BF = BC.getBinaryFunctionAtAddress(SymbolAddress + Addend);
-    if (!BF)
-      continue;
-    BF->setHasAddressTaken(HasAddressTaken);
+    for (const auto &Rel : Sec.relocations()) {
+      const uint64_t RelAddr = Rel.Offset + Sec.getAddress();
+      if (isAddressInVTable(RelAddr))
+        continue;
+      BinaryFunction *BF = BC.getFunctionForSymbol(Rel.Symbol);
+      if (!BF)
+        continue;
+      BF->setHasAddressTaken(true);
+    }
+    for (const auto &Rel : Sec.dynamicRelocations()) {
+      const uint64_t RelAddr = Rel.Offset + Sec.getAddress();
+      if (isAddressInVTable(RelAddr))
+        continue;
+      BinaryFunction *BF =
+          BC.getBinaryFunctionContainingAddress(Rel.Addend,
+                                                /*CheckPastEnd*/ false,
+                                                /*UseMaxSize*/ true);
+      if (!BF)
+        continue;
+      BF->setHasAddressTaken(true);
+    }
   }
-  return Error::success();
 }
-
-Error IdenticalCodeFolding::markFunctionsUnsafeToFold(BinaryContext &BC) {
-  if (!BC.isX86())
-    BC.outs() << "BOLT-WARNING: safe ICF is only supported for x86\n";
-  processSymbolTable(BC);
-  for (const auto &Sec : BC.sections()) {
-    if (!Sec.hasSectionRef() || !Sec.isRela())
-      continue;
-    const SectionRef &SecRef = Sec.getSectionRef();
-    section_iterator RelocatedSecIter = cantFail(SecRef.getRelocatedSection());
-    assert(RelocatedSecIter != SecRef.getObject()->section_end() &&
-           "Relocation section exists without corresponding relocated section");
-    const SectionRef &RelocatedSecRef = *RelocatedSecIter;
-    const StringRef RelocatedSectionName = cantFail(RelocatedSecRef.getName());
-    const bool SkipRelocs =
-        StringSwitch<bool>(RelocatedSectionName)
-            .Cases(".plt", ".got.plt", ".eh_frame", ".gcc_except_table",
-                   ".fini_array", ".init_array", true)
-            .Default(false);
-    if (!RelocatedSecRef.isData() || SkipRelocs)
-      continue;
-
-    Error ErrorStatus =
-        processDataRelocations(BC, SecRef, /* HasAddressTaken */ true);
-    if (ErrorStatus)
-      return ErrorStatus;
-  }
-  ErrorOr<BinarySection &> SecRelDataIA =
-      BC.getUniqueSectionByName(".rela.init_array");
-  if (SecRelDataIA) {
-    const SectionRef SecRefRelData = SecRelDataIA->getSectionRef();
-    Error ErrorStatus =
-        processDataRelocations(BC, SecRefRelData, /* !HasAddressTaken */ false);
-    if (ErrorStatus)
-      return ErrorStatus;
-  }
-  ErrorOr<BinarySection &> SecRelDataFIA =
-      BC.getUniqueSectionByName(".rela.fini_array");
-  if (SecRelDataFIA) {
-    const SectionRef SecRefRelData = SecRelDataFIA->getSectionRef();
-    Error ErrorStatus =
-        processDataRelocations(BC, SecRefRelData, /* !HasAddressTaken */ false);
-    if (ErrorStatus)
-      return ErrorStatus;
-  }
-
+void IdenticalCodeFolding::analyzeFunctions(BinaryContext &BC) {
   ParallelUtilities::WorkFuncTy WorkFun = [&](BinaryFunction &BF) {
-    for (const BinaryBasicBlock *BB : BF.getLayout().blocks()) {
-      for (const MCInst &Inst : *BB) {
+    for (const BinaryBasicBlock *BB : BF.getLayout().blocks())
+      for (const MCInst &Inst : *BB)
         if (!(BC.MIB->isCall(Inst) || BC.MIB->isBranch(Inst)))
           BF.processInstructionForFuncReferences(BC, Inst);
-      }
-    }
   };
   ParallelUtilities::PredicateTy SkipFunc =
       [&](const BinaryFunction &BF) -> bool {
@@ -459,7 +418,15 @@ Error IdenticalCodeFolding::markFunctionsUnsafeToFold(BinaryContext &BC) {
              << '\n';
     }
   });
-  return Error::success();
+}
+void IdenticalCodeFolding::markFunctionsUnsafeToFold(BinaryContext &BC) {
+  NamedRegionTimer MarkFunctionsUnsafeToFoldTimer(
+      "markFunctionsUnsafeToFold", "markFunctionsUnsafeToFold", "ICF breakdown",
+      "ICF breakdown", opts::TimeICF);
+  if (!BC.isX86())
+    BC.outs() << "BOLT-WARNING: safe ICF is only supported for x86\n";
+  analyzeDataRelocations(BC);
+  analyzeFunctions(BC);
 }
 
 Error IdenticalCodeFolding::runOnFunctions(BinaryContext &BC) {
@@ -597,8 +564,7 @@ Error IdenticalCodeFolding::runOnFunctions(BinaryContext &BC) {
     LLVM_DEBUG(SinglePass.stopTimer());
   };
   if (opts::ICF == ICFLevel::Safe)
-    if (Error Err = markFunctionsUnsafeToFold(BC))
-      return Err;
+    markFunctionsUnsafeToFold(BC);
   hashFunctions();
   createCongruentBuckets();
 

bolt/lib/Rewrite/RewriteInstance.cpp

@@ -2120,6 +2120,64 @@ void RewriteInstance::adjustCommandLineOptions() {
   }
 }
 
+namespace {
+template <typename ELFT>
+int64_t getRelocationAddend(const ELFObjectFile<ELFT> *Obj,
+                            const RelocationRef &RelRef) {
+  using ELFShdrTy = typename ELFT::Shdr;
+  using Elf_Rela = typename ELFT::Rela;
+  int64_t Addend = 0;
+  const ELFFile<ELFT> &EF = Obj->getELFFile();
+  DataRefImpl Rel = RelRef.getRawDataRefImpl();
+  const ELFShdrTy *RelocationSection = cantFail(EF.getSection(Rel.d.a));
+  switch (RelocationSection->sh_type) {
+  default:
+    llvm_unreachable("unexpected relocation section type");
+  case ELF::SHT_REL:
+    break;
+  case ELF::SHT_RELA: {
+    const Elf_Rela *RelA = Obj->getRela(Rel);
+    Addend = RelA->r_addend;
+    break;
+  }
+  }
+
+  return Addend;
+}
+
+int64_t getRelocationAddend(const ELFObjectFileBase *Obj,
+                            const RelocationRef &Rel) {
+  return getRelocationAddend(cast<ELF64LEObjectFile>(Obj), Rel);
+}
+
+template <typename ELFT>
+uint32_t getRelocationSymbol(const ELFObjectFile<ELFT> *Obj,
+                             const RelocationRef &RelRef) {
+  using ELFShdrTy = typename ELFT::Shdr;
+  uint32_t Symbol = 0;
+  const ELFFile<ELFT> &EF = Obj->getELFFile();
+  DataRefImpl Rel = RelRef.getRawDataRefImpl();
+  const ELFShdrTy *RelocationSection = cantFail(EF.getSection(Rel.d.a));
+  switch (RelocationSection->sh_type) {
+  default:
+    llvm_unreachable("unexpected relocation section type");
+  case ELF::SHT_REL:
+    Symbol = Obj->getRel(Rel)->getSymbol(EF.isMips64EL());
+    break;
+  case ELF::SHT_RELA:
+    Symbol = Obj->getRela(Rel)->getSymbol(EF.isMips64EL());
+    break;
+  }
+
+  return Symbol;
+}
+
+uint32_t getRelocationSymbol(const ELFObjectFileBase *Obj,
+                             const RelocationRef &Rel) {
+  return getRelocationSymbol(cast<ELF64LEObjectFile>(Obj), Rel);
+}
+} // anonymous namespace
+
 bool RewriteInstance::analyzeRelocation(
     const RelocationRef &Rel, uint64_t &RType, std::string &SymbolName,
     bool &IsSectionRelocation, uint64_t &SymbolAddress, int64_t &Addend,

bolt/test/X86/icf-safe-process-rela-data.test

@@ -1,5 +1,4 @@
-## Check that BOLT handles correctly folding functions with --icf=safe that can be referenced.
-## This checks when only reference to a function is in .rela.data section.
+## Check that BOLT handles correctly folding functions with --icf=safe that are only reference in a .rela.data section.
 
 # REQUIRES: system-linux
 # RUN: llvm-mc -filetype=obj -triple x86_64-unknown-linux %s -o %t1.o

bolt/test/X86/icf-safe-test1-no-relocs.test

@@ -1,11 +1,11 @@
-## Checks that BOLT handles correctly with no relocations with --icf=safe option.
+## Check that BOLT handles correctly a binary with no relocations with the --icf=safe option.
 
 # REQUIRES: system-linux
 # RUN: llvm-mc -filetype=obj -triple x86_64-unknown-linux %s -o %t1.o
 # RUN: %clang %cflags %t1.o -o %t.exe
 # RUN: not llvm-bolt --no-threads %t.exe --icf=safe -debug -debug-only=bolt-icf -o %t.bolt 2>&1 | FileCheck --check-prefix=SAFEICFCHECK %s
 
-# SAFEICFCHECK: BOLT-ERROR: Binary built without relocations. Safe ICF is not supported
+# SAFEICFCHECK: BOLT-ERROR: binary built without relocations. Safe ICF is not supported
 
 ## int main(int argc, char **argv) {
 ##   return temp;

bolt/test/X86/icf-safe-test1.test

@@ -1,5 +1,5 @@
-## Check that BOLT handles correctly folding functions with --icf=safe that can be referenced.
-## It invokes BOLT twice first testing CFG path, and second when function has to be disassembled.
+## Check that BOLT handles correctly folding functions with --icf=safe that can be referenced by non-control flow instructions.
+## It invokes BOLT twice first testing CFG path, and second when functions have to be disassembled.
 
 # REQUIRES: system-linux
 # RUN: llvm-mc -filetype=obj -triple x86_64-unknown-linux %s -o %t1.o