[PATCH] D37123: [dwarfdump] Pretty print location expressions and location lists
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 28 20:38:13 PDT 2017
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
More information about the llvm-commits
mailing list