[PATCH] D31439: PR32382: Emit complex DWARF expressions with the correct location description kind
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 3 14:35:49 PDT 2017
dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.
Couple of minor things. Not sure I fully understand everything here, but seems plausible.
================
Comment at: include/llvm/IR/DebugInfoMetadata.h:2252-2257
+ bool endsWithXDeref() const {
+ unsigned Last = 0;
+ for (auto &Op : expr_ops())
+ Last = Op.getOp();
+ return Last == dwarf::DW_OP_xderef;
+ }
----------------
linear algorithm for testing the last element? that doesn't seem ideal (getElements().back() instead?) (conditional on !getElements().empty())
================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:1536-1540
+ for (I = 0; I < N - 1; ++I) {
+ Elts[I] = Elts[I + 1];
+ if (N > 3 && I == N - 4 && Elts[I + 1] == dwarf::DW_OP_LLVM_fragment)
+ break;
+ }
----------------
aprantl wrote:
> dblaikie wrote:
> > This seems a bit complicated - any way to simplify it/make it more obvious? (or at least add comments)
> >
> > Some of it looks the same as the condition in the case 0 of this switch. Could it be factored into a common function wiht a name that might aid readability?
> It doesn't have enough in common to be factored out, but perhaps adding the comment
> ```
> //[DW_OP_deref, ..., DW_OP_LLVM_fragment, x, y]
> // -> [..., DW_OP_deref, DW_OP_LLVM_fragment, x, y]
> ```
> makes it more obvious what is happening there.
Maybe it'd be easier if it were a std::copy?
if (deref) {
auto End = Elts.end();
if (Elts.size() >= 3 && std::prev(End, 3) == fragment)
std::prev(End, 3);
std::move(std::next(Elts.begin()), End, Elts.begin());
*std::prev(End) = fragment;
}
Something like that?
================
Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:228-230
+ // FIXME:
+ // Don't emit locations that cannot be expressed without DW_OP_stack_value.
+
----------------
Should fixme be conditional on "when using DWARF N < M" (where M is whatever version that added stack_value). Would be good to mention that in the comment if that's the case.
https://reviews.llvm.org/D31439
More information about the llvm-commits
mailing list