diff --git a/llvm/include/llvm/IR/BasicBlock.h b/llvm/include/llvm/IR/BasicBlock.h index ed25cb96ae964..179305e9260f9 100644 --- a/llvm/include/llvm/IR/BasicBlock.h +++ b/llvm/include/llvm/IR/BasicBlock.h @@ -93,15 +93,6 @@ class BasicBlock final : public Value, // Basic blocks are data objects also /// if necessary. void setIsNewDbgInfoFormat(bool NewFlag); - /// Validate any DPMarkers / DPValues attached to instructions in this block, - /// and block-level stored data too (TrailingDPValues). - /// \p Assert Should this method fire an assertion if a problem is found? - /// \p Msg Should this method print a message to errs() if a problem is found? - /// \p OS Output stream to write errors to. - /// \returns True if a problem is found. - bool validateDbgValues(bool Assert = true, bool Msg = false, - raw_ostream *OS = nullptr); - /// Record that the collection of DPValues in \p M "trails" after the last /// instruction of this block. These are equivalent to dbg.value intrinsics /// that exist at the end of a basic block with no terminator (a transient diff --git a/llvm/include/llvm/IR/DebugProgramInstruction.h b/llvm/include/llvm/IR/DebugProgramInstruction.h index 97089098ee535..2dd546ce709c7 100644 --- a/llvm/include/llvm/IR/DebugProgramInstruction.h +++ b/llvm/include/llvm/IR/DebugProgramInstruction.h @@ -224,7 +224,7 @@ class DPValue : public DbgRecord, protected DebugValueUser { // DebugValueUser superclass instead. The referred to Value can either be a // ValueAsMetadata or a DIArgList. - DILocalVariable *Variable; + TrackingMDNodeRef Variable; DIExpression *Expression; DIExpression *AddressExpression; @@ -331,7 +331,7 @@ class DPValue : public DbgRecord, protected DebugValueUser { void addVariableLocationOps(ArrayRef NewValues, DIExpression *NewExpr); - void setVariable(DILocalVariable *NewVar) { Variable = NewVar; } + void setVariable(DILocalVariable *NewVar); void setExpression(DIExpression *NewExpr) { Expression = NewExpr; } @@ -349,7 +349,8 @@ class DPValue : public DbgRecord, protected DebugValueUser { void setKillLocation(); bool isKillLocation() const; - DILocalVariable *getVariable() const { return Variable; } + DILocalVariable *getVariable() const; + MDNode *getRawVariable() const { return Variable; } DIExpression *getExpression() const { return Expression; } diff --git a/llvm/lib/IR/AsmWriter.cpp b/llvm/lib/IR/AsmWriter.cpp index a3da6ca811489..4e1e48b4ad4a3 100644 --- a/llvm/lib/IR/AsmWriter.cpp +++ b/llvm/lib/IR/AsmWriter.cpp @@ -1143,15 +1143,15 @@ void SlotTracker::processDbgRecordMetadata(const DbgRecord &DR) { // Process metadata used by DbgRecords; we only specifically care about the // DILocalVariable, DILocation, and DIAssignID fields, as the Value and // Expression fields should only be printed inline and so do not use a slot. - CreateMetadataSlot(DPV->getVariable()); + CreateMetadataSlot(DPV->getRawVariable()); if (DPV->isDbgAssign()) - CreateMetadataSlot(DPV->getAssignID()); + CreateMetadataSlot(cast(DPV->getRawAssignID())); } else if (const DPLabel *DPL = dyn_cast(&DR)) { CreateMetadataSlot(DPL->getLabel()); } else { llvm_unreachable("unsupported DbgRecord kind"); } - CreateMetadataSlot(DR.getDebugLoc()); + CreateMetadataSlot(DR.getDebugLoc().getAsMDNode()); } void SlotTracker::processInstructionMetadata(const Instruction &I) { diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp index c110c4c1437c3..25aa326116451 100644 --- a/llvm/lib/IR/BasicBlock.cpp +++ b/llvm/lib/IR/BasicBlock.cpp @@ -122,67 +122,6 @@ void BasicBlock::convertFromNewDbgValues() { assert(!getTrailingDPValues()); } -bool BasicBlock::validateDbgValues(bool Assert, bool Msg, raw_ostream *OS) { - bool RetVal = false; - if (!OS) - OS = &errs(); - - // Helper lambda for reporting failures: via assertion, printing, and return - // value. - auto TestFailure = [Assert, Msg, &RetVal, OS](bool Val, const char *Text) { - // Did the test fail? - if (Val) - return; - - // If we're asserting, then fire off an assertion. - if (Assert) - llvm_unreachable(Text); - - if (Msg) - *OS << Text << "\n"; - RetVal = true; - }; - - // We should have the same debug-format as the parent function. - TestFailure(getParent()->IsNewDbgInfoFormat == IsNewDbgInfoFormat, - "Parent function doesn't have the same debug-info format"); - - // Only validate if we are using the new format. - if (!IsNewDbgInfoFormat) - return RetVal; - - // Match every DPMarker to every Instruction and vice versa, and - // verify that there are no invalid DPValues. - for (auto It = begin(); It != end(); ++It) { - if (!It->DbgMarker) - continue; - - // Validate DebugProgramMarkers. - DPMarker *CurrentDebugMarker = It->DbgMarker; - - // If this is a marker, it should match the instruction and vice versa. - TestFailure(CurrentDebugMarker->MarkedInstr == &*It, - "Debug Marker points to incorrect instruction?"); - - // Now validate any DPValues in the marker. - for (DbgRecord &DPR : CurrentDebugMarker->getDbgValueRange()) { - // Validate DebugProgramValues. - TestFailure(DPR.getMarker() == CurrentDebugMarker, - "Not pointing at correct next marker!"); - - // Verify that no DbgValues appear prior to PHIs. - TestFailure( - !isa(It), - "DebugProgramValues must not appear before PHI nodes in a block!"); - } - } - - // Except transiently when removing + re-inserting the block terminator, there - // should be no trailing DPValues. - TestFailure(!getTrailingDPValues(), "Trailing DPValues in block"); - return RetVal; -} - #ifndef NDEBUG void BasicBlock::dumpDbgValues() const { for (auto &Inst : *this) { diff --git a/llvm/lib/IR/DebugProgramInstruction.cpp b/llvm/lib/IR/DebugProgramInstruction.cpp index 389bac4de6a1c..3a8b94a87bbcf 100644 --- a/llvm/lib/IR/DebugProgramInstruction.cpp +++ b/llvm/lib/IR/DebugProgramInstruction.cpp @@ -175,6 +175,8 @@ DPValue *DPValue::createLinkedDPVAssign(Instruction *LinkedInstr, Value *Val, return NewDPVAssign; } +void DPValue::setVariable(DILocalVariable *NewVar) { Variable.reset(NewVar); } + iterator_range DPValue::location_ops() const { auto *MD = getRawLocation(); // If a Value has been deleted, the "location" for this DPValue will be @@ -313,6 +315,10 @@ bool DPValue::isKillLocation() const { any_of(location_ops(), [](Value *V) { return isa(V); }); } +DILocalVariable *DPValue::getVariable() const { + return cast(Variable.get()); +} + std::optional DPValue::getFragmentSizeInBits() const { if (auto Fragment = getExpression()->getFragmentInfo()) return Fragment->SizeInBits; diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp index 4f321bc516cc3..3741e5deaa4cd 100644 --- a/llvm/lib/IR/Verifier.cpp +++ b/llvm/lib/IR/Verifier.cpp @@ -173,11 +173,36 @@ struct VerifierSupport { } } + void Write(const DbgRecord *DR) { + if (DR) + DR->print(*OS, MST, false); + } + void Write(const DPValue *V) { if (V) V->print(*OS, MST, false); } + void Write(DPValue::LocationType Type) { + switch (Type) { + case DPValue::LocationType::Value: + *OS << "value"; + break; + case DPValue::LocationType::Declare: + *OS << "declare"; + break; + case DPValue::LocationType::Assign: + *OS << "assign"; + break; + case DPValue::LocationType::End: + *OS << "end"; + break; + case DPValue::LocationType::Any: + *OS << "any"; + break; + }; + } + void Write(const Metadata *MD) { if (!MD) return; @@ -522,8 +547,10 @@ class Verifier : public InstVisitor, VerifierSupport { void visitTemplateParams(const MDNode &N, const Metadata &RawParams); + void visit(DPValue &DPV); // InstVisitor overrides... using InstVisitor::visit; + void visitDbgRecords(Instruction &I); void visit(Instruction &I); void visitTruncInst(TruncInst &I); @@ -649,7 +676,22 @@ class Verifier : public InstVisitor, VerifierSupport { } \ } while (false) +void Verifier::visitDbgRecords(Instruction &I) { + if (!I.DbgMarker) + return; + CheckDI(I.DbgMarker->MarkedInstr == &I, "Instruction has invalid DbgMarker", + &I); + CheckDI(!isa(&I) || !I.hasDbgValues(), + "PHI Node must not have any attached DbgRecords", &I); + for (DPValue &DPV : DPValue::filter(I.getDbgValueRange())) { + CheckDI(DPV.getMarker() == I.DbgMarker, "DbgRecord had invalid DbgMarker", + &I, &DPV); + visit(DPV); + } +} + void Verifier::visit(Instruction &I) { + visitDbgRecords(I); for (unsigned i = 0, e = I.getNumOperands(); i != e; ++i) Check(I.getOperand(i) != nullptr, "Operand is null", &I); InstVisitor::visit(I); @@ -2976,12 +3018,9 @@ void Verifier::visitBasicBlock(BasicBlock &BB) { } // Confirm that no issues arise from the debug program. - if (BB.IsNewDbgInfoFormat) { - // Configure the validate function to not fire assertions, instead print - // errors and return true if there's a problem. - bool RetVal = BB.validateDbgValues(false, true, OS); - Check(!RetVal, "Invalid configuration of new-debug-info data found"); - } + if (BB.IsNewDbgInfoFormat) + CheckDI(!BB.getTrailingDPValues(), "Basic Block has trailing DbgRecords!", + &BB); } void Verifier::visitTerminator(Instruction &I) { @@ -6148,6 +6187,70 @@ static DISubprogram *getSubprogram(Metadata *LocalScope) { return nullptr; } +void Verifier::visit(DPValue &DPV) { + CheckDI(DPV.getType() == DPValue::LocationType::Value || + DPV.getType() == DPValue::LocationType::Declare || + DPV.getType() == DPValue::LocationType::Assign, + "invalid #dbg record type", &DPV, DPV.getType()); + // The location for a DPValue must be either a ValueAsMetadata, DIArgList, or + // an empty MDNode (which is a legacy representation for an "undef" location). + auto *MD = DPV.getRawLocation(); + CheckDI(isa(MD) || isa(MD) || + (isa(MD) && !cast(MD)->getNumOperands()), + "invalid #dbg record address/value", &DPV, MD); + CheckDI(isa(DPV.getRawVariable()), + "invalid #dbg record variable", &DPV, DPV.getRawVariable()); + CheckDI(DPV.getExpression(), "missing #dbg record expression", &DPV, + DPV.getExpression()); + + if (DPV.isDbgAssign()) { + CheckDI(isa(DPV.getRawAssignID()), + "invalid #dbg_assign DIAssignID", &DPV, DPV.getRawAssignID()); + const auto *RawAddr = DPV.getRawAddress(); + // Similarly to the location above, the address for an assign DPValue must + // be a ValueAsMetadata or an empty MDNode, which represents an undef + // address. + CheckDI( + isa(RawAddr) || + (isa(RawAddr) && !cast(RawAddr)->getNumOperands()), + "invalid #dbg_assign address", &DPV, DPV.getRawAddress()); + CheckDI(DPV.getAddressExpression(), + "missing #dbg_assign address expression", &DPV, + DPV.getAddressExpression()); + // All of the linked instructions should be in the same function as DPV. + for (Instruction *I : at::getAssignmentInsts(&DPV)) + CheckDI(DPV.getFunction() == I->getFunction(), + "inst not in same function as #dbg_assign", I, &DPV); + } + + if (MDNode *N = DPV.getDebugLoc().getAsMDNode()) { + CheckDI(isa(N), "invalid #dbg record location", &DPV, N); + visitDILocation(*cast(N)); + } + + BasicBlock *BB = DPV.getParent(); + Function *F = BB ? BB->getParent() : nullptr; + + // The scopes for variables and !dbg attachments must agree. + DILocalVariable *Var = DPV.getVariable(); + DILocation *Loc = DPV.getDebugLoc(); + CheckDI(Loc, "missing #dbg record DILocation", &DPV, BB, F); + + DISubprogram *VarSP = getSubprogram(Var->getRawScope()); + DISubprogram *LocSP = getSubprogram(Loc->getRawScope()); + if (!VarSP || !LocSP) + return; // Broken scope chains are checked elsewhere. + + CheckDI(VarSP == LocSP, + "mismatched subprogram between #dbg record variable and DILocation", + &DPV, BB, F, Var, Var->getScope()->getSubprogram(), Loc, + Loc->getScope()->getSubprogram()); + + // This check is redundant with one in visitLocalVariable(). + CheckDI(isType(Var->getRawType()), "invalid type ref", Var, + Var->getRawType()); +} + void Verifier::visitVPIntrinsic(VPIntrinsic &VPI) { if (auto *VPCast = dyn_cast(&VPI)) { auto *RetTy = cast(VPCast->getType()); diff --git a/llvm/unittests/IR/DebugInfoTest.cpp b/llvm/unittests/IR/DebugInfoTest.cpp index 65db93e537f7d..c99f928de8a9d 100644 --- a/llvm/unittests/IR/DebugInfoTest.cpp +++ b/llvm/unittests/IR/DebugInfoTest.cpp @@ -1082,7 +1082,10 @@ TEST(MetadataTest, DPValueConversionRoutines) { EXPECT_FALSE(BB1->IsNewDbgInfoFormat); // Validating the block for DPValues / DPMarkers shouldn't fail -- there's // no data stored right now. - EXPECT_FALSE(BB1->validateDbgValues(false, false)); + bool BrokenDebugInfo = false; + bool Error = verifyModule(*M, &errs(), &BrokenDebugInfo); + EXPECT_FALSE(Error); + EXPECT_FALSE(BrokenDebugInfo); // Function and module should be marked as not having the new format too. EXPECT_FALSE(F->IsNewDbgInfoFormat); @@ -1135,13 +1138,17 @@ TEST(MetadataTest, DPValueConversionRoutines) { EXPECT_TRUE(!Inst.DbgMarker || Inst.DbgMarker->StoredDPValues.empty()); // Validating the first block should continue to not be a problem, - EXPECT_FALSE(BB1->validateDbgValues(false, false)); + Error = verifyModule(*M, &errs(), &BrokenDebugInfo); + EXPECT_FALSE(Error); + EXPECT_FALSE(BrokenDebugInfo); // But if we were to break something, it should be able to fire. Don't attempt // to comprehensively test the validator, it's a smoke-test rather than a // "proper" verification pass. DPV1->setMarker(nullptr); // A marker pointing the wrong way should be an error. - EXPECT_TRUE(BB1->validateDbgValues(false, false)); + Error = verifyModule(*M, &errs(), &BrokenDebugInfo); + EXPECT_FALSE(Error); + EXPECT_TRUE(BrokenDebugInfo); DPV1->setMarker(FirstInst->DbgMarker); DILocalVariable *DLV1 = DPV1->getVariable();