Addressing reviewers

Author: paschalis-mpeis

bolt/include/bolt/Core/BinarySection.h

@@ -66,10 +66,8 @@ class BinarySection {
   // from the original section address.
   RelocationSetType DynamicRelocations;
 
-  /// 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;
+  // Pending relocations for this section.
+  std::vector<Relocation> PendingRelocations;
 
   struct BinaryPatch {
     uint64_t Offset;
@@ -377,10 +375,9 @@ class BinarySection {
     DynamicRelocations.emplace(Reloc);
   }
 
-  /// 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 relocation against the original contents of this section.
+  void addPendingRelocation(const Relocation &Rel) {
+    PendingRelocations.push_back(Rel);
   }
 
   /// Add patch to the input contents of this section.

bolt/lib/Core/BinaryFunction.cpp

@@ -1689,6 +1689,8 @@ bool BinaryFunction::scanExternalRefs() {
     // Create relocation for every fixup.
     for (const MCFixup &Fixup : Fixups) {
       std::optional<Relocation> Rel = BC.MIB->createRelocation(Fixup, *BC.MAB);
+      // Can be skipped under the right circumstances.
+      Rel->setOptional();
       if (!Rel) {
         Success = false;
         continue;
@@ -1718,7 +1720,7 @@ bool BinaryFunction::scanExternalRefs() {
   // Add relocations unless disassembly failed for this function.
   if (!DisassemblyFailed)
     for (Relocation &Rel : FunctionRelocations)
-      getOriginSection()->addPendingRelocation(Rel, /*Optional*/ true);
+      getOriginSection()->addPendingRelocation(Rel);
 
   // Inform BinaryContext that this function symbols will not be defined and
   // relocations should not be created against them.

bolt/lib/Core/BinarySection.cpp

@@ -176,15 +176,15 @@ void BinarySection::flushPendingRelocations(raw_pwrite_stream &OS,
               SectionFileOffset + Patch.Offset);
 
   uint64_t SkippedPendingRelocations = 0;
-  for (auto &[Reloc, Optional] : PendingRelocations) {
+  for (Relocation &Reloc : 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)) {
+    // Safely skip any optional pending relocation that cannot be encoded.
+    if (Reloc.isOptional() &&
+        !Relocation::canEncodeValue(Reloc.Type, Value,
+                                    SectionAddress + Reloc.Offset)) {
 
       // A successful run of 'scanExternalRefs' means that all pending
       // relocations are flushed. Otherwise, PatchEntries should run.
@@ -219,9 +219,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");
+  if (SkippedPendingRelocations > 0 && opts::Verbosity >= 1) {
+    BC.outs() << "BOLT-INFO: skipped " << SkippedPendingRelocations
+              << " out-of-range optional relocations\n";
+  }
 }
 
 BinarySection::~BinarySection() { updateContents(nullptr, 0); }

bolt/unittests/Core/BinaryContext.cpp

@@ -177,8 +177,9 @@ TEST_P(BinaryContextTester,
   // Create symbol 'Func0x4'
   MCSymbol *RelSymbol = BC->getOrCreateGlobalSymbol(4, "Func");
   ASSERT_TRUE(RelSymbol);
-  BS.addPendingRelocation(Relocation{8, RelSymbol, ELF::R_AARCH64_CALL26, 0, 0},
-                          /*Optional*/ true);
+  Relocation Reloc{8, RelSymbol, ELF::R_AARCH64_CALL26, 0, 0};
+  Reloc.setOptional();
+  BS.addPendingRelocation(Reloc);
 
   SmallVector<char> Vect;
   raw_svector_ostream OS(Vect);
@@ -201,29 +202,26 @@ TEST_P(BinaryContextTester,
   // relocations that cannot be encoded, given that PatchEntries runs.
   opts::ForcePatch = true;
 
-  bool DebugFlagPrev = ::llvm::DebugFlag;
-  ::llvm::DebugFlag = true;
-  testing::internal::CaptureStderr();
+  opts::Verbosity = 1;
+  testing::internal::CaptureStdout();
 
   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},
-                          /*Optional*/ true);
+  Relocation Reloc{8, RelSymbol, ELF::R_AARCH64_CALL26, 0, 0};
+  Reloc.setOptional();
+  BS.addPendingRelocation(Reloc);
 
   SmallVector<char> Vect;
   raw_svector_ostream OS(Vect);
 
   // 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;
+  outs().flush();
+  std::string CapturedStdOut = testing::internal::GetCapturedStdout();
+  EXPECT_EQ(CapturedStdOut,
+            "BOLT-INFO: skipped 1 out-of-range optional relocations\n");
 }
 
 #endif