Debugging of optimized code: When locals have their address taken as part of a call use their stack slots as location expressions for debug info.

[wolfgangp]

Summary

This patch proposes a small improvement to debug information for local variables whose address is taken and passed to a function. It is only relevant with optimization.

Example:

/******/
int data = 17;
int zero = 0;
int *ptr;

extern void foo(int i, int *p);

int main()
{

int val;
val = data;
foo(1, &val); // <== generates indirect+0 entry
foo(2, &data);
return zero;

}
/*******/

Compiled with -O2 -g -fno-omit-frame-pointer, the dwarfdump output for 'val' shows (Linux-X86):

DW_TAG_variable

DW_AT_location              <loclist with 2 entries follows>
[ 0]<lowpc=0x0000000e><highpc=0x00000015>DW_OP_reg0 DW_OP_piece 4
[ 1]<lowpc=0x00000015><highpc=0x0000001f>DW_OP_breg4+0
DW_AT_name                  "val"

The second location list entry for 'val' is an indirection of the register that gets the address of 'val', taken by the first call to 'foo()'.
That location list entry is only valid until the register is killed, which causes the range for the entry to be shorter than what would be feasible.

The patch proposes to detect FrameIndexSDNodes in SelectionDAG and generate FrameIndexDbgValues for them when we see a DW_op_deref expression attached to the incoming node. This will ultimately generate DBG_VALUE instructions with stack location operands, which will be valid longer than the current indirections. With the patch, dwarfdump shows:

DW_TAG_variable

DW_AT_location              <loclist with 2 entries follows>
[ 0]<lowpc=0x0000000e><highpc=0x00000015>DW_OP_reg0 DW_OP_piece 4
[ 1]<lowpc=0x00000015><highpc=0x0000003a>DW_OP_breg6-4
DW_AT_name                  "val"

Which makes 'val' visible in the debugger longer.

Note that when the frame pointer is eliminated, we don't get a longer range for the second entry in this example. This is due to the fact that subsequent call instructions are currently deemed to imp-def the stack pointer, ending the range for val's location prematurely. This is an orthogonal issue and is best addressed separately.

[majnemer]

Does this do the right thing if the stack offset is reused for another purpose via the lifetime intrinsics?

[aprantl]

Thanks!

Why does this need to be restricted to "simple" derefs? I understand that it's easier to convince ourselves that a single DW_OP_deref is correct, but do you have a counterexample where checking whether the first opcode in the expression is a deref would not be correct?

I haven't checked the code yet, but maybe you already know the answer: Does this also work if the variable has more than one value? If the FrameIndexDbgValue also gets lowered into DBG_VALUE we should be fine. If it ends up as a stack slot entry in the MMI table the following example should break:

int foo(bool c) {
  int i = 0;
  if (c)
    i = 1;
  else
    bar(&i)
  return i;
}

[wolfgangp]

In D23283#509185, @majnemer wrote:

Does this do the right thing if the stack offset is reused for another purpose via the lifetime intrinsics?

If a stack slot is reused for something else one of the later passes (LiveDebugVariables, LiveDebugValues or DbgValueHistoryCalculator) should detect this and generate the correct location info. That said, in my - admittedly simple - examples I haven't seen a case where stack slots are actually shared between different stack objects (user variables is what we're interested here), so it's possible I have missed something.

[wolfgangp]

In D23283#510083, @aprantl wrote:

Thanks!

Why does this need to be restricted to "simple" derefs? I understand that it's easier to convince ourselves that a single DW_OP_deref is correct, but do you have a counterexample where checking whether the first opcode in the expression is a deref would not be correct?

No, I don't have a counterexample, but this was conservatively directed at locals that have their address taken as part of a call, based on code in LowerDbgDeclare() in Local.cpp that explicitly generates a single DW_op_deref in that case. It may be possible to improve on it, but I haven't had time to look at any more complex examples yet.

I haven't checked the code yet, but maybe you already know the answer: Does this also work if the variable has more than one value? If the FrameIndexDbgValue also gets lowered into DBG_VALUE we should be fine. If it ends up as a stack slot entry in the MMI table the following example should break:

int foo(bool c) {
  int i = 0;
  if (c)
    i = 1;
  else
    bar(&i)
  return i;
}

The frameindex is preserved in the DBG_VALUEs, the actual stack slot doesn't get assigned until the PrologueEpilogueInserter/Frame Finalization pass, so I think from that point of view it's fine, but your example actually does not benefit from the change. It does what it used to do, i.e. generate an indirect reg+0 DBG_VALUE. I have to check why.

[aprantl]

In D23283#510252, @wolfgangp wrote:

In D23283#509185, @majnemer wrote:

Does this do the right thing if the stack offset is reused for another purpose via the lifetime intrinsics?

If a stack slot is reused for something else one of the later passes (LiveDebugVariables, LiveDebugValues or DbgValueHistoryCalculator) should detect this and generate the correct location info. That said, in my - admittedly simple - examples I haven't seen a case where stack slots are actually shared between different stack objects (user variables is what we're interested here), so it's possible I have missed something.

This is true if the stack slot is represented by a DBG_VALUE. If it is stored in the MMI table (together with the other dbg.declares) none of these passes even see it.

[aprantl]

In D23283#510313, @wolfgangp wrote:

In D23283#510083, @aprantl wrote:

Thanks!

Why does this need to be restricted to "simple" derefs? I understand that it's easier to convince ourselves that a single DW_OP_deref is correct, but do you have a counterexample where checking whether the first opcode in the expression is a deref would not be correct?

No, I don't have a counterexample, but this was conservatively directed at locals that have their address taken as part of a call, based on code in LowerDbgDeclare() in Local.cpp that explicitly generates a single DW_op_deref in that case. It may be possible to improve on it, but I haven't had time to look at any more complex examples yet.

My intuition is that it shouldn't matter what other operations we perform on the value stored in the stack slot after dereferencing the pointer; in particular we should ignore any DW_OP_piece at the end of the expression.

[wolfgangp]

In D23283#510350, @aprantl wrote:

In D23283#510252, @wolfgangp wrote:

In D23283#509185, @majnemer wrote:

Does this do the right thing if the stack offset is reused for another purpose via the lifetime intrinsics?

If a stack slot is reused for something else one of the later passes (LiveDebugVariables, LiveDebugValues or DbgValueHistoryCalculator) should detect this and generate the correct location info. That said, in my - admittedly simple - examples I haven't seen a case where stack slots are actually shared between different stack objects (user variables is what we're interested here), so it's possible I have missed something.

This is true if the stack slot is represented by a DBG_VALUE. If it is stored in the MMI table (together with the other dbg.declares) none of these passes even see it.

But doesn't lowerDbgDeclare() generate dbg.value intrinsics for all uses from the list of dbg.declares? That way they are exposed to the 3 passes.

[aprantl]

Everything described by DBG_VALUE instructions is handled by LiveDebugValues etc.
Note, however that llvm::LowerDbgDeclare() currently does not remove all dbg.declare (arrays are skipped, for example).

My concern was that SelectionDAG would enter SDDbgValues describing a FrameIndex into the MMI sidetable, but I just confirmed that this isn't the case. It only calls MMI.setVariableDbgInfo() for dbg.declare instrinsics. So this is fine.

[wolfgangp]

Revised patch to account for DanglingDebugInfo nodes, which I had overlooked in the first attempt. Relaxed the check for a deref node to allow more general DI expressions with a top DW_op_deref node.

One more test case needed modification (array.ll).

[aprantl]

Found one bug, but otherwise this is looking good now, thanks!
LGTM (with all remaining review comments are addressed).

include/llvm/IR/DebugInfoMetadata.h

2159 ↗

