[PATCH] D84119: [Debuginfo] (7/N) Support for DW_OP_implicit_pointer for named and unnamed variables (second strategy).

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Suggested on llvm-dev: I reckon you also need to add logic to the ConvertDebugDeclareToDebugValue helpers too, or otherwise factor them in. This patch only covers the two fast-cases that mem2reg considers, single store allocas and single block allocas, I believe (98%) everything else goes through ConvertDebugDeclareToDebugValue.

The tests should also be IR tests that run a single pass (mem2reg) and ensure they do the right thing for a fixed IR input; anything else is vulnerable to changes elsewhere in the compiler.



================
Comment at: llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp:421
   }
+
+  if (AI->getModule()->getDwarfVersion() >= 5) {
----------------
All this new code added will need comments added explaining the intention behind it; it's hard to review otherwise.


================
Comment at: llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp:434-437
+      if (Itr != EndItr && Itr->getOp() == dwarf::DW_OP_deref)
+        Itr++;
+      else
+        Ops.push_back(dwarf::DW_OP_LLVM_explicit_pointer);
----------------
This is a side-note rather than a review comment:

As I understand it, this is ensuring that if someone writes:

    dbg.value(%ptr, !123, !DIExpression(DW_OP_deref))

Then we just make the variable refer to the promoted value, rather than making it an implicit pointer. IMO, this isn't something we truly need to support, as a dbg.value like that is is already faulty, see PR40628 [0], they're vulnerable to store re-ordering.

(The overall solution to DW_OP_deref in dbg.values is probably to have the IR verifyer deny them).

https://bugs.llvm.org/show_bug.cgi?id=40628


================
Comment at: llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp:547-552
+    for (User *U : AI->users())
+      if (StoreInst *SI = dyn_cast<StoreInst>(U))
+        StoresByIndex.push_back(
+            std::make_pair(LBI.getInstructionIndex(SI), SI));
+
+    llvm::sort(StoresByIndex, less_first());
----------------
StoresByIndex isn't modified after creation; why does it need to be cleared and re-created here?


================
Comment at: llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp:577
+      if (I == StoresByIndex.begin()) {
+#if 0 // do nothing
+	if (StoresByIndex.empty())
----------------
(The disabled code here isn't for review, right)?


================
Comment at: llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp:601-619
+        DIBuilder DIB(*AI->getModule(), /*AllowUnresolved*/ false);
+        auto *DIVar = DI->getVariable();
+        auto *DIExpr = DI->getExpression();
+        SmallVector<uint64_t, 8> Ops;
+        DIExpression::expr_op_iterator Itr = DIExpr->expr_op_begin();
+        DIExpression::expr_op_iterator EndItr = DIExpr->expr_op_end();
+        if (Itr != EndItr && Itr->getOp() == dwarf::DW_OP_deref)
----------------
It seems a fair amount of this is shared with the single-value promoter, it can probably be moved to a helper function.


================
Comment at: llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp:620
+        DIB.insertDbgValueIntrinsic(ReplVal, DIVar, DIExpr, NewLoc, DI);
+
+        DbgValueByIndexTy::iterator Iter = llvm::lower_bound(
----------------
It's unclear what the code below is doing without a comment.


================
Comment at: llvm/test/DebugInfo/X86/dwarfdump-implicit_pointer_instcomb.c:1-2
+// RUN: clang %s -O2 -gdwarf-5 -o %t.o
+// RUN: llvm-dwarfdump  %t.o | FileCheck %s
+
----------------
As with patch six, we can't call clang from llvm tests, this needs to be tested by using "opt" on unoptimized IR. Because this is testing an IR pass, it should test that the correct LLVM-IR output is produced, not an object file.

I agree there should be some kind of end-to-end test; as said in patch six, this is best done in debug-info / Dexter tests, where we can ensure that the debugging experience is as expected, without being too specific about how it's done (which introduces a maintenence burden).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84119