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

[jmorse]

Summary

As reported in https://github.com/llvm/llvm-project/issues/53584 , there are a few lurking optimisations that don't update debug-info when instructions are mutated (a fairly rare event anyway). When this happens, it's nice to get bug reports, but it's even better for the developer to not crash the compiler and instead ignore any broken variable locations.

To that end, convert assertions about instruction references always referring to register definitions into "if" conditions -- when a non-register-def operand is referred to in transferDebugInstrRef, the variable value will remain "None" and become "optimised out" when seen in a debugger.

I've done the same for DBG_PHIs, and that's exposed a more interesting issue: If there's only ever one DBG_PHI for a value, we don't need to do any further work to determine the instruction references value, it's the one referred to by the DBG_PHI. But:

• If there's more than one DBG_PHI (see value 7 in the added test),
• and one of them is invalid in some way,
• We ignore it, leaving a single DBG_PHI recorded, making the instruction reference value be whatever the remaining DBG_PHI refers to.

In the test below (the DBG_PHIs for value number 7, in blocks 1 and 2), that's undesirable because we essentially ignore the illegal side of the branch in favour of the other. A better solution is to accept that the debug-info is broken, and not produce a variable location. I've implemented that by making the value seen by DBG_PHI Optional, recording None values when a broken DBG_PHI is seen, then producing no variable value from resolveDbgPHIs if any were broken.

I've added some LLVM_DEBUG lines to complain if any of these broken values are seen.

[Orlando]

LGTM. Converting badly-formed variable locations into "no location" rather than crashing the compiler SGTM. Is it worth fixing the defect in the bug report too in a separate patch?

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

4–5

Could you add a sentence to describe the purpose of the test?

[jmorse]

In D118998#3304180, @Orlando wrote:

LGTM. Converting badly-formed variable locations into "no location" rather than crashing the compiler SGTM. Is it worth fixing the defect in the bug report too in a separate patch?

Separate patch is best IMO, it'll manifest as a dropped location right now, which is also what fixing the function mentioned in the bug report will do. This also gives more choices when cherry picking to the release branch.

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

4–5

Good point, I'll fold that into the landed verion.

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h

private:
   /// Map from debug instruction number to the MachineInstr labelled with that
   /// number, and its location within the function. Used to transform
   /// instruction numbers in DBG_INSTR_REFs into machine value numbers.
   std::map<uint64_t, InstAndNum> DebugInstrNumToInstr;
 
   /// 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; }
   };
 
   /// Map from instruction numbers defined by DBG_PHIs to a record of what that
   /// DBG_PHI read and where. Populated and edited during the machine value
   /// location problem -- we use LLVMs SSA Updater to fix changes by
   /// optimizations that destroy PHI instructions.

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

if (InstrIt != DebugInstrNumToInstr.end()) {
     // Pick out the designated operand. It might be a memory reference, if
     // a register def was folded into a stack store.
     if (OpNo == MachineFunction::DebugOperandMemNumber &&
         TargetInstr.hasOneMemOperand()) {
       Optional<LocIdx> L = findLocationForMemOperand(TargetInstr);
       if (L)
         NewID = ValueIDNum(BlockNo, InstrIt->second.second, *L);
     } else if (OpNo != MachineFunction::DebugOperandMemNumber) {
-      assert(OpNo < TargetInstr.getNumOperands());
+      // 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);
 
-      // Today, this can only be a register.
-      assert(MO.isReg() && MO.isDef());
-
+        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);
         }
+      }
+
+      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) {
     // It's actually a PHI value. Which value it is might not be obvious, use
     // the resolver helper to find out.
     NewID = resolveDbgPHIs(*MI.getParent()->getParent(), MLiveOuts, MLiveIns,
                            MI, InstNo);
   }
 
