[llvm-objdump] Address further feedback

gulfemsavrun · gulfemsavrun · commit 768700e6ccb2 · 2025-11-05T11:09:16.000-08:00
diff --git a/llvm/tools/llvm-objdump/SourcePrinter.cpp b/llvm/tools/llvm-objdump/SourcePrinter.cpp
@@ -130,11 +130,28 @@ void LiveElementPrinter::addInlinedFunction(DWARFDie FuncDie,
   // Map the element's high address (HighPC) to its pointer for fast range end
   // lookup.
   LiveElementsByEndAddress[FuncHighPC].push_back(LiveElements.back().get());
-  // Map the pointer to its DWARF discovery index (ElementIdx) for deterministic
+  // Map the pointer to its DWARF discovery index for deterministic
   // ordering.
   ElementPtrToIndex[LiveElements.back().get()] = LiveElements.size() - 1;
 }
 
+/// Registers the most recently added LiveVariable into all data structures.
+void LiveElementPrinter::registerNewVariable() {
+  assert(
+      !LiveElements.empty() &&
+      "registerNewVariable called before element was added to LiveElements.");
+  LiveVariable *CurrentVar =
+      static_cast<LiveVariable *>(LiveElements.back().get());
+  // Map from a LiveElement pointer to its index in the LiveElements.
+  ElementPtrToIndex[CurrentVar] = LiveElements.size() - 1;
+
+  if (const auto &Range = CurrentVar->getLocExpr().Range) {
+    // Add the variable to address-based maps.
+    LiveElementsByAddress[Range->LowPC].push_back(CurrentVar);
+    LiveElementsByEndAddress[Range->HighPC].push_back(CurrentVar);
+  }
+}
+
 void LiveElementPrinter::addVariable(DWARFDie FuncDie, DWARFDie VarDie) {
   uint64_t FuncLowPC, FuncHighPC, SectionIndex;
   FuncDie.getLowAndHighPC(FuncLowPC, FuncHighPC, SectionIndex);
@@ -154,33 +171,21 @@ void LiveElementPrinter::addVariable(DWARFDie FuncDie, DWARFDie VarDie) {
   for (const DWARFLocationExpression &LocExpr : *Locs) {
     std::unique_ptr<LiveVariable> NewVar;
     if (LocExpr.Range) {
-      NewVar = std::make_unique<LiveVariable>(LocExpr, VarName, U, FuncDie);
+      LiveElements.emplace_back(
+          std::make_unique<LiveVariable>(LocExpr, VarName, U, FuncDie));
     } else {
       // If the LocExpr does not have an associated range, it is valid for
       // the whole of the function.
       // TODO: technically it is not valid for any range covered by another
       // LocExpr, does that happen in reality?
       DWARFLocationExpression WholeFuncExpr{
           DWARFAddressRange(FuncLowPC, FuncHighPC, SectionIndex), LocExpr.Expr};
-      NewVar =
-          std::make_unique<LiveVariable>(WholeFuncExpr, VarName, U, FuncDie);
+      LiveElements.emplace_back(
+          std::make_unique<LiveVariable>(WholeFuncExpr, VarName, U, FuncDie));
     }
 
-    // Add the new variable to all the data structures.
-    if (NewVar) {
-      LiveElements.emplace_back(std::move(NewVar));
-      LiveVariable *CurrentVar =
-          static_cast<LiveVariable *>(LiveElements.back().get());
-      // Map from a LiveElement pointer to its index in the LiveElements vector.
-      ElementPtrToIndex.emplace(CurrentVar, LiveElements.size() - 1);
-      if (CurrentVar->getLocExpr().Range) {
-        // Add the variable to address-based maps.
-        LiveElementsByAddress[CurrentVar->getLocExpr().Range->LowPC].push_back(
-            CurrentVar);
-        LiveElementsByEndAddress[CurrentVar->getLocExpr().Range->HighPC]
-            .push_back(CurrentVar);
-      }
-    }
+    // Register the new variable to all data structures.
+    registerNewVariable();
   }
 }
 
@@ -229,9 +234,8 @@ unsigned LiveElementPrinter::moveToFirstVarColumn(formatted_raw_ostream &OS) {
 unsigned LiveElementPrinter::getOrCreateColumn(unsigned ElementIdx) {
   // Check if the element already has an assigned column.
   auto it = ElementToColumn.find(ElementIdx);
-  if (it != ElementToColumn.end()) {
+  if (it != ElementToColumn.end())
     return it->second;
-  }
 
   unsigned ColIdx;
   if (!FreeCols.empty()) {
@@ -268,12 +272,12 @@ void LiveElementPrinter::freeColumn(unsigned ColIdx) {
 
 std::vector<unsigned>
 LiveElementPrinter::getSortedActiveElementIndices() const {
-  // Get all ElementIdx values that currently have an assigned column.
+  // Get all element indexes that currently have an assigned column.
   std::vector<unsigned> Indices;
   for (const auto &Pair : ElementToColumn)
     Indices.push_back(Pair.first);
 
-  // Sort by ElementIdx, which is the DWARF discovery order.
+  // Sort by the DWARF discovery order.
   llvm::stable_sort(Indices);
   return Indices;
 }
@@ -350,16 +354,12 @@ void LiveElementPrinter::update(object::SectionedAddress ThisAddr,
         return;
 
       const std::vector<LiveElement *> &ElementList = It->second;
-      // Get the ElementIdx for sorting and column management.
       for (LiveElement *LE : ElementList) {
         auto IndexIt = ElementPtrToIndex.find(LE);
-        if (IndexIt == ElementPtrToIndex.end()) {
-          LLVM_DEBUG(
-              dbgs()
-              << "Error: LiveElement in map but not in ElementPtrToIndex!\n");
-          continue;
-        }
+        assert(IndexIt != ElementPtrToIndex.end() &&
+               "LiveElement in address map but missing from index map!");
 
+        // Get the element index for sorting and column management.
         unsigned ElementIdx = IndexIt->second;
         // Skip elements that already have a column.
         if (ElementToColumn.count(ElementIdx))
@@ -379,8 +379,8 @@ void LiveElementPrinter::update(object::SectionedAddress ThisAddr,
     // Collect elements starting at NextAddr (the address immediately
     // following the instruction).
     CollectNewElements(LiveElementsByAddress.find(NextAddr.Address));
-    // Sort elements by DWARF discovery order (ElementIdx) for deterministic
-    // column assignment.
+    // Sort elements by DWARF discovery order for deterministic column
+    // assignment.
     llvm::stable_sort(NewLiveElements, [](const auto &A, const auto &B) {
       return A.first < B.first;
     });
@@ -482,10 +482,9 @@ void LiveElementPrinter::printAfterOtherLine(formatted_raw_ostream &OS,
 void LiveElementPrinter::printBetweenInsts(formatted_raw_ostream &OS,
                                            bool MustPrint) {
   bool PrintedSomething = false;
-  // Get all active elements, sorted by discovery order (ElementIdx).
+  // Get all active elements, sorted by discovery order.
   std::vector<unsigned> SortedElementIndices = getSortedActiveElementIndices();
-  // The outer loop iterates over the deterministic DWARF discovery order
-  // (ElementIdx).
+  // The outer loop iterates over the deterministic DWARF discovery order.
   for (unsigned ElementIdx : SortedElementIndices) {
     // Look up the physical column index (ColIdx) assigned to this
     // element. We use .at() because we are certain the element is active.
@@ -566,51 +565,29 @@ void LiveElementPrinter::printAfterInst(formatted_raw_ostream &OS) {
   }
 }
 
-void LiveElementPrinter::printStartLine(formatted_raw_ostream &OS,
-                                        object::SectionedAddress Addr) {
-  // Only print the start line for inlined functions if DFLimitsOnly is
+void LiveElementPrinter::printBoundaryLine(formatted_raw_ostream &OS,
+                                           object::SectionedAddress Addr,
+                                           bool IsEnd) {
+  // Only print the start/end line for inlined functions if DFLimitsOnly is
   // enabled.
   if (DbgInlinedFunctions != DFLimitsOnly)
     return;
 
-  // Use the map to find all elements that start at the given address.
-  std::vector<unsigned> ElementIndices;
-  auto It = LiveElementsByAddress.find(Addr.Address);
-  if (It != LiveElementsByAddress.end()) {
-    for (LiveElement *LE : It->second) {
-      // Look up the ElementIdx from the pointer.
-      auto IndexIt = ElementPtrToIndex.find(LE);
-      if (IndexIt != ElementPtrToIndex.end())
-        ElementIndices.push_back(IndexIt->second);
-    }
-  }
-
-  // Sort the indices to ensure deterministic output order (by DWARF discovery
-  // order).
-  llvm::stable_sort(ElementIndices);
-
-  for (unsigned ElementIdx : ElementIndices) {
-    LiveElement *LE = LiveElements[ElementIdx].get();
-    LE->printElementLine(OS, Addr, false);
-  }
-}
-
-void LiveElementPrinter::printEndLine(formatted_raw_ostream &OS,
-                                      object::SectionedAddress Addr) {
-  // Only print the end line for inlined functions if DFLimitsOnly is
-  // enabled.
-  if (DbgInlinedFunctions != DFLimitsOnly)
-    return;
+  // Select the appropriate map based on whether we are checking the start
+  // (LowPC) or end (HighPC) address.
+  const auto &AddressMap =
+      IsEnd ? LiveElementsByEndAddress : LiveElementsByAddress;
 
-  // Use the map to find elements that end at the given address.
+  // Use the map to find all elements that start/end at the given address.
   std::vector<unsigned> ElementIndices;
-  auto It = LiveElementsByEndAddress.find(Addr.Address);
-  if (It != LiveElementsByEndAddress.end()) {
+  auto It = AddressMap.find(Addr.Address);
+  if (It != AddressMap.end()) {
     for (LiveElement *LE : It->second) {
-      // Look up the ElementIdx from the pointer.
+      // Look up the element index from the pointer.
       auto IndexIt = ElementPtrToIndex.find(LE);
-      if (IndexIt != ElementPtrToIndex.end())
-        ElementIndices.push_back(IndexIt->second);
+      assert(IndexIt != ElementPtrToIndex.end() &&
+             "LiveElement found in address map but missing index!");
+      ElementIndices.push_back(IndexIt->second);
     }
   }
 
@@ -620,7 +597,7 @@ void LiveElementPrinter::printEndLine(formatted_raw_ostream &OS,
 
   for (unsigned ElementIdx : ElementIndices) {
     LiveElement *LE = LiveElements[ElementIdx].get();
-    LE->printElementLine(OS, Addr, true);
+    LE->printElementLine(OS, Addr, IsEnd);
   }
 }
 
diff --git a/llvm/tools/llvm-objdump/SourcePrinter.h b/llvm/tools/llvm-objdump/SourcePrinter.h
@@ -9,6 +9,7 @@
 #ifndef LLVM_TOOLS_LLVM_OBJDUMP_SOURCEPRINTER_H
 #define LLVM_TOOLS_LLVM_OBJDUMP_SOURCEPRINTER_H
 
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/IndexedMap.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/StringSet.h"
@@ -107,9 +108,9 @@ class LiveElementPrinter {
   llvm::MapVector<uint64_t, std::vector<LiveElement *>>
       LiveElementsByEndAddress;
   // Map from a LiveElement pointer to its index in the LiveElements vector.
-  std::unordered_map<LiveElement *, unsigned> ElementPtrToIndex;
+  llvm::DenseMap<LiveElement *, unsigned> ElementPtrToIndex;
   // Map from a live element index to column index for efficient lookup.
-  std::unordered_map<unsigned, unsigned> ElementToColumn;
+  llvm::DenseMap<unsigned, unsigned> ElementToColumn;
   // Vector of columns currently used for printing live ranges.
   std::vector<Column> ActiveCols;
   // Set of available column indices kept sorted for efficient reuse.
@@ -120,6 +121,8 @@ class LiveElementPrinter {
   const MCRegisterInfo &MRI;
   const MCSubtargetInfo &STI;
 
+  void registerNewVariable();
+
   void addInlinedFunction(DWARFDie FuncDie, DWARFDie InlinedFuncDie);
   void addVariable(DWARFDie FuncDie, DWARFDie VarDie);
 
@@ -143,7 +146,7 @@ class LiveElementPrinter {
   void freeColumn(unsigned ColIdx);
 
   // Returns the indices of all currently active elements, sorted by their DWARF
-  // discovery order (ElementIdx).
+  // discovery order.
   std::vector<unsigned> getSortedActiveElementIndices() const;
 
 public:
@@ -192,10 +195,9 @@ class LiveElementPrinter {
   /// Print the live element ranges to the right of a disassembled instruction.
   void printAfterInst(formatted_raw_ostream &OS);
 
-  /// Print a line to idenfity the start of a live element.
-  void printStartLine(formatted_raw_ostream &OS, object::SectionedAddress Addr);
-  /// Print a line to idenfity the end of a live element.
-  void printEndLine(formatted_raw_ostream &OS, object::SectionedAddress Addr);
+  /// Print a line to idenfity the start/end of a live element.
+  void printBoundaryLine(formatted_raw_ostream &OS,
+                         object::SectionedAddress Addr, bool IsEnd);
 };
 
 class SourcePrinter {
diff --git a/llvm/tools/llvm-objdump/llvm-objdump.cpp b/llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -688,7 +688,7 @@ class PrettyPrinter {
             LiveElementPrinter &LEP) {
     if (SP && (PrintSource || PrintLines))
       SP->printSourceLine(OS, Address, ObjectFilename, LEP);
-    LEP.printStartLine(OS, Address);
+    LEP.printBoundaryLine(OS, Address, false);
     LEP.printBetweenInsts(OS, false);
 
     printRawData(Bytes, Address.Address, OS, STI);
@@ -935,7 +935,7 @@ class ARMPrettyPrinter : public PrettyPrinter {
                  LiveElementPrinter &LEP) override {
     if (SP && (PrintSource || PrintLines))
       SP->printSourceLine(OS, Address, ObjectFilename, LEP);
-    LEP.printStartLine(OS, Address);
+    LEP.printBoundaryLine(OS, Address, false);
     LEP.printBetweenInsts(OS, false);
 
     size_t Start = OS.tell();
@@ -990,7 +990,7 @@ class AArch64PrettyPrinter : public PrettyPrinter {
                  LiveElementPrinter &LEP) override {
     if (SP && (PrintSource || PrintLines))
       SP->printSourceLine(OS, Address, ObjectFilename, LEP);
-    LEP.printStartLine(OS, Address);
+    LEP.printBoundaryLine(OS, Address, false);
     LEP.printBetweenInsts(OS, false);
 
     size_t Start = OS.tell();
@@ -1029,7 +1029,7 @@ class RISCVPrettyPrinter : public PrettyPrinter {
                  LiveElementPrinter &LEP) override {
     if (SP && (PrintSource || PrintLines))
       SP->printSourceLine(OS, Address, ObjectFilename, LEP);
-    LEP.printStartLine(OS, Address);
+    LEP.printBoundaryLine(OS, Address, false);
     LEP.printBetweenInsts(OS, false);
 
     size_t Start = OS.tell();
@@ -2593,7 +2593,7 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
 
         object::SectionedAddress NextAddr = {
             SectionAddr + Index + VMAAdjustment + Size, Section.getIndex()};
-        LEP.printEndLine(FOS, NextAddr);
+        LEP.printBoundaryLine(FOS, NextAddr, true);
 
         Index += Size;
       }
