minor improvement on error handling

Created using spr 1.3.7

Author: cachemeifyoucan

bolt/include/bolt/Core/BinaryFunction.h

@@ -192,9 +192,6 @@ class BinaryFunction {
 
     mutable MCSymbol *FunctionConstantIslandLabel{nullptr};
     mutable MCSymbol *FunctionColdConstantIslandLabel{nullptr};
-
-    // Returns constant island alignment
-    uint16_t getAlignment() const { return sizeof(uint64_t); }
   };
 
   static constexpr uint64_t COUNT_NO_PROFILE =
@@ -2114,9 +2111,7 @@ class BinaryFunction {
     return *std::prev(CodeIter) <= *DataIter;
   }
 
-  uint16_t getConstantIslandAlignment() const {
-    return Islands ? Islands->getAlignment() : 1;
-  }
+  uint16_t getConstantIslandAlignment() const;
 
   /// If there is a constant island in the range [StartOffset, EndOffset),
   /// return its address.
@@ -2168,6 +2163,11 @@ class BinaryFunction {
     return Islands && !Islands->DataOffsets.empty();
   }
 
+  /// Return true if the whole function is a constant island.
+  bool isDataObject() const {
+    return Islands && Islands->CodeOffsets.size() == 0;
+  }
+
   bool isStartOfConstantIsland(uint64_t Offset) const {
     return hasConstantIsland() && Islands->DataOffsets.count(Offset);
   }

bolt/include/bolt/Core/MCPlusBuilder.h

