[llvm] be5734d - [DebugInfo][InstrRef] Don't fire assertions if debug-info is faulty
Jeremy Morse via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 10 03:25:21 PST 2022
Author: Jeremy Morse
Date: 2022-02-10T11:25:08Z
New Revision: be5734ddaae3521f8ccf3dbf4e747f51aa448936
URL: https://github.com/llvm/llvm-project/commit/be5734ddaae3521f8ccf3dbf4e747f51aa448936
DIFF: https://github.com/llvm/llvm-project/commit/be5734ddaae3521f8ccf3dbf4e747f51aa448936.diff
LOG: [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
Added:
llvm/test/DebugInfo/MIR/InstrRef/livedebugvalues_illegal_locs.mir
Modified:
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h
Removed:
################################################################################
diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
index b6632f9a779b6..4c2484f9d98f1 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
+++ b/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
diff erent 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));
}
diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h
index d778561db471c..1ddcb488feae2 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h
+++ b/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; }
};
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
index 0000000000000..53e9148bf5505
--- /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
+
+...
More information about the llvm-commits
mailing list