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

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 5 inline comments as done.
rnk added a comment.

I think the remaining concerns are:

1. Should we split the location list base address change into another patch
2. Should we try to avoid one-time dynamic initialization for the DW_OP description table
3. Should we remove the "OP_" prefix

In general, I'd like to defer anything that people feel can wait, so that I can minimize the amount of time that I'm carrying this patch locally. It has a high risk for conflicts vs. people adding new dwarf tests, and I've already put in a lot of work updating the test suite.



================
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()); }
+
----------------
dblaikie wrote:
> 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?
Good question, removed.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:870-871
+  if (!Error.empty()) {
+    // FIXME: Races.
+    static bool OneReport;
+    if (!OneReport)
----------------
dblaikie wrote:
> 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)
I made this an optional method that the creator can call to enable register name dumping. It returns an error. All clients that don't call it will just get dumps like "OP_reg5" instead of "OP_reg5 RDI", which isn't a big deal.


================
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);
----------------
dblaikie wrote:
> 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)
The problem with braced initialization is that not all 256 entries are filled, so it requires duplicating not just the order of the expression opcode entries, but also the values. I didn't like that. We could make the Descriptions vector not be static, and make the caller take the vector by value so it owns it.

I was able to rewrite this to return a constexpr array inside a struct wrapper, but MSVC didn't like it.


https://reviews.llvm.org/D37123