[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 ↗

(On Diff #176996)

Why is this necessary?

[jmorse]

test/DebugInfo/X86/pieces-3.ll

40 ↗

(On Diff #176996)

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 ↗

(On Diff #176996)

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 be prevented from leaking 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);
+        (*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) {
       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)