[BOLT] Make Relocations a class and add optional field #131638
Merged
Conversation
This file contains 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
paschalis-mpeis
requested review from
aaupov,
maksfb,
rafaelauler,
ayermolo,
dcci and
yota9
as code owners
|
@llvm/pr-subscribers-bolt Author: Paschalis Mpeis (paschalis-mpeis) ChangesFull diff: https://github.com/llvm/llvm-project/pull/131638.diff 13 Files Affected:
diff --git a/bolt/include/bolt/Core/BinaryContext.h b/bolt/include/bolt/Core/BinaryContext.h
index 9a0485989201f..0da3815306a60 100644
--- a/bolt/include/bolt/Core/BinaryContext.h
+++ b/bolt/include/bolt/Core/BinaryContext.h
@@ -1296,7 +1296,7 @@ class BinaryContext {
/// Add a Section relocation at a given \p Address.
void addRelocation(uint64_t Address, MCSymbol *Symbol, uint32_t Type,
- uint64_t Addend = 0, uint64_t Value = 0);
+ bool Optional, uint64_t Addend = 0, uint64_t Value = 0);
/// Return a relocation registered at a given \p Address, or nullptr if there
/// is no relocation at such address.
@@ -1309,7 +1309,7 @@ class BinaryContext {
/// Register dynamic relocation at \p Address.
void addDynamicRelocation(uint64_t Address, MCSymbol *Symbol, uint32_t Type,
- uint64_t Addend, uint64_t Value = 0);
+ bool Optional, uint64_t Addend, uint64_t Value = 0);
/// Return a dynamic relocation registered at a given \p Address, or nullptr
/// if there is no dynamic relocation at such address.
diff --git a/bolt/include/bolt/Core/BinaryFunction.h b/bolt/include/bolt/Core/BinaryFunction.h
index a92cb466c5992..09e19a33986de 100644
--- a/bolt/include/bolt/Core/BinaryFunction.h
+++ b/bolt/include/bolt/Core/BinaryFunction.h
@@ -1261,7 +1261,7 @@ class BinaryFunction {
/// against \p Symbol.
/// Assert if the \p Address is not inside this function.
void addRelocation(uint64_t Address, MCSymbol *Symbol, uint32_t RelType,
- uint64_t Addend, uint64_t Value);
+ bool Optional, uint64_t Addend, uint64_t Value);
/// Return the name of the section this function originated from.
std::optional<StringRef> getOriginSectionName() const {
diff --git a/bolt/include/bolt/Core/BinarySection.h b/bolt/include/bolt/Core/BinarySection.h
index ad2fed2cf27eb..10f633fda6640 100644
--- a/bolt/include/bolt/Core/BinarySection.h
+++ b/bolt/include/bolt/Core/BinarySection.h
@@ -359,15 +359,18 @@ class BinarySection {
/// Add a new relocation at the given /p Offset.
void addRelocation(uint64_t Offset, MCSymbol *Symbol, uint32_t Type,
- uint64_t Addend, uint64_t Value = 0) {
+ bool Optional, uint64_t Addend, uint64_t Value = 0) {
assert(Offset < getSize() && "offset not within section bounds");
- Relocations.emplace(Relocation{Offset, Symbol, Type, Addend, Value});
+ Relocations.emplace(
+ Relocation{Offset, Symbol, Type, Optional, Addend, Value});
}
/// Add a dynamic relocation at the given /p Offset.
void addDynamicRelocation(uint64_t Offset, MCSymbol *Symbol, uint32_t Type,
- uint64_t Addend, uint64_t Value = 0) {
- addDynamicRelocation(Relocation{Offset, Symbol, Type, Addend, Value});
+ bool Optional, uint64_t Addend,
+ uint64_t Value = 0) {
+ addDynamicRelocation(
+ Relocation{Offset, Symbol, Type, Optional, Addend, Value});
}
void addDynamicRelocation(const Relocation &Reloc) {
@@ -401,13 +404,13 @@ class BinarySection {
/// Lookup the relocation (if any) at the given /p Offset.
const Relocation *getDynamicRelocationAt(uint64_t Offset) const {
- Relocation Key{Offset, 0, 0, 0, 0};
+ Relocation Key{Offset, 0, 0, 0, 0, 0};
auto Itr = DynamicRelocations.find(Key);
return Itr != DynamicRelocations.end() ? &*Itr : nullptr;
}
std::optional<Relocation> takeDynamicRelocationAt(uint64_t Offset) {
- Relocation Key{Offset, 0, 0, 0, 0};
+ Relocation Key{Offset, 0, 0, 0, 0, 0};
auto Itr = DynamicRelocations.find(Key);
if (Itr == DynamicRelocations.end())
diff --git a/bolt/include/bolt/Core/Relocation.h b/bolt/include/bolt/Core/Relocation.h
index 1f18dad008ca2..3a18249fa6bf9 100644
--- a/bolt/include/bolt/Core/Relocation.h
+++ b/bolt/include/bolt/Core/Relocation.h
@@ -47,6 +47,9 @@ struct Relocation {
/// Relocation type.
uint32_t Type;
+ /// Relocations added by optimizations can be optional.
+ bool Optional;
+
/// The offset from the \p Symbol base used to compute the final
/// value of this relocation.
uint64_t Addend;
diff --git a/bolt/lib/Core/BinaryContext.cpp b/bolt/lib/Core/BinaryContext.cpp
index 7466d2f8c91eb..f6d191eef5b41 100644
--- a/bolt/lib/Core/BinaryContext.cpp
+++ b/bolt/lib/Core/BinaryContext.cpp
@@ -2284,21 +2284,21 @@ ErrorOr<int64_t> BinaryContext::getSignedValueAtAddress(uint64_t Address,
}
void BinaryContext::addRelocation(uint64_t Address, MCSymbol *Symbol,
- uint32_t Type, uint64_t Addend,
+ uint32_t Type, bool Optional, uint64_t Addend,
uint64_t Value) {
ErrorOr<BinarySection &> Section = getSectionForAddress(Address);
assert(Section && "cannot find section for address");
- Section->addRelocation(Address - Section->getAddress(), Symbol, Type, Addend,
- Value);
+ Section->addRelocation(Address - Section->getAddress(), Symbol, Type,
+ Optional, Addend, Value);
}
void BinaryContext::addDynamicRelocation(uint64_t Address, MCSymbol *Symbol,
- uint32_t Type, uint64_t Addend,
- uint64_t Value) {
+ uint32_t Type, bool Optional,
+ uint64_t Addend, uint64_t Value) {
ErrorOr<BinarySection &> Section = getSectionForAddress(Address);
assert(Section && "cannot find section for address");
Section->addDynamicRelocation(Address - Section->getAddress(), Symbol, Type,
- Addend, Value);
+ Optional, Addend, Value);
}
bool BinaryContext::removeRelocationAt(uint64_t Address) {
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index acf04cee769c1..4b97aebe37c3c 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -4623,8 +4623,8 @@ bool BinaryFunction::isAArch64Veneer() const {
}
void BinaryFunction::addRelocation(uint64_t Address, MCSymbol *Symbol,
- uint32_t RelType, uint64_t Addend,
- uint64_t Value) {
+ uint32_t RelType, bool Optional,
+ uint64_t Addend, uint64_t Value) {
assert(Address >= getAddress() && Address < getAddress() + getMaxSize() &&
"address is outside of the function");
uint64_t Offset = Address - getAddress();
@@ -4635,7 +4635,7 @@ void BinaryFunction::addRelocation(uint64_t Address, MCSymbol *Symbol,
std::map<uint64_t, Relocation> &Rels =
IsCI ? Islands->Relocations : Relocations;
if (BC.MIB->shouldRecordCodeRelocation(RelType))
- Rels[Offset] = Relocation{Offset, Symbol, RelType, Addend, Value};
+ Rels[Offset] = Relocation{Offset, Symbol, RelType, Optional, Addend, Value};
}
} // namespace bolt
diff --git a/bolt/lib/Core/BinarySection.cpp b/bolt/lib/Core/BinarySection.cpp
index b16e0a4333aa2..a4968c27f3a25 100644
--- a/bolt/lib/Core/BinarySection.cpp
+++ b/bolt/lib/Core/BinarySection.cpp
@@ -49,8 +49,8 @@ BinarySection::hash(const BinaryData &BD,
uint64_t Offset = BD.getAddress() - getAddress();
const uint64_t EndOffset = BD.getEndAddress() - getAddress();
- auto Begin = Relocations.lower_bound(Relocation{Offset, 0, 0, 0, 0});
- auto End = Relocations.upper_bound(Relocation{EndOffset, 0, 0, 0, 0});
+ auto Begin = Relocations.lower_bound(Relocation{Offset, 0, 0, 0, 0, 0});
+ auto End = Relocations.upper_bound(Relocation{EndOffset, 0, 0, 0, 0, 0});
const StringRef Contents = getContents();
while (Begin != End) {
@@ -275,7 +275,8 @@ void BinarySection::reorderContents(const std::vector<BinaryData *> &Order,
// of the reordered segment to force LLVM to recognize and map this
// section.
MCSymbol *ZeroSym = BC.registerNameAtAddress("Zero", 0, 0, 0);
- addRelocation(OS.tell(), ZeroSym, Relocation::getAbs64(), 0xdeadbeef);
+ addRelocation(OS.tell(), ZeroSym, Relocation::getAbs64(),
+ /*Optional*/ false, 0xdeadbeef);
uint64_t Zero = 0;
OS.write(reinterpret_cast<const char *>(&Zero), sizeof(Zero));
diff --git a/bolt/lib/Core/JumpTable.cpp b/bolt/lib/Core/JumpTable.cpp
index 6f588d2b95fd6..b8124fc9dd129 100644
--- a/bolt/lib/Core/JumpTable.cpp
+++ b/bolt/lib/Core/JumpTable.cpp
@@ -92,7 +92,8 @@ void bolt::JumpTable::updateOriginal() {
// to the original jump table.
if (BC.HasRelocations)
getOutputSection().removeRelocationAt(EntryOffset);
- getOutputSection().addRelocation(EntryOffset, Entry, RelType, RelAddend);
+ getOutputSection().addRelocation(EntryOffset, Entry, RelType,
+ /*Optional*/ false, RelAddend);
EntryOffset += EntrySize;
}
}
diff --git a/bolt/lib/Rewrite/LinuxKernelRewriter.cpp b/bolt/lib/Rewrite/LinuxKernelRewriter.cpp
index 5a5e044184d0b..d142e002128e2 100644
--- a/bolt/lib/Rewrite/LinuxKernelRewriter.cpp
+++ b/bolt/lib/Rewrite/LinuxKernelRewriter.cpp
@@ -487,8 +487,8 @@ void LinuxKernelRewriter::processLKKSymtab(bool IsGPL) {
if (!BF)
continue;
- BC.addRelocation(EntryAddress, BF->getSymbol(), Relocation::getPC32(), 0,
- *Offset);
+ BC.addRelocation(EntryAddress, BF->getSymbol(), Relocation::getPC32(),
+ /*Optional*/ false, 0, *Offset);
}
}
@@ -559,7 +559,7 @@ void LinuxKernelRewriter::processInstructionFixups() {
Fixup.Section.addRelocation(Fixup.Offset, &Fixup.Label,
Fixup.IsPCRelative ? ELF::R_X86_64_PC32
: ELF::R_X86_64_64,
- /*Addend*/ 0);
+ /*Optional*/ false, /*Addend*/ 0);
}
}
@@ -828,7 +828,8 @@ Error LinuxKernelRewriter::rewriteORCTables() {
if (Label)
ORCUnwindIPSection->addRelocation(UnwindIPWriter.getOffset(), Label,
- Relocation::getPC32(), /*Addend*/ 0);
+ Relocation::getPC32(),
+ /*Optional*/ false, /*Addend*/ 0);
const int32_t IPValue =
IP - ORCUnwindIPSection->getAddress() - UnwindIPWriter.getOffset();
@@ -1074,7 +1075,8 @@ Error LinuxKernelRewriter::rewriteStaticCalls() {
StaticCallSection->getAddress() +
(Entry.ID - 1) * STATIC_CALL_ENTRY_SIZE;
StaticCallSection->addRelocation(EntryOffset, Entry.Label,
- ELF::R_X86_64_PC32, /*Addend*/ 0);
+ ELF::R_X86_64_PC32, /*Optional*/ false,
+ /*Addend*/ 0);
}
return Error::success();
@@ -1378,7 +1380,7 @@ Error LinuxKernelRewriter::rewriteBugTable() {
BC.MIB->getOrCreateInstLabel(Inst, "__BUG_", BC.Ctx.get());
const uint64_t EntryOffset = (ID - 1) * BUG_TABLE_ENTRY_SIZE;
BugTableSection->addRelocation(EntryOffset, Label, ELF::R_X86_64_PC32,
- /*Addend*/ 0);
+ /*Optional*/ false, /*Addend*/ 0);
}
}
@@ -1388,7 +1390,7 @@ Error LinuxKernelRewriter::rewriteBugTable() {
if (!EmittedIDs.count(ID)) {
const uint64_t EntryOffset = (ID - 1) * BUG_TABLE_ENTRY_SIZE;
BugTableSection->addRelocation(EntryOffset, nullptr, ELF::R_X86_64_PC32,
- /*Addend*/ 0);
+ /*Optional*/ false, /*Addend*/ 0);
}
}
}
@@ -1904,9 +1906,10 @@ Error LinuxKernelRewriter::rewriteStaticKeysJumpTable() {
(EntryID - 1) * 16;
StaticKeysJumpSection->addRelocation(EntryOffset, Label,
ELF::R_X86_64_PC32,
- /*Addend*/ 0);
+ /*Optional*/ false, /*Addend*/ 0);
StaticKeysJumpSection->addRelocation(EntryOffset + 4, Target,
- ELF::R_X86_64_PC32, /*Addend*/ 0);
+ ELF::R_X86_64_PC32,
+ /*Optional*/ false, /*Addend*/ 0);
}
}
}
diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index 560b30c6c676c..5c9fa0edf0f55 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -1420,7 +1420,7 @@ void RewriteInstance::updateRtFiniReloc() {
// desired value.
FiniArraySection->addPendingRelocation(Relocation{
/*Offset*/ 0, /*Symbol*/ nullptr, /*Type*/ Relocation::getAbs64(),
- /*Addend*/ RT->getRuntimeFiniAddress(), /*Value*/ 0});
+ /*Optional*/ false, /*Addend*/ RT->getRuntimeFiniAddress(), /*Value*/ 0});
}
void RewriteInstance::registerFragments() {
@@ -1919,7 +1919,8 @@ void RewriteInstance::relocateEHFrameSection() {
// Create a relocation against an absolute value since the goal is to
// preserve the contents of the section independent of the new values
// of referenced symbols.
- RelocatedEHFrameSection->addRelocation(Offset, nullptr, RelType, Value);
+ RelocatedEHFrameSection->addRelocation(Offset, nullptr, RelType,
+ /*Optional*/ false, Value);
};
Error E = EHFrameParser::parse(DE, EHFrameSection->getAddress(), createReloc);
@@ -2455,7 +2456,8 @@ void RewriteInstance::readDynamicRelocations(const SectionRef &Section,
if (Symbol)
SymbolIndex[Symbol] = getRelocationSymbol(InputFile, Rel);
- BC->addDynamicRelocation(Rel.getOffset(), Symbol, RType, Addend);
+ BC->addDynamicRelocation(Rel.getOffset(), Symbol, RType, /*Optional*/ false,
+ Addend);
}
}
@@ -2486,7 +2488,8 @@ void RewriteInstance::readDynamicRelrRelocations(BinarySection &Section) {
LLVM_DEBUG(dbgs() << "BOLT-DEBUG: R_*_RELATIVE relocation at 0x"
<< Twine::utohexstr(Address) << " to 0x"
<< Twine::utohexstr(Addend) << '\n';);
- BC->addDynamicRelocation(Address, nullptr, RType, Addend);
+ BC->addDynamicRelocation(Address, nullptr, RType, /*Optional*/ false,
+ Addend);
};
DataExtractor DE = DataExtractor(Section.getContents(),
@@ -2686,8 +2689,8 @@ void RewriteInstance::handleRelocation(const SectionRef &RelocatedSection,
// This might be a relocation for an ABS symbols like __global_pointer$ on
// RISC-V
ContainingBF->addRelocation(Rel.getOffset(), ReferencedSymbol,
- Relocation::getType(Rel), 0,
- cantFail(Symbol.getValue()));
+ Relocation::getType(Rel), /*Optional*/ false,
+ 0, cantFail(Symbol.getValue()));
return;
}
}
@@ -2719,7 +2722,7 @@ void RewriteInstance::handleRelocation(const SectionRef &RelocatedSection,
// the code. It's required to properly handle cases where
// "symbol + addend" references an object different from "symbol".
ContainingBF->addRelocation(Rel.getOffset(), ReferencedSymbol, RType,
- Addend, ExtractedValue);
+ /*Optional*/ false, Addend, ExtractedValue);
} else {
LLVM_DEBUG({
dbgs() << "BOLT-DEBUG: not creating PC-relative relocation at"
@@ -2953,10 +2956,10 @@ void RewriteInstance::handleRelocation(const SectionRef &RelocatedSection,
if (IsFromCode)
ContainingBF->addRelocation(Rel.getOffset(), ReferencedSymbol, RType,
- Addend, ExtractedValue);
+ /*Optional*/ false, Addend, ExtractedValue);
else if (IsToCode || ForceRelocation)
- BC->addRelocation(Rel.getOffset(), ReferencedSymbol, RType, Addend,
- ExtractedValue);
+ BC->addRelocation(Rel.getOffset(), ReferencedSymbol, RType,
+ /*Optional*/ false, Addend, ExtractedValue);
else
LLVM_DEBUG(dbgs() << "BOLT-DEBUG: ignoring relocation from data to data\n");
}
diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index 613b24c4553e2..54422eb6103b7 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -2291,7 +2291,8 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
auto [RelSymbol, RelAddend] = extractFixupExpr(Fixup);
- return Relocation({RelOffset, RelSymbol, RelType, RelAddend, 0});
+ return Relocation(
+ {RelOffset, RelSymbol, RelType, /*Optional*/ false, RelAddend, 0});
}
uint16_t getMinFunctionAlignment() const override { return 4; }
diff --git a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
index 51550857ae5ea..cb754af3943cb 100644
--- a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
+++ b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
@@ -2467,7 +2467,8 @@ class X86MCPlusBuilder : public MCPlusBuilder {
auto [RelSymbol, RelAddend] = extractFixupExpr(Fixup);
- return Relocation({RelOffset, RelSymbol, RelType, RelAddend, 0});
+ return Relocation(
+ {RelOffset, RelSymbol, RelType, /*Optional*/ false, RelAddend, 0});
}
bool replaceImmWithSymbolRef(MCInst &Inst, const MCSymbol *Symbol,
diff --git a/bolt/unittests/Core/BinaryContext.cpp b/bolt/unittests/Core/BinaryContext.cpp
index 09d16966334da..de041becedf4e 100644
--- a/bolt/unittests/Core/BinaryContext.cpp
+++ b/bolt/unittests/Core/BinaryContext.cpp
@@ -96,12 +96,12 @@ TEST_P(BinaryContextTester, FlushPendingRelocCALL26) {
DataSize, 4);
MCSymbol *RelSymbol1 = BC->getOrCreateGlobalSymbol(4, "Func1");
ASSERT_TRUE(RelSymbol1);
- BS.addPendingRelocation(
- Relocation{8, RelSymbol1, ELF::R_AARCH64_CALL26, 0, 0});
+ BS.addPendingRelocation(Relocation{8, RelSymbol1, ELF::R_AARCH64_CALL26,
+ /*Optional*/ false, 0, 0});
MCSymbol *RelSymbol2 = BC->getOrCreateGlobalSymbol(16, "Func2");
ASSERT_TRUE(RelSymbol2);
- BS.addPendingRelocation(
- Relocation{12, RelSymbol2, ELF::R_AARCH64_CALL26, 0, 0});
+ BS.addPendingRelocation(Relocation{12, RelSymbol2, ELF::R_AARCH64_CALL26,
+ /*Optional*/ false, 0, 0});
SmallVector<char> Vect(DataSize);
raw_svector_ostream OS(Vect);
@@ -138,12 +138,12 @@ TEST_P(BinaryContextTester, FlushPendingRelocJUMP26) {
(uint8_t *)Data, Size, 4);
MCSymbol *RelSymbol1 = BC->getOrCreateGlobalSymbol(4, "Func1");
ASSERT_TRUE(RelSymbol1);
- BS.addPendingRelocation(
- Relocation{8, RelSymbol1, ELF::R_AARCH64_JUMP26, 0, 0});
+ BS.addPendingRelocation(Relocation{8, RelSymbol1, ELF::R_AARCH64_JUMP26,
+ /*Optional*/ false, 0, 0});
MCSymbol *RelSymbol2 = BC->getOrCreateGlobalSymbol(16, "Func2");
ASSERT_TRUE(RelSymbol2);
- BS.addPendingRelocation(
- Relocation{12, RelSymbol2, ELF::R_AARCH64_JUMP26, 0, 0});
+ BS.addPendingRelocation(Relocation{12, RelSymbol2, ELF::R_AARCH64_JUMP26,
+ /*Optional*/ false, 0, 0});
SmallVector<char> Vect(Size);
raw_svector_ostream OS(Vect);
|
|
Since |
|
You mean default it to Sure, that should remove the changes in addRelocation calls. |
Yep. Thanks! |
|
Done. Since I added a setter to a struct, I'm inclined to change |
Yes, sounds good. And please add a constructor where you don't have to specify |
maksfb
reviewed
Mar 18, 2025
paschalis-mpeis
changed the title
|
Thanks Maksim. If no further comments from other reviewers I will be merging by ~ EOD. |
paschalis-mpeis
deleted the
users/paschalis-mpeis/bolt-optional-relocations
branch
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.
This patch converts
Relocationsfrom a struct to a class, and introduces theOptionalfield. Patch #116964 will use it.Some optimizations, like
scanExternalRefs, create relocations that patch the old code.Under certain circumstances these may be skipped without correctness implications.