[PATCH] D84118: [Debuginfo] (6/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.

Could you elaborate on why module-level information needs to be preserved? As mentioned on llvm-dev, I think you might have missed a place where promotion from stack to SSA/values happens -- IMHO we shouldn't add module-level debuginfo-tracking unless it's absolutely necessary.



================
Comment at: llvm/include/llvm/IR/Module.h:205
+  /// we will preserve location of variable in DenseMap to use later
+  DenseMap<Value *, Value *> VarAddrMap;
 
----------------
You'll need to use a value handle (WeakVH perhaps) to store Value references, to avoid storing pointers that become dangling pointers later. The comment should indicate what's being mapped -- I believe it's old alloca to the DILocalVariable, because insertVarAddressMap inserts the second operand of a dbg.declare?

In addition, it looks like salvageDebugInfoForDbgValues never dereferences the mapped value, so could this instead be a set instead of a map?


================
Comment at: llvm/lib/IR/Instruction.cpp:84-85
 iplist<Instruction>::iterator Instruction::eraseFromParent() {
+  // Try to preserve debug information attached to the instruction.
+  salvageDebugInfo();
+
----------------
Does this make the debug-info metadata code in the Instruction destructor redundant?


================
Comment at: llvm/lib/IR/Instruction.cpp:795-798
+  if (auto DDI = dyn_cast<DbgVariableIntrinsic>(this))
+    if (getModule()->getDwarfVersion() >= 5)
+      if (DDI->isAddressOfVariable())
+        getModule()->insertVarAddressMap(DDI);
----------------
The rest of Instruction.cpp is very detail-agnostic, and it seems out of place to check the DWARF version here (and presumably meaningless for CodeView). Could this be turned into a more generic compile option, or target option, so that it's less DWARF specific?

(It might be that LLVM doesn't support such a form of option).


================
Comment at: llvm/lib/IR/Instruction.cpp:807-808
+
+void Instruction::salvageDebugInfoForDbgValues(
+    ArrayRef<DbgVariableIntrinsic *> DbgUsers) {
+  auto &Ctx = getContext();
----------------
Quite a lot of this function is now dedicated to "if we're an alloca being deleted, that had associated dbg.declares". IMO, it's clearer to put that code into a separate function.


================
Comment at: llvm/test/DebugInfo/X86/implicit_pointer_temp_dyn_alloc.cc:1-2
+// RUN: clang %s -O2 -gdwarf-5 -S -emit-llvm -o %t.ll
+// RUN: FileCheck %s --input-file=%t.ll
+
----------------
We can't call clang from within llvm as it's not guaranteed to be built. Getting the unoptimised IR with `-O0 -gdwarf-5 -emit-llvm -S -Xclang -disable-O0-optnone`, and writing a normal lit test for that, would be best.

You might also want to put an end-to-end Dexter test into the debuginfo-tests.


================
Comment at: llvm/unittests/IR/DebugInfoTest.cpp:185
   I.eraseFromParent();
-  EXPECT_TRUE(isa<UndefValue>(DVIs[0]->getValue()));
+  EXPECT_FALSE(isa<UndefValue>(DVIs[0]->getValue()));
 }
----------------
This should be a positive test that the `dbg.value` points at a correct thing, not that it doesn't point at an undef. There are a variety of other Values it could point at by mistake.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84118