[PATCH] D69886: [DebugInfo] Support for DW_OP_implicit_pointer (Post IR transformation phase)

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

Hey @alok, thanks for working on this.

Currently, the operands of dbg.value constitute a `{value, variable, expression}` tuple. This patch seems to change that to `{value | variable-to-be-deref'd, variable, expression | expression-with-implicit-pointer}`. What alternatives have you considered? While reading your tests, I found it hard to reason about how implicit pointer values are referenced & defined. This made me concerned about the amount of hidden state that the current approach would introduce into IR. Further, I'm not really convinced this does the right thing in the presence of inlining (at least, not yet). And I'm keen to avoid extending SelectionDAG to represent implicit pointer debug values, as that presumably creates work on the GlobalISel side as well.

One possible alternative (haven't fully thought this through yet, but here goes!) might be to have `dbg.value(value, variable, expr)` return a token value, instead of void. That token could then be referenced later, like: `dbg.value(%implicit_pointer_token, <variable>, DIExpression(DW_OP_implicit_pointer(<offset>)))`. The main advantage of this is that it makes the link between a description of the implicit pointer and its dereference explicit. I think this will be a lot easier to reason about. But there are more concrete benefits. E.g., if a pass were to move the DW_OP_implicit_pointer dbg.value //before// the def of its implicit pointer token, we'd detect the use-before-def instead of silently emitting the wrong value.

p.s: Could you number your patches (e.g. [1/3], [2/3], etc) to make them easier to find? Could you also make the tests for each patch independent so that they can be landed incrementally?



================
Comment at: llvm/test/DebugInfo/dwarfdump-implicit_pointer_instcomb.ll:1
+; RUN: clang %s -O2 -gdwarf-5 -o %t.o
+; RUN: llvm-dwarfdump  %t.o | FileCheck %s
----------------
alok wrote:
> jmorse wrote:
> > I don't believe running clang in an internal LLVM test is supported, this should probably be running llc -O2 -filetype=obj instead? Same with the other tests.
> Thanks for pointing it out. I shall incorporate this comment.
Please include the original C source when applicable, so that future authors have steps to regenerate the test if needed.


================
Comment at: llvm/test/DebugInfo/dwarfdump-implicit_pointer_mem2reg.ll:7
+; 1. Test if More than one member location list is printed 
+;    (for pointer to scalar)
+; CHECK:       DW_AT_location        (
----------------
Please only capitalize words where necessary.


================
Comment at: llvm/test/DebugInfo/dwarfdump-implicit_pointer_mem2reg.ll:9
+; CHECK:       DW_AT_location        (
+; CHECK-NEXT:     : DW_OP_lit0, DW_OP_stack_value
+; CHECK-NEXT:     : DW_OP_implicit_pointer 0x53 +0
----------------
"arr1" is metadata !18, right? Where does lit0 come from?


================
Comment at: llvm/test/DebugInfo/dwarfdump-implicit_pointer_mem2reg.ll:11
+; CHECK-NEXT:     : DW_OP_implicit_pointer 0x53 +0
+; CHECK-NEXT:  DW_AT_name    ("ptr2")
+
----------------
Please structure your checks as:

```
; <description of test 1>
; CHECK-LABEL: DW_AT_name  (<variable name>)
; CHECK-NEXT: ...

; <description of test 2>
...
```


================
Comment at: llvm/test/DebugInfo/dwarfdump-implicit_pointer_sroa_inline.ll:9
+; CHECK-NEXT:    DW_AT_location
+; CHECK-NEXT:     : DW_OP_implicit_pointer 0x5d +0
+; CHECK-NEXT:     : DW_OP_implicit_pointer 0x5d +4)
----------------
Please avoid hardcoding constants in check lines. If the specific constant isn't important, don't match it. Otherwise, you can capture it with a filecheck variable, e.g.:

```
DW_OP_implicit_pointer [[DIE:0x.+]] +0
DW_OP_implicit_pointer [[DIE]] +4
```


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

https://reviews.llvm.org/D69886