Skip to content

[NFC] Separate high-level-dependent portions of DWARFExpression #139175

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Sterling-Augustine
Copy link
Contributor

Move all expression verification into its only client: DWARFVerifier. Move all printing code (which was a mix of static and member functions) into a separate class.

Dwarf expressions are used in many contexts without Dwarf units and other higher-level Dwarf concepts. The code currently includes conditionals which fall back to defaults if some high-level construct is not available. For example, prettyPrintBaseTypeRef checks U for null.

These checks mean that a Dwarf expressions can be used without high-level run time dependencies on Dwarf unit. But as coded they cannot be used without high level build time dependencies on Dwarf unit, which brings in many additional dependencies that are often unneeded.

One in a series of NFC DebugInfo/DWARF refactoring changes to layer it more cleanly, so that binary CFI parsing can be used from low-level code, (such as byte strings created via .cfi_escape) without circular dependencies. The final goal is to make a more limited dwarf library usable from lower-level code.

Move all expression verification into its only client: DWARFVerifier.
Move all printing code (which was a mix of static and member functions)
into a separate class.

Dwarf expressions are used in many contexts without Dwarf units and
other higher-level Dwarf concepts. The code currently includes
conditionals which fall back to defaults if some high-level construct
is not available. For example, prettyPrintBaseTypeRef checks U for
null.

These checks mean that a Dwarf expressions can be used without
high-level *run* time* dependencies on Dwarf unit. But as coded they
cannot be used without high level *build* time dependencies on Dwarf
unit.

One in a series of NFC DebugInfo/DWARF refactoring changes to layer it
more cleanly, so that binary CFI parsing can be used from low-level
code, (such as byte strings created via .cfi_escape) without circular
dependencies.
@llvmbot
Copy link
Member

llvmbot commented May 8, 2025

@llvm/pr-subscribers-lldb

@llvm/pr-subscribers-debuginfo

Author: None (Sterling-Augustine)

Changes

Move all expression verification into its only client: DWARFVerifier. Move all printing code (which was a mix of static and member functions) into a separate class.

Dwarf expressions are used in many contexts without Dwarf units and other higher-level Dwarf concepts. The code currently includes conditionals which fall back to defaults if some high-level construct is not available. For example, prettyPrintBaseTypeRef checks U for null.

These checks mean that a Dwarf expressions can be used without high-level run time dependencies on Dwarf unit. But as coded they cannot be used without high level build time dependencies on Dwarf unit, which brings in many additional dependencies that are often unneeded.

One in a series of NFC DebugInfo/DWARF refactoring changes to layer it more cleanly, so that binary CFI parsing can be used from low-level code, (such as byte strings created via .cfi_escape) without circular dependencies. The final goal is to make a more limited dwarf library usable from lower-level code.


Patch is 22.16 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/139175.diff

