[PATCH] D31439: PR32382: Emit complex DWARF expressions with the correct location description kind

[David Blaikie via Phabricator via llvm-commits]

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