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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Thanks for the review, it seems like this is headed in the right direction. I haven't tried debuginfo-tests yet, but I'll do that and get back.

BTW, we were thinking about adding tests for cdb to debuginfo-tests. cdb is the Windows command line debugger. I doubt the existing perl script will run on Windows out of the box, but we'll figure something out.



================
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);
----------------
aprantl wrote:
> Can you explain why the DW_OP_deref needs to go at the end of the expression rather than the beginning?
We have existing IR that looks like this:
  dbg.declare(%block_params, !Var("foo"), !DIExpression(DW_OP_constu, 4, DW_OP_minus))

I *think* this means: take the pointer value in %block_params, subtract 4 from it, and look in memory to find the value.

I would expect this to be equivalent to:
  dbg.value(%block_params, !Var("foo"), !DIExpression(DW_OP_constu, 4, DW_OP_minus, DW_OP_deref))

Meaning, the DIExpression is applied to the IR value, which ultimately becomes a virtual register, and then a register or spilled stack slot. We apply the DIExpression of a dbg.declare to that value to get the address of the variable, and *then* we deref once more to get the value.

Does that makes sense? This isn't an area that I'm familiar with yet, but that's what I'm thinking here.


================
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"
----------------
aprantl wrote:
> Shouldn't this be an NFC commit?
It isn't. I think we were getting wrong DBG_VALUEs in these cases. In this case, %agg.result starts in RDI and is spilled to RSP+8. Before this change, SDISel would make an indirect DBG_VALUE of a vreg copy of RDI. LiveDebugVariables doesn't do anything different if the original DBG_VALUE instruction was indirect and the vreg was spilled, it just makes an instruction like DBG_VALUE(<fi#N>, 0, ...) without adding a deref to the expression. It only propagates IsIndirect if the MachineOperand remained a register after register allocation.


================
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)
+
----------------
aprantl wrote:
> That's a *very* different expression. Why is this correct?
It's a different expr, but I added the dwarfdump test to confirm that we emit the same final DWARF before and after this change. We still emit the location of [RBX-400].


================
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)
----------------
aprantl wrote:
> Why the change?
I regenerated the IR with clang, and it doesn't come out with DW_OP_deref anymore. I didn't refresh the whole test case, but I could if you like. This is how I got it:
  $ clang -g -S -emit-llvm -x c++ ss.c -fsanitize=safe-stack --target=x86_64-unknown-linux -O1 -o - | opt -S -safe-stack -o ss.ll


================
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
----------------
aprantl wrote:
> I don't see a change in this patch that would have this effect.
Clang no longer emits this DW_OP_deref after r300523. I renamed the old `op_deref.ll` test to `vla2.ll`, since it was really only using op_deref because clang used to use it for VLAs. vla2.ll is actually a much better test, it has dwarfdump checks, and I think those dwarf expressions are "more right" now.


https://reviews.llvm.org/D37311