10 Files Affected:

  • (modified) llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h (+65-23)
  • (modified) llvm/include/llvm/DebugInfo/DWARF/DWARFVerifier.h (+18)
  • (modified) llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp (+4-2)
  • (modified) llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp (+2-1)
  • (modified) llvm/lib/DebugInfo/DWARF/DWARFDie.cpp (+2-2)
  • (modified) llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp (+62-86)
  • (modified) llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp (+30-1)
  • (modified) llvm/lib/DebugInfo/LogicalView/Readers/LVDWARFReader.cpp (+2-2)
  • (modified) llvm/tools/llvm-objdump/SourcePrinter.cpp (+1-1)
  • (modified) llvm/unittests/DebugInfo/DWARF/DWARFExpressionCompactPrinterTest.cpp (+1-1)
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h
index 00228a32173f1..0549b71cbaead 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h
@@ -76,6 +76,9 @@ class DWARFExpression {
 
   private:
     friend class DWARFExpression::iterator;
+    friend class DWARFExpressionPrinter;
+    friend class DWARFVerifier;
+
     uint8_t Opcode; ///< The Op Opcode, DW_OP_<something>.
     Description Desc;
     bool Error = false;
@@ -98,11 +101,6 @@ class DWARFExpression {
     }
     uint64_t getEndOffset() const { return EndOffset; }
     bool isError() const { return Error; }
-    bool print(raw_ostream &OS, DIDumpOptions DumpOpts,
-               const DWARFExpression *Expr, DWARFUnit *U) const;
-
-    /// Verify \p Op. Does not affect the return of \a isError().
-    static bool verify(const Operation &Op, DWARFUnit *U);
 
   private:
     bool extract(DataExtractor Data, uint8_t AddressSize, uint64_t Offset,
@@ -152,26 +150,12 @@ class DWARFExpression {
   iterator begin() const { return iterator(this, 0); }
   iterator end() const { return iterator(this, Data.getData().size()); }
 
-  void print(raw_ostream &OS, DIDumpOptions DumpOpts, DWARFUnit *U,
-             bool IsEH = false) const;
-
-  /// Print the expression in a format intended to be compact and useful to a
-  /// user, but not perfectly unambiguous, or capable of representing every
-  /// valid DWARF expression. Returns true if the expression was sucessfully
-  /// printed.
-  bool printCompact(raw_ostream &OS,
-                    std::function<StringRef(uint64_t RegNum, bool IsEH)>
-                        GetNameForDWARFReg = nullptr);
-
-  bool verify(DWARFUnit *U);
-
   bool operator==(const DWARFExpression &RHS) const;
 
   StringRef getData() const { return Data.getData(); }
 
-  static bool prettyPrintRegisterOp(DWARFUnit *U, raw_ostream &OS,
-                                    DIDumpOptions DumpOpts, uint8_t Opcode,
-                                    const ArrayRef<uint64_t> Operands);
+  friend class DWARFExpressionPrinter;
+  friend class DWARFVerifier;
 
 private:
   DataExtractor Data;
@@ -183,5 +167,63 @@ inline bool operator==(const DWARFExpression::iterator &LHS,
                        const DWARFExpression::iterator &RHS) {
   return LHS.Expr == RHS.Expr && LHS.Offset == RHS.Offset;
 }
-}
-#endif
+
+// This functionality is separated from the main data structure so that nothing
+// in DWARFExpression.cpp needs build-time dependencies on DWARFUnit or other
+// higher-level Dwarf structures. This approach creates better layering and
+// allows DWARFExpression to be used from code which can't have dependencies on
+// those higher-level structures.
+
+  class DWARFUnit;
+  struct DIDumpOptions;
+  class raw_ostream;
+
+class DWARFExpressionPrinter {
+ public:
+   /// Print a Dwarf expression/
+   /// \param E to be printed
+   /// \param OS to this stream
+   /// \param GetNameForDWARFReg callback to return dwarf register name
+   static void print(const DWARFExpression *E, raw_ostream &OS,
+                     DIDumpOptions DumpOpts, DWARFUnit *U, bool IsEH = false);
+
+   /// Print the expression in a format intended to be compact and useful to a
+   /// user, but not perfectly unambiguous, or capable of representing every
+   /// valid DWARF expression. Returns true if the expression was sucessfully
+   /// printed.
+   ///
+   /// \param E to be printed
+   /// \param OS to this stream
+   /// \param GetNameForDWARFReg callback to return dwarf register name
+   ///
+   /// \returns true if the expression was successfully printed
+   static bool printCompact(const DWARFExpression *E, raw_ostream &OS,
+                            std::function<StringRef(uint64_t RegNum, bool IsEH)>
+                                GetNameForDWARFReg = nullptr);
+
+  /// Pretty print a register opcode and operands.
+  /// \param U within the context of this Dwarf unit, if any.
+  /// \param OS to this stream
+  /// \param DumpOpts with these options
+  /// \param Opcode to print
+  /// \param Operands to the opcode
+  ///
+  /// returns true if the Op was successfully printed
+  static bool prettyPrintRegisterOp(DWARFUnit *U, raw_ostream &OS,
+                                     DIDumpOptions DumpOpts, uint8_t Opcode,
+                                     ArrayRef<uint64_t> Operands);
+
+ private:
+   static bool printOp(const DWARFExpression::Operation *Op, raw_ostream &OS,
+                       DIDumpOptions DumpOpts, const DWARFExpression *Expr,
+                       DWARFUnit *U);
+
+   static void prettyPrintBaseTypeRef(DWARFUnit *U, raw_ostream &OS,
+                                      DIDumpOptions DumpOpts,
+                                      ArrayRef<uint64_t> Operands,
+                                      unsigned Operand);
+};
+
+} // end namespace llvm
+
+#endif // LLVM_DEBUGINFO_DWARF_DWARFEXPRESSION_H
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFVerifier.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFVerifier.h
index 717f9cedc4ee3..7c998a623769a 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFVerifier.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFVerifier.h
@@ -14,6 +14,7 @@
 #include "llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h"
 #include "llvm/DebugInfo/DWARF/DWARFAddressRange.h"
 #include "llvm/DebugInfo/DWARF/DWARFDie.h"
+#include "llvm/DebugInfo/DWARF/DWARFExpression.h"
 #include "llvm/DebugInfo/DWARF/DWARFUnitIndex.h"
 #include <cstdint>
 #include <map>
@@ -319,6 +320,23 @@ class DWARFVerifier {
   void verifyDebugNames(const DWARFSection &AccelSection,
                         const DataExtractor &StrData);
 
+  /// Verify that the the expression is valid within the context of unit U.
+  ///
+  /// \param E expression to verify.
+  /// \param U containing DWARFUnit, if any.
+  ///
+  /// returns true if E is a valid expression.
+  bool verifyExpression(const DWARFExpression &E, DWARFUnit *U);
+
+  /// Verify that the the expression operation is valid within the context of
+  /// unit U.
+  ///
+  /// \param Op operation to verify
+  /// \param U containing DWARFUnit, if any
+  ///
+  /// returns true if Op is a valid Dwarf operation
+  bool verifyExpressionOp(const DWARFExpression::Operation &Op, DWARFUnit *U);
+
 public:
   DWARFVerifier(raw_ostream &S, DWARFContext &D,
                 DIDumpOptions DumpOpts = DIDumpOptions::getForSingleDIE());
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
index 5bdc257fd8d89..c9b69169770e3 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
@@ -13,6 +13,7 @@
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/DebugInfo/DIContext.h"
 #include "llvm/DebugInfo/DWARF/DWARFDataExtractor.h"
+#include "llvm/DebugInfo/DWARF/DWARFExpression.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/DataExtractor.h"
 #include "llvm/Support/Errc.h"
@@ -109,7 +110,8 @@ void UnwindLocation::dump(raw_ostream &OS, DIDumpOptions DumpOpts) const {
       OS << " in addrspace" << *AddrSpace;
     break;
   case DWARFExpr: {
-    Expr->print(OS, DumpOpts, nullptr);
+    if (Expr)
+      DWARFExpressionPrinter::print(&(*Expr), OS, DumpOpts, nullptr);
     break;
   }
   case Constant:
@@ -943,7 +945,7 @@ void CFIProgram::printOperand(raw_ostream &OS, DIDumpOptions DumpOpts,
   case OT_Expression:
     assert(Instr.Expression && "missing DWARFExpression object");
     OS << " ";
-    Instr.Expression->print(OS, DumpOpts, nullptr);
+    DWARFExpressionPrinter::print(&(*Instr.Expression), OS, DumpOpts, nullptr);
     break;
   }
 }
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
index ec7af792efb06..fc71be32fdd79 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
@@ -116,7 +116,8 @@ static void dumpExpression(raw_ostream &OS, DIDumpOptions DumpOpts,
   std::optional<dwarf::DwarfFormat> Format;
   if (U)
     Format = U->getFormat();
-  DWARFExpression(Extractor, AddressSize, Format).print(OS, DumpOpts, U);
+  DWARFExpression E(Extractor, AddressSize, Format);
+  DWARFExpressionPrinter::print(&E, OS, DumpOpts, U);
 }
 
 bool DWARFLocationTable::dumpLocationList(
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDie.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
index a0ce7810f91b0..08dd9d30812d1 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
@@ -98,8 +98,8 @@ static void dumpLocationExpr(raw_ostream &OS, const DWARFFormValue &FormValue,
   ArrayRef<uint8_t> Expr = *FormValue.getAsBlock();
   DataExtractor Data(StringRef((const char *)Expr.data(), Expr.size()),
                      Ctx.isLittleEndian(), 0);
-  DWARFExpression(Data, U->getAddressByteSize(), U->getFormParams().Format)
-      .print(OS, DumpOpts, U);
+  DWARFExpression DE(Data, U->getAddressByteSize(), U->getFormParams().Format);
+  DWARFExpressionPrinter::print(&DE, OS, DumpOpts, U);
 }
 
 static DWARFDie resolveReferencedType(DWARFDie D, DWARFFormValue F) {
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp b/llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp
index 2ae5ff3efc8c5..dd1325d8f7491 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp
@@ -237,10 +237,23 @@ bool DWARFExpression::Operation::extract(DataExtractor Data,
   return true;
 }
 
-static void prettyPrintBaseTypeRef(DWARFUnit *U, raw_ostream &OS,
-                                   DIDumpOptions DumpOpts,
-                                   ArrayRef<uint64_t> Operands,
-                                   unsigned Operand) {
+std::optional<unsigned> DWARFExpression::Operation::getSubCode() const {
+  if (!Desc.Op.size() || Desc.Op[0] != Operation::SizeSubOpLEB)
+    return std::nullopt;
+  return Operands[0];
+}
+
+bool DWARFExpression::operator==(const DWARFExpression &RHS) const {
+  if (AddressSize != RHS.AddressSize || Format != RHS.Format)
+    return false;
+  return Data.getData() == RHS.Data.getData();
+}
+
+void DWARFExpressionPrinter::prettyPrintBaseTypeRef(DWARFUnit *U,
+                                                    raw_ostream &OS,
+                                                    DIDumpOptions DumpOpts,
+                                                    ArrayRef<uint64_t> Operands,
+                                                    unsigned Operand) {
   assert(Operand < Operands.size() && "operand out of bounds");
   if (!U) {
     OS << format(" <base_type ref: 0x%" PRIx64 ">", Operands[Operand]);
@@ -259,10 +272,9 @@ static void prettyPrintBaseTypeRef(DWARFUnit *U, raw_ostream &OS,
   }
 }
 
-bool DWARFExpression::prettyPrintRegisterOp(DWARFUnit *U, raw_ostream &OS,
-                                            DIDumpOptions DumpOpts,
-                                            uint8_t Opcode,
-                                            ArrayRef<uint64_t> Operands) {
+bool DWARFExpressionPrinter::prettyPrintRegisterOp(
+    DWARFUnit *U, raw_ostream &OS, DIDumpOptions DumpOpts, uint8_t Opcode,
+    ArrayRef<uint64_t> Operands) {
   if (!DumpOpts.GetNameForDWARFReg)
     return false;
 
@@ -293,87 +305,84 @@ bool DWARFExpression::prettyPrintRegisterOp(DWARFUnit *U, raw_ostream &OS,
   return false;
 }
 
-std::optional<unsigned> DWARFExpression::Operation::getSubCode() const {
-  if (!Desc.Op.size() || Desc.Op[0] != Operation::SizeSubOpLEB)
-    return std::nullopt;
-  return Operands[0];
-}
-
-bool DWARFExpression::Operation::print(raw_ostream &OS, DIDumpOptions DumpOpts,
-                                       const DWARFExpression *Expr,
-                                       DWARFUnit *U) const {
-  if (Error) {
+bool DWARFExpressionPrinter::printOp(const DWARFExpression::Operation *Op,
+                                     raw_ostream &OS, DIDumpOptions DumpOpts,
+                                     const DWARFExpression *Expr,
+                                     DWARFUnit *U) {
+  if (Op->Error) {
     OS << "<decoding error>";
     return false;
   }
 
-  StringRef Name = OperationEncodingString(Opcode);
+  StringRef Name = OperationEncodingString(Op->Opcode);
   assert(!Name.empty() && "DW_OP has no name!");
   OS << Name;
 
-  if ((Opcode >= DW_OP_breg0 && Opcode <= DW_OP_breg31) ||
-      (Opcode >= DW_OP_reg0 && Opcode <= DW_OP_reg31) ||
-      Opcode == DW_OP_bregx || Opcode == DW_OP_regx ||
-      Opcode == DW_OP_regval_type)
-    if (prettyPrintRegisterOp(U, OS, DumpOpts, Opcode, Operands))
+  if ((Op->Opcode >= DW_OP_breg0 && Op->Opcode <= DW_OP_breg31) ||
+      (Op->Opcode >= DW_OP_reg0 && Op->Opcode <= DW_OP_reg31) ||
+      Op->Opcode == DW_OP_bregx || Op->Opcode == DW_OP_regx ||
+      Op->Opcode == DW_OP_regval_type)
+    if (prettyPrintRegisterOp(U, OS, DumpOpts, Op->Opcode, Op->Operands))
       return true;
 
-  for (unsigned Operand = 0; Operand < Desc.Op.size(); ++Operand) {
-    unsigned Size = Desc.Op[Operand];
-    unsigned Signed = Size & Operation::SignBit;
+  for (unsigned Operand = 0; Operand < Op->Desc.Op.size(); ++Operand) {
+    unsigned Size = Op->Desc.Op[Operand];
+    unsigned Signed = Size & DWARFExpression::Operation::SignBit;
 
-    if (Size == Operation::SizeSubOpLEB) {
-      StringRef SubName = SubOperationEncodingString(Opcode, Operands[Operand]);
+    if (Size == DWARFExpression::Operation::SizeSubOpLEB) {
+      StringRef SubName =
+          SubOperationEncodingString(Op->Opcode, Op->Operands[Operand]);
       assert(!SubName.empty() && "DW_OP SubOp has no name!");
       OS << " " << SubName;
-    } else if (Size == Operation::BaseTypeRef && U) {
+    } else if (Size == DWARFExpression::Operation::BaseTypeRef && U) {
       // For DW_OP_convert the operand may be 0 to indicate that conversion to
       // the generic type should be done. The same holds for DW_OP_reinterpret,
       // which is currently not supported.
-      if (Opcode == DW_OP_convert && Operands[Operand] == 0)
+      if (Op->Opcode == DW_OP_convert && Op->Operands[Operand] == 0)
         OS << " 0x0";
       else
-        prettyPrintBaseTypeRef(U, OS, DumpOpts, Operands, Operand);
-    } else if (Size == Operation::WasmLocationArg) {
+        prettyPrintBaseTypeRef(U, OS, DumpOpts, Op->Operands, Operand);
+    } else if (Size == DWARFExpression::Operation::WasmLocationArg) {
       assert(Operand == 1);
-      switch (Operands[0]) {
+      switch (Op->Operands[0]) {
       case 0:
       case 1:
       case 2:
       case 3: // global as uint32
       case 4:
-        OS << format(" 0x%" PRIx64, Operands[Operand]);
+        OS << format(" 0x%" PRIx64, Op->Operands[Operand]);
         break;
       default: assert(false);
       }
-    } else if (Size == Operation::SizeBlock) {
-      uint64_t Offset = Operands[Operand];
-      for (unsigned i = 0; i < Operands[Operand - 1]; ++i)
+    } else if (Size == DWARFExpression::Operation::SizeBlock) {
+      uint64_t Offset = Op->Operands[Operand];
+      for (unsigned i = 0; i < Op->Operands[Operand - 1]; ++i)
         OS << format(" 0x%02x", Expr->Data.getU8(&Offset));
     } else {
       if (Signed)
-        OS << format(" %+" PRId64, (int64_t)Operands[Operand]);
-      else if (Opcode != DW_OP_entry_value &&
-               Opcode != DW_OP_GNU_entry_value)
-        OS << format(" 0x%" PRIx64, Operands[Operand]);
+        OS << format(" %+" PRId64, (int64_t)Op->Operands[Operand]);
+      else if (Op->Opcode != DW_OP_entry_value &&
+               Op->Opcode != DW_OP_GNU_entry_value)
+        OS << format(" 0x%" PRIx64, Op->Operands[Operand]);
     }
   }
   return true;
 }
 
-void DWARFExpression::print(raw_ostream &OS, DIDumpOptions DumpOpts,
-                            DWARFUnit *U, bool IsEH) const {
+void DWARFExpressionPrinter::print(const DWARFExpression *E, raw_ostream &OS,
+                                   DIDumpOptions DumpOpts, DWARFUnit *U,
+                                   bool IsEH) {
   uint32_t EntryValExprSize = 0;
   uint64_t EntryValStartOffset = 0;
-  if (Data.getData().empty())
+  if (E->Data.getData().empty())
     OS << "<empty>";
 
-  for (auto &Op : *this) {
+  for (auto &Op : *E) {
     DumpOpts.IsEH = IsEH;
-    if (!Op.print(OS, DumpOpts, this, U)) {
+    if (!printOp(&Op, OS, DumpOpts, E, U)) {
       uint64_t FailOffset = Op.getEndOffset();
-      while (FailOffset < Data.getData().size())
-        OS << format(" %02x", Data.getU8(&FailOffset));
+      while (FailOffset < E->Data.getData().size())
+        OS << format(" %02x", E->Data.getU8(&FailOffset));
       return;
     }
 
@@ -391,39 +400,11 @@ void DWARFExpression::print(raw_ostream &OS, DIDumpOptions DumpOpts,
         OS << ")";
     }
 
-    if (Op.getEndOffset() < Data.getData().size())
+    if (Op.getEndOffset() < E->Data.getData().size())
       OS << ", ";
   }
 }
 
-bool DWARFExpression::Operation::verify(const Operation &Op, DWARFUnit *U) {
-  for (unsigned Operand = 0; Operand < Op.Desc.Op.size(); ++Operand) {
-    unsigned Size = Op.Desc.Op[Operand];
-
-    if (Size == Operation::BaseTypeRef) {
-      // For DW_OP_convert the operand may be 0 to indicate that conversion to
-      // the generic type should be done, so don't look up a base type in that
-      // case. The same holds for DW_OP_reinterpret, which is currently not
-      // supported.
-      if (Op.Opcode == DW_OP_convert && Op.Operands[Operand] == 0)
-        continue;
-      auto Die = U->getDIEForOffset(U->getOffset() + Op.Operands[Operand]);
-      if (!Die || Die.getTag() != dwarf::DW_TAG_base_type)
-        return false;
-    }
-  }
-
-  return true;
-}
-
-bool DWARFExpression::verify(DWARFUnit *U) {
-  for (auto &Op : *this)
-    if (!Operation::verify(Op, U))
-      return false;
-
-  return true;
-}
-
 /// A user-facing string representation of a DWARF expression. This might be an
 /// Address expression, in which case it will be implicitly dereferenced, or a
 /// Value expression.
@@ -546,16 +527,11 @@ static bool printCompactDWARFExpr(
   return true;
 }
 
-bool DWARFExpression::printCompact(
-    raw_ostream &OS,
+bool DWARFExpressionPrinter::printCompact(
+    const DWARFExpression *E, raw_ostream &OS,
     std::function<StringRef(uint64_t RegNum, bool IsEH)> GetNameForDWARFReg) {
-  return printCompactDWARFExpr(OS, begin(), end(), GetNameForDWARFReg);
+  return printCompactDWARFExpr(OS, E->begin(), E->end(), GetNameForDWARFReg);
 }
 
-bool DWARFExpression::operator==(const DWARFExpression &RHS) const {
-  if (AddressSize != RHS.AddressSize || Format != RHS.Format)
-    return false;
-  return Data.getData() == RHS.Data.getData();
-}
 
 } // namespace llvm
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp b/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
index 43a62bdd8390d..c12786cac8686 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
@@ -659,6 +659,35 @@ unsigned DWARFVerifier::verifyDieRanges(const DWARFDie &Die,
   return NumErrors;
 }
 
+bool DWARFVerifier::verifyExpressionOp(const DWARFExpression::Operation &Op,
+                                       DWARFUnit *U) {
+  for (unsigned Operand = 0; Operand < Op.Desc.Op.size(); ++Operand) {
+    unsigned Size = Op.Desc.Op[Operand];
+
+    if (Size == DWARFExpression::Operation::BaseTypeRef) {
+      // For DW_OP_convert the operand may be 0 to indicate that conversion to
+      // the generic type should be done, so don't look up a base type in that
+      // case. The same holds for DW_OP_reinterpret, which is currently not
+      // supported.
+      if (Op.Opcode == DW_OP_convert && Op.Operands[Operand] == 0)
+        continue;
+      auto Die = U->getDIEForOffset(U->getOffset() + Op.Operands[Operand]);
+      if (!Die || Die.getTag() != dwarf::DW_TAG_base_type)
+        return false;
+    }
+  }
+
+  return true;
+}
+
+bool DWARFVerifier::verifyExpression(const DWARFExpression &E, DWARFUnit *U) {
+  for (auto &Op : E)
+    if (!verifyExpressionOp(Op, U))
+      return false;
+
+  return true;
+}
+
 unsigned DWARFVerifier::verifyDebugInfoAttribute(const DWARFDie &Die,
                                                  DWARFAttribute &AttrValue) {
   unsigned NumErrors = 0;
@@ -727,7 +756,7 @@ unsigned DWARFVerifier::verifyDebugInfoAttribute(const DWARFDie &Die,
             any_of(Expression, [](cons...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented May 8, 2025

@llvm/pr-subscribers-llvm-binary-utilities

Author: None (Sterling-Augustine)

Changes

Move all expression verification into its only client: DWARFVerifier. Move all printing code (which was a mix of static and member functions) into a separate class.

Dwarf expressions are used in many contexts without Dwarf units and other higher-level Dwarf concepts. The code currently includes conditionals which fall back to defaults if some high-level construct is not available. For example, prettyPrintBaseTypeRef checks U for null.

These checks mean that a Dwarf expressions can be used without high-level run time dependencies on Dwarf unit. But as coded they cannot be used without high level build time dependencies on Dwarf unit, which brings in many additional dependencies that are often unneeded.

One in a series of NFC DebugInfo/DWARF refactoring changes to layer it more cleanly, so that binary CFI parsing can be used from low-level code, (such as byte strings created via .cfi_escape) without circular dependencies. The final goal is to make a more limited dwarf library usable from lower-level code.


Patch is 22.16 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/139175.diff

10 Files Affected:

  • (modified) llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h (+65-23)
  • (modified) llvm/include/llvm/DebugInfo/DWARF/DWARFVerifier.h (+18)
  • (modified) llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp (+4-2)
  • (modified) llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp (+2-1)
  • (modified) llvm/lib/DebugInfo/DWARF/DWARFDie.cpp (+2-2)
  • (modified) llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp (+62-86)
  • (modified) llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp (+30-1)
  • (modified) llvm/lib/DebugInfo/LogicalView/Readers/LVDWARFReader.cpp (+2-2)
  • (modified) llvm/tools/llvm-objdump/SourcePrinter.cpp (+1-1)
  • (modified) llvm/unittests/DebugInfo/DWARF/DWARFExpressionCompactPrinterTest.cpp (+1-1)
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h
index 00228a32173f1..0549b71cbaead 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h
@@ -76,6 +76,9 @@ class DWARFExpression {
 
   private:
     friend class DWARFExpression::iterator;
+    friend class DWARFExpressionPrinter;
+    friend class DWARFVerifier;
+
     uint8_t Opcode; ///< The Op Opcode, DW_OP_<something>.
     Description Desc;
     bool Error = false;
@@ -98,11 +101,6 @@ class DWARFExpression {
     }
     uint64_t getEndOffset() const { return EndOffset; }
     bool isError() const { return Error; }
-    bool print(raw_ostream &OS, DIDumpOptions DumpOpts,
-               const DWARFExpression *Expr, DWARFUnit *U) const;
-
-    /// Verify \p Op. Does not affect the return of \a isError().
-    static bool verify(const Operation &Op, DWARFUnit *U);
 
   private:
     bool extract(DataExtractor Data, uint8_t AddressSize, uint64_t Offset,
@@ -152,26 +150,12 @@ class DWARFExpression {
   iterator begin() const { return iterator(this, 0); }
   iterator end() const { return iterator(this, Data.getData().size()); }
 
-  void print(raw_ostream &OS, DIDumpOptions DumpOpts, DWARFUnit *U,
-             bool IsEH = false) const;
-
-  /// Print the expression in a format intended to be compact and useful to a
-  /// user, but not perfectly unambiguous, or capable of representing every
-  /// valid DWARF expression. Returns true if the expression was sucessfully
-  /// printed.
-  bool printCompact(raw_ostream &OS,
-                    std::function<StringRef(uint64_t RegNum, bool IsEH)>
-                        GetNameForDWARFReg = nullptr);
-
-  bool verify(DWARFUnit *U);
-
   bool operator==(const DWARFExpression &RHS) const;
 
   StringRef getData() const { return Data.getData(); }
 
-  static bool prettyPrintRegisterOp(DWARFUnit *U, raw_ostream &OS,
-                                    DIDumpOptions DumpOpts, uint8_t Opcode,
-                                    const ArrayRef<uint64_t> Operands);
+  friend class DWARFExpressionPrinter;
+  friend class DWARFVerifier;
 
 private:
   DataExtractor Data;
@@ -183,5 +167,63 @@ inline bool operator==(const DWARFExpression::iterator &LHS,
                        const DWARFExpression::iterator &RHS) {
   return LHS.Expr == RHS.Expr && LHS.Offset == RHS.Offset;
 }
-}
-#endif
+
+// This functionality is separated from the main data structure so that nothing
+// in DWARFExpression.cpp needs build-time dependencies on DWARFUnit or other
+// higher-level Dwarf structures. This approach creates better layering and
+// allows DWARFExpression to be used from code which can't have dependencies on
+// those higher-level structures.
+
+  class DWARFUnit;
+  struct DIDumpOptions;
+  class raw_ostream;
+
+class DWARFExpressionPrinter {
+ public:
+   /// Print a Dwarf expression/
+   /// \param E to be printed
+   /// \param OS to this stream
+   /// \param GetNameForDWARFReg callback to return dwarf register name
+   static void print(const DWARFExpression *E, raw_ostream &OS,
+                     DIDumpOptions DumpOpts, DWARFUnit *U, bool IsEH = false);
+
+   /// Print the expression in a format intended to be compact and useful to a
+   /// user, but not perfectly unambiguous, or capable of representing every
+   /// valid DWARF expression. Returns true if the expression was sucessfully
+   /// printed.
+   ///
+   /// \param E to be printed
+   /// \param OS to this stream
+   /// \param GetNameForDWARFReg callback to return dwarf register name
+   ///
+   /// \returns true if the expression was successfully printed
+   static bool printCompact(const DWARFExpression *E, raw_ostream &OS,
+                            std::function<StringRef(uint64_t RegNum, bool IsEH)>
+                                GetNameForDWARFReg = nullptr);
+
+  /// Pretty print a register opcode and operands.
+  /// \param U within the context of this Dwarf unit, if any.
+  /// \param OS to this stream
+  /// \param DumpOpts with these options
+  /// \param Opcode to print
+  /// \param Operands to the opcode
+  ///
+  /// returns true if the Op was successfully printed
+  static bool prettyPrintRegisterOp(DWARFUnit *U, raw_ostream &OS,
+                                     DIDumpOptions DumpOpts, uint8_t Opcode,
+                                     ArrayRef<uint64_t> Operands);
+
+ private:
+   static bool printOp(const DWARFExpression::Operation *Op, raw_ostream &OS,
+                       DIDumpOptions DumpOpts, const DWARFExpression *Expr,
+                       DWARFUnit *U);
+
+   static void prettyPrintBaseTypeRef(DWARFUnit *U, raw_ostream &OS,
+                                      DIDumpOptions DumpOpts,
+                                      ArrayRef<uint64_t> Operands,
+                                      unsigned Operand);
+};
+
+} // end namespace llvm
+
+#endif // LLVM_DEBUGINFO_DWARF_DWARFEXPRESSION_H
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFVerifier.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFVerifier.h
index 717f9cedc4ee3..7c998a623769a 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFVerifier.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFVerifier.h
@@ -14,6 +14,7 @@
 #include "llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h"
 #include "llvm/DebugInfo/DWARF/DWARFAddressRange.h"
 #include "llvm/DebugInfo/DWARF/DWARFDie.h"
+#include "llvm/DebugInfo/DWARF/DWARFExpression.h"
 #include "llvm/DebugInfo/DWARF/DWARFUnitIndex.h"
 #include <cstdint>
 #include <map>
@@ -319,6 +320,23 @@ class DWARFVerifier {
   void verifyDebugNames(const DWARFSection &AccelSection,
                         const DataExtractor &StrData);
 
+  /// Verify that the the expression is valid within the context of unit U.
+  ///
+  /// \param E expression to verify.
+  /// \param U containing DWARFUnit, if any.
+  ///
+  /// returns true if E is a valid expression.
+  bool verifyExpression(const DWARFExpression &E, DWARFUnit *U);
+
+  /// Verify that the the expression operation is valid within the context of
+  /// unit U.
+  ///
+  /// \param Op operation to verify
+  /// \param U containing DWARFUnit, if any
+  ///
+  /// returns true if Op is a valid Dwarf operation
+  bool verifyExpressionOp(const DWARFExpression::Operation &Op, DWARFUnit *U);
+
 public:
   DWARFVerifier(raw_ostream &S, DWARFContext &D,
                 DIDumpOptions DumpOpts = DIDumpOptions::getForSingleDIE());
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
index 5bdc257fd8d89..c9b69169770e3 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
@@ -13,6 +13,7 @@
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/DebugInfo/DIContext.h"
 #include "llvm/DebugInfo/DWARF/DWARFDataExtractor.h"
+#include "llvm/DebugInfo/DWARF/DWARFExpression.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/DataExtractor.h"
 #include "llvm/Support/Errc.h"
@@ -109,7 +110,8 @@ void UnwindLocation::dump(raw_ostream &OS, DIDumpOptions DumpOpts) const {
       OS << " in addrspace" << *AddrSpace;
     break;
   case DWARFExpr: {
-    Expr->print(OS, DumpOpts, nullptr);
+    if (Expr)
+      DWARFExpressionPrinter::print(&(*Expr), OS, DumpOpts, nullptr);
     break;
   }
   case Constant:
@@ -943,7 +945,7 @@ void CFIProgram::printOperand(raw_ostream &OS, DIDumpOptions DumpOpts,
   case OT_Expression:
     assert(Instr.Expression && "missing DWARFExpression object");
     OS << " ";
-    Instr.Expression->print(OS, DumpOpts, nullptr);
+    DWARFExpressionPrinter::print(&(*Instr.Expression), OS, DumpOpts, nullptr);
     break;
   }
 }
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
index ec7af792efb06..fc71be32fdd79 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
@@ -116,7 +116,8 @@ static void dumpExpression(raw_ostream &OS, DIDumpOptions DumpOpts,
   std::optional<dwarf::DwarfFormat> Format;
   if (U)
     Format = U->getFormat();
-  DWARFExpression(Extractor, AddressSize, Format).print(OS, DumpOpts, U);
+  DWARFExpression E(Extractor, AddressSize, Format);
+  DWARFExpressionPrinter::print(&E, OS, DumpOpts, U);
 }
 
 bool DWARFLocationTable::dumpLocationList(
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDie.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
index a0ce7810f91b0..08dd9d30812d1 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
@@ -98,8 +98,8 @@ static void dumpLocationExpr(raw_ostream &OS, const DWARFFormValue &FormValue,
   ArrayRef<uint8_t> Expr = *FormValue.getAsBlock();
   DataExtractor Data(StringRef((const char *)Expr.data(), Expr.size()),
                      Ctx.isLittleEndian(), 0);
-  DWARFExpression(Data, U->getAddressByteSize(), U->getFormParams().Format)
-      .print(OS, DumpOpts, U);
+  DWARFExpression DE(Data, U->getAddressByteSize(), U->getFormParams().Format);
+  DWARFExpressionPrinter::print(&DE, OS, DumpOpts, U);
 }
 
 static DWARFDie resolveReferencedType(DWARFDie D, DWARFFormValue F) {
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp b/llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp
index 2ae5ff3efc8c5..dd1325d8f7491 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp
@@ -237,10 +237,23 @@ bool DWARFExpression::Operation::extract(DataExtractor Data,
   return true;
 }
 
-static void prettyPrintBaseTypeRef(DWARFUnit *U, raw_ostream &OS,
-                                   DIDumpOptions DumpOpts,
-                                   ArrayRef<uint64_t> Operands,
-                                   unsigned Operand) {
+std::optional<unsigned> DWARFExpression::Operation::getSubCode() const {
+  if (!Desc.Op.size() || Desc.Op[0] != Operation::SizeSubOpLEB)
+    return std::nullopt;
+  return Operands[0];
+}
+
+bool DWARFExpression::operator==(const DWARFExpression &RHS) const {
+  if (AddressSize != RHS.AddressSize || Format != RHS.Format)
+    return false;
+  return Data.getData() == RHS.Data.getData();
+}
+
+void DWARFExpressionPrinter::prettyPrintBaseTypeRef(DWARFUnit *U,
+                                                    raw_ostream &OS,
+                                                    DIDumpOptions DumpOpts,
+                                                    ArrayRef<uint64_t> Operands,
+                                                    unsigned Operand) {
   assert(Operand < Operands.size() && "operand out of bounds");
   if (!U) {
     OS << format(" <base_type ref: 0x%" PRIx64 ">", Operands[Operand]);
@@ -259,10 +272,9 @@ static void prettyPrintBaseTypeRef(DWARFUnit *U, raw_ostream &OS,
   }
 }
 
-bool DWARFExpression::prettyPrintRegisterOp(DWARFUnit *U, raw_ostream &OS,
-                                            DIDumpOptions DumpOpts,
-                                            uint8_t Opcode,
-                                            ArrayRef<uint64_t> Operands) {
+bool DWARFExpressionPrinter::prettyPrintRegisterOp(
+    DWARFUnit *U, raw_ostream &OS, DIDumpOptions DumpOpts, uint8_t Opcode,
+    ArrayRef<uint64_t> Operands) {
   if (!DumpOpts.GetNameForDWARFReg)
     return false;
 
@@ -293,87 +305,84 @@ bool DWARFExpression::prettyPrintRegisterOp(DWARFUnit *U, raw_ostream &OS,
   return false;
 }
 
-std::optional<unsigned> DWARFExpression::Operation::getSubCode() const {
-  if (!Desc.Op.size() || Desc.Op[0] != Operation::SizeSubOpLEB)
-    return std::nullopt;
-  return Operands[0];
-}
-
-bool DWARFExpression::Operation::print(raw_ostream &OS, DIDumpOptions DumpOpts,
-                                       const DWARFExpression *Expr,
-                                       DWARFUnit *U) const {
-  if (Error) {
+bool DWARFExpressionPrinter::printOp(const DWARFExpression::Operation *Op,
+                                     raw_ostream &OS, DIDumpOptions DumpOpts,
+                                     const DWARFExpression *Expr,
+                                     DWARFUnit *U) {
+  if (Op->Error) {
     OS << "<decoding error>";
     return false;
   }
 
-  StringRef Name = OperationEncodingString(Opcode);
+  StringRef Name = OperationEncodingString(Op->Opcode);
   assert(!Name.empty() && "DW_OP has no name!");
   OS << Name;
 
-  if ((Opcode >= DW_OP_breg0 && Opcode <= DW_OP_breg31) ||
-      (Opcode >= DW_OP_reg0 && Opcode <= DW_OP_reg31) ||
-      Opcode == DW_OP_bregx || Opcode == DW_OP_regx ||
-      Opcode == DW_OP_regval_type)
-    if (prettyPrintRegisterOp(U, OS, DumpOpts, Opcode, Operands))
+  if ((Op->Opcode >= DW_OP_breg0 && Op->Opcode <= DW_OP_breg31) ||
+      (Op->Opcode >= DW_OP_reg0 && Op->Opcode <= DW_OP_reg31) ||
+      Op->Opcode == DW_OP_bregx || Op->Opcode == DW_OP_regx ||
+      Op->Opcode == DW_OP_regval_type)
+    if (prettyPrintRegisterOp(U, OS, DumpOpts, Op->Opcode, Op->Operands))
       return true;
 
-  for (unsigned Operand = 0; Operand < Desc.Op.size(); ++Operand) {
-    unsigned Size = Desc.Op[Operand];
-    unsigned Signed = Size & Operation::SignBit;
+  for (unsigned Operand = 0; Operand < Op->Desc.Op.size(); ++Operand) {
+    unsigned Size = Op->Desc.Op[Operand];
+    unsigned Signed = Size & DWARFExpression::Operation::SignBit;
 
-    if (Size == Operation::SizeSubOpLEB) {
-      StringRef SubName = SubOperationEncodingString(Opcode, Operands[Operand]);
+    if (Size == DWARFExpression::Operation::SizeSubOpLEB) {
+      StringRef SubName =
+          SubOperationEncodingString(Op->Opcode, Op->Operands[Operand]);
       assert(!SubName.empty() && "DW_OP SubOp has no name!");
       OS << " " << SubName;
-    } else if (Size == Operation::BaseTypeRef && U) {
+    } else if (Size == DWARFExpression::Operation::BaseTypeRef && U) {
       // For DW_OP_convert the operand may be 0 to indicate that conversion to
       // the generic type should be done. The same holds for DW_OP_reinterpret,
       // which is currently not supported.
-      if (Opcode == DW_OP_convert && Operands[Operand] == 0)
+      if (Op->Opcode == DW_OP_convert && Op->Operands[Operand] == 0)
         OS << " 0x0";
       else
-        prettyPrintBaseTypeRef(U, OS, DumpOpts, Operands, Operand);
-    } else if (Size == Operation::WasmLocationArg) {
+        prettyPrintBaseTypeRef(U, OS, DumpOpts, Op->Operands, Operand);
+    } else if (Size == DWARFExpression::Operation::WasmLocationArg) {
       assert(Operand == 1);
-      switch (Operands[0]) {
+      switch (Op->Operands[0]) {
       case 0:
       case 1:
       case 2:
       case 3: // global as uint32
       case 4:
-        OS << format(" 0x%" PRIx64, Operands[Operand]);
+        OS << format(" 0x%" PRIx64, Op->Operands[Operand]);
         break;
       default: assert(false);
       }
-    } else if (Size == Operation::SizeBlock) {
-      uint64_t Offset = Operands[Operand];
-      for (unsigned i = 0; i < Operands[Operand - 1]; ++i)
+    } else if (Size == DWARFExpression::Operation::SizeBlock) {
+      uint64_t Offset = Op->Operands[Operand];
+      for (unsigned i = 0; i < Op->Operands[Operand - 1]; ++i)
         OS << format(" 0x%02x", Expr->Data.getU8(&Offset));
     } else {
       if (Signed)
-        OS << format(" %+" PRId64, (int64_t)Operands[Operand]);
-      else if (Opcode != DW_OP_entry_value &&
-               Opcode != DW_OP_GNU_entry_value)
-        OS << format(" 0x%" PRIx64, Operands[Operand]);
+        OS << format(" %+" PRId64, (int64_t)Op->Operands[Operand]);
+      else if (Op->Opcode != DW_OP_entry_value &&
+               Op->Opcode != DW_OP_GNU_entry_value)
+        OS << format(" 0x%" PRIx64, Op->Operands[Operand]);
     }
   }
   return true;
 }
 
-void DWARFExpression::print(raw_ostream &OS, DIDumpOptions DumpOpts,
-                            DWARFUnit *U, bool IsEH) const {
+void DWARFExpressionPrinter::print(const DWARFExpression *E, raw_ostream &OS,
+                                   DIDumpOptions DumpOpts, DWARFUnit *U,
+                                   bool IsEH) {
   uint32_t EntryValExprSize = 0;
   uint64_t EntryValStartOffset = 0;
-  if (Data.getData().empty())
+  if (E->Data.getData().empty())
     OS << "<empty>";
 
-  for (auto &Op : *this) {
+  for (auto &Op : *E) {
     DumpOpts.IsEH = IsEH;
-    if (!Op.print(OS, DumpOpts, this, U)) {
+    if (!printOp(&Op, OS, DumpOpts, E, U)) {
       uint64_t FailOffset = Op.getEndOffset();
-      while (FailOffset < Data.getData().size())
-        OS << format(" %02x", Data.getU8(&FailOffset));
+      while (FailOffset < E->Data.getData().size())
+        OS << format(" %02x", E->Data.getU8(&FailOffset));
       return;
     }
 
@@ -391,39 +400,11 @@ void DWARFExpression::print(raw_ostream &OS, DIDumpOptions DumpOpts,
         OS << ")";
     }
 
-    if (Op.getEndOffset() < Data.getData().size())
+    if (Op.getEndOffset() < E->Data.getData().size())
       OS << ", ";
   }
 }
 
-bool DWARFExpression::Operation::verify(const Operation &Op, DWARFUnit *U) {
-  for (unsigned Operand = 0; Operand < Op.Desc.Op.size(); ++Operand) {
-    unsigned Size = Op.Desc.Op[Operand];
-
-    if (Size == Operation::BaseTypeRef) {
-      // For DW_OP_convert the operand may be 0 to indicate that conversion to
-      // the generic type should be done, so don't look up a base type in that
-      // case. The same holds for DW_OP_reinterpret, which is currently not
-      // supported.
-      if (Op.Opcode == DW_OP_convert && Op.Operands[Operand] == 0)
-        continue;
-      auto Die = U->getDIEForOffset(U->getOffset() + Op.Operands[Operand]);
-      if (!Die || Die.getTag() != dwarf::DW_TAG_base_type)
-        return false;
-    }
-  }
-
-  return true;
-}
-
-bool DWARFExpression::verify(DWARFUnit *U) {
-  for (auto &Op : *this)
-    if (!Operation::verify(Op, U))
-      return false;
-
-  return true;
-}
-
 /// A user-facing string representation of a DWARF expression. This might be an
 /// Address expression, in which case it will be implicitly dereferenced, or a
 /// Value expression.
@@ -546,16 +527,11 @@ static bool printCompactDWARFExpr(
   return true;
 }
 
-bool DWARFExpression::printCompact(
-    raw_ostream &OS,
+bool DWARFExpressionPrinter::printCompact(
+    const DWARFExpression *E, raw_ostream &OS,
     std::function<StringRef(uint64_t RegNum, bool IsEH)> GetNameForDWARFReg) {
-  return printCompactDWARFExpr(OS, begin(), end(), GetNameForDWARFReg);
+  return printCompactDWARFExpr(OS, E->begin(), E->end(), GetNameForDWARFReg);
 }
 
-bool DWARFExpression::operator==(const DWARFExpression &RHS) const {
-  if (AddressSize != RHS.AddressSize || Format != RHS.Format)
-    return false;
-  return Data.getData() == RHS.Data.getData();
-}
 
 } // namespace llvm
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp b/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
index 43a62bdd8390d..c12786cac8686 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
@@ -659,6 +659,35 @@ unsigned DWARFVerifier::verifyDieRanges(const DWARFDie &Die,
   return NumErrors;
 }
 
+bool DWARFVerifier::verifyExpressionOp(const DWARFExpression::Operation &Op,
+                                       DWARFUnit *U) {
+  for (unsigned Operand = 0; Operand < Op.Desc.Op.size(); ++Operand) {
+    unsigned Size = Op.Desc.Op[Operand];
+
+    if (Size == DWARFExpression::Operation::BaseTypeRef) {
+      // For DW_OP_convert the operand may be 0 to indicate that conversion to
+      // the generic type should be done, so don't look up a base type in that
+      // case. The same holds for DW_OP_reinterpret, which is currently not
+      // supported.
+      if (Op.Opcode == DW_OP_convert && Op.Operands[Operand] == 0)
+        continue;
+      auto Die = U->getDIEForOffset(U->getOffset() + Op.Operands[Operand]);
+      if (!Die || Die.getTag() != dwarf::DW_TAG_base_type)
+        return false;
+    }
+  }
+
+  return true;
+}
+
+bool DWARFVerifier::verifyExpression(const DWARFExpression &E, DWARFUnit *U) {
+  for (auto &Op : E)
+    if (!verifyExpressionOp(Op, U))
+      return false;
+
+  return true;
+}
+
 unsigned DWARFVerifier::verifyDebugInfoAttribute(const DWARFDie &Die,
                                                  DWARFAttribute &AttrValue) {
   unsigned NumErrors = 0;
@@ -727,7 +756,7 @@ unsigned DWARFVerifier::verifyDebugInfoAttribute(const DWARFDie &Die,
             any_of(Expression, [](cons...
[truncated]

Copy link

github-actions bot commented May 8, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@dwblaikie
Copy link
Collaborator

@jmorse DWARF expressions might be your wheelhouse/you know someone who'd be up for reviewing this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants