Skip to content

[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

Conversation

paschalis-mpeis
Copy link
Member

@paschalis-mpeis paschalis-mpeis commented Jan 20, 2025

addPendingRelocation now becomes the only way to add a pending
relocation. Can no longer use addRelocation for this.
Update the only user (BinaryContextTester).

@paschalis-mpeis paschalis-mpeis marked this pull request as ready for review January 20, 2025 15:48
@llvmbot llvmbot added the BOLT label Jan 20, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 20, 2025

@llvm/pr-subscribers-bolt

Author: Paschalis Mpeis (paschalis-mpeis)

Changes

Use addPendingRelocation for that.


Full diff: https://github.com/llvm/llvm-project/pull/123635.diff

2 Files Affected:

  • (modified) bolt/include/bolt/Core/BinarySection.h (+2-8)
  • (modified) bolt/unittests/Core/BinaryContext.cpp (+9-7)
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`).
@paschalis-mpeis paschalis-mpeis force-pushed the users/paschalis-mpeis/bolt-addrelocs-nopending branch from f235cee to 186a2ca Compare January 20, 2025 16:05
Copy link
Contributor

@maksfb maksfb left a 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!

@paschalis-mpeis paschalis-mpeis changed the title [BOLT] addRelocation does not add pending relocs [BOLT] Prevent addRelocation from adding pending relocs Feb 11, 2025
@paschalis-mpeis
Copy link
Member Author

Done. Thanks Maks.

@paschalis-mpeis paschalis-mpeis merged commit 385af28 into main Feb 12, 2025
7 checks passed
@paschalis-mpeis paschalis-mpeis deleted the users/paschalis-mpeis/bolt-addrelocs-nopending branch February 12, 2025 15:24
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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants