diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h --- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h +++ b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h @@ -851,10 +851,16 @@ /// 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 ValueRead; + /// Register/Stack location the DBG_PHI reads -- or None if it referred to + /// something unexpected. + Optional ReadLoc; operator unsigned() const { return InstrNum; } }; diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp --- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp +++ b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp @@ -1091,15 +1091,25 @@ 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 @@ 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 @@ // 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 @@ // 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 @@ } // 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 @@ // 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 @@ 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 @@ // 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 @@ // 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)); } diff --git a/llvm/test/DebugInfo/MIR/InstrRef/livedebugvalues_illegal_locs.mir b/llvm/test/DebugInfo/MIR/InstrRef/livedebugvalues_illegal_locs.mir new file mode 100644 --- /dev/null +++ b/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 + +...