[DebugInfo][InstrRef] Don't fire assertions if debug-info is faulty

It's inevitable that optimisation passes will fail to update debug-info:
when that happens, it's best if the compiler doesn't crash as a result.
Therefore, downgrade a few assertions / failure modes that would crash
when illegal debug-info was seen, to instead drop variable locations. In
practice this means that an instruction reference to a nonexistant or
illegal operand should be tolerated.

Differential Revision: https://reviews.llvm.org/D118998

Author: jmorse

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

@@ -1091,15 +1091,25 @@ bool InstrRefBasedLDV::transferDebugInstrRef(MachineInstr &MI,
       if (L)
         NewID = ValueIDNum(BlockNo, InstrIt->second.second, *L);
     } else if (OpNo != MachineFunction::DebugOperandMemNumber) {
-      assert(OpNo < TargetInstr.getNumOperands());
-      const MachineOperand &MO = TargetInstr.getOperand(OpNo);
-
-      // Today, this can only be a register.
-      assert(MO.isReg() && MO.isDef());
+      // Permit the debug-info to be completely wrong: identifying a nonexistant
+      // operand, or one that is not a register definition, means something
+      // unexpected happened during optimisation. Broken debug-info, however,
+      // shouldn't crash the compiler -- instead leave the variable value as
+      // None, which will make it appear "optimised out".
+      if (OpNo < TargetInstr.getNumOperands()) {
+        const MachineOperand &MO = TargetInstr.getOperand(OpNo);
+
+        if (MO.isReg() && MO.isDef() && MO.getReg()) {
+          unsigned LocID = MTracker->getLocID(MO.getReg());
+          LocIdx L = MTracker->LocIDToLocIdx[LocID];
+          NewID = ValueIDNum(BlockNo, InstrIt->second.second, L);
+        }
+      }
 
-      unsigned LocID = MTracker->getLocID(MO.getReg());
-      LocIdx L = MTracker->LocIDToLocIdx[LocID];
-      NewID = ValueIDNum(BlockNo, InstrIt->second.second, L);
+      if (!NewID) {
+        LLVM_DEBUG(
+            { dbgs() << "Seen instruction reference to illegal operand\n"; });
+      }
     }
     // else: NewID is left as None.
   } else if (PHIIt != DebugPHINumToValue.end() && PHIIt->InstrNum == InstNo) {
@@ -1249,7 +1259,16 @@ bool InstrRefBasedLDV::transferDebugPHI(MachineInstr &MI) {
   const MachineOperand &MO = MI.getOperand(0);
   unsigned InstrNum = MI.getOperand(1).getImm();
 
-  if (MO.isReg()) {
+  auto EmitBadPHI = [this, &MI, InstrNum](void) -> bool {
+    // Helper lambda to do any accounting when we fail to find a location for
+    // a DBG_PHI. This can happen if DBG_PHIs are malformed, or refer to a
+    // dead stack slot, for example.
+    // Record a DebugPHIRecord with an empty value + location.
+    DebugPHINumToValue.push_back({InstrNum, MI.getParent(), None, None});
+    return true;
+  };
+
+  if (MO.isReg() && MO.getReg()) {
     // The value is whatever's currently in the register. Read and record it,
     // to be analysed later.
     Register Reg = MO.getReg();
@@ -1261,15 +1280,14 @@ bool InstrRefBasedLDV::transferDebugPHI(MachineInstr &MI) {
     // Ensure this register is tracked.
     for (MCRegAliasIterator RAI(MO.getReg(), TRI, true); RAI.isValid(); ++RAI)
       MTracker->lookupOrTrackRegister(*RAI);
-  } else {
+  } else if (MO.isFI()) {
     // The value is whatever's in this stack slot.
-    assert(MO.isFI());
     unsigned FI = MO.getIndex();
 
     // If the stack slot is dead, then this was optimized away.
     // FIXME: stack slot colouring should account for slots that get merged.
     if (MFI->isDeadObjectIndex(FI))
-      return true;
+      return EmitBadPHI();
 
     // Identify this spill slot, ensure it's tracked.
     Register Base;
@@ -1280,7 +1298,7 @@ bool InstrRefBasedLDV::transferDebugPHI(MachineInstr &MI) {
     // We might be able to find a value, but have chosen not to, to avoid
     // tracking too much stack information.
     if (!SpillNo)
-      return true;
+      return EmitBadPHI();
 
     // Problem: what value should we extract from the stack? LLVM does not
     // record what size the last store to the slot was, and it would become
@@ -1315,8 +1333,17 @@ bool InstrRefBasedLDV::transferDebugPHI(MachineInstr &MI) {
     }
 
     // Record this DBG_PHI for later analysis.
-    auto DbgPHI = DebugPHIRecord({InstrNum, MI.getParent(), *Result, *SpillLoc});
+    auto DbgPHI =
+        DebugPHIRecord({InstrNum, MI.getParent(), *Result, *SpillLoc});
     DebugPHINumToValue.push_back(DbgPHI);
+  } else {
+    // Else: if the operand is neither a legal register or a stack slot, then
+    // we're being fed illegal debug-info. Record an empty PHI, so that any
+    // debug users trying to read this number will be put off trying to
+    // interpret the value.
+    LLVM_DEBUG(
+        { dbgs() << "Seen DBG_PHI with unrecognised operand format\n"; });
+    return EmitBadPHI();
   }
 
   return true;
@@ -3165,7 +3192,10 @@ bool InstrRefBasedLDV::ExtendRanges(MachineFunction &MF,
   // either live-through machine values, or PHIs.
   for (auto &DBG_PHI : DebugPHINumToValue) {
     // Identify unresolved block-live-ins.
-    ValueIDNum &Num = DBG_PHI.ValueRead;
+    if (!DBG_PHI.ValueRead)
+      continue;
+
+    ValueIDNum &Num = *DBG_PHI.ValueRead;
     if (!Num.isPHI())
       continue;
 
@@ -3566,17 +3596,24 @@ Optional<ValueIDNum> InstrRefBasedLDV::resolveDbgPHIsImpl(
   if (LowerIt == UpperIt)
     return None;
 
+  // If any DBG_PHIs referred to a location we didn't understand, don't try to
+  // compute a value. There might be scenarios where we could recover a value
+  // for some range of DBG_INSTR_REFs, but at this point we can have high
+  // confidence that we've seen a bug.
+  auto DBGPHIRange = make_range(LowerIt, UpperIt);
+  for (const DebugPHIRecord &DBG_PHI : DBGPHIRange)
+    if (!DBG_PHI.ValueRead)
+      return None;
+
   // If there's only one DBG_PHI, then that is our value number.
   if (std::distance(LowerIt, UpperIt) == 1)
-    return LowerIt->ValueRead;
-
-  auto DBGPHIRange = make_range(LowerIt, UpperIt);
+    return *LowerIt->ValueRead;
 
   // Pick out the location (physreg, slot) where any PHIs must occur. It's
   // technically possible for us to merge values in different registers in each
   // block, but highly unlikely that LLVM will generate such code after register
   // allocation.
-  LocIdx Loc = LowerIt->ReadLoc;
+  LocIdx Loc = *LowerIt->ReadLoc;
 
   // We have several DBG_PHIs, and a use position (the Here inst). All each
   // DBG_PHI does is identify a value at a program position. We can treat each
@@ -3595,7 +3632,7 @@ Optional<ValueIDNum> InstrRefBasedLDV::resolveDbgPHIsImpl(
   // for the SSAUpdater.
   for (const auto &DBG_PHI : DBGPHIRange) {
     LDVSSABlock *Block = Updater.getSSALDVBlock(DBG_PHI.MBB);
-    const ValueIDNum &Num = DBG_PHI.ValueRead;
+    const ValueIDNum &Num = *DBG_PHI.ValueRead;
     AvailableValues.insert(std::make_pair(Block, Num.asU64()));
   }
 
@@ -3629,7 +3666,7 @@ Optional<ValueIDNum> InstrRefBasedLDV::resolveDbgPHIsImpl(
   // Define all the input DBG_PHI values in ValidatedValues.
   for (const auto &DBG_PHI : DBGPHIRange) {
     LDVSSABlock *Block = Updater.getSSALDVBlock(DBG_PHI.MBB);
-    const ValueIDNum &Num = DBG_PHI.ValueRead;
+    const ValueIDNum &Num = *DBG_PHI.ValueRead;
     ValidatedValues.insert(std::make_pair(Block, Num));
   }
 

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h

@@ -851,10 +851,16 @@ class InstrRefBasedLDV : public LDVImpl {
   /// Record of where we observed a DBG_PHI instruction.
   class DebugPHIRecord {
   public:
-    uint64_t InstrNum;      ///< Instruction number of this DBG_PHI.
-    MachineBasicBlock *MBB; ///< Block where DBG_PHI occurred.
-    ValueIDNum ValueRead;   ///< The value number read by the DBG_PHI.
-    LocIdx ReadLoc;         ///< Register/Stack location the DBG_PHI reads.
+    /// Instruction number of this DBG_PHI.
+    uint64_t InstrNum;
+    /// Block where DBG_PHI occurred.
+    MachineBasicBlock *MBB;
+    /// The value number read by the DBG_PHI -- or None if it didn't refer to
+    /// a value.
+    Optional<ValueIDNum> ValueRead;
+    /// Register/Stack location the DBG_PHI reads -- or None if it referred to
+    /// something unexpected.
+    Optional<LocIdx> ReadLoc;
 
     operator unsigned() const { return InstrNum; }
   };

llvm/test/DebugInfo/MIR/InstrRef/livedebugvalues_illegal_locs.mir

@@ -0,0 +1,119 @@
+--- |
+  ; RUN: llc %s -march=x86-64 -run-pass=livedebugvalues -o - -experimental-debug-variable-locations |  FileCheck %s -implicit-check-not=DBG_VALUE
+  ;
+  ; Test that InstrRefBasedLDV / livedebugvalues doesn't crash when it sees
+  ; illegal instruction referencing debug-info. This can be caused by
+  ; optimisations not updating debug-info or doing it incorrectly, which is
+  ; inevitable. When that happens, we should safely drop the variable location,
+  ; but not crash.
+
+  define i32 @_Z8bb_to_bb() local_unnamed_addr !dbg !12 {
+  entry:
+    ret i32 0, !dbg !17
+  }
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!7, !8, !9, !10}
+  !llvm.ident = !{!11}
+  !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 10.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, globals: !3, debugInfoForProfiling: true, nameTableKind: None)
+  !1 = !DIFile(filename: "main.cpp", directory: "F:\")
+  !2 = !{}
+  !3 = !{!4}
+  !4 = !DIGlobalVariableExpression(var: !5, expr: !DIExpression())
+  !5 = distinct !DIGlobalVariable(name: "start", scope: !0, file: !1, line: 4, type: !6, isLocal: false, isDefinition: true)
+  !6 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !7 = !{i32 2, !"Dwarf Version", i32 4}
+  !8 = !{i32 2, !"Debug Info Version", i32 3}
+  !9 = !{i32 1, !"wchar_size", i32 2}
+  !10 = !{i32 7, !"PIC Level", i32 2}
+  !11 = !{!"clang version 10.0.0"}
+  !12 = distinct !DISubprogram(name: "bb_to_bb", linkageName: "bb_to_bb", scope: !1, file: !1, line: 6, type: !13, scopeLine: 6, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !15)
+  !13 = !DISubroutineType(types: !14)
+  !14 = !{!6, !6}
+  !15 = !{!16}
+  !16 = !DILocalVariable(name: "myVar", scope: !12, file: !1, line: 7, type: !6)
+  !17 = !DILocation(line: 10, scope: !12)
+
+...
+---
+name: _Z8bb_to_bb
+debugValueSubstitutions:
+  - { srcinst: 4, srcop: 0, dstinst: 3, dstop: 0, subreg: 0 }
+body:  |
+  bb.0.entry:
+    successors: %bb.1, %bb.2
+    ; CHECK-LABE: bb.0.entry:
+
+    $rax = MOV64ri 1, debug-instr-number 1, debug-location !17
+    DBG_INSTR_REF 1, 0, !16, !DIExpression(), debug-location !17
+    ;; First check that picking out location works as usual.
+    ; CHECK:      DBG_INSTR_REF 1, 0
+    ; CHECK-NEXT: DBG_VALUE $rax
+
+    $rax = MOV64ri 1, debug-instr-number 2, debug-location !17
+    DBG_INSTR_REF 2, 999, !16, !DIExpression(), debug-location !17
+    ;; Test out of bounds operand number.
+    ; CHECK:      DBG_INSTR_REF 2, 999
+    ; CHECK-NEXT: DBG_VALUE $noreg
+
+    $rax = MOV64ri 1, debug-instr-number 3, debug-location !17
+    DBG_INSTR_REF 3, 1, !16, !DIExpression(), debug-location !17
+    ;; Test non-register operand
+    ; CHECK:      DBG_INSTR_REF 3, 1
+    ; CHECK-NEXT: DBG_VALUE $noreg
+
+    ;; FIXME: We should test what happens when this meta-instruction is seen
+    ;; by livedbugvalues with an instruction number. However, right now it's
+    ;; impossible to turn the machine-code verifier off when loading MIR?
+    ;KILL implicit killed $eflags, debug-instr-number 4, debug-location !17
+    ;DBG_INSTR_REF 4, 0, !16, !DIExpression(), debug-location !17
+    ;;; Test non-def operand
+    ;; check:      DBG_INSTR_REF 4, 0
+    ;; check-next: DBG_VALUE $noreg
+
+    $noreg = MOV32ri 1, debug-instr-number 5, debug-location !17
+    DBG_INSTR_REF 5, 0, !16, !DIExpression(), debug-location !17
+    ;; Def of $noreg?
+    ; CHECK:      DBG_INSTR_REF 5, 0
+    ; CHECK-NEXT: DBG_VALUE $noreg
+
+    JCC_1 %bb.1, 1, implicit $eflags
+    JMP_1 %bb.2
+
+  bb.1:
+    successors: %bb.3
+    ; CHECK-LABEL: bb.1:
+
+    DBG_PHI $rax, 6
+    DBG_INSTR_REF 6, 1, !16, !DIExpression(), debug-location !17
+    ;; Test out-of-bounds reference to a DBG_PHI.
+    ; CHECK:      DBG_INSTR_REF 6, 1
+    ; CHECK-NEXT: DBG_VALUE $noreg
+
+    DBG_PHI $noreg, 7
+    JMP_1 %bb.3
+
+  bb.2:
+    successors: %bb.3
+    ; CHECK-LABEL: bb.2:
+    DBG_PHI 1, 6
+    DBG_INSTR_REF 6, 0, !16, !DIExpression(), debug-location !17
+    ;; Test non-reg operand to DBG_PHI. It's not clear if this can ever happen
+    ;; as the result of an optimisation, but lets test for it anyway.
+    ; CHECK:      DBG_INSTR_REF 6, 0
+    ; CHECK-NEXT: DBG_VALUE $noreg
+
+    DBG_PHI 1, 7
+    JMP_1 %bb.3
+
+  bb.3:
+    ; CHECK-LABEL: bb.3:
+    DBG_INSTR_REF 7, 0, !16, !DIExpression(), debug-location !17
+    ;; PHI resolution of illegal inputs shouldn't crash either. It should also
+    ;; come out as a $noreg location.
+    ; CHECK:      DBG_INSTR_REF 7, 0
+    ; CHECK-NEXT: DBG_VALUE $noreg
+
+    RET 0, debug-location !17
+
+...