[BOLT] Add support for safe-icf #116275
Merged
[BOLT] Add support for safe-icf #116275
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D65799965
ayermolo
requested review from
aaupov,
maksfb,
rafaelauler and
dcci
as code owners
|
@llvm/pr-subscribers-bolt Author: Alexander Yermolovich (ayermolo) ChangesIdentical Code Folding (ICF) folds functions that are "identical" into one function, and updates symbol addresses to the new address. This reduces the size of a binary, but can lead to problems. For example when function pointers are compared. Either explicitly in the code or generated code by optimization passes like Indirect Call Promotion (ICP). After ICF what used to be two different addresses become the same address. This can lead to a different code path being taken. This is where Safe ICF comes in. Linker (LLD) does it using address significant section generated by clang. If symbol is in it, or an object doesn't have this section symbols are not folded. BOLT does not have the information regarding which objects do not have this section, so can't re-use this mechanism. This implementation scans .text section and conservatively marks functions which symbols, and are not used in the control flow instructions, as unsafe. It also scans through the .rela.data section and does the same for relocations that reference functions. The latter handles the case when function pointer is stored in a local or global variable. Patch is 86.39 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/116275.diff 25 Files Affected:
diff --git a/bolt/include/bolt/Core/BinaryContext.h b/bolt/include/bolt/Core/BinaryContext.h
index 08ce892054874c..92889ffe162ae1 100644
--- a/bolt/include/bolt/Core/BinaryContext.h
+++ b/bolt/include/bolt/Core/BinaryContext.h
@@ -282,6 +282,12 @@ class BinaryContext {
std::unique_ptr<DWARFContext> DwCtx,
JournalingStreams Logger);
+ /// Returns addend of a relocation.
+ static int64_t getRelocationAddend(const ELFObjectFileBase *Obj,
+ const RelocationRef &Rel);
+ /// Returns symbol of a relocation.
+ static uint32_t getRelocationSymbol(const ELFObjectFileBase *Obj,
+ const RelocationRef &Rel);
/// Superset of compiler units that will contain overwritten code that needs
/// new debug info. In a few cases, functions may end up not being
/// overwritten, but it is okay to re-generate debug info for them.
@@ -1350,6 +1356,9 @@ class BinaryContext {
return Code.size();
}
+ /// Processes .text section to identify function references.
+ void processInstructionForFuncReferences(const MCInst &Inst);
+
/// Compute the native code size for a range of instructions.
/// Note: this can be imprecise wrt the final binary since happening prior to
/// relaxation, as well as wrt the original binary because of opcode
diff --git a/bolt/include/bolt/Core/BinaryFunction.h b/bolt/include/bolt/Core/BinaryFunction.h
index 0b3682353f736e..5e1ddcc6bff9a2 100644
--- a/bolt/include/bolt/Core/BinaryFunction.h
+++ b/bolt/include/bolt/Core/BinaryFunction.h
@@ -428,6 +428,9 @@ class BinaryFunction {
/// Function order for streaming into the destination binary.
uint32_t Index{-1U};
+ /// Indicates if the function is safe to fold.
+ bool IsSafeToICF{true};
+
/// Get basic block index assuming it belongs to this function.
unsigned getIndex(const BinaryBasicBlock *BB) const {
assert(BB->getIndex() < BasicBlocks.size());
@@ -817,6 +820,12 @@ class BinaryFunction {
return nullptr;
}
+ /// Indicates if the function is safe to fold.
+ bool isSafeToICF() const { return IsSafeToICF; }
+
+ /// Sets the function is not safe to fold.
+ void setUnsetToICF() { IsSafeToICF = false; }
+
/// Returns the raw binary encoding of this function.
ErrorOr<ArrayRef<uint8_t>> getData() const;
diff --git a/bolt/include/bolt/Passes/IdenticalCodeFolding.h b/bolt/include/bolt/Passes/IdenticalCodeFolding.h
index b4206fa3607445..75d5f19ddfc7be 100644
--- a/bolt/include/bolt/Passes/IdenticalCodeFolding.h
+++ b/bolt/include/bolt/Passes/IdenticalCodeFolding.h
@@ -31,11 +31,21 @@ class IdenticalCodeFolding : public BinaryFunctionPass {
}
public:
- explicit IdenticalCodeFolding(const cl::opt<bool> &PrintPass)
- : BinaryFunctionPass(PrintPass) {}
+ explicit IdenticalCodeFolding(const cl::opt<bool> &PrintPass, bool IsSafeICF)
+ : BinaryFunctionPass(PrintPass), IsSafeICF(IsSafeICF) {}
const char *getName() const override { return "identical-code-folding"; }
Error runOnFunctions(BinaryContext &BC) override;
+
+private:
+ /// Create a skip list of functions that should not be folded.
+ Error createFoldSkipList(BinaryContext &BC);
+ /// Processes relocations in the .data section to identify function
+ /// references.
+ void processDataRelocations(BinaryContext &BC,
+ const SectionRef &SecRefRelData);
+
+ bool IsSafeICF;
};
} // namespace bolt
diff --git a/bolt/lib/Core/BinaryContext.cpp b/bolt/lib/Core/BinaryContext.cpp
index f246750209d6c4..31352bed6fdab7 100644
--- a/bolt/lib/Core/BinaryContext.cpp
+++ b/bolt/lib/Core/BinaryContext.cpp
@@ -75,8 +75,63 @@ cl::opt<std::string> CompDirOverride(
"location, which is used with DW_AT_dwo_name to construct a path "
"to *.dwo files."),
cl::Hidden, cl::init(""), cl::cat(BoltCategory));
+
+cl::opt<bool> ICF("icf", cl::desc("fold functions with identical code"),
+ cl::cat(BoltOptCategory));
+
+cl::opt<bool> SafeICF("safe-icf",
+ cl::desc("Enable safe identical code folding"),
+ cl::cat(BoltOptCategory));
} // namespace opts
+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;
+}
+
+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;
+}
+} // anonymous namespace
+
namespace llvm {
namespace bolt {
@@ -156,6 +211,16 @@ BinaryContext::~BinaryContext() {
clearBinaryData();
}
+uint32_t BinaryContext::getRelocationSymbol(const ELFObjectFileBase *Obj,
+ const RelocationRef &Rel) {
+ return ::getRelocationSymbol(cast<ELF64LEObjectFile>(Obj), Rel);
+}
+
+int64_t BinaryContext::getRelocationAddend(const ELFObjectFileBase *Obj,
+ const RelocationRef &Rel) {
+ return ::getRelocationAddend(cast<ELF64LEObjectFile>(Obj), Rel);
+}
+
/// Create BinaryContext for a given architecture \p ArchName and
/// triple \p TripleName.
Expected<std::unique_ptr<BinaryContext>> BinaryContext::createBinaryContext(
@@ -1945,6 +2010,27 @@ static void printDebugInfo(raw_ostream &OS, const MCInst &Instruction,
OS << " discriminator:" << Row.Discriminator;
}
+static bool skipInstruction(const MCInst &Inst, const BinaryContext &BC) {
+ const bool IsX86 = BC.isX86();
+ return (BC.MIB->isPseudo(Inst) || BC.MIB->isUnconditionalBranch(Inst) ||
+ (IsX86 && BC.MIB->isConditionalBranch(Inst)) ||
+ BC.MIB->isCall(Inst) || BC.MIB->isBranch(Inst));
+}
+void BinaryContext::processInstructionForFuncReferences(const MCInst &Inst) {
+ if (!opts::SafeICF || skipInstruction(Inst, *this))
+ return;
+ for (const MCOperand &Op : MCPlus::primeOperands(Inst)) {
+ if (Op.isExpr()) {
+ const MCExpr &Expr = *Op.getExpr();
+ if (Expr.getKind() == MCExpr::SymbolRef) {
+ const MCSymbol &Symbol = cast<MCSymbolRefExpr>(Expr).getSymbol();
+ if (BinaryFunction *BF = getFunctionForSymbol(&Symbol))
+ BF->setUnsetToICF();
+ }
+ }
+ }
+}
+
void BinaryContext::printInstruction(raw_ostream &OS, const MCInst &Instruction,
uint64_t Offset,
const BinaryFunction *Function,
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index c12217d549479b..07d386efb26ec5 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1626,6 +1626,8 @@ bool BinaryFunction::scanExternalRefs() {
if (!BC.HasRelocations)
continue;
+ BC.processInstructionForFuncReferences(Instruction);
+
if (BranchTargetSymbol) {
BC.MIB->replaceBranchTarget(Instruction, BranchTargetSymbol,
Emitter.LocalCtx.get());
diff --git a/bolt/lib/Passes/IdenticalCodeFolding.cpp b/bolt/lib/Passes/IdenticalCodeFolding.cpp
index 38e080c9dd6213..f09482dc08c212 100644
--- a/bolt/lib/Passes/IdenticalCodeFolding.cpp
+++ b/bolt/lib/Passes/IdenticalCodeFolding.cpp
@@ -11,8 +11,10 @@
//===----------------------------------------------------------------------===//
#include "bolt/Passes/IdenticalCodeFolding.h"
+#include "bolt/Core/BinaryContext.h"
#include "bolt/Core/HashUtilities.h"
#include "bolt/Core/ParallelUtilities.h"
+#include "bolt/Rewrite/RewriteInstance.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/ThreadPool.h"
@@ -31,6 +33,7 @@ using namespace bolt;
namespace opts {
extern cl::OptionCategory BoltOptCategory;
+extern cl::opt<unsigned> Verbosity;
static cl::opt<bool>
ICFUseDFS("icf-dfs", cl::desc("use DFS ordering when using -icf option"),
@@ -341,6 +344,75 @@ typedef std::unordered_map<BinaryFunction *, std::vector<BinaryFunction *>,
namespace llvm {
namespace bolt {
+void IdenticalCodeFolding::processDataRelocations(
+ BinaryContext &BC, const SectionRef &SecRefRelData) {
+ for (const RelocationRef &Rel : SecRefRelData.relocations()) {
+ 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)
+ assert(false && "Only ELFObjectFileBase is supported");
+ const int64_t Addend = BinaryContext::getRelocationAddend(ELFObj, Rel);
+ BinaryFunction *BF = BC.getBinaryFunctionAtAddress(SymbolAddress + Addend);
+ if (!BF)
+ continue;
+ BF->setUnsetToICF();
+ }
+}
+
+Error IdenticalCodeFolding::createFoldSkipList(BinaryContext &BC) {
+ Error ErrorStatus = Error::success();
+ ErrorOr<BinarySection &> SecRelData = BC.getUniqueSectionByName(".rela.data");
+ if (!BC.HasRelocations)
+ ErrorStatus = joinErrors(
+ std::move(ErrorStatus),
+ createFatalBOLTError(Twine("BOLT-ERROR: Binary built without "
+ "relocations. Safe ICF is not supported")));
+ if (ErrorStatus)
+ return ErrorStatus;
+ if (SecRelData) {
+ SectionRef SecRefRelData = SecRelData->getSectionRef();
+ processDataRelocations(BC, SecRefRelData);
+ }
+
+ ParallelUtilities::WorkFuncTy WorkFun = [&](BinaryFunction &BF) {
+ if (BF.getState() == BinaryFunction::State::CFG) {
+ for (const BinaryBasicBlock *BB : BF.getLayout().blocks())
+ for (const MCInst &Inst : *BB)
+ BC.processInstructionForFuncReferences(Inst);
+ }
+ };
+ ParallelUtilities::PredicateTy SkipFunc =
+ [&](const BinaryFunction &BF) -> bool { return (bool)ErrorStatus; };
+ ParallelUtilities::runOnEachFunction(
+ BC, ParallelUtilities::SchedulingPolicy::SP_TRIVIAL, WorkFun, SkipFunc,
+ "markUnsafe", /*ForceSequential*/ false, 2);
+
+ LLVM_DEBUG({
+ std::vector<StringRef> Vect;
+ std::mutex PrintMutex;
+ ParallelUtilities::WorkFuncTy WorkFun = [&](BinaryFunction &BF) {
+ if (BF.isSafeToICF())
+ return;
+ std::lock_guard<std::mutex> Lock(PrintMutex);
+ Vect.push_back(BF.getOneName());
+ };
+ ParallelUtilities::PredicateTy SkipFunc =
+ [&](const BinaryFunction &BF) -> bool { return false; };
+ ParallelUtilities::runOnEachFunction(
+ BC, ParallelUtilities::SchedulingPolicy::SP_TRIVIAL, WorkFun, SkipFunc,
+ "markUnsafe", /*ForceSequential*/ false, 2);
+ llvm::sort(Vect);
+ for (const auto &FuncName : Vect)
+ dbgs() << "BOLT-DEBUG: skipping function " << FuncName << '\n';
+ });
+ return ErrorStatus;
+}
+
Error IdenticalCodeFolding::runOnFunctions(BinaryContext &BC) {
const size_t OriginalFunctionCount = BC.getBinaryFunctions().size();
uint64_t NumFunctionsFolded = 0;
@@ -350,6 +422,9 @@ Error IdenticalCodeFolding::runOnFunctions(BinaryContext &BC) {
std::atomic<uint64_t> NumFoldedLastIteration{0};
CongruentBucketsMap CongruentBuckets;
+ auto SkipFuncShared = [&](const BinaryFunction &BF) {
+ return !shouldOptimize(BF) || !BF.isSafeToICF();
+ };
// Hash all the functions
auto hashFunctions = [&]() {
NamedRegionTimer HashFunctionsTimer("hashing", "hashing", "ICF breakdown",
@@ -369,7 +444,7 @@ Error IdenticalCodeFolding::runOnFunctions(BinaryContext &BC) {
};
ParallelUtilities::PredicateTy SkipFunc = [&](const BinaryFunction &BF) {
- return !shouldOptimize(BF);
+ return SkipFuncShared(BF);
};
ParallelUtilities::runOnEachFunction(
@@ -385,7 +460,7 @@ Error IdenticalCodeFolding::runOnFunctions(BinaryContext &BC) {
"ICF breakdown", opts::TimeICF);
for (auto &BFI : BC.getBinaryFunctions()) {
BinaryFunction &BF = BFI.second;
- if (!this->shouldOptimize(BF))
+ if (SkipFuncShared(BF))
continue;
CongruentBuckets[&BF].emplace(&BF);
}
@@ -475,7 +550,11 @@ Error IdenticalCodeFolding::runOnFunctions(BinaryContext &BC) {
LLVM_DEBUG(SinglePass.stopTimer());
};
-
+ if (IsSafeICF) {
+ if (Error Err = createFoldSkipList(BC)) {
+ return Err;
+ }
+ }
hashFunctions();
createCongruentBuckets();
diff --git a/bolt/lib/Rewrite/BinaryPassManager.cpp b/bolt/lib/Rewrite/BinaryPassManager.cpp
index b0906041833484..df612c0a8f2e51 100644
--- a/bolt/lib/Rewrite/BinaryPassManager.cpp
+++ b/bolt/lib/Rewrite/BinaryPassManager.cpp
@@ -54,6 +54,8 @@ extern cl::opt<bool> PrintDynoStats;
extern cl::opt<bool> DumpDotAll;
extern cl::opt<std::string> AsmDump;
extern cl::opt<bolt::PLTCall::OptType> PLT;
+extern cl::opt<bool> ICF;
+extern cl::opt<bool> SafeICF;
static cl::opt<bool>
DynoStatsAll("dyno-stats-all",
@@ -65,9 +67,6 @@ static cl::opt<bool>
cl::desc("eliminate unreachable code"), cl::init(true),
cl::cat(BoltOptCategory));
-cl::opt<bool> ICF("icf", cl::desc("fold functions with identical code"),
- cl::cat(BoltOptCategory));
-
static cl::opt<bool> JTFootprintReductionFlag(
"jt-footprint-reduction",
cl::desc("make jump tables size smaller at the cost of using more "
@@ -397,8 +396,9 @@ Error BinaryFunctionPassManager::runAllPasses(BinaryContext &BC) {
Manager.registerPass(std::make_unique<StripRepRet>(NeverPrint),
opts::StripRepRet);
- Manager.registerPass(std::make_unique<IdenticalCodeFolding>(PrintICF),
- opts::ICF);
+ Manager.registerPass(
+ std::make_unique<IdenticalCodeFolding>(PrintICF, opts::SafeICF),
+ opts::ICF || opts::SafeICF);
Manager.registerPass(
std::make_unique<SpecializeMemcpy1>(NeverPrint, opts::SpecializeMemcpy1),
@@ -422,8 +422,9 @@ Error BinaryFunctionPassManager::runAllPasses(BinaryContext &BC) {
Manager.registerPass(std::make_unique<Inliner>(PrintInline));
- Manager.registerPass(std::make_unique<IdenticalCodeFolding>(PrintICF),
- opts::ICF);
+ Manager.registerPass(
+ std::make_unique<IdenticalCodeFolding>(PrintICF, opts::SafeICF),
+ opts::ICF || opts::SafeICF);
Manager.registerPass(std::make_unique<PLTCall>(PrintPLT));
diff --git a/bolt/lib/Rewrite/BoltDiff.cpp b/bolt/lib/Rewrite/BoltDiff.cpp
index 74b5ca18abce42..1a8eb5cd2ce39d 100644
--- a/bolt/lib/Rewrite/BoltDiff.cpp
+++ b/bolt/lib/Rewrite/BoltDiff.cpp
@@ -29,6 +29,7 @@ namespace opts {
extern cl::OptionCategory BoltDiffCategory;
extern cl::opt<bool> NeverPrint;
extern cl::opt<bool> ICF;
+extern cl::opt<bool> SafeICF;
static cl::opt<bool> IgnoreLTOSuffix(
"ignore-lto-suffix",
@@ -698,7 +699,7 @@ void RewriteInstance::compare(RewriteInstance &RI2) {
// Pre-pass ICF
if (opts::ICF) {
- IdenticalCodeFolding ICF(opts::NeverPrint);
+ IdenticalCodeFolding ICF(opts::NeverPrint, opts::SafeICF);
outs() << "BOLT-DIFF: Starting ICF pass for binary 1";
BC->logBOLTErrorsAndQuitOnFatal(ICF.runOnFunctions(*BC));
outs() << "BOLT-DIFF: Starting ICF pass for binary 2";
diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index a4c50cbc3e2bbf..70c9eeef3cdc57 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -2111,64 +2111,6 @@ 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,
@@ -2196,7 +2138,7 @@ bool RewriteInstance::analyzeRelocation(
return true;
ExtractedValue = Relocation::extractValue(RType, *Value, Rel.getOffset());
- Addend = getRelocationAddend(InputFile, Rel);
+ Addend = BinaryContext::getRelocationAddend(InputFile, Rel);
const bool IsPCRelative = Relocation::isPCRelative(RType);
const uint64_t PCRelOffset = IsPCRelative && !IsAArch64 ? Rel.getOffset() : 0;
@@ -2396,7 +2338,7 @@ void RewriteInstance::readDynamicRelocations(const SectionRef &Section,
StringRef SymbolName = "<none>";
MCSymbol *Symbol = nullptr;
uint64_t SymbolAddress = 0;
- const uint64_t Addend = getRelocationAddend(InputFile, Rel);
+ const uint64_t Addend = BinaryContext::getRelocationAddend(InputFile, Rel);
symbol_iterator SymbolIter = Rel.getSymbol();
if (SymbolIter != InputFile->symbol_end()) {
@@ -2421,7 +2363,7 @@ void RewriteInstance::readDynamicRelocations(const SectionRef &Section,
IsJmpRelocation[RType] = true;
if (Symbol)
- SymbolIndex[Symbol] = getRelocationSymbol(InputFile, Rel);
+ SymbolIndex[Symbol] = BinaryContext::getRelocationSymbol(InputFile, Rel);
BC->addDynamicRelocation(Rel.getOffset(), Symbol, RType, Addend);
}
diff --git a/bolt/test/X86/Inputs/helperSafeICF.s b/bolt/test/X86/Inputs/helperSafeICF.s
new file mode 100644
index 00000000000000..8ab47324454cbc
--- /dev/null
+++ b/bolt/test/X86/Inputs/helperSafeICF.s
@@ -0,0 +1,39 @@
+# clang++ -c helper.cpp -o helper.o
+# int FooVar = 1;
+# int BarVar = 2;
+#
+# int fooGlobalFuncHelper(int a, int b) {
+# return 5;
+# }
+ .text
+ .file "helper.cpp"
+ .globl _Z19fooGlobalFuncHelperii # -- Begin function _Z1...
[truncated]
|
maksfb
reviewed
Nov 14, 2024
dcci
reviewed
Nov 15, 2024
maksfb
reviewed
Nov 15, 2024
aaupov
reviewed
Nov 16, 2024
dcci
reviewed
Nov 19, 2024
dcci
reviewed
Nov 20, 2024
dcci
reviewed
Nov 20, 2024
dcci
reviewed
Nov 20, 2024
dcci
reviewed
Nov 20, 2024
dcci
reviewed
Nov 20, 2024
dcci
reviewed
Nov 20, 2024
aaupov
reviewed
Nov 20, 2024
There was a problem hiding this comment.
Left another round of comments. Will check tests next. Thank you for your patience, it’s a big change.
NB: we have to add ICF-specific processing into scanExternalRefs/disassembly which seems hacky. I think the proper approach would be to have a way to run passes early during disassembly/scanning. Don’t want to make it gating this PR though.
dcci
reviewed
Nov 20, 2024
maksfb
reviewed
Dec 10, 2024
maksfb
reviewed
Dec 10, 2024
maksfb
reviewed
Dec 10, 2024
maksfb
reviewed
Dec 11, 2024
aaupov
reviewed
Dec 13, 2024
|
|
||
| private: | ||
| /// Bit vector of memory addresses of vtables. | ||
| llvm::BitVector VtableBitVector; |
There was a problem hiding this comment.
Would it make sense to use SparseBitVector?
There was a problem hiding this comment.
My understanding random access is O(n) for it?
Considering how much debug information processing takes, seems fine?
There was a problem hiding this comment.
Yes, random access is O(N), but sequential is not:
testing/setting bits in a SparseBitVector is O(distance away from last set bit).
Vtables should occupy a vanishingly small part of the binary, so sparse shouldn't be too inefficient. You can also drop an explicit size. Can you check the two and compare peak RSS (disabling debug info update) and pass wall time?
Or let @maksfb weigh in.
There was a problem hiding this comment.
BitVector
12:10.63 real, 2190.50 user, 3352.54 sys, 0 amem, 50543836 mmem
12:15.47 real, 2171.61 user, 3110.00 sys, 0 amem, 50486944 mmem
12:15.42 real, 2199.27 user, 3336.12 sys, 0 amem, 50550316 mmem
SparceBItVector
11:56.70 real, 2173.22 user, 3025.61 sys, 0 amem, 50546564 mmem
12:25.82 real, 2196.06 user, 3279.76 sys, 0 amem, 50561464 mmem
11:48.03 real, 2254.86 user, 3216.55 sys, 0 amem, 50547536 mmem
Seems slightly faster with SparseBitVector. 🤷
aaupov
approved these changes
Dec 14, 2024
|
|
||
| private: | ||
| /// Bit vector of memory addresses of vtables. | ||
| llvm::BitVector VtableBitVector; |
There was a problem hiding this comment.
Yes, random access is O(N), but sequential is not:
testing/setting bits in a SparseBitVector is O(distance away from last set bit).
Vtables should occupy a vanishingly small part of the binary, so sparse shouldn't be too inefficient. You can also drop an explicit size. Can you check the two and compare peak RSS (disabling debug info update) and pass wall time?
Or let @maksfb weigh in.
| @@ -398,7 +396,7 @@ Error BinaryFunctionPassManager::runAllPasses(BinaryContext &BC) { | |||
| opts::StripRepRet); | |||
|
|
|||
| Manager.registerPass(std::make_unique<IdenticalCodeFolding>(PrintICF), | |||
| opts::ICF); | |||
| opts::ICF != IdenticalCodeFolding::ICFLevel::None); | |||
There was a problem hiding this comment.
Reduces diff changes. But it's a nit, so up to you.
maksfb
reviewed
Dec 14, 2024
dcci
reviewed
Dec 16, 2024
maksfb
reviewed
Dec 17, 2024
maksfb
reviewed
Dec 17, 2024
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 17, 2025
opt-dist: fix deprecated BOLT -icf=1 option Replaced deprecated `-icf=1` BOLT option. Spotted in recent CI run (https://github.com/rust-lang-ci/rust/actions/runs/15080898417/job/42397253162): ``` BOLT-WARNING: specifying numeric value "1" for option -icf is deprecated ``` Change was added in llvm/llvm-project#116275 Btw, now there also exist new option `-icf=safe`, will be nice to try it too.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 18, 2025
opt-dist: fix deprecated BOLT -icf=1 option Replaced deprecated `-icf=1` BOLT option. Spotted in recent CI run (https://github.com/rust-lang-ci/rust/actions/runs/15080898417/job/42397253162): ``` BOLT-WARNING: specifying numeric value "1" for option -icf is deprecated ``` Change was added in llvm/llvm-project#116275 Btw, now there also exist new option `-icf=safe`, will be nice to try it too.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Identical Code Folding (ICF) folds functions that are identical into one function, and updates symbol addresses to the new address. This reduces the size of a binary, but can lead to problems. For example when function pointers are compared. This can be done either explicitly in the code or generated IR by optimization passes like Indirect Call Promotion (ICP). After ICF what used to be two different addresses become the same address. This can lead to a different code path being taken.
This is where safe ICF comes in. Linker (LLD) does it using address significant section generated by clang. If symbol is in it, or an object doesn't have this section symbols are not folded.
BOLT does not have the information regarding which objects do not have this section, so can't re-use this mechanism.
This implementation scans code section and conservatively marks functions symbols as unsafe. It treats symbols as unsafe if they are used in non-control flow instruction. It also scans through the data relocation sections and does the same for relocations that reference a function symbol. The latter handles the case when function pointer is stored in a local or global variable, etc. If a relocation address points within a vtable these symbols are skipped.