[Lldb-commits] [PATCH] D70644: [DebugInfo] Support for DW_OP_implicit_pointer (llvm.dbg_derefval)

[Jeremy Morse via Phabricator via lldb-commits]

jmorse added a comment.

Thanks for the patch, comments inline. A couple of things in general:

As mentioned on the mailing list thread, it bothers me a little that DbgVariableIntrinsic::getVariableLocation is going to try and return something that isn't a "Real" location. In fact, wouldn't the assertion at the end of getVariableLocation fire if the operand was a non-null non-ValueAsMetadata piece of metadata? It might make sense to add another level to the class hierarchy, to distinguish variable intrinsics that definitely always have a location, and the "more complicated" situation dbg.derefval has.

(I tried to think up what I'd like a class hierarchy to look like for all the debugging intrinsics, but they all have substantially different use cases).

I haven't reviewed the changes to SelectionDAG, I think those are probably better placed in D69886 <https://reviews.llvm.org/D69886> with the other CodeGen changes, would you be able to move the changes into that review? (Both modify SelectionDAGBuilder.cpp). That way all the SelectionDAG modifications are in one place, and have the tests with them.



================
Comment at: llvm/docs/SourceLevelDebugging.rst:261
+This intrinsic is identical to `llvm.dbg.value`, except that it is used to
+describe dereferenced value of pointer variable. The first argument is metadata.
+The second argument is a `local variable <LangRef.html#dilocalvariable>`_
----------------
I'd add some motivation to the first sentence, such as: "it it used to describe the dereferenced value of a pointer variable, when the pointer itself is no longer available in the program".


================
Comment at: llvm/docs/SourceLevelDebugging.rst:266-267
+interpreted based on the third argument. If the third argument represents
+DW_OP_LLVM_implicit_pointer, the first argument is
+ `local variable <LangRef.html#dilocalvariable>`_ , otherwise it is the value
+(wrapped as metadata).
----------------
IMO "is" should read "must", i.e. "the first argument must be a local variable", to indicate this is required for correctness.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:369
 
+  /// Build and insert a DBG_VALUE instructions specifying that dereferenced
+  /// value of \p Variable is given by \p V (suitably modified by \p Expr).
----------------
"that the dereferenced"?


================
Comment at: llvm/include/llvm/CodeGen/MachineInstrBuilder.h:227-238
+  const MachineInstrBuilder &addMetadata(const MDNode *MD,
+                                         bool IsImplicitPointer = false) const {
     MI->addOperand(*MF, MachineOperand::CreateMetadata(MD));
-    assert((MI->isDebugValue() ? static_cast<bool>(MI->getDebugVariable())
-                               : true) &&
+    assert(((MI->isDebugValue() && !IsImplicitPointer)
+                ? static_cast<bool>(MI->getDebugVariable())
+                : true) &&
            "first MDNode argument of a DBG_VALUE not a variable");
----------------
Perhaps it's worth adding a new "add" method, i.e. "addImplicitPtrMetadata"? That avoids the extra argument, and in the call sites below it will be a little clearer what's happening, as a "true" argument isn't especially helpful.

Plus, then you can assert that only DILocalVariables (or whatever) are added as the implicit pointer metadata.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:717-718
   if (isDbgValueDescribedByReg(MI) || MI.getOperand(0).isImm() ||
-      MI.getOperand(0).isFPImm() || MI.getOperand(0).isCImm()) {
+      MI.getOperand(0).isFPImm() || MI.getOperand(0).isCImm() ||
+      MI.getOperand(0).isMetadata()) {
     // Use normal VarLoc constructor for registers and immediates.
----------------
The main VarLoc constructor called on line 720 is going to not recognise the metadata operand and leave some fields uninitialized (it should probably actually assert in this case). I reckon you'll need to add a new location "Kind", and have it emitted in the BuildDbgValue method.

The LiveDebugValues algorithm should (TM) handle and propagate this kind of location fine.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:1447
   }
+  case Intrinsic::dbg_derefval: {
+    // This form of DBG_VALUE is target-independent.
----------------
Perhaps easier here instead to have dbg_derefval and dbg_value share code, with an additional assertion that MetadataAsValue operands can only come from dbg_value's?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70644