[llvm] [BOLT] Skip out-of-range pending relocations (PR #116964)
Paschalis Mpeis via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 20 08:27:24 PST 2025
https://github.com/paschalis-mpeis updated https://github.com/llvm/llvm-project/pull/116964
>From 186a2cad9727b1bc080379ef7c65c748a019cf09 Mon Sep 17 00:00:00 2001
From: Paschalis Mpeis <Paschalis.Mpeis at arm.com>
Date: Mon, 20 Jan 2025 15:39:59 +0000
Subject: [PATCH 1/3] [BOLT] addRelocation does not add pending relocs
`addPendingRelocation` now becomes the only way to add a pending
relocation. Can no longer use `addRelocation` for this.
Update the only user (`BinaryContextTester`).
---
bolt/include/bolt/Core/BinarySection.h | 10 ++--------
bolt/unittests/Core/BinaryContext.cpp | 16 +++++++++-------
2 files changed, 11 insertions(+), 15 deletions(-)
diff --git a/bolt/include/bolt/Core/BinarySection.h b/bolt/include/bolt/Core/BinarySection.h
index d362961176b326..dedee361882497 100644
--- a/bolt/include/bolt/Core/BinarySection.h
+++ b/bolt/include/bolt/Core/BinarySection.h
@@ -358,15 +358,9 @@ class BinarySection {
/// Add a new relocation at the given /p Offset.
void addRelocation(uint64_t Offset, MCSymbol *Symbol, uint64_t Type,
- uint64_t Addend, uint64_t Value = 0,
- bool Pending = false) {
+ uint64_t Addend, uint64_t Value = 0) {
assert(Offset < getSize() && "offset not within section bounds");
- if (!Pending) {
- Relocations.emplace(Relocation{Offset, Symbol, Type, Addend, Value});
- } else {
- PendingRelocations.emplace_back(
- Relocation{Offset, Symbol, Type, Addend, Value});
- }
+ Relocations.emplace(Relocation{Offset, Symbol, Type, Addend, Value});
}
/// Add a dynamic relocation at the given /p Offset.
diff --git a/bolt/unittests/Core/BinaryContext.cpp b/bolt/unittests/Core/BinaryContext.cpp
index 05b898d34af56c..6e8ad4f62baeff 100644
--- a/bolt/unittests/Core/BinaryContext.cpp
+++ b/bolt/unittests/Core/BinaryContext.cpp
@@ -92,12 +92,13 @@ TEST_P(BinaryContextTester, FlushPendingRelocCALL26) {
DataSize, 4);
MCSymbol *RelSymbol1 = BC->getOrCreateGlobalSymbol(4, "Func1");
ASSERT_TRUE(RelSymbol1);
- BS.addRelocation(8, RelSymbol1, ELF::R_AARCH64_CALL26, 0, 0, true);
+ BS.addPendingRelocation(
+ Relocation{8, RelSymbol1, ELF::R_AARCH64_CALL26, 0, 0});
MCSymbol *RelSymbol2 = BC->getOrCreateGlobalSymbol(16, "Func2");
ASSERT_TRUE(RelSymbol2);
- BS.addRelocation(12, RelSymbol2, ELF::R_AARCH64_CALL26, 0, 0, true);
+ BS.addPendingRelocation(
+ Relocation{12, RelSymbol2, ELF::R_AARCH64_CALL26, 0, 0});
- std::error_code EC;
SmallVector<char> Vect(DataSize);
raw_svector_ostream OS(Vect);
@@ -133,12 +134,13 @@ TEST_P(BinaryContextTester, FlushPendingRelocJUMP26) {
(uint8_t *)Data, Size, 4);
MCSymbol *RelSymbol1 = BC->getOrCreateGlobalSymbol(4, "Func1");
ASSERT_TRUE(RelSymbol1);
- BS.addRelocation(8, RelSymbol1, ELF::R_AARCH64_JUMP26, 0, 0, true);
+ BS.addPendingRelocation(
+ Relocation{8, RelSymbol1, ELF::R_AARCH64_JUMP26, 0, 0});
MCSymbol *RelSymbol2 = BC->getOrCreateGlobalSymbol(16, "Func2");
ASSERT_TRUE(RelSymbol2);
- BS.addRelocation(12, RelSymbol2, ELF::R_AARCH64_JUMP26, 0, 0, true);
+ BS.addPendingRelocation(
+ Relocation{12, RelSymbol2, ELF::R_AARCH64_JUMP26, 0, 0});
- std::error_code EC;
SmallVector<char> Vect(Size);
raw_svector_ostream OS(Vect);
@@ -216,4 +218,4 @@ TEST_P(BinaryContextTester, BaseAddressSegmentsSmallerThanAlignment) {
BC->getBaseAddressForMapping(0xaaaaaaab1000, 0x1000);
ASSERT_TRUE(BaseAddress.has_value());
ASSERT_EQ(*BaseAddress, 0xaaaaaaaa0000ULL);
-}
\ No newline at end of file
+}
>From d34cb521b1ae133a1e49a1ec12c21e3e5c8b32ed Mon Sep 17 00:00:00 2001
From: Paschalis Mpeis <Paschalis.Mpeis at arm.com>
Date: Mon, 20 Jan 2025 16:08:02 +0000
Subject: [PATCH 2/3] BOLT asserts on out-of-range pending relocs
---
bolt/unittests/Core/BinaryContext.cpp | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/bolt/unittests/Core/BinaryContext.cpp b/bolt/unittests/Core/BinaryContext.cpp
index 6e8ad4f62baeff..4289395751dffa 100644
--- a/bolt/unittests/Core/BinaryContext.cpp
+++ b/bolt/unittests/Core/BinaryContext.cpp
@@ -157,6 +157,33 @@ TEST_P(BinaryContextTester, FlushPendingRelocJUMP26) {
<< "Wrong forward branch value\n";
}
+// TODO: BOLT currently asserts when a relocation is out of range.
+TEST_P(BinaryContextTester, FlushOptionalOutOfRangePendingRelocCALL26) {
+ if (GetParam() != Triple::aarch64)
+ GTEST_SKIP();
+
+ // This test checks that flushPendingRelocations skips flushing any optional
+ // pending relocations that cannot be encoded.
+
+ BinarySection &BS = BC->registerOrUpdateSection(
+ ".text", ELF::SHT_PROGBITS, ELF::SHF_EXECINSTR | ELF::SHF_ALLOC);
+ MCSymbol *RelSymbol = BC->getOrCreateGlobalSymbol(4, "Func");
+ ASSERT_TRUE(RelSymbol);
+ BS.addPendingRelocation(
+ Relocation{8, RelSymbol, ELF::R_AARCH64_CALL26, 0, 0});
+
+ SmallVector<char> Vect;
+ raw_svector_ostream OS(Vect);
+
+ // FIXME: bolt must be able to skip pending relocs that are out of range.
+ EXPECT_DEBUG_DEATH(
+ // Resolve relocation symbol to a high value so encoding will be out of
+ // range.
+ BS.flushPendingRelocations(OS,
+ [&](const MCSymbol *S) { return 0x800000F; }),
+ ".*only PC \\+/- 128MB is allowed for direct call");
+}
+
#endif
TEST_P(BinaryContextTester, BaseAddress) {
>From b3902db9e7c40fc6bb04cb671a2601d35a14e51d Mon Sep 17 00:00:00 2001
From: Paschalis Mpeis <Paschalis.Mpeis at arm.com>
Date: Mon, 20 Jan 2025 16:16:58 +0000
Subject: [PATCH 3/3] [BOLT] Skip flushing optional out-of-range pending relocs
When a pending relocation is created it is also marked whether it is
optional or not. It can be optional when such relocation is added as
part of an optimization (i.e., `scanExternalRefs`).
When bolt tries to `flushPendingRelocations`, it safely skips any
optional relocations that cannot be encoded.
Background:
BOLT, as part of scanExternalRefs, identifies external references from
calls and creates some pending relocations for them. Those when flushed
will update references to point to the optimized functions. This
optimization can be disabled using `--no-scan`.
BOLT can assert if any of these pending relocations cannot be encoded.
This patch does not disable this optimization but instead selectively
applies it given that a pending relocation is optional.
---
bolt/include/bolt/Core/BinarySection.h | 13 ++++++----
bolt/include/bolt/Core/Relocation.h | 5 +++-
bolt/lib/Core/BinaryFunction.cpp | 2 +-
bolt/lib/Core/BinarySection.cpp | 14 ++++++++++-
bolt/lib/Core/Relocation.cpp | 33 ++++++++++++++++++++++++++
bolt/unittests/Core/BinaryContext.cpp | 25 +++++++++++--------
6 files changed, 74 insertions(+), 18 deletions(-)
diff --git a/bolt/include/bolt/Core/BinarySection.h b/bolt/include/bolt/Core/BinarySection.h
index dedee361882497..37d0932a5c142e 100644
--- a/bolt/include/bolt/Core/BinarySection.h
+++ b/bolt/include/bolt/Core/BinarySection.h
@@ -66,8 +66,10 @@ class BinarySection {
// from the original section address.
RelocationSetType DynamicRelocations;
- // Pending relocations for this section.
- std::vector<Relocation> PendingRelocations;
+ /// Pending relocations for this section and whether they are optional, i.e.,
+ /// added as part of an optimization. In that case they can be safely omitted
+ /// if flushPendingRelocations discovers they cannot be encoded.
+ std::vector<std::pair<Relocation, bool>> PendingRelocations;
struct BinaryPatch {
uint64_t Offset;
@@ -374,9 +376,10 @@ class BinarySection {
DynamicRelocations.emplace(Reloc);
}
- /// Add relocation against the original contents of this section.
- void addPendingRelocation(const Relocation &Rel) {
- PendingRelocations.push_back(Rel);
+ /// Add relocation against the original contents of this section. When added
+ /// as part of an optimization it is marked as \p Optional.
+ void addPendingRelocation(const Relocation &Rel, bool Optional = false) {
+ PendingRelocations.push_back({Rel, Optional});
}
/// Add patch to the input contents of this section.
diff --git a/bolt/include/bolt/Core/Relocation.h b/bolt/include/bolt/Core/Relocation.h
index 933f62a31f8fd7..177cc0c70431f4 100644
--- a/bolt/include/bolt/Core/Relocation.h
+++ b/bolt/include/bolt/Core/Relocation.h
@@ -64,9 +64,12 @@ struct Relocation {
/// and \P Type mismatch occurred.
static bool skipRelocationProcess(uint64_t &Type, uint64_t Contents);
- // Adjust value depending on relocation type (make it PC relative or not)
+ /// Adjust value depending on relocation type (make it PC relative or not).
static uint64_t encodeValue(uint64_t Type, uint64_t Value, uint64_t PC);
+ /// Return true if there are enough bits to encode the relocation value.
+ static bool canEncodeValue(uint64_t Type, uint64_t Value, uint64_t PC);
+
/// Extract current relocated value from binary contents. This is used for
/// RISC architectures where values are encoded in specific bits depending
/// on the relocation value. For X86, we limit to sign extending the value
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 5da777411ba7a1..5d1e5ca92ca131 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1672,7 +1672,7 @@ bool BinaryFunction::scanExternalRefs() {
// Add relocations unless disassembly failed for this function.
if (!DisassemblyFailed)
for (Relocation &Rel : FunctionRelocations)
- getOriginSection()->addPendingRelocation(Rel);
+ getOriginSection()->addPendingRelocation(Rel, /*Optional*/ true);
// Inform BinaryContext that this function symbols will not be defined and
// relocations should not be created against them.
diff --git a/bolt/lib/Core/BinarySection.cpp b/bolt/lib/Core/BinarySection.cpp
index 9ad49ca1b3a038..a37cc7603df285 100644
--- a/bolt/lib/Core/BinarySection.cpp
+++ b/bolt/lib/Core/BinarySection.cpp
@@ -165,11 +165,19 @@ void BinarySection::flushPendingRelocations(raw_pwrite_stream &OS,
OS.pwrite(Patch.Bytes.data(), Patch.Bytes.size(),
SectionFileOffset + Patch.Offset);
- for (Relocation &Reloc : PendingRelocations) {
+ uint64_t SkippedPendingRelocations = 0;
+ for (auto &[Reloc, Optional] : PendingRelocations) {
uint64_t Value = Reloc.Addend;
if (Reloc.Symbol)
Value += Resolver(Reloc.Symbol);
+ // Safely skip any pending relocation that cannot be encoded and was added
+ // as part of an optimization.
+ if (Optional && !Relocation::canEncodeValue(
+ Reloc.Type, Value, SectionAddress + Reloc.Offset)) {
+ ++SkippedPendingRelocations;
+ continue;
+ }
Value = Relocation::encodeValue(Reloc.Type, Value,
SectionAddress + Reloc.Offset);
@@ -188,6 +196,10 @@ void BinarySection::flushPendingRelocations(raw_pwrite_stream &OS,
}
clearList(PendingRelocations);
+
+ if (SkippedPendingRelocations > 0)
+ LLVM_DEBUG(dbgs() << "BOLT-INFO: Skipped " << SkippedPendingRelocations
+ << " pending relocations as they were out of range\n");
}
BinarySection::~BinarySection() { updateContents(nullptr, 0); }
diff --git a/bolt/lib/Core/Relocation.cpp b/bolt/lib/Core/Relocation.cpp
index 4e888a5b147aca..3bc5c20ceb6739 100644
--- a/bolt/lib/Core/Relocation.cpp
+++ b/bolt/lib/Core/Relocation.cpp
@@ -361,6 +361,16 @@ static uint64_t encodeValueX86(uint64_t Type, uint64_t Value, uint64_t PC) {
return Value;
}
+static bool canEncodeValueAArch64(uint64_t Type, uint64_t Value, uint64_t PC) {
+ switch (Type) {
+ default:
+ llvm_unreachable("unsupported relocation");
+ case ELF::R_AARCH64_CALL26:
+ case ELF::R_AARCH64_JUMP26:
+ return isInt<28>(Value - PC);
+ }
+}
+
static uint64_t encodeValueAArch64(uint64_t Type, uint64_t Value, uint64_t PC) {
switch (Type) {
default:
@@ -393,6 +403,16 @@ static uint64_t encodeValueAArch64(uint64_t Type, uint64_t Value, uint64_t PC) {
return Value;
}
+static uint64_t canEncodeValueRISCV(uint64_t Type, uint64_t Value,
+ uint64_t PC) {
+ switch (Type) {
+ default:
+ llvm_unreachable("unsupported relocation");
+ case ELF::R_RISCV_64:
+ return true;
+ }
+}
+
static uint64_t encodeValueRISCV(uint64_t Type, uint64_t Value, uint64_t PC) {
switch (Type) {
default:
@@ -838,6 +858,19 @@ uint64_t Relocation::encodeValue(uint64_t Type, uint64_t Value, uint64_t PC) {
}
}
+bool Relocation::canEncodeValue(uint64_t Type, uint64_t Value, uint64_t PC) {
+ switch (Arch) {
+ default:
+ llvm_unreachable("Unsupported architecture");
+ case Triple::aarch64:
+ return canEncodeValueAArch64(Type, Value, PC);
+ case Triple::riscv64:
+ return canEncodeValueRISCV(Type, Value, PC);
+ case Triple::x86_64:
+ return true;
+ }
+}
+
uint64_t Relocation::extractValue(uint64_t Type, uint64_t Contents,
uint64_t PC) {
switch (Arch) {
diff --git a/bolt/unittests/Core/BinaryContext.cpp b/bolt/unittests/Core/BinaryContext.cpp
index 4289395751dffa..9ba0bba16c9f35 100644
--- a/bolt/unittests/Core/BinaryContext.cpp
+++ b/bolt/unittests/Core/BinaryContext.cpp
@@ -157,7 +157,6 @@ TEST_P(BinaryContextTester, FlushPendingRelocJUMP26) {
<< "Wrong forward branch value\n";
}
-// TODO: BOLT currently asserts when a relocation is out of range.
TEST_P(BinaryContextTester, FlushOptionalOutOfRangePendingRelocCALL26) {
if (GetParam() != Triple::aarch64)
GTEST_SKIP();
@@ -165,23 +164,29 @@ TEST_P(BinaryContextTester, FlushOptionalOutOfRangePendingRelocCALL26) {
// This test checks that flushPendingRelocations skips flushing any optional
// pending relocations that cannot be encoded.
+ bool DebugFlagPrev = ::llvm::DebugFlag;
+ ::llvm::DebugFlag = true;
+ testing::internal::CaptureStderr();
+
BinarySection &BS = BC->registerOrUpdateSection(
".text", ELF::SHT_PROGBITS, ELF::SHF_EXECINSTR | ELF::SHF_ALLOC);
MCSymbol *RelSymbol = BC->getOrCreateGlobalSymbol(4, "Func");
ASSERT_TRUE(RelSymbol);
- BS.addPendingRelocation(
- Relocation{8, RelSymbol, ELF::R_AARCH64_CALL26, 0, 0});
+ BS.addPendingRelocation(Relocation{8, RelSymbol, ELF::R_AARCH64_CALL26, 0, 0},
+ /*Optional*/ true);
SmallVector<char> Vect;
raw_svector_ostream OS(Vect);
- // FIXME: bolt must be able to skip pending relocs that are out of range.
- EXPECT_DEBUG_DEATH(
- // Resolve relocation symbol to a high value so encoding will be out of
- // range.
- BS.flushPendingRelocations(OS,
- [&](const MCSymbol *S) { return 0x800000F; }),
- ".*only PC \\+/- 128MB is allowed for direct call");
+ // Resolve relocation symbol to a high value so encoding will be out of range.
+ BS.flushPendingRelocations(OS, [&](const MCSymbol *S) { return 0x800000F; });
+
+ std::string CapturedStdErr = testing::internal::GetCapturedStderr();
+ EXPECT_TRUE(CapturedStdErr.find("BOLT-INFO: Skipped 1 pending relocations as "
+ "they were out of range") !=
+ std::string::npos);
+
+ ::llvm::DebugFlag = DebugFlagPrev;
}
#endif
More information about the llvm-commits
mailing list