[DebugInfo] Emit "undef" DBG_VALUEs when SDNodes are optimised out

[jmorse]

Summary

This is a fix for PR39896 [0], where SDNodes that have been optimised out do not lead to "DBG_VALUE undef" instructions being created. Such undef instructions are necessary to terminate earlier variable ranges, otherwise variable values leak past the point where they're valid.

The "invalidated" flag of SDDbgValue is currently being abused to mean two things:

1. The corresponding SDNode is now invalid
2. This SDDbgValue should not be emitted

Of which there are several legitimate combinations of meaning:

• The SDNode has been invalidated and we should emit "DBG_VALUE undef"
• The SDNode has been invalidated but the debug data was salvaged, don't emit anything for this SDDbgValue
• This SDDbgValue has been emitted

I've added an "Emitted" field to distinguish when the SDNode is invalid and when we just don't want to emit it. A SDDbgValue can be re-emitted too, because it seems to happen twice for some dbg.addrs, specifically DebugInfo/X86/dbg-addr-dse.ll gets DBG_VALUEs at the start of the function and in the middle.

As well as the regression test added, I've edited three tests, explained here:

LLVM :: DebugInfo/X86/pieces-3.ll
    The tail of this function is some completely superfluous computations that
     have dbg.values attached, and gets optimised out. Previously this was ignored
     by SelectionDAG, but now we emit a DBG_VALUE undef it reduces the range of
     the variables. Seeing how the tail is needless, and isn't generated by clang for
     the given source code, I've just deleted it. Otherwise emitting undef is correct,
     because the designated value was optimised out.
LLVM :: DebugInfo/NVPTX/debug-info.ll
     The full debug info section is part of the test here, and has some kind of fractional
     change. I've just updated the lines to the latest version.
LLVM :: DebugInfo/X86/dbg-value-inlined-parameter.ll
      According to a FIXME, some parameter was accidentally dropped on Linux.
      It now doesn't, so I've normalised the test here.

[0] https://bugs.llvm.org/show_bug.cgi?id=39896

[jmorse]

Un-WIPed this revision which should be ready for review now: added llvm-commits, reviewers, and the like.

[aprantl]

The basic idea looks good to me.

test/DebugInfo/X86/pieces-3.ll

40

Why is this necessary?

[jmorse]

test/DebugInfo/X86/pieces-3.ll

40

IMHO lines 41-45 are wrong. On line 40 we dbg.value assign the (truncated) return value to the 0-32 fragment of "i1", rotate and truncate %outer.coerce1 to get the other part of "i1" on line 42, then on lines 43-45 we dbg.value assign it to the same 0-32 fragment of "i1". The lower field of "i1" thus gets two dbg.value assignments where the original C program had only one. The second refers to values that SelectionDAG drops -- previously this was ignored, but now it un-necessarily shortens the range of the first assignment.

Originally I just deleted this block of code because it seemed invalid (and clang doesn't generate it any more). Prodding further, a better solution might be pointing the lines 43-44 dbg.values at the fragment of i1 that they truly correspond to, I'll upload a revised patch that does this.

[jmorse]

In a test, alter the fragment that two dbg.values refer to. I believe they were incorrect in the first place but silently ignored.

[aprantl]

test/DebugInfo/X86/pieces-3.ll

40

Totally possible that this is bitrotted or was never correct to begin with. Thanks for explaining!

[vsk]

LGTM, thanks! I'd suggest running through debuginfo-tests if you haven't already (ninja check-debuginfo).

[aprantl]

Looks like this caused a noticeable shrinking of location lists, but that means we're getting more accurate: http://lnt.llvm.org/db_default/v4/nts/graph?plot.0=1357.1607042.4&highlight_run=117379 !

lib/CodeGen/SelectionDAG/InstrEmitter.cpp

 InstrEmitter::EmitDbgValue(SDDbgValue *SD,
                            DenseMap<SDValue, unsigned> &VRBaseMap) {
   MDNode *Var = SD->getVariable();
   MDNode *Expr = SD->getExpression();
   DebugLoc DL = SD->getDebugLoc();
   assert(cast<DILocalVariable>(Var)->isValidLocationForIntrinsic(DL) &&
          "Expected inlined-at fields to agree");
 
+  SD->setIsEmitted();
+
+  if (SD->isInvalidated()) {
+    // An invalidated SDNode must generate an undef DBG_VALUE: although the
+    // original value is no longer computed, earlier DBG_VALUEs live ranges
+    // must not leak into later code.
+    auto MIB = BuildMI(*MF, DL, TII->get(TargetOpcode::DBG_VALUE));
+    MIB.addReg(0U);
+    MIB.addReg(0U, RegState::Debug);
+    MIB.addMetadata(Var);
+    MIB.addMetadata(Expr);
+    return &*MIB;
+  }
+
   if (SD->getKind() == SDDbgValue::FRAMEIX) {
     // Stack address; this needs to be lowered in target-dependent fashion.
     // EmitTargetCodeForFrameDebugValue is responsible for allocation.
     auto FrameMI = BuildMI(*MF, DL, TII->get(TargetOpcode::DBG_VALUE))
                        .addFrameIndex(SD->getFrameIx());
     if (SD->isIndirect())
       // Push [fi + 0] onto the DIExpression stack.
       FrameMI.addImm(0);

lib/CodeGen/SelectionDAG/SDNodeDbgValue.h

private:
   } u;
   DIVariable *Var;
   DIExpression *Expr;
   DebugLoc DL;
   unsigned Order;
   enum DbgValueKind kind;
   bool IsIndirect;
   bool Invalid = false;
+  bool Emitted = false;
 
 public:
   /// Constructor for non-constants.
   SDDbgValue(DIVariable *Var, DIExpression *Expr, SDNode *N, unsigned R,
              bool indir, DebugLoc dl, unsigned O)
       : Var(Var), Expr(Expr), DL(std::move(dl)), Order(O), IsIndirect(indir) {
     kind = SDNODE;
     u.s.Node = N;
public:
   unsigned getOrder() const { return Order; }
 
   /// setIsInvalidated / isInvalidated - Setter / getter of the "Invalidated"
   /// property. A SDDbgValue is invalid if the SDNode that produces the value is
   /// deleted.
   void setIsInvalidated() { Invalid = true; }
   bool isInvalidated() const { return Invalid; }
 
+  /// setIsEmitted / isEmitted - Getter/Setter for flag indicating that this
+  /// SDDbgValue has been emitted to an MBB.
+  void setIsEmitted() { Emitted = true; }
+  bool isEmitted() const { return Emitted; }
+
+  /// clearIsEmitted - Reset Emitted flag, for certain special cases where
+  /// dbg.addr is emitted twice.
+  void clearIsEmitted() { Emitted = false; }
+
   LLVM_DUMP_METHOD void dump(raw_ostream &OS) const;
 };
 
 /// Holds the information from a dbg_label node through SDISel.
 /// We do not use SDValue here to avoid including its header.
 class SDDbgLabel {
   MDNode *Label;
   DebugLoc DL;

lib/CodeGen/SelectionDAG/ScheduleDAGFast.cpp

for (unsigned i = 0; i != NumNodes; ++i) {
     SDNode *N = Sequence[NumNodes-i-1];
     LLVM_DEBUG(N->dump(DAG));
     Emitter.EmitNode(N, false, false, VRBaseMap);
 
     // Emit any debug values associated with the node.
     if (N->getHasDebugValue()) {
       MachineBasicBlock::iterator InsertPos = Emitter.getInsertPos();
       for (auto DV : DAG->GetDbgValues(N)) {
-        if (DV->isInvalidated())
-          continue;
+        if (!DV->isEmitted())
           if (auto *DbgMI = Emitter.EmitDbgValue(DV, VRBaseMap))
             BB->insert(InsertPos, DbgMI);
-        DV->setIsInvalidated();
       }
     }
   }
 
   LLVM_DEBUG(dbgs() << '\n');
 
   InsertPos = Emitter.getInsertPos();
   return Emitter.getBlock();

lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp

ProcessSDDbgValues(SDNode *N, SelectionDAG *DAG, InstrEmitter &Emitter,
   if (!N->getHasDebugValue())
     return;
 
   // Opportunistically insert immediate dbg_value uses, i.e. those with the same
   // source order number as N.
   MachineBasicBlock *BB = Emitter.getBlock();
   MachineBasicBlock::iterator InsertPos = Emitter.getInsertPos();
   for (auto DV : DAG->GetDbgValues(N)) {
-    if (DV->isInvalidated())
+    if (DV->isEmitted())
       continue;
     unsigned DVOrder = DV->getOrder();
     if (!Order || DVOrder == Order) {
       MachineInstr *DbgMI = Emitter.EmitDbgValue(DV, VRBaseMap);
       if (DbgMI) {
         Orders.push_back({DVOrder, DbgMI});
         BB->insert(InsertPos, DbgMI);
       }
-      DV->setIsInvalidated();
     }
   }
 }
 
 // ProcessSourceNode - Process nodes with source order numbers. These are added
 // to a vector which EmitSchedule uses to determine how to insert dbg_value
 // instructions in the right order.
 static void
EmitSchedule(MachineBasicBlock::iterator &InsertPos) {
   bool HasDbg = DAG->hasDebugValues();
 
   // If this is the first BB, emit byval parameter dbg_value's.
   if (HasDbg && BB->getParent()->begin() == MachineFunction::iterator(BB)) {
     SDDbgInfo::DbgIterator PDI = DAG->ByvalParmDbgBegin();
     SDDbgInfo::DbgIterator PDE = DAG->ByvalParmDbgEnd();
     for (; PDI != PDE; ++PDI) {
       MachineInstr *DbgMI= Emitter.EmitDbgValue(*PDI, VRBaseMap);
-      if (DbgMI)
+      if (DbgMI) {
         BB->insert(InsertPos, DbgMI);
+        // We re-emit the dbg_value closer to its use, too, after instructions
+        // are emitted to the BB.
+        (*PDI)->clearIsEmitted();
+      }
     }
   }
 
   for (unsigned i = 0, e = Sequence.size(); i != e; i++) {
     SUnit *SU = Sequence[i];
     if (!SU) {
       // Null SUnit* is a noop.
       TII->insertNoop(*Emitter.getBlock(), InsertPos);
for (unsigned i = 0, e = Orders.size(); i != e && DI != DE; ++i) {
       unsigned Order = Orders[i].first;
       MachineInstr *MI = Orders[i].second;
       // Insert all SDDbgValue's whose order(s) are before "Order".
       if (!MI)
         continue;
       for (; DI != DE; ++DI) {
         if ((*DI)->getOrder() < LastOrder || (*DI)->getOrder() >= Order)
           break;
-        if ((*DI)->isInvalidated())
+        if ((*DI)->isEmitted())
           continue;
 
         MachineInstr *DbgMI = Emitter.EmitDbgValue(*DI, VRBaseMap);
         if (DbgMI) {
           if (!LastOrder)
             // Insert to start of the BB (after PHIs).
             BB->insert(BBBegin, DbgMI);
           else {
             // Insert at the instruction, which may be in a different
             // block, if the block was split by a custom inserter.
             MachineBasicBlock::iterator Pos = MI;
             MI->getParent()->insert(Pos, DbgMI);
           }
         }
       }
       LastOrder = Order;
     }
     // Add trailing DbgValue's before the terminator. FIXME: May want to add
     // some of them before one or more conditional branches?
     SmallVector<MachineInstr*, 8> DbgMIs;
     for (; DI != DE; ++DI) {
-      if ((*DI)->isInvalidated())
+      if ((*DI)->isEmitted())
         continue;
       assert((*DI)->getOrder() >= LastOrder &&
              "emitting DBG_VALUE out of order");
       if (MachineInstr *DbgMI = Emitter.EmitDbgValue(*DI, VRBaseMap))
         DbgMIs.push_back(DbgMI);
     }
 
     MachineBasicBlock *InsertBB = Emitter.getBlock();

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

if (SizeInBits) {
       Expr = *Fragment;
     }
     // Clone the SDDbgValue and move it to To.
     SDDbgValue *Clone =
         getDbgValue(Var, Expr, ToNode, To.getResNo(), Dbg->isIndirect(),
                     Dbg->getDebugLoc(), Dbg->getOrder());
     ClonedDVs.push_back(Clone);
 
-    if (InvalidateDbg)
+    if (InvalidateDbg) {
+      // Invalidate value and indicate the SDDbgValue should not be emitted.
       Dbg->setIsInvalidated();
+      Dbg->setIsEmitted();
+    }
   }
 
   for (SDDbgValue *Dbg : ClonedDVs)
     AddDbgValue(Dbg, ToNode, false);
 }
 
 void SelectionDAG::salvageDebugInfo(SDNode &N) {
   if (!N.getHasDebugValue())
case ISD::ADD:
         DIExpr = DIExpression::prepend(DIExpr, DIExpression::NoDeref, Offset,
                                        DIExpression::NoDeref,
                                        DIExpression::WithStackValue);
         SDDbgValue *Clone =
             getDbgValue(DV->getVariable(), DIExpr, N0.getNode(), N0.getResNo(),
                         DV->isIndirect(), DV->getDebugLoc(), DV->getOrder());
         ClonedDVs.push_back(Clone);
         DV->setIsInvalidated();
+        DV->setIsEmitted();
         LLVM_DEBUG(dbgs() << "SALVAGE: Rewriting";
                    N0.getNode()->dumprFull(this);
                    dbgs() << " into " << *DIExpr << '\n');
       }
     }
   }
 
   for (SDDbgValue *Dbg : ClonedDVs)

test/CodeGen/X86/pr39896.ll

+; RUN: llc %s -start-after=codegenprepare -stop-after=expand-isel-pseudos -o - | FileCheck %s
+
+; PR39896: When code such as %conv below is dropped by SelectionDAG for having
+; no users, don't just drop the dbg.value record associated with it. Instead,
+; the corresponding variable should receive a "DBG_VALUE" undef to terminate
+; earlier variable live ranges.
+
+; ModuleID = 'run.c'
+source_filename = "run.c"
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.14.0"
+
+@a = global i8 25, align 1, !dbg !0
+
+define signext i16 @b() !dbg !12 {
+entry:
+; CHECK: DBG_VALUE 23680, $noreg, ![[VARNUM:[0-9]+]],
+  call void @llvm.dbg.value(metadata i16 23680, metadata !17, metadata !DIExpression()), !dbg !18
+  %0 = load i8, i8* @a, align 1, !dbg !18
+  %conv = sext i8 %0 to i16, !dbg !18
+; CHECK: DBG_VALUE $noreg, $noreg, ![[VARNUM]],
+  call void @llvm.dbg.value(metadata i16 %conv, metadata !17, metadata !DIExpression()), !dbg !18
+  %call = call i32 (...) @optimize_me_not(), !dbg !18
+  %1 = load i8, i8* @a, align 1, !dbg !18
+  %conv1 = sext i8 %1 to i16, !dbg !18
+  ret i16 %conv1, !dbg !18
+}
+declare i32 @optimize_me_not(...)
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!7, !8, !9, !10}
+!llvm.ident = !{!11}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "a", scope: !2, file: !3, line: 1, type: !6, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "clang version 8.0.0 (trunk 348101) (llvm/trunk 348109)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, globals: !5, nameTableKind: GNU)
+!3 = !DIFile(filename: "run.c", directory: "/Users/dcci/work/llvm/build-debug/bin")
+!4 = !{}
+!5 = !{!0}
+!6 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+!7 = !{i32 2, !"Dwarf Version", i32 4}
+!8 = !{i32 2, !"Debug Info Version", i32 3}
+!9 = !{i32 1, !"wchar_size", i32 4}
+!10 = !{i32 7, !"PIC Level", i32 2}
+!11 = !{!"clang version something"}
+!12 = distinct !DISubprogram(name: "b", scope: !3, file: !3, line: 2, type: !13, scopeLine: 2, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !2, retainedNodes: !16)
+!13 = !DISubroutineType(types: !14)
+!14 = !{!15}
+!15 = !DIBasicType(name: "short", size: 16, encoding: DW_ATE_signed)
+!16 = !{!17}
+!17 = !DILocalVariable(name: "i", scope: !12, file: !3, line: 3, type: !15)
+!18 = !DILocation(line: 3, scope: !12)

test/DebugInfo/NVPTX/debug-info.ll

 ; CHECK: // .b8 49                               // DW_AT_abstract_origin
 ; CHECK: // .b8 19                               // DW_FORM_ref4
 ; CHECK: // .b8 0                                // EOM(1)
 ; CHECK: // .b8 0                                // EOM(2)
 ; CHECK: // .b8 0                                // EOM(3)
 ; CHECK: // }
 ; CHECK: // .section .debug_info
 ; CHECK: // {
-; CHECK: // .b32 10025                           // Length of Unit
+; CHECK: // .b32 10030                           // Length of Unit
 ; CHECK: // .b8 2                                // DWARF version number
 ; CHECK: // .b8 0
 ; CHECK: // .b32 .debug_abbrev                   // Offset Into Abbrev. Section
 ; CHECK: // .b8 8                                // Address Size (in bytes)
-; CHECK: // .b8 1                                // Abbrev [1] 0xb:0x2722 DW_TAG_compile_unit
+; CHECK: // .b8 1                                // Abbrev [1] 0xb:0x2727 DW_TAG_compile_unit
 ; CHECK: // .b8 0                                // DW_AT_producer
 ; CHECK: // .b8 4                                // DW_AT_language
 ; CHECK: // .b8 0
 ; CHECK: // .b8 100,101,98,117,103,45,105,110,102,111,46,99,117 // DW_AT_name
 ; CHECK: // .b8 0
 ; CHECK: // .b32 .debug_line                     // DW_AT_stmt_list
 ; CHECK: // .b8 47,115,111,109,101,47,100,105,114,101,99,116,111,114,121 // DW_AT_comp_dir
 ; CHECK: // .b8 0

 ; CHECK: // .b32 1554                            // DW_AT_type
 ; CHECK: // .b8 39                               // Abbrev [39] 0x2665:0xb DW_TAG_formal_parameter
 ; CHECK: // .b8 114,101,115                      // DW_AT_name
 ; CHECK: // .b8 0
 ; CHECK: // .b8 12                               // DW_AT_decl_file
 ; CHECK: // .b8 3                                // DW_AT_decl_line
 ; CHECK: // .b32 3345                            // DW_AT_type
 ; CHECK: // .b8 0                                // End Of Children Mark
-; CHECK: // .b8 40                               // Abbrev [40] 0x2671:0xbb DW_TAG_subprogram
+; CHECK: // .b8 40                               // Abbrev [40] 0x2671:0xc0 DW_TAG_subprogram
 ; CHECK: // .b64 Lfunc_begin0                    // DW_AT_low_pc
 ; CHECK: // .b64 Lfunc_end0                      // DW_AT_high_pc
 ; CHECK: // .b8 1                                // DW_AT_frame_base
 ; CHECK: // .b8 156
 ; CHECK: // .b8 95,90,53,115,97,120,112,121,105,102,80,102,83,95 // DW_AT_MIPS_linkage_name
 ; CHECK: // .b8 0
 ; CHECK: // .b8 115,97,120,112,121               // DW_AT_name
 ; CHECK: // .b8 0

 ; CHECK: // .b8 12                               // DW_AT_call_file
 ; CHECK: // .b8 6                                // DW_AT_call_line
 ; CHECK: // .b8 42                               // Abbrev [42] 0x26f7:0x17 DW_TAG_inlined_subroutine
 ; CHECK: // .b32 9785                            // DW_AT_abstract_origin
 ; CHECK: // .b64 Ltmp2                           // DW_AT_low_pc
 ; CHECK: // .b64 Ltmp3                           // DW_AT_high_pc
 ; CHECK: // .b8 12                               // DW_AT_call_file
 ; CHECK: // .b8 6                                // DW_AT_call_line
-; CHECK: // .b8 43                               // Abbrev [43] 0x270e:0x1d DW_TAG_inlined_subroutine
+; CHECK: // .b8 43                               // Abbrev [43] 0x270e:0x22 DW_TAG_inlined_subroutine
 ; CHECK: // .b32 9791                            // DW_AT_abstract_origin
 ; CHECK: // .b64 Ltmp8                           // DW_AT_low_pc
 ; CHECK: // .b64 Ltmp9                           // DW_AT_high_pc
 ; CHECK: // .b8 12                               // DW_AT_call_file
 ; CHECK: // .b8 8                                // DW_AT_call_line
 ; CHECK: // .b8 44                               // Abbrev [44] 0x2725:0x5 DW_TAG_formal_parameter
 ; CHECK: // .b32 9820                            // DW_AT_abstract_origin
 ; CHECK: // .b8 0                                // End Of Children Mark

test/DebugInfo/X86/dbg-value-inlined-parameter.ll

 ;CHECK: DW_TAG_inlined_subroutine
 ;CHECK-NEXT: DW_AT_abstract_origin {{.*}} "foo"
 ;CHECK-NEXT: DW_AT_low_pc [DW_FORM_addr]
 ;CHECK-NEXT: DW_AT_high_pc [DW_FORM_data4]
 ;CHECK-NEXT: DW_AT_call_file
 ;CHECK-NEXT: DW_AT_call_line
 
 ;CHECK: DW_TAG_formal_parameter
-;FIXME: Linux shouldn't drop this parameter either...
 ;CHECK-NOT: DW_TAG
-;DARWIN:   DW_AT_abstract_origin {{.*}} "sp"
-;DARWIN: DW_TAG_formal_parameter
+;CHECK:   DW_AT_abstract_origin {{.*}} "sp"
+;CHECK: DW_TAG_formal_parameter
+;CHECK-NOT: DW_TAG
 ;CHECK: DW_AT_abstract_origin {{.*}} "nums"
 ;CHECK-NOT: DW_TAG_formal_parameter
 
 source_filename = "test/DebugInfo/X86/dbg-value-inlined-parameter.ll"
 
 %struct.S1 = type { float*, i32 }
 
 @p = common global %struct.S1 zeroinitializer, align 8, !dbg !0

test/DebugInfo/X86/pieces-3.ll

 ; Function Attrs: nounwind ssp uwtable
 define i32 @foo(i64 %outer.coerce0, i64 %outer.coerce1) #0 !dbg !4 {
   call void @llvm.dbg.value(metadata i64 %outer.coerce0, metadata !24, metadata !25), !dbg !26
   call void @llvm.dbg.declare(metadata !{null}, metadata !27, metadata !28), !dbg !26
   call void @llvm.dbg.value(metadata i64 %outer.coerce1, metadata !29, metadata !30), !dbg !26
   call void @llvm.dbg.declare(metadata !{null}, metadata !31, metadata !32), !dbg !26
   ; The 'trunc' generates no extra code, thus i1 is visible throughout its scope.
   %outer.sroa.1.8.extract.trunc = trunc i64 %outer.coerce1 to i32, !dbg !33
   call void @llvm.dbg.value(metadata i32 %outer.sroa.1.8.extract.trunc, metadata !34, metadata !35), !dbg !33
-  %outer.sroa.1.12.extract.shift = lshr i64 %outer.coerce1, 32, !dbg !33
-  %outer.sroa.1.12.extract.trunc = trunc i64 %outer.sroa.1.12.extract.shift to i32, !dbg !33
-  call void @llvm.dbg.value(metadata i64 %outer.sroa.1.12.extract.shift, metadata !34, metadata !35), !dbg !33
-  call void @llvm.dbg.value(metadata i32 %outer.sroa.1.12.extract.trunc, metadata !34, metadata !35), !dbg !33
-  call void @llvm.dbg.declare(metadata !{null}, metadata !34, metadata !35), !dbg !33
   ret i32 %outer.sroa.1.8.extract.trunc, !dbg !36
 }
 
 ; Function Attrs: nounwind readnone
 declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
 
 ; Function Attrs: nounwind
 declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture, i8* nocapture readonly, i64, i1) #2