bool InstrRefBasedLDV::transferDebugPHI(MachineInstr &MI) {
   if (VTracker || TTracker)
     return true;
 
   // First operand is the value location, either a stack slot or register.
   // Second is the debug instruction number of the original PHI.
   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();
     ValueIDNum Num = MTracker->readReg(Reg);
     auto PHIRec = DebugPHIRecord(
         {InstrNum, MI.getParent(), Num, MTracker->lookupOrTrackRegister(Reg)});
     DebugPHINumToValue.push_back(PHIRec);
 
     // 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;
     StackOffset Offs = TFI->getFrameIndexReference(*MI.getMF(), FI, Base);
     SpillLoc SL = {Base, Offs};
     Optional<SpillLocationNo> SpillNo = MTracker->getOrTrackSpillLoc(SL);
 
     // 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
     // sketchy after stack slot colouring anyway. Take a look at what values
     // are stored on the stack, and pick the largest one that wasn't def'd
     // by a spill (i.e., the value most likely to have been def'd in a register
     // and then spilt.
     std::array<unsigned, 4> CandidateSizes = {64, 32, 16, 8};
if (MO.isReg() && MO.getReg()) {
     // locations.
     if (!Result) {
       unsigned SpillID = MTracker->getLocID(*SpillNo, {64, 0});
       SpillLoc = MTracker->getSpillMLoc(SpillID);
       Result = MTracker->readMLoc(*SpillLoc);
     }
 
     // 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;
 }
 
 void InstrRefBasedLDV::transferRegisterDef(MachineInstr &MI) {
   // Meta Instructions do not affect the debug liveness of any register they
   // define.
bool InstrRefBasedLDV::ExtendRanges(MachineFunction &MF,
   // both live-ins and live-outs for decision making in the variable value
   // dataflow problem.
   buildMLocValueMap(MF, MInLocs, MOutLocs, MLocTransfer);
 
   // Patch up debug phi numbers, turning unknown block-live-in values into
   // 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;
 
     unsigned BlockNo = Num.getBlock();
     LocIdx LocNo = Num.getLoc();
     Num = MInLocs[BlockNo][LocNo.asU64()];
   }
   // Later, we'll be looking up ranges of instruction numbers.
auto RangePair = std::equal_range(DebugPHINumToValue.begin(),
                                     DebugPHINumToValue.end(), InstrNum);
   auto LowerIt = RangePair.first;
   auto UpperIt = RangePair.second;
 
   // No DBG_PHI means there can be no location.
   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
   // DBG_PHI like it's a Def of a value, and the use position is a Use of a
   // value, just like SSA. We use the bulk-standard LLVM SSA updater class to
   // determine which Def is used at the Use, and any PHIs that happen along
   // the way.
   // Adapted LLVM SSA Updater:
   LDVSSAUpdater Updater(Loc, MLiveIns);
   // Map of which Def or PHI is the current value in each block.
   DenseMap<LDVSSABlock *, BlockValueNum> AvailableValues;
   // Set of PHIs that we have created along the way.
   SmallVector<LDVSSAPhi *, 8> CreatedPHIs;
 
   // Each existing DBG_PHI is a Def'd value under this model. Record these Defs
   // 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()));
   }
 
   LDVSSABlock *HereBlock = Updater.getSSALDVBlock(Here.getParent());
   const auto &AvailIt = AvailableValues.find(HereBlock);
   if (AvailIt != AvailableValues.end()) {
     // Actually, we already know what the value is -- the Use is in the same
     // block as the Def.
Optional<ValueIDNum> InstrRefBasedLDV::resolveDbgPHIsImpl(
   //    predecessor values the same, leading to a non-PHI machine value number?
   //    (SSAUpdater doesn't know that either). Remap validated PHIs into the
   //    the ValidatedValues collection below to sort this out.
   DenseMap<LDVSSABlock *, ValueIDNum> ValidatedValues;
 
   // 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));
   }
 
   // Sort PHIs to validate into RPO-order.
   SmallVector<LDVSSAPhi *, 8> SortedPHIs;
   for (auto &PHI : CreatedPHIs)
     SortedPHIs.push_back(PHI);
 

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

+--- |
+  ; 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
+
+...