-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[BOLT] Prevent addRelocation from adding pending relocs #123635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
paschalis-mpeis
merged 1 commit into
main
from
users/paschalis-mpeis/bolt-addrelocs-nopending
Feb 12, 2025
Merged
[BOLT] Prevent addRelocation from adding pending relocs #123635
paschalis-mpeis
merged 1 commit into
main
from
users/paschalis-mpeis/bolt-addrelocs-nopending
Feb 12, 2025
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
Member
|
@llvm/pr-subscribers-bolt Author: Paschalis Mpeis (paschalis-mpeis) ChangesUse addPendingRelocation for that. Full diff: https://github.com/llvm/llvm-project/pull/123635.diff 2 Files Affected:
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
+}
|
`addPendingRelocation` now becomes the only way to add a pending relocation. Can no longer use `addRelocation` for this. Update the only user (`BinaryContextTester`).
f235cee to
186a2ca
Compare
maksfb
approved these changes
Feb 7, 2025
Contributor
maksfb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please convert the title into imperative form before commit. Thanks!
Member
Author
|
Done. Thanks Maks. |
flovent
pushed a commit
to flovent/llvm-project
that referenced
this pull request
Feb 13, 2025
`addPendingRelocation` is the only way to add a pending relocation. Can no longer use `addRelocation` for this. Update the only user (`BinaryContextTester`).
joaosaffran
pushed a commit
to joaosaffran/llvm-project
that referenced
this pull request
Feb 14, 2025
`addPendingRelocation` is the only way to add a pending relocation. Can no longer use `addRelocation` for this. Update the only user (`BinaryContextTester`).
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Feb 24, 2025
`addPendingRelocation` is the only way to add a pending relocation. Can no longer use `addRelocation` for this. Update the only user (`BinaryContextTester`).
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.
addPendingRelocationnow becomes the only way to add a pendingrelocation. Can no longer use
addRelocationfor this.Update the only user (
BinaryContextTester).