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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.



>> 2. Should we try to avoid one-time dynamic initialization for the DW_OP description table
> 
> Would it perhaps make sense to initialize the table from the macros in `Dwarf.def`?

+1, worth considering.

>> 3. Should we remove the "OP_" prefix
> 
> I think we should implement two behaviors: When DumpOptions::Brief is set, we should print expressions with DW_OP_ stripped and registers in human-readable form, e.g.: `rdi+42, deref, stack-value`, otherwise it should print the full expression, e.g.: `DW_OP_breg1, DW_OP_constu 42, DW_OP_deref, DW_OP_stack_value`.

Sounds good & consistent to me!

> (And at some point soon, I'll upload a patch to make brief the default instead of verbose).

Excellent

With all that in mind - Signing off on this contingent on removing teh drop(3) (so printing DW_OP_* rather than OP_*). Considering all the other stuff for consideration before or shortly after this is committed, as you see fit, Reid.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h:100
+    uint32_t Offset;
+    Op Op;
+    iterator(DWARFExpression *Expr, uint32_t Offset)
----------------
Naming the variable the same as the type is a bit awkward (given the need to then qualify the class name later ("class Op")) - maybe 'O' would do? The scope's short enough the name shouldn't be too confusing.

Or naming the type "Operation" might be helpful.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp:106
+  static DescVector &Descriptions = getDescriptions();
+  assert(OpCode < Descriptions.size());
+  return Descriptions[OpCode];
----------------
rnk wrote:
> aprantl wrote:
> > Does this assert on invalid DWARF? Or is this an internal consistency check?
> `Descriptions` actually has 256 entries, and the opcodes are one byte, so this can't fail as written.
Then shouldn't the parameter be a one byte sized type & that ought to follow through to the source of the data?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp:228
+  assert(!Name.empty() && "DW_OP has no name!");
+  OS << Name.drop_front(3);
+
----------------
Reckon it's worth dropping 6? (OP_ is pretty redundant)

Though as-is, or with that suggestion, it's a bit inconsistent with our other DWARF enum printing (that keeps the DW_ prefix). 

Maybe we should just drop the DW_ suffix everywhere? (I wonder about dropping DW_*_ everywhere - but maybe that'll only be done in 'brief' mode)


https://reviews.llvm.org/D37123