[PATCH] D37311: [DebugInfo] Lower dbg.declare to DBG_VALUE with DW_OP_deref

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

As discussed on IRC, I think that this is *generally* a good idea, but I'm skeptical about a few issues with the concrete implementation that I commented on inline.
These changes have to be made very carefully. Have you tried running the debuginfo-tests with your patch? To do this you need to checkout the debuginfo-tests repository into the clang/test directory and have either gdb or lldb installed and working (yes it's magical and works with both!).

We need to make sure that any change to the IR testcases can actually be reproduced by recompiling the original source with clang.

thanks for working on this!



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5138
       } else {
-        SDV = DAG.getDbgValue(Variable, Expression, N.getNode(), N.getResNo(),
-                              true, dl, SDNodeOrder);
+        SDV = DAG.getDbgValue(Variable, Expression->appendDeref(), N.getNode(),
+                              N.getResNo(), dl, SDNodeOrder);
----------------
Can you explain why the DW_OP_deref needs to go at the end of the expression rather than the beginning?


================
Comment at: llvm/test/DebugInfo/X86/dbg-declare-arg.ll:12
 ; CHECK-NEXT:   DW_AT_location [DW_FORM_sec_offset] (
-; CHECK-NEXT:     0x{{.*}} - 0x{{.*}}: DW_OP_breg7 RSP+8)
+; CHECK-NEXT:     0x{{.*}} - 0x{{.*}}: DW_OP_breg7 RSP+8, DW_OP_deref)
 ; CHECK-NEXT:   DW_AT_name {{.*}}"my_a"
----------------
Shouldn't this be an NFC commit?


================
Comment at: llvm/test/DebugInfo/X86/safestack-byval.ll:18
 ; CHECK: ![[ZZZ:.*]] = !DILocalVariable(name: "zzz",
-; CHECK: DBG_VALUE {{.*}} ![[ZZZ]], !DIExpression(DW_OP_deref, DW_OP_constu, 400, DW_OP_minus)
+; CHECK: DBG_VALUE {{.*}} ![[ZZZ]], !DIExpression(DW_OP_constu, 400, DW_OP_minus, DW_OP_deref)
+
----------------
That's a *very* different expression. Why is this correct?


================
Comment at: llvm/test/DebugInfo/X86/safestack-byval.ll:88
 !22 = !DILocation(line: 8, column: 9, scope: !12)
-!23 = !DIExpression(DW_OP_deref, DW_OP_constu, 400, DW_OP_minus)
+!23 = !DIExpression(DW_OP_constu, 400, DW_OP_minus)
 !24 = !DILocation(line: 8, column: 28, scope: !12)
----------------
Why the change?


================
Comment at: llvm/test/DebugInfo/X86/vla.ll:36
   %vla = alloca i32, i64 %1, align 16, !dbg !17
-  call void @llvm.dbg.declare(metadata i32* %vla, metadata !18, metadata !DIExpression(DW_OP_deref)), !dbg !17
+  call void @llvm.dbg.declare(metadata i32* %vla, metadata !18, metadata !DIExpression()), !dbg !17
   %arrayidx = getelementptr inbounds i32, i32* %vla, i64 0, !dbg !22
----------------
I don't see a change in this patch that would have this effect.


https://reviews.llvm.org/D37311