[DebugInfo] Keep DBG_VALUE undef in LiveDebugVariables

[uabelho]

Summary

[DebugInfo] Keep DBG_VALUE undef in LiveDebugVariables

Fixes PR36579.

For cases where we had e.g.

DBG_VALUE 42
[...]
DBG_VALUE undef

LiveDebugVariables would discard all undef DBG_VALUEs and then it would
look like the variable had the value 42 throughout the rest of the
function, which is incorrect.

With this patch we don't remove all undef DBG_VALUEs in LiveDebugVariables
so they will be kept after register allocation just like other DBG_VALUEs
which will yield more correct debug information.

[uabelho]

I don't know this code very well so I did an attempt to remove the undef removal, and then I ran a bunch of tests to see what
would go wrong. I then ended up in failed assertions when trying to find the undef value in "locations" and "LocNoMap" so
I gurded that with !isUndef() checks.

Does this make sense? Any other obvious cases where we need to do something special with undef?

We could perhaps change the "getLocationNo" method so it inserts an undef value in "locations" as well, but I've no idea
if that would make anything better.

[aprantl]

lib/CodeGen/LiveDebugVariables.cpp

1097 ↗

(On Diff #151694)

Just for my understanding: What is preventing you from adding the undef location into the map?

[bjope]

lib/CodeGen/LiveDebugVariables.cpp

1097 ↗

(On Diff #151694)

The reason afaik: locNo() is ~0U for isUndef() (that is actually how undef is identified). So adding it to the LocNoMap, which actually is implemented as a vector and not a map, would make that vector very long.

Maybe that can be solved somehow :

• chopping off another bit in DbgValueLocation to describe IsUndef similar to WasIndirect
• or by reserving locNo 0 for isUndef instead of using ~0U)
• or is it possible to use a map instead of a vector to implement the LocNoMap

[aprantl]

Looks good after adding an additional comment explaining why the undef values have to be treated differently. Thanks!

lib/CodeGen/LiveDebugVariables.cpp

1095 ↗

(On Diff #151694)

Please add a comment explaining why they aren't placed in the LocNoMap so future readers understand this.

1097 ↗

(On Diff #151694)

Got it. The current patch doesn't look to bad either.

[uabelho]

Added some comments about why undef values are not added to "locations".

Also added an isUndef() check in UserValue::rewriteLocations so we don't try to update the locNo
of undef locNo (which should always be the same, ~0).

[uabelho]

Thanks aprantl and bjope!

I think I addressed the comments. I ran tests on the patch all night and found another case where we need to special case isUndef(),
I fixed that too in the updated patch.

[uabelho]

Do you still think this looks ok?

I think I'll submit this tomorrow morning when I get to the office unless something new pops up in my tests tonight.

[aprantl]

lib/CodeGen/LiveDebugVariables.cpp

227 ↗

(On Diff #151898)

Can you delete the getLocationNo - part?

992 ↗

(On Diff #151898)

I think with the comment this is fine.

llvm/trunk/lib/CodeGen/LiveDebugVariables.cpp

while (End->next) {
       End = End->next;
     }
     End->leader = L1;
     End->next = L1->next;
     L1->next = L2;
     return L1;
   }
 
-  /// getLocationNo - Return the location number that matches Loc.
+  /// Return the location number that matches Loc.
+  ///
+  /// For undef values we always return location number UndefLocNo without
+  /// inserting anything in locations. Since locations is a vector and the
+  /// location number is the position in the vector and UndefLocNo is ~0,
+  /// we would need a very big vector to put the value at the right position.
   unsigned getLocationNo(const MachineOperand &LocMO) {
     if (LocMO.isReg()) {
       if (LocMO.getReg() == 0)
         return UndefLocNo;
       // For register locations we dont care about use/def and other flags.
       for (unsigned i = 0, e = locations.size(); i != e; ++i)
         if (locations[i].isReg() &&
             locations[i].getReg() == LocMO.getReg() &&
for (unsigned i = 0; i != Defs.size(); ++i) {
     // For physregs, we only mark the start slot idx. DwarfDebug will see it
     // as if the DBG_VALUE is valid up until the end of the basic block, or
     // the next def of the physical register. So we do not need to extend the
     // range. It might actually happen that the DBG_VALUE is the last use of
     // the physical register (e.g. if this is an unused input argument to a
     // function).
   }
 
-  // Erase all the undefs.
-  for (LocMap::iterator I = locInts.begin(); I.valid();)
-    if (I.value().isUndef())
-      I.erase();
-    else
-      ++I;
-
   // The computed intervals may extend beyond the range of the debug
   // location's lexical scope. In this case, splitting of an interval
   // can result in an interval outside of the scope being created,
   // causing extra unnecessary DBG_VALUEs to be emitted. To prevent
   // this, trim the intervals to the lexical scope.
 
   LexicalScope *Scope = LS.findLexicalScope(dl);
   if (!Scope)
UserValue::splitLocation(unsigned OldLocNo, ArrayRef<unsigned> NewRegs,
   LocMapI.goToBegin();
   while (LocMapI.valid()) {
     DbgValueLocation v = LocMapI.value();
     if (v.locNo() == OldLocNo) {
       LLVM_DEBUG(dbgs() << "Erasing [" << LocMapI.start() << ';'
                         << LocMapI.stop() << ")\n");
       LocMapI.erase();
     } else {
-      if (v.locNo() > OldLocNo)
+      // Undef values always have location number UndefLocNo, so don't change
+      // locNo in that case. See getLocationNo().
+      if (!v.isUndef() && v.locNo() > OldLocNo)
         LocMapI.setValueUnchecked(v.changeLocNo(v.locNo() - 1));
       ++LocMapI;
     }
   }
 
   LLVM_DEBUG({
     dbgs() << "Split result: \t";
     print(dbgs(), nullptr);
void UserValue::rewriteLocations(VirtRegMap &VRM, const TargetRegisterInfo &TRI,
   }
 
   // Update the interval map, but only coalesce left, since intervals to the
   // right use the old location numbers. This should merge two contiguous
   // DBG_VALUE intervals with different vregs that were allocated to the same
   // physical register.
   for (LocMap::iterator I = locInts.begin(); I.valid(); ++I) {
     DbgValueLocation Loc = I.value();
+    // Undef values don't exist in locations (and thus not in LocNoMap either)
+    // so skip over them. See getLocationNo().
+    if (Loc.isUndef())
+      continue;
     unsigned NewLocNo = LocNoMap[Loc.locNo()];
     I.setValueUnchecked(Loc.changeLocNo(NewLocNo));
     I.setStart(I.start());
   }
 }
 
 /// Find an iterator for inserting a DBG_VALUE instruction.
 static MachineBasicBlock::iterator
void UserValue::insertDebugValue(MachineBasicBlock *MBB, SlotIndex StartIdx,
                                  DbgValueLocation Loc, bool Spilled,
                                  LiveIntervals &LIS,
                                  const TargetInstrInfo &TII,
                                  const TargetRegisterInfo &TRI) {
   SlotIndex MBBEndIdx = LIS.getMBBEndIdx(&*MBB);
   // Only search within the current MBB.
   StopIdx = (MBBEndIdx < StopIdx) ? MBBEndIdx : StopIdx;
   MachineBasicBlock::iterator I = findInsertLocation(MBB, StartIdx, LIS);
-  MachineOperand &MO = locations[Loc.locNo()];
+  // Undef values don't exist in locations so create new "noreg" register MOs
+  // for them. See getLocationNo().
+  MachineOperand MO = !Loc.isUndef() ?
+    locations[Loc.locNo()] :
+    MachineOperand::CreateReg(/* Reg */ 0, /* isDef */ false, /* isImp */ false,
+                              /* isKill */ false, /* isDead */ false,
+                              /* isUndef */ false, /* isEarlyClobber */ false,
+                              /* SubReg */ 0, /* isDebug */ true);
+
   ++NumInsertedDebugValues;
 
   assert(cast<DILocalVariable>(Variable)
              ->isValidLocationForIntrinsic(getDebugLoc()) &&
          "Expected inlined-at fields to agree");
 
   // If the location was spilled, the new DBG_VALUE will be indirect. If the
   // original DBG_VALUE was indirect, we need to add DW_OP_deref to indicate

llvm/trunk/test/DebugInfo/X86/live-debug-vars-discard-invalid.mir

body:             |
 
   bb.5:
     RET 0, debug-location !32
 ...
 
 # CHECK-LABEL: name: foobar
 
 # CHECK-LABEL: bb.1:
-## After solving https://bugs.llvm.org/show_bug.cgi?id=36579 we expect to get a
-##   DBG_VALUE debug-use $noreg
-## here.
-# CHECK-NOT:    DBG_VALUE
+# CHECK:        DBG_VALUE debug-use $noreg
 
 # CHECK-LABEL: bb.2:
-## After solving https://bugs.llvm.org/show_bug.cgi?id=36579 we expect to get a
-##   DBG_VALUE debug-use $noreg
-## here.
-# CHECK-NOT:    DBG_VALUE
-# CHECK:        dead renamable $rcx = IMPLICIT_DEF
+# CHECK:        DBG_VALUE debug-use $noreg
+# CHECK-NEXT:   dead renamable $rcx = IMPLICIT_DEF
 # CHECK-NEXT:   dead renamable $rcx = IMPLICIT_DEF
 # CHECK-NEXT:   dead renamable $rcx = IMPLICIT_DEF
 # CHECK-NEXT:   dead renamable $rcx = IMPLICIT_DEF
 # CHECK-NEXT:   DBG_VALUE debug-use $rcx, debug-use $noreg, !18, !DIExpression()
 
 # CHECK-LABEL: bb.3:
 # CHECK:        dead renamable $rcx = IMPLICIT_DEF
 # CHECK-NEXT:   DBG_VALUE 0, debug-use $noreg, !23, !DIExpression()

llvm/trunk/test/DebugInfo/X86/live-debug-vars-keep-undef.ll

+; RUN: llc -O1 -o - %s | FileCheck %s
+
+; Check that dbg.value(undef) calls are not simply discarded.
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @foo() !dbg !7 {
+entry:
+  call void @llvm.dbg.value(metadata i32 42, metadata !11, metadata !DIExpression()), !dbg !13
+  call void (...) @bar(), !dbg !14
+  call void @llvm.dbg.value(metadata i32 undef, metadata !11, metadata !DIExpression()), !dbg !13
+  call void (...) @bar(), !dbg !15
+  ret void, !dbg !16
+}
+
+declare void @bar(...)
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+
+attributes #0 = { nounwind readnone speculatable }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 7.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "test.c", directory: "/tmp")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 7.0.0"}
+!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 3, type: !8, isLocal: false, isDefinition: true, scopeLine: 3, isOptimized: true, unit: !0, retainedNodes: !10)
+!8 = !DISubroutineType(types: !9)
+!9 = !{null}
+!10 = !{!11}
+!11 = !DILocalVariable(name: "x", scope: !7, file: !1, line: 4, type: !12)
+!12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!13 = !DILocation(line: 4, column: 7, scope: !7)
+!14 = !DILocation(line: 5, column: 3, scope: !7)
+!15 = !DILocation(line: 7, column: 3, scope: !7)
+!16 = !DILocation(line: 8, column: 1, scope: !7)
+
+; Original C program:
+;  void bar();
+;
+;  void foo() {
+;    int x = 42;
+;    bar();
+;    x = 23;
+;    bar();
+;  }
+;
+; Then the dbg.value for the x = 23 setting has been replaced with dbg.value(undef)
+; in this ll file to provoke the wanted behavior in LiveDebugVariables.
+
+; Variable 'x' should thus be updated two times, first to 42 and then to undef.
+
+; CHECK-LABEL: foo:
+; CHECK:               #DEBUG_VALUE: foo:x <- 42
+; CHECK:               callq   bar
+; CHECK:               #DEBUG_VALUE: foo:x <- undef
+; CHECK:               callq   bar