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

Thu Jul 30 05:46:19 PDT 2020

jmorse added a comment.

I sense this is the bulk of the work; and it looks good. I've posted on llvm-dev asking whether the creation of artificial variables could instead be deferred til DWARF emission. That'll avoid anything having to deal with a new form of DBG_VALUE, and us having to think about their lifetimes. Plus, there's greater scope for de-duplication, as I understand it this patch will generate a new artificial variable for each dbg.value, for each level of indirection.

On that topic -- do we need to give the artificial variable a name? I'm no DWARF expert, but can we just not attach a DW_AT_name to the DIE?

================
Comment at: llvm/include/llvm/CodeGen/MachineInstrBuilder.h:240-242
+    assert(((MI->isDebugValue())
+                ? static_cast<bool>(MI->getDebugImplicitVariable())
+                : true) &&
----------------
I think the preferred form is `!MI->isDebugValue() || static_cast....`, i.e., the assertion always passes if the MI isn't a DBG_VALUE, or if it is, the getDebugImplicitVariable() part is tested. This avoids the reader having to consider an un-necessary ternary operator.

================
Comment at: llvm/include/llvm/CodeGen/MachineInstrBuilder.h:244-246
+    assert(
+        (MI->isDebugLabel() ? static_cast<bool>(MI->getDebugLabel()) : true) &&
+        "first MDNode argument of a DBG_LABEL not a label");
----------------
(Same as above)

================
Comment at: llvm/include/llvm/IR/DIBuilder.h:619
                        uint32_t AlignInBits = 0);

+    DILocalVariable *createImplicitTempVariable(DIScope *Scope, DIType *Ty,
----------------
This'll want a docu-comment as to createImplicitTempVariable's purpose.

================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2540

+  /// Return whether this is an Implicit Pointer
+  bool isImplicitPointer() const {
----------------
nit: Full stops at ends of comments, here and elsewhere.

================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2546
+
+  /// Return the number of indirction levels
+  unsigned explicitPointerCount() const {
----------------
"indirection"

================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:101
   Value *getVariableLocation(bool AllowNullOp = true) const;

+  Value *getDerefVariable(bool AllowNullOp = true) const;
----------------
Doc comment needed too please.

================
Comment at: llvm/lib/CodeGen/AsmPrinter/DebugLocEntry.h:67-69
+  /// Or an implicit pointer variable target.
+  const DIVariable *ImpPtr;
+
----------------
Shouldn't this be in the union?

================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h:81-83
+  // objects. We keep index of dereferenced object's at first operand of
+  // DW_OP_implicit_pointer operation temporarily. This temporary value is
+  // later replaced by actual value in function DwarfDebug::emitDebugLocEntry.
----------------

================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h:84
+  // later replaced by actual value in function DwarfDebug::emitDebugLocEntry.
+  SmallVector<const DINode *, 8> ImplicitVars;
+
----------------
Would it be possible to use a subtype of DINode instead, such as DIVariable? That'll increase type safety and make it clearer what's going on.

================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h:133
+  unsigned findOrInsertImplicitVar(const DINode *Var) {
+    SmallVector<const DINode *, 8>::iterator It =
+        std::find(ImplicitVars.begin(), ImplicitVars.end(), Var);
----------------
I'd suggest the return type can be `auto` here.

================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2518
+        DIE *Die = CU->getDIE(ImpVar);
+        if (Die)
+          ValOffset = Die->getDebugSectionOffset();
----------------
I reckon it's better to `assert(Die);`, there should be no scenarios where the implicit pointer DIE isn't emitted, right? Otherwise, if this `if` condition is false, you'll write the unmodified ValOffset into the output dwarf, which could be misleading.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1323-1324

+bool SelectionDAGBuilder::handleDebugValueWrapper(const DbgValueInst &DI,
+                                                  DebugLoc dl, unsigned Order) {
+  DILocalVariable *Variable = DI.getVariable();
----------------
NB: this function definitely needs some comments in it as to what's going on.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1331
+  unsigned Level = Expression->explicitPointerCount();
+  if (Level > 0) {
+    DIScope *Scope = DI.getDebugLoc()->getScope();
----------------
Better to test for Level==0 and have the normal-dbg-value case return early; then the rest of this function doesn't need an extra level of indentation / scope.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1390
+      return true;
+    }
+  }
----------------
Correct me if I'm wrong, but isn't generation of DW_OP_LLVM_implicit_pointer only done during instruction selection? If so, can we not reject implicit_pointer expressions at this stage as being illegal IR?

================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:117-119
+  if (U)
+    DWARFExpression(Extractor, AddressSize, U->getFormParams().Format)
+        .print(OS, MRI, U);
----------------
It looks like this contradicts the immediately preceeding comment, could you update the comment please?

================
Comment at: llvm/test/DebugInfo/X86/dwarfdump-implicit_pointer_instcomb.ll:4-12
+; CHECK-LABEL:  DW_AT_name    ("__implicit_ptr_tmp_0")
+; CHECK-LABEL:  DW_TAG_variable
+; CHECK-NEXT:  DW_AT_location
+; CHECK-NEXT:  DW_OP_implicit_pointer {{0x.+}} +0
+
+; CHECK-LABEL:  DW_TAG_formal_parameter
+; CHECK-NEXT:  DW_AT_location
----------------
Don't both ptr and ptrVar become implicit pointers? Does this lead to two implicit_ptr_tmp variables being created?

It'd also be good to capture/compare the operand to DW_OP_implicit_pointer to the DIE address that it refers to, to confirm that it points to the correct place.

================
Comment at: llvm/test/DebugInfo/X86/dwarfdump-implicit_pointer_instcomb.ll:7
+; CHECK-NEXT:  DW_AT_location
+; CHECK-NEXT:  DW_OP_implicit_pointer {{0x.+}} +0
+
----------------
CHECK-NEXT: DW_AT_name "ptrVar"?

================
Comment at: llvm/test/DebugInfo/X86/dwarfdump-implicit_pointer_mem2reg.ll:5
+; CHECK-LABEL:  DW_AT_name    ("arr1")
+
+; 1. Test if more than one member location list is printed 
----------------
Again, in this test it would be good to ensure that DW_OP_implicit_pointer points at the correct DIE. If the intention here is to only test for multiple levels of implicit pointer, it's probably sufficient to only check one variable.

================
Comment at: llvm/test/DebugInfo/X86/dwarfdump-implicit_pointer_sroa.ll:3
+; RUN: llvm-dwarfdump  %t.o | FileCheck %s
+
+; CHECK-LABEL:  DW_AT_name    ("arr2")
----------------
It'd be good to test for non-zero offsets into an implicit variable -- i.e., in the original C, have ptr3 something like:

  ptr3 = &arr1[1];

================
Comment at: llvm/test/DebugInfo/X86/dwarfdump-implicit_pointer_sroa_inline.ll:7-11
+; CHECK:       DW_TAG_formal_parameter
+; CHECK-NEXT:    DW_AT_location
+; CHECK-NEXT:     : DW_OP_implicit_pointer {{0x.+}} +0
+; CHECK-NEXT:     : DW_OP_implicit_pointer {{0x.+}} +0)
+; CHECK-NEXT:    DW_AT_abstract_origin       ({{0x.+}} "ptr")
----------------
So, there'll be a DW_OP_implicit_pointer for each value a pointer-variable can point at, rather than one DW_OP_implicit_pointer and two entries in the variable/location-list it refers to?

(This might not be the most efficient way of doing it; maybe it's sufficient for now though).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84115