[PATCH] [dwarfdump] Pretty print location expressions and location lists

[David Blaikie]

Looks pretty reasonable.

Alexey - want to have a quick glance over it, since the dumping code is more your domain these days?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFExpression.h:134
@@ +133,3 @@
+
+bool operator==(const DWARFExpression::iterator &LHS,
+                const DWARFExpression::iterator &RHS) {
----------------
This would need to be defined with the "inline" keyword or it'll create ODR violations/duplicate symbol linker errors, no?

I'd just put the definition inline in the friend declaration:

  friend bool op==(X, Y) {
    ...
  }

================
Comment at: include/llvm/DebugInfo/DWARFExpression.h:99
@@ +98,3 @@
+  public:
+    void operator++() {
+      Offset = Op.isError() ? Expression->Data.getData().size() : Op.EndOffset;
----------------
would be nice to implement ++ the usual way, returning a reference to the object, rather than void (I forget which one is pre or post increment - anyway, whichever way it's meant to work, either a copy of the unincremented value, or a reference to the incremented value)

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp:130
@@ +129,3 @@
+    if (LocSection.Data.size()) {
+      DWARFDebugLoc DebugLoc(LocSection.Relocs);
+      DataExtractor Data(LocSection.Data, U->getContext().isLittleEndian(), 0);
----------------
Could possibly abstract over these two differences using a function template helper?

Or maybe it'd suffice to put the if (LL)/else outside the if conditios so it can be shared (& could add an else to the main if (sec.data.size()) that returns early to make that work? Dunno how that'd look)

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:31
@@ +30,3 @@
+
+    DataExtractor Data(StringRef((const char*)E.Loc.data(), E.Loc.size()),
+                       Unit->getDebugInfoExtractor().isLittleEndian(),
----------------
Maybe we should decide whether binary data bytes should be signed or unsigned. Either change DataExtractor to deal with ArrayRef<unsigned char> or change DwarfDebugLoc::Entry::Loc to be a SmallVector<char, N> instead.

================
Comment at: lib/DebugInfo/DWARF/DWARFExpression.cpp:32
@@ +31,3 @@
+  Descriptions.resize(0xff);
+  Descriptions[DW_OP_addr] = Desc(Op::Dwarf2, Op::SizeAddr);
+  Descriptions[DW_OP_deref] = Desc(Op::Dwarf2);
----------------
This might be more succinctly to define as some kind of array literal?

================
Comment at: lib/DebugInfo/DWARF/DWARFExpression.cpp:128
@@ +127,3 @@
+
+    switch (Size & ~Op::SignBit) {
+    case Op::Size1:
----------------
Perhaps sink this switch into a separate function to avoid the Operands[Operand] repetition in every case & improve readability a little by splitting things up?

http://reviews.llvm.org/D6771

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/