[BOLT][NFCI] Use MetadataRewriter interface to update SDT markers

Migrate SDT markers processing to the new MetadataRewriter interface.

Depends on D154020

Reviewed By: Amir

Differential Revision: https://reviews.llvm.org/D154021

Author: maksfb

bolt/include/bolt/Core/BinaryBasicBlock.h

@@ -103,7 +103,7 @@ class BinaryBasicBlock {
   /// After output/codegen, map output offsets of instructions in this basic
   /// block to instruction offsets in the original function. Note that the
   /// output basic block could be different from the input basic block.
-  /// We only map instruction of interest, such as calls, and sdt markers.
+  /// We only map instruction of interest, such as calls and markers.
   ///
   /// We store the offset array in a basic block to facilitate BAT tables
   /// generation. Otherwise, the mapping could be done at function level.

bolt/include/bolt/Core/BinaryContext.h

@@ -673,9 +673,6 @@ class BinaryContext {
   /// List of functions that always trap.
   std::vector<const BinaryFunction *> TrappedFunctions;
 
-  /// Map SDT locations to SDT markers info
-  std::unordered_map<uint64_t, SDTMarkerInfo> SDTMarkers;
-
   /// Map linux kernel program locations/instructions to their pointers in
   /// special linux kernel sections
   std::unordered_map<uint64_t, std::vector<LKInstructionMarkerInfo>> LKMarkers;

bolt/include/bolt/Core/BinaryFunction.h

@@ -1702,6 +1702,8 @@ class BinaryFunction {
   /// Indicate that another function body was merged with this function.
   void setHasFunctionsFoldedInto() { HasFunctionsFoldedInto = true; }
 
+  void setHasSDTMarker(bool V) { HasSDTMarker = V; }
+
   BinaryFunction &setPersonalityFunction(uint64_t Addr) {
     assert(!PersonalityFunction && "can't set personality function twice");
     PersonalityFunction = BC.getOrCreateGlobalSymbol(Addr, "FUNCat");

bolt/include/bolt/Core/BinarySection.h

@@ -509,18 +509,6 @@ inline raw_ostream &operator<<(raw_ostream &OS, const BinarySection &Section) {
   return OS;
 }
 
-struct SDTMarkerInfo {
-  uint64_t PC;
-  uint64_t Base;
-  uint64_t Semaphore;
-  StringRef Provider;
-  StringRef Name;
-  StringRef Args;
-
-  /// The offset of PC within the note section
-  unsigned PCOffset;
-};
-
 /// Linux Kernel special sections point to a specific instruction in many cases.
 /// Unlike SDTMarkerInfo, these markers can come from different sections.
 struct LKInstructionMarkerInfo {

bolt/include/bolt/Rewrite/MetadataRewriters.h

@@ -15,9 +15,11 @@ namespace llvm {
 namespace bolt {
 
 class MetadataRewriter;
+class BinaryContext;
 
-/// The list of rewriter build functions (e.g. createNewMetadataRewriter()) that
-/// return std::unique_ptr<MetadataRewriter>.
+// The list of rewriter build functions.
+
+std::unique_ptr<MetadataRewriter> createSDTRewriter(BinaryContext &);
 
 } // namespace bolt
 } // namespace llvm

bolt/include/bolt/Rewrite/RewriteInstance.h

@@ -198,9 +198,6 @@ class RewriteInstance {
   /// Update debug and other auxiliary information in the file.
   void updateMetadata();
 
-  /// Update SDTMarkers' locations for the output binary.
-  void updateSDTMarkers();
-
   /// Update LKMarkers' locations for the output binary.
   void updateLKMarkers();
 
@@ -398,16 +395,10 @@ class RewriteInstance {
   /// of appending contents to it.
   bool willOverwriteSection(StringRef SectionName);
 
-  /// Parse .note.stapsdt section
-  void parseSDTNotes();
-
   /// Parse .pseudo_probe_desc section and .pseudo_probe section
   /// Setup Pseudo probe decoder
   void parsePseudoProbe();
 
-  /// Print all SDT markers
-  void printSDTMarkers();
-
 public:
   /// Standard ELF sections we overwrite.
   static constexpr const char *SectionsToOverwrite[] = {
@@ -589,10 +580,6 @@ class RewriteInstance {
   /// .note.gnu.build-id section.
   ErrorOr<BinarySection &> BuildIDSection{std::errc::bad_address};
 
-  /// .note.stapsdt section.
-  /// Contains information about statically defined tracing points
-  ErrorOr<BinarySection &> SDTSection{std::errc::bad_address};
-
   /// .pseudo_probe_desc section.
   /// Contains information about pseudo probe description, like its related
   /// function

bolt/lib/Core/BinaryFunction.cpp

@@ -1988,19 +1988,20 @@ bool BinaryFunction::buildCFG(MCPlusBuilder::AllocatorIdTy AllocatorId) {
         updateOffset(LastInstrOffset);
     }
 
-    const uint64_t InstrInputAddr = I->first + Address;
-    bool IsSDTMarker =
-        MIB->isNoop(Instr) && BC.SDTMarkers.count(InstrInputAddr);
-    bool IsLKMarker = BC.LKMarkers.count(InstrInputAddr);
+    bool IsLKMarker = BC.LKMarkers.count(I->first + Address);
     // Mark all nops with Offset for profile tracking purposes.
     if (MIB->isNoop(Instr) || IsLKMarker) {
-      if (!MIB->getOffset(Instr))
+      // If "Offset" annotation is not present, set it and mark the nop for
+      // deletion.
+      if (!MIB->getOffset(Instr)) {
         MIB->setOffset(Instr, static_cast<uint32_t>(Offset), AllocatorId);
-      if (IsSDTMarker || IsLKMarker)
-        HasSDTMarker = true;
-      else
         // Annotate ordinary nops, so we can safely delete them if required.
-        MIB->addAnnotation(Instr, "NOP", static_cast<uint32_t>(1), AllocatorId);
+        if (!IsLKMarker)
+          MIB->addAnnotation(Instr, "NOP", static_cast<uint32_t>(1),
+                             AllocatorId);
+      }
+      if (IsLKMarker)
+        HasSDTMarker = true;
     }
 
     if (!InsertBB) {

bolt/lib/Rewrite/CMakeLists.txt

@@ -17,6 +17,7 @@ add_llvm_library(LLVMBOLTRewrite
   MachORewriteInstance.cpp
   MetadataManager.cpp
   RewriteInstance.cpp
+  SDTRewriter.cpp
 
   DISABLE_LLVM_LINK_LLVM_DYLIB
 

bolt/lib/Rewrite/RewriteInstance.cpp

@@ -196,10 +196,6 @@ static cl::opt<bool> PrintLoopInfo("print-loops",
                                    cl::desc("print loop related information"),
                                    cl::Hidden, cl::cat(BoltCategory));
 
-static cl::opt<bool> PrintSDTMarkers("print-sdt",
-                                     cl::desc("print all SDT markers"),
-                                     cl::Hidden, cl::cat(BoltCategory));
-
 enum PrintPseudoProbesOptions {
   PPP_None = 0,
   PPP_Probes_Section_Decode = 0x1,
@@ -634,51 +630,6 @@ Error RewriteInstance::discoverStorage() {
   return Error::success();
 }
 
-void RewriteInstance::parseSDTNotes() {
-  if (!SDTSection)
-    return;
-
-  StringRef Buf = SDTSection->getContents();
-  DataExtractor DE = DataExtractor(Buf, BC->AsmInfo->isLittleEndian(),
-                                   BC->AsmInfo->getCodePointerSize());
-  uint64_t Offset = 0;
-
-  while (DE.isValidOffset(Offset)) {
-    uint32_t NameSz = DE.getU32(&Offset);
-    DE.getU32(&Offset); // skip over DescSz
-    uint32_t Type = DE.getU32(&Offset);
-    Offset = alignTo(Offset, 4);
-
-    if (Type != 3)
-      errs() << "BOLT-WARNING: SDT note type \"" << Type
-             << "\" is not expected\n";
-
-    if (NameSz == 0)
-      errs() << "BOLT-WARNING: SDT note has empty name\n";
-
-    StringRef Name = DE.getCStr(&Offset);
-
-    if (!Name.equals("stapsdt"))
-      errs() << "BOLT-WARNING: SDT note name \"" << Name
-             << "\" is not expected\n";
-
-    // Parse description
-    SDTMarkerInfo Marker;
-    Marker.PCOffset = Offset;
-    Marker.PC = DE.getU64(&Offset);
-    Marker.Base = DE.getU64(&Offset);
-    Marker.Semaphore = DE.getU64(&Offset);
-    Marker.Provider = DE.getCStr(&Offset);
-    Marker.Name = DE.getCStr(&Offset);
-    Marker.Args = DE.getCStr(&Offset);
-    Offset = alignTo(Offset, 4);
-    BC->SDTMarkers[Marker.PC] = Marker;
-  }
-
-  if (opts::PrintSDTMarkers)
-    printSDTMarkers();
-}
-
 void RewriteInstance::parsePseudoProbe() {
   if (!PseudoProbeDescSection && !PseudoProbeSection) {
     // pesudo probe is not added to binary. It is normal and no warning needed.
@@ -728,19 +679,6 @@ void RewriteInstance::parsePseudoProbe() {
   }
 }
 
-void RewriteInstance::printSDTMarkers() {
-  outs() << "BOLT-INFO: Number of SDT markers is " << BC->SDTMarkers.size()
-         << "\n";
-  for (auto It : BC->SDTMarkers) {
-    SDTMarkerInfo &Marker = It.second;
-    outs() << "BOLT-INFO: PC: " << utohexstr(Marker.PC)
-           << ", Base: " << utohexstr(Marker.Base)
-           << ", Semaphore: " << utohexstr(Marker.Semaphore)
-           << ", Provider: " << Marker.Provider << ", Name: " << Marker.Name
-           << ", Args: " << Marker.Args << "\n";
-  }
-}
-
 void RewriteInstance::parseBuildID() {
   if (!BuildIDSection)
     return;
@@ -1853,7 +1791,6 @@ Error RewriteInstance::readSpecialSections() {
   LSDASection = BC->getUniqueSectionByName(".gcc_except_table");
   EHFrameSection = BC->getUniqueSectionByName(".eh_frame");
   BuildIDSection = BC->getUniqueSectionByName(".note.gnu.build-id");
-  SDTSection = BC->getUniqueSectionByName(".note.stapsdt");
   PseudoProbeDescSection = BC->getUniqueSectionByName(".pseudo_probe_desc");
   PseudoProbeSection = BC->getUniqueSectionByName(".pseudo_probe");
 
@@ -1910,8 +1847,6 @@ Error RewriteInstance::readSpecialSections() {
   if (std::optional<std::string> FileBuildID = getPrintableBuildID())
     BC->setFileBuildID(*FileBuildID);
 
-  parseSDTNotes();
-
   // Read .dynamic/PT_DYNAMIC.
   return readELFDynamic();
 }
@@ -3245,7 +3180,7 @@ void RewriteInstance::preprocessProfileData() {
 }
 
 void RewriteInstance::initializeMetadataManager() {
-  // TODO
+  MetadataManager.registerRewriter(createSDTRewriter(*BC));
 }
 
 void RewriteInstance::processMetadataPreCFG() {
@@ -3621,7 +3556,6 @@ void RewriteInstance::updateMetadata() {
   MetadataManager.runFinalizersAfterEmit();
 
   // TODO: use MetadataManager for updates.
-  updateSDTMarkers();
   updateLKMarkers();
   parsePseudoProbe();
   updatePseudoProbes();
@@ -3902,29 +3836,6 @@ void RewriteInstance::encodePseudoProbes() {
   }
 }
 
-void RewriteInstance::updateSDTMarkers() {
-  NamedRegionTimer T("updateSDTMarkers", "update SDT markers", TimerGroupName,
-                     TimerGroupDesc, opts::TimeRewrite);
-
-  if (!SDTSection)
-    return;
-  SDTSection->registerPatcher(std::make_unique<SimpleBinaryPatcher>());
-
-  SimpleBinaryPatcher *SDTNotePatcher =
-      static_cast<SimpleBinaryPatcher *>(SDTSection->getPatcher());
-  for (auto &SDTInfoKV : BC->SDTMarkers) {
-    const uint64_t OriginalAddress = SDTInfoKV.first;
-    SDTMarkerInfo &SDTInfo = SDTInfoKV.second;
-    const BinaryFunction *F =
-        BC->getBinaryFunctionContainingAddress(OriginalAddress);
-    if (!F)
-      continue;
-    const uint64_t NewAddress =
-        F->translateInputToOutputAddress(OriginalAddress);
-    SDTNotePatcher->addLE64Patch(SDTInfo.PCOffset, NewAddress);
-  }
-}
-
 void RewriteInstance::updateLKMarkers() {
   if (BC->LKMarkers.size() == 0)
     return;