(On Diff #67606)

Please add a doxygen comment to the function.
Should we rename this to startsWithDeref?

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

4751 ↗

(On Diff #67606)

I think we usually write this as "auto *FISDN = ..."

4758 ↗

(On Diff #67606)

After relaxing the condition, you'll want the tail of the dwarf expression here, not just an empty expression.

test/DebugInfo/AArch64/coalescing.ll

28 ↗

(On Diff #67606)

Could you please add a comment with the disassembled dwarf expression?

[wolfgangp]

Thanks for the review.

include/llvm/IR/DebugInfoMetadata.h

2159 ↗

(On Diff #67606)

Sure, sounds better.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

4758 ↗

(On Diff #67606)

Oh, yes, thanks for catching that.

llvm/trunk/include/llvm/IR/DebugInfoMetadata.h

public:
   }
   /// @}
 
   bool isValid() const;
 
   static bool classof(const Metadata *MD) {
     return MD->getMetadataID() == DIExpressionKind;
   }
+
+  /// \brief Is the first element a DW_OP_deref?.
+  bool startsWithDeref() const {
+    return getNumElements() > 0 && getElement(0) == dwarf::DW_OP_deref;
+  }
 };
 
 class DIObjCProperty : public DINode {
   friend class LLVMContextImpl;
   friend class MDNode;
 
   unsigned Line;
   unsigned Attributes;

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h

bool EmitFuncArgumentDbgValue(const Value *V, DILocalVariable *Variable,
                                 const SDValue &N);
 
   /// Return the next block after MBB, or nullptr if there is none.
   MachineBasicBlock *NextBlock(MachineBasicBlock *MBB);
 
   /// Update the DAG and DAG builder with the relevant information after
   /// a new root node has been created which could be a tail call.
   void updateDAGForMaybeTailCall(SDValue MaybeTC);
+
+  /// Return the appropriate SDDbgValue based on N.
+  SDDbgValue *getDbgValue(SDValue N, DILocalVariable *Variable,
+                          DIExpression *Expr, int64_t Offset, DebugLoc dl,
+                          unsigned DbgSDNodeOrder);
 };
 
 /// RegsForValue - This struct represents the registers (physical or virtual)
 /// that a particular set of values is assigned, and the type information about
 /// the value. The most common situation is to represent one value at a time,
 /// but struct or array values are handled element-wise as multiple values.  The
 /// splitting of aggregates is performed recursively, so that we never have
 /// aggregate-typed registers. The values at this point do not necessarily have

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

if (DDI.getDI()) {
     DIExpression *Expr = DI->getExpression();
     assert(Variable->isValidLocationForIntrinsic(dl) &&
            "Expected inlined-at fields to agree");
     uint64_t Offset = DI->getOffset();
     SDDbgValue *SDV;
     if (Val.getNode()) {
       if (!EmitFuncArgumentDbgValue(V, Variable, Expr, dl, Offset, false,
                                     Val)) {
-        SDV = DAG.getDbgValue(Variable, Expr, Val.getNode(), Val.getResNo(),
-                              false, Offset, dl, DbgSDNodeOrder);
+        SDV = getDbgValue(Val, Variable, Expr, Offset, dl, DbgSDNodeOrder);
         DAG.AddDbgValue(SDV, Val.getNode(), false);
       }
     } else
       DEBUG(dbgs() << "Dropping debug info for " << *DI << "\n");
     DanglingDebugInfoMap[V] = DanglingDebugInfo();
   }
 }
 
FuncInfo.ArgDbgValues.push_back(
             .addOperand(*Op)
             .addImm(Offset)
             .addMetadata(Variable)
             .addMetadata(Expr));
 
   return true;
 }
 
+/// Return the appropriate SDDbgValue based on N.
+SDDbgValue *SelectionDAGBuilder::getDbgValue(SDValue N,
+                                             DILocalVariable *Variable,
+                                             DIExpression *Expr, int64_t Offset,
+                                             DebugLoc dl,
+                                             unsigned DbgSDNodeOrder) {
+  SDDbgValue *SDV;
+  auto *FISDN = dyn_cast<FrameIndexSDNode>(N.getNode());
+  if (FISDN && Expr->startsWithDeref()) {
+    // Construct a FrameIndexDbgValue for FrameIndexSDNodes so we can describe
+    // stack slot locations as such instead of as indirectly addressed
+    // locations.
+    ArrayRef<uint64_t> TrailingElements(Expr->elements_begin() + 1,
+                                        Expr->elements_end());
+    DIExpression *DerefedDIExpr =
+        DIExpression::get(*DAG.getContext(), TrailingElements);
+    int FI = FISDN->getIndex();
+    SDV = DAG.getFrameIndexDbgValue(Variable, DerefedDIExpr, FI, 0, dl,
+                                    DbgSDNodeOrder);
+  } else {
+    SDV = DAG.getDbgValue(Variable, Expr, N.getNode(), N.getResNo(), false,
+                          Offset, dl, DbgSDNodeOrder);
+  }
+  return SDV;
+}
+
 // VisualStudio defines setjmp as _setjmp
 #if defined(_MSC_VER) && defined(setjmp) && \
                          !defined(setjmp_undefined_for_msvc)
 #  pragma push_macro("setjmp")
 #  undef setjmp
 #  define setjmp_undefined_for_msvc
 #endif
 
if (isa<ConstantInt>(V) || isa<ConstantFP>(V) || isa<UndefValue>(V)) {
       // this point if it hasn't been done yet.
       SDValue N = NodeMap[V];
       if (!N.getNode() && isa<Argument>(V))
         // Check unused arguments map.
         N = UnusedArgNodeMap[V];
       if (N.getNode()) {
         if (!EmitFuncArgumentDbgValue(V, Variable, Expression, dl, Offset,
                                       false, N)) {
-          SDV = DAG.getDbgValue(Variable, Expression, N.getNode(), N.getResNo(),
-                                false, Offset, dl, SDNodeOrder);
+          SDV = getDbgValue(N, Variable, Expression, Offset, dl, SDNodeOrder);
           DAG.AddDbgValue(SDV, N.getNode(), false);
         }
       } else if (!V->use_empty() ) {
         // Do not call getValue(V) yet, as we don't want to generate code.
         // Remember it for later.
         DanglingDebugInfo DDI(&DI, dl, SDNodeOrder);
         DanglingDebugInfoMap[V] = DDI;
       } else {

llvm/trunk/test/DebugInfo/AArch64/coalescing.ll

entry:
   %0 = bitcast i32* %size to i8*, !dbg !15
   %call = call i8* @_Z3fooPv(i8* %0) #3, !dbg !15
   call void @llvm.dbg.value(metadata i32* %size, i64 0, metadata !10, metadata !16), !dbg !17
   ; CHECK: .debug_info contents:
   ; CHECK: DW_TAG_variable
   ; CHECK-NEXT: DW_AT_location
   ; CHECK-NEXT: DW_AT_name {{.*}}"size"
   ; CHECK: .debug_loc contents:
-  ; CHECK: Location description: 70 00
+  ; Expecting the encoding for sp+12: DW_OP_breg31 0c
+  ; CHECK: Location description: 8f 0c
   ret void, !dbg !18
 }
 
 ; Function Attrs: optsize
 declare i8* @_Z3fooPv(i8*) #1
 
 ; Function Attrs: nounwind readnone
 declare void @llvm.dbg.value(metadata, i64, metadata, metadata) #2

llvm/trunk/test/DebugInfo/X86/array.ll

 ;   f(array);
 ;   return array[0];
 ; }
 ;
 ; RUN: llc -filetype=asm %s -o - | FileCheck %s
 ; Test that we only emit register-indirect locations for the array array.
 ; rdar://problem/14874886
 ;
-; FIXME: If we described this location as RSP-relative instead of RDI-relative
-; the live range would be larger.
-; CHECK:     ##DEBUG_VALUE: main:array <- [%RDI+0]
+; CHECK:     ##DEBUG_VALUE: main:array <- [%RSP+0]
 ; CHECK-NOT: ##DEBUG_VALUE: main:array <- %R{{.*}}
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.9.0"
 
 @main.array = private unnamed_addr constant [4 x i32] [i32 0, i32 1, i32 2, i32 3], align 16
 
 ; Function Attrs: nounwind ssp uwtable
 define void @f(i32* nocapture %p) #0 !dbg !4 {

llvm/trunk/test/DebugInfo/X86/bbjoin.ll

 ; RUN: llc -mtriple=x86_64-apple-macosx10.9.0 %s -stop-after=livedebugvars \
 ; RUN:   -o - | FileCheck %s
 ; Generated from:
 ; void g(int *);
 ; int f() {
 ;   int x = 23;
 ;   g(&x);
 ;   if (x == 42)
 ;     ++x;
 ;   return x; // check that x is not a constant here.
 ; }
 ; CHECK: ![[X:.*]] = !DILocalVariable(name: "x",
 ; CHECK: bb.0.entry:
 ; CHECK:   DBG_VALUE 23, 0, ![[X]],
-; CHECK:   DBG_VALUE debug-use %rdi, debug-use _, ![[X]]
+; CHECK:   DBG_VALUE %rsp, 4, ![[X]]
 ; CHECK: bb.1.if.then:
 ; CHECK:   DBG_VALUE 43, 0, ![[X]],
 ; CHECK: bb.2.if.end:
 ; CHECK-NOT:  DBG_VALUE 23, 0, ![[X]],
 ; CHECK:   RETQ %eax
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.11.0"

llvm/trunk/test/DebugInfo/X86/dbg-value-const-byref.ll

 ; CHECK:     Location description: 11 07
 ;        rax, piece 0x00000004
 ; CHECK: Beginning address offset: [[C2]]
 ; CHECK:    Ending address offset: [[R1:.*]]
 ; CHECK:     Location description: 50 93 04
 ;         rdi+0
 ; CHECK: Beginning address offset: [[R1]]
 ; CHECK:    Ending address offset: [[R2:.*]]
-; CHECK:     Location description: 75 00
+; CHECK:     Location description: 77 04
+;         rsp+4
 ;
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.9.0"
 
 ; Function Attrs: nounwind ssp uwtable
 define i32 @foo() #0 !dbg !4 {
 entry:
   %i = alloca i32, align 4

llvm/trunk/test/DebugInfo/X86/debug-loc-frame.ll

+; REQUIRES: object-emission
+
+; Check that when variables are allocated on the stack we generate debug locations
+; for the stack location directly instead of generating a register+offset indirection.
+
+; RUN: %llc_dwarf -O2 -filetype=obj -disable-post-ra < %s | llvm-dwarfdump - | FileCheck %s
+;
+; int data = 17;
+; int sum  = 0;
+; int zero = 0;
+; int *ptr;
+;
+; extern void foo(int i, int *p);
+;
+; int main()
+; {
+;   int val;
+;   val = data;
+;   foo(1, &val);
+;   foo(2, &data);
+;   return zero;
+; }
+;
+; CHECK:      .debug_info contents
+; CHECK:      DW_TAG_subprogram
+; CHECK-NOT:  NULL
+; CHECK:      DW_TAG_variable
+; CHECK:      DW_AT_location [DW_FORM_sec_offset] ([[DEBUGLOCOFFSET:0x[0-9a-f]+]]){{[[:space:]].*}}"val"
+
+; See that 'val' has at least one location entry with a DW_op_breg? operand.
+; The DWARF DW_op_breg* ops are encoded from 0x70 to 0x8f, but checking for an
+; op in the range from 0x70 to 0x7f should suffice because that range covers
+; all integer GPRs.
+;
+; CHECK: .debug_loc contents:
+; CHECK-NOT: .debug{{.*}} contents
+; CHECK: [[DEBUGLOCOFFSET]]: Beginning
+; CHECK-NOT: {{0x[0-9a-f]+}}: Beginning
+; CHECK: Location description: 7{{[0-9a-f] .*}}
+
+; ModuleID = 'frame.c'
+source_filename = "frame.c"
+
+@data = global i32 17, align 4
+@sum = local_unnamed_addr global i32 0, align 4
+@zero = local_unnamed_addr global i32 0, align 4
+@ptr = common local_unnamed_addr global i32* null, align 8
+
+; Function Attrs: nounwind uwtable
+define i32 @main() local_unnamed_addr !dbg !13 {
+entry:
+  %val = alloca i32, align 4
+  %0 = bitcast i32* %val to i8*, !dbg !18
+  call void @llvm.lifetime.start(i64 4, i8* %0), !dbg !18
+  %1 = load i32, i32* @data, align 4, !dbg !19, !tbaa !20
+  tail call void @llvm.dbg.value(metadata i32 %1, i64 0, metadata !17, metadata !24), !dbg !25
+  store i32 %1, i32* %val, align 4, !dbg !26, !tbaa !20
+  tail call void @llvm.dbg.value(metadata i32* %val, i64 0, metadata !17, metadata !27), !dbg !25
+  call void @foo(i32 1, i32* nonnull %val), !dbg !28
+  call void @foo(i32 2, i32* nonnull @data), !dbg !29
+  %2 = load i32, i32* @zero, align 4, !dbg !30, !tbaa !20
+  call void @llvm.lifetime.end(i64 4, i8* %0), !dbg !31
+  ret i32 %2, !dbg !32
+}
+
+; Function Attrs: argmemonly nounwind
+declare void @llvm.lifetime.start(i64, i8* nocapture)
+
+declare void @foo(i32, i32*) local_unnamed_addr
+
+; Function Attrs: argmemonly nounwind
+declare void @llvm.lifetime.end(i64, i8* nocapture)
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.value(metadata, i64, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!10, !11}
+!llvm.ident = !{!12}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 3.9.0 (trunk 273961)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, globals: !3)
+!1 = !DIFile(filename: "frame.c", directory: "/home/user/test")
+!2 = !{}
+!3 = !{!4, !6, !7, !8}
+!4 = distinct !DIGlobalVariable(name: "data", scope: !0, file: !1, line: 1, type: !5, isLocal: false, isDefinition: true, variable: i32* @data)
+!5 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+!6 = distinct !DIGlobalVariable(name: "sum", scope: !0, file: !1, line: 2, type: !5, isLocal: false, isDefinition: true, variable: i32* @sum)
+!7 = distinct !DIGlobalVariable(name: "zero", scope: !0, file: !1, line: 3, type: !5, isLocal: false, isDefinition: true, variable: i32* @zero)
+!8 = distinct !DIGlobalVariable(name: "ptr", scope: !0, file: !1, line: 4, type: !9, isLocal: false, isDefinition: true, variable: i32** @ptr)
+!9 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !5, size: 64, align: 64)
+!10 = !{i32 2, !"Dwarf Version", i32 4}
+!11 = !{i32 2, !"Debug Info Version", i32 3}
+!12 = !{!"clang version 3.9.0 (trunk 273961)"}
+!13 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 8, type: !14, isLocal: false, isDefinition: true, scopeLine: 9, isOptimized: true, unit: !0, variables: !16)
+!14 = !DISubroutineType(types: !15)
+!15 = !{!5}
+!16 = !{!17}
+!17 = !DILocalVariable(name: "val", scope: !13, file: !1, line: 10, type: !5)
+!18 = !DILocation(line: 10, column: 3, scope: !13)
+!19 = !DILocation(line: 11, column: 9, scope: !13)
+!20 = !{!21, !21, i64 0}
+!21 = !{!"int", !22, i64 0}
+!22 = !{!"omnipotent char", !23, i64 0}
+!23 = !{!"Simple C/C++ TBAA"}
+!24 = !DIExpression()
+!25 = !DILocation(line: 10, column: 7, scope: !13)
+!26 = !DILocation(line: 11, column: 7, scope: !13)
+!27 = !DIExpression(DW_OP_deref)
+!28 = !DILocation(line: 12, column: 3, scope: !13)
+!29 = !DILocation(line: 13, column: 3, scope: !13)
+!30 = !DILocation(line: 14, column: 10, scope: !13)
+!31 = !DILocation(line: 15, column: 1, scope: !13)
+!32 = !DILocation(line: 14, column: 3, scope: !13)