[PATCH] D158185: [DebugInfo] Process single-location debug values in variadic form when producing DWARF

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando accepted this revision.
Orlando added a comment.

LGTM too with some inline nits/questions.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h:159
+    // which requires it to be non-variadic.
+    if (auto E = DIExpression::convertToNonVariadicExpression(
+            ValueLoc->getExpression()))
----------------
style nit: outer `if` should have braces.


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:1347
 }
 bool DIExpression::isEntryValue() const {
+  auto singleLocElts = getSingleLocationExpressionElements();
----------------
Can we have DIExpressions where there are two entry_value ops (applied to two `DW_OP_LLVM_arg`s)?

Perhaps that's a separate issue or maybe there is special case `isEntryValue`-equivalent code somewhere for variaidics?


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:1353
 bool DIExpression::startsWithDeref() const {
-  return getNumElements() > 0 && getElement(0) == dwarf::DW_OP_deref;
+  auto singleLocElts = getSingleLocationExpressionElements();
+  return singleLocElts.size() > 0 && singleLocElts[0] == dwarf::DW_OP_deref;
----------------
Should these functions be asserting that they're not variadic?


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:1548
+  if (getNumElements() < 2 || Elements[0] != dwarf::DW_OP_LLVM_arg ||
+      Elements[1] != 0)
+    return Elements;
----------------
What's the purpose of the `Elements[1] != 0` check? Could we ever end up with a variadic dbg intrinsic that has 2 location args, where only the second is referenced in the expression (e.g. `DW_OP_LLVM_arg, 1, ...` with arg 0 not referenced)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158185/new/

https://reviews.llvm.org/D158185