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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

(Adrian: Reckon it's worth dropping the "OP_" prefix too, to simplify? (or bump it up to "DW_OP_" and then maybe have the brief version drop "DW_X_" prefixes across the board (since they're unambiguous?)



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLoc.h:72
     uint32_t Length;
     SmallVector<unsigned char, 4> Loc;
   };
----------------
Worth switching this from 'unsigned char' to 'char' to avoid the conversions back and forth on reading/writing? (more compatible with DataExtractor, etc)


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h:125
+  iterator end() { return iterator(this, Data.getData().size()); }
+  iterator_range<iterator> operations() { return make_range(begin(), end()); }
+
----------------
What's the point of this function? Since the DWARFExpression is already a range it seems strange to have a range accessor on top of that. It's used in one place where "operations()" could be replaced with "*this" I think?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:870-871
+  if (!Error.empty()) {
+    // FIXME: Races.
+    static bool OneReport;
+    if (!OneReport)
----------------
Worth fixing sooner rather than later? (ie: before this is committed) - maybe a callback for warnings here?

(in addition to racing, it's not re-usable - multiple uses of libDebugInfo in a single process would see this only once which seems unfortunate)


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:32-33
+    OS.indent(Indent);
+    OS << format("0x%016" PRIx64, E.Begin+Unit->getBaseAddress()) << " - "
+       << format("0x%016" PRIx64, E.End+Unit->getBaseAddress()) << ": ";
+
----------------
Formatting (I guess there should be spaces around the '+')


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:32-33
+    OS.indent(Indent);
+    OS << format("0x%016" PRIx64, E.Begin+Unit->getBaseAddress()) << " - "
+       << format("0x%016" PRIx64, E.End+Unit->getBaseAddress()) << ": ";
+
----------------
dblaikie wrote:
> Formatting (I guess there should be spaces around the '+')
This handling of the base address could probably be a separate patch & would justify plumbing through the DWARFUnit, etc, simplifying this patch that needs it for the expression dumping as well.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDie.cpp:104
+
+    if (LocSection.Data.size()) {
+      DWARFDebugLoc DebugLoc;
----------------
Prefer !empty over size, I think. (similarly below)


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp:32-33
+
+  Descriptions.resize(0xff);
+  Descriptions[DW_OP_addr] = Desc(Op::Dwarf2, Op::SizeAddr);
+  Descriptions[DW_OP_deref] = Desc(Op::Dwarf2);
----------------
This reinitializes (possibly racing) the vector contents every time it's called? Maybe use {} init instead?

(I see it's static and only called once, but still a bit weird - I'd probably just drop this function and use {} init in the function below that needs it)


https://reviews.llvm.org/D37123