@@ -51,6 +51,7 @@ class raw_ostream;
 
 namespace bolt {
 class BinaryBasicBlock;
+class BinaryContext;
 class BinaryFunction;
 
 /// Different types of indirect branches encountered during disassembly.
@@ -530,10 +531,15 @@ class MCPlusBuilder {
     return 0;
   }
 
+  /// Create a helper function to increment counter for Instrumentation
+  virtual void createInstrCounterIncrFunc(BinaryContext &BC) {
+    llvm_unreachable("not implemented");
+  }
+
   /// Create increment contents of target by 1 for Instrumentation
-  virtual InstructionListType
-  createInstrIncMemory(const MCSymbol *Target, MCContext *Ctx, bool IsLeaf,
-                       unsigned CodePointerSize) const {
+  virtual InstructionListType createInstrIncMemory(const MCSymbol *Target,
+                                                   MCContext *Ctx, bool IsLeaf,
+                                                   unsigned CodePointerSize) {
     llvm_unreachable("not implemented");
     return InstructionListType();
   }

bolt/lib/Core/BinaryFunction.cpp

@@ -284,6 +284,33 @@ BinaryFunction::getBasicBlockContainingOffset(uint64_t Offset) {
   return (Offset < BB->getOffset() + BB->getOriginalSize()) ? BB : nullptr;
 }
 
+uint16_t BinaryFunction::getConstantIslandAlignment() const {
+  if (Islands == nullptr)
+    return 1;
+
+  // For constant island inside a function, the default 8-byte alignment is
+  // probably good enough.
+  const uint16_t DefaultAlignment = sizeof(uint64_t);
+  if (!isDataObject())
+    return DefaultAlignment;
+
+  // If the constant island itself is a binary function, get its alignment
+  // based on its size, original address, and its owning section's alignment.
+  const uint64_t MaxAlignment =
+      std::min(uint64_t(1) << llvm::countr_zero(getAddress()),
+               OriginSection->getAlignment());
+  const uint64_t MinAlignment =
+      std::max((uint64_t)DefaultAlignment,
+               uint64_t(1) << (63 - llvm::countl_zero(getSize())));
+  uint64_t Alignment = std::min(MinAlignment, MaxAlignment);
+  if (Alignment >> 16) {
+    BC.errs() << "BOLT-ERROR: the constant island's alignment is too big: 0x"
+              << Twine::utohexstr(Alignment) << "\n";
+    exit(1);
+  }
+  return (uint16_t)Alignment;
+}
+
 void BinaryFunction::markUnreachableBlocks() {
   std::stack<BinaryBasicBlock *> Stack;
 

bolt/lib/Core/GDBIndex.cpp

@@ -77,7 +77,8 @@ void GDBIndex::updateGdbIndexSection(
     exit(1);
   }
   DenseSet<uint64_t> OriginalOffsets;
-  for (unsigned Index = 0, Units = BC.DwCtx->getNumCompileUnits();
+  for (unsigned Index = 0, PresentUnitsIndex = 0,
+                Units = BC.DwCtx->getNumCompileUnits();
        Index < Units; ++Index) {
     const DWARFUnit *CU = BC.DwCtx->getUnitAtIndex(Index);
     if (SkipTypeUnits && CU->isTypeUnit())
@@ -90,7 +91,7 @@ void GDBIndex::updateGdbIndexSection(
     }
 
     OriginalOffsets.insert(Offset);
-    OffsetToIndexMap[Offset] = Index;
+    OffsetToIndexMap[Offset] = PresentUnitsIndex++;
   }
 
   // Ignore old address table.
@@ -125,16 +126,52 @@ void GDBIndex::updateGdbIndexSection(
 
   using MapEntry = std::pair<uint32_t, CUInfo>;
   std::vector<MapEntry> CUVector(CUMap.begin(), CUMap.end());
+  // Remove the CUs we won't emit anyway.
+  CUVector.erase(std::remove_if(CUVector.begin(), CUVector.end(),
+                                [&OriginalOffsets](const MapEntry &It) {
+                                  // Skipping TU for DWARF5 when they are not
+                                  // included in CU list.
+                                  return OriginalOffsets.count(It.first) == 0;
+                                }),
+                 CUVector.end());
   // Need to sort since we write out all of TUs in .debug_info before CUs.
   std::sort(CUVector.begin(), CUVector.end(),
             [](const MapEntry &E1, const MapEntry &E2) -> bool {
               return E1.second.Offset < E2.second.Offset;
             });
+  // Create the original CU index -> updated CU index mapping,
+  // as the sort above could've changed the order and we have to update
+  // indices correspondingly in address map and constant pool.
+  std::unordered_map<uint32_t, uint32_t> OriginalCUIndexToUpdatedCUIndexMap;
+  OriginalCUIndexToUpdatedCUIndexMap.reserve(CUVector.size());
+  for (uint32_t I = 0; I < CUVector.size(); ++I) {
+    OriginalCUIndexToUpdatedCUIndexMap[OffsetToIndexMap.at(CUVector[I].first)] =
+        I;
+  }
+  const auto RemapCUIndex = [&OriginalCUIndexToUpdatedCUIndexMap,
+                             CUVectorSize = CUVector.size(),
+                             TUVectorSize = getGDBIndexTUEntryVector().size()](
+                                uint32_t OriginalIndex) {
+    if (OriginalIndex >= CUVectorSize) {
+      if (OriginalIndex >= CUVectorSize + TUVectorSize) {
+        errs() << "BOLT-ERROR: .gdb_index unknown CU index\n";
+        exit(1);
+      }
+      // The index is into TU CU List, which we don't reorder, so return as is.
+      return OriginalIndex;
+    }
+
+    const auto It = OriginalCUIndexToUpdatedCUIndexMap.find(OriginalIndex);
+    if (It == OriginalCUIndexToUpdatedCUIndexMap.end()) {
+      errs() << "BOLT-ERROR: .gdb_index unknown CU index\n";
+      exit(1);
+    }
+
+    return It->second;
+  };
+
   // Writing out CU List <Offset, Size>
   for (auto &CUInfo : CUVector) {
-    // Skipping TU for DWARF5 when they are not included in CU list.
-    if (!OriginalOffsets.count(CUInfo.first))
-      continue;
     write64le(Buffer, CUInfo.second.Offset);
     // Length encoded in CU doesn't contain first 4 bytes that encode length.
     write64le(Buffer + 8, CUInfo.second.Length + 4);
@@ -160,12 +197,13 @@ void GDBIndex::updateGdbIndexSection(
   // Generate new address table.
   for (const std::pair<const uint64_t, DebugAddressRangesVector> &CURangesPair :
        ARangesSectionWriter.getCUAddressRanges()) {
-    const uint32_t CUIndex = OffsetToIndexMap[CURangesPair.first];
+    const uint32_t OriginalCUIndex = OffsetToIndexMap[CURangesPair.first];
+    const uint32_t UpdatedCUIndex = RemapCUIndex(OriginalCUIndex);
     const DebugAddressRangesVector &Ranges = CURangesPair.second;
     for (const DebugAddressRange &Range : Ranges) {
       write64le(Buffer, Range.LowPC);
       write64le(Buffer + 8, Range.HighPC);
-      write32le(Buffer + 16, CUIndex);
+      write32le(Buffer + 16, UpdatedCUIndex);
       Buffer += 20;
     }
   }
@@ -178,6 +216,56 @@ void GDBIndex::updateGdbIndexSection(
   // Copy over the rest of the original data.
   memcpy(Buffer, Data, TrailingSize);
 
+  // Fixup CU-indices in constant pool.
+  const char *const OriginalConstantPoolData =
+      GdbIndexContents.data() + ConstantPoolOffset;
+  uint8_t *const UpdatedConstantPoolData =
+      NewGdbIndexContents + ConstantPoolOffset + Delta;
+
+  const char *OriginalSymbolTableData =
+      GdbIndexContents.data() + SymbolTableOffset;
+  std::set<uint32_t> CUVectorOffsets;
+  // Parse the symbol map and extract constant pool CU offsets from it.
+  while (OriginalSymbolTableData < OriginalConstantPoolData) {
+    const uint32_t NameOffset = read32le(OriginalSymbolTableData);
+    const uint32_t CUVectorOffset = read32le(OriginalSymbolTableData + 4);
+    OriginalSymbolTableData += 8;
+
+    // Iff both are zero, then the slot is considered empty in the hash-map.
+    if (NameOffset || CUVectorOffset) {
+      CUVectorOffsets.insert(CUVectorOffset);
+    }
+  }
+
+  // Update the CU-indicies in the constant pool
+  for (const auto CUVectorOffset : CUVectorOffsets) {
+    const char *CurrentOriginalConstantPoolData =
+        OriginalConstantPoolData + CUVectorOffset;
+    uint8_t *CurrentUpdatedConstantPoolData =
+        UpdatedConstantPoolData + CUVectorOffset;
+
+    const uint32_t Num = read32le(CurrentOriginalConstantPoolData);
+    CurrentOriginalConstantPoolData += 4;
+    CurrentUpdatedConstantPoolData += 4;
+
+    for (uint32_t J = 0; J < Num; ++J) {
+      const uint32_t OriginalCUIndexAndAttributes =
+          read32le(CurrentOriginalConstantPoolData);
+      CurrentOriginalConstantPoolData += 4;
+
+      // We only care for the index, which is the lowest 24 bits, other bits are
+      // left as is.
+      const uint32_t OriginalCUIndex =
+          OriginalCUIndexAndAttributes & ((1 << 24) - 1);
+      const uint32_t Attributes = OriginalCUIndexAndAttributes >> 24;
+      const uint32_t UpdatedCUIndexAndAttributes =
+          RemapCUIndex(OriginalCUIndex) | (Attributes << 24);
+
+      write32le(CurrentUpdatedConstantPoolData, UpdatedCUIndexAndAttributes);
+      CurrentUpdatedConstantPoolData += 4;
+    }
+  }
+
   // Register the new section.
   BC.registerOrUpdateNoteSection(".gdb_index", NewGdbIndexContents,
                                  NewGdbIndexSize);

bolt/lib/Passes/Instrumentation.cpp

@@ -753,6 +753,8 @@ void Instrumentation::createAuxiliaryFunctions(BinaryContext &BC) {
       createSimpleFunction("__bolt_fini_trampoline",
                            BC.MIB->createReturnInstructionList(BC.Ctx.get()));
     }
+    if (BC.isAArch64())
+      BC.MIB->createInstrCounterIncrFunc(BC);
   }
 }
 

bolt/lib/Rewrite/RewriteInstance.cpp

@@ -1312,7 +1312,9 @@ void RewriteInstance::discoverFileObjects() {
 
   // Annotate functions with code/data markers in AArch64
   for (auto &[Address, Type] : MarkerSymbols) {
-    auto *BF = BC->getBinaryFunctionContainingAddress(Address, true, true);
+    auto *BF = BC->getBinaryFunctionContainingAddress(Address,
+                                                      /*CheckPastEnd*/ false,
+                                                      /*UseMaxSize*/ true);
 
     if (!BF) {
       // Stray marker