Skip to content

Commit ed35ad1

Browse files
SLTozerSLTozer
authored
[RemoveDIs][DebugInfo] Add DPValue checks to the verifier, prepare DPValue for parsing support (#79810)
As part of the RemoveDIs project, this patch adds support for checking DPValues in the verifier. Although this is not strictly parsing-related, and we currently automatically convert back to the old debug info format immediately after parsing, we are approaching the point where the we can operate end-to-end in the new debug info format, at which point it is appropriate that we can actually validate modules in the new format. This patch also contains some changes that aren't strictly parsing-related, but are necessary class refactors for parsing support, and are used in the verifier checks (i.e. changing the DILocalVariable field to be a tracking MD reference, and adding a Verifier check to confirm that it is a DILocalVariable).
1 parent 183b6b5 commit ed35ad1

File tree

7 files changed

+132
-85
lines changed

7 files changed

+132
-85
lines changed

‎llvm/include/llvm/IR/BasicBlock.h

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -93,15 +93,6 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
9393
/// if necessary.
9494
void setIsNewDbgInfoFormat(bool NewFlag);
9595

96-
/// Validate any DPMarkers / DPValues attached to instructions in this block,
97-
/// and block-level stored data too (TrailingDPValues).
98-
/// \p Assert Should this method fire an assertion if a problem is found?
99-
/// \p Msg Should this method print a message to errs() if a problem is found?
100-
/// \p OS Output stream to write errors to.
101-
/// \returns True if a problem is found.
102-
bool validateDbgValues(bool Assert = true, bool Msg = false,
103-
raw_ostream *OS = nullptr);
104-
10596
/// Record that the collection of DPValues in \p M "trails" after the last
10697
/// instruction of this block. These are equivalent to dbg.value intrinsics
10798
/// that exist at the end of a basic block with no terminator (a transient

‎llvm/include/llvm/IR/DebugProgramInstruction.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ class DPValue : public DbgRecord, protected DebugValueUser {
224224
// DebugValueUser superclass instead. The referred to Value can either be a
225225
// ValueAsMetadata or a DIArgList.
226226

227-
DILocalVariable *Variable;
227+
TrackingMDNodeRef Variable;
228228
DIExpression *Expression;
229229
DIExpression *AddressExpression;
230230

@@ -331,7 +331,7 @@ class DPValue : public DbgRecord, protected DebugValueUser {
331331
void addVariableLocationOps(ArrayRef<Value *> NewValues,
332332
DIExpression *NewExpr);
333333

334-
void setVariable(DILocalVariable *NewVar) { Variable = NewVar; }
334+
void setVariable(DILocalVariable *NewVar);
335335

336336
void setExpression(DIExpression *NewExpr) { Expression = NewExpr; }
337337

@@ -349,7 +349,8 @@ class DPValue : public DbgRecord, protected DebugValueUser {
349349
void setKillLocation();
350350
bool isKillLocation() const;
351351

352-
DILocalVariable *getVariable() const { return Variable; }
352+
DILocalVariable *getVariable() const;
353+
MDNode *getRawVariable() const { return Variable; }
353354

354355
DIExpression *getExpression() const { return Expression; }
355356

‎llvm/lib/IR/AsmWriter.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1143,15 +1143,15 @@ void SlotTracker::processDbgRecordMetadata(const DbgRecord &DR) {
11431143
// Process metadata used by DbgRecords; we only specifically care about the
11441144
// DILocalVariable, DILocation, and DIAssignID fields, as the Value and
11451145
// Expression fields should only be printed inline and so do not use a slot.
1146-
CreateMetadataSlot(DPV->getVariable());
1146+
CreateMetadataSlot(DPV->getRawVariable());
11471147
if (DPV->isDbgAssign())
1148-
CreateMetadataSlot(DPV->getAssignID());
1148+
CreateMetadataSlot(cast<MDNode>(DPV->getRawAssignID()));
11491149
} else if (const DPLabel *DPL = dyn_cast<const DPLabel>(&DR)) {
11501150
CreateMetadataSlot(DPL->getLabel());
11511151
} else {
11521152
llvm_unreachable("unsupported DbgRecord kind");
11531153
}
1154-
CreateMetadataSlot(DR.getDebugLoc());
1154+
CreateMetadataSlot(DR.getDebugLoc().getAsMDNode());
11551155
}
11561156

11571157
void SlotTracker::processInstructionMetadata(const Instruction &I) {

‎llvm/lib/IR/BasicBlock.cpp

Lines changed: 0 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -122,67 +122,6 @@ void BasicBlock::convertFromNewDbgValues() {
122122
assert(!getTrailingDPValues());
123123
}
124124

125-
bool BasicBlock::validateDbgValues(bool Assert, bool Msg, raw_ostream *OS) {
126-
bool RetVal = false;
127-
if (!OS)
128-
OS = &errs();
129-
130-
// Helper lambda for reporting failures: via assertion, printing, and return
131-
// value.
132-
auto TestFailure = [Assert, Msg, &RetVal, OS](bool Val, const char *Text) {
133-
// Did the test fail?
134-
if (Val)
135-
return;
136-
137-
// If we're asserting, then fire off an assertion.
138-
if (Assert)
139-
llvm_unreachable(Text);
140-
141-
if (Msg)
142-
*OS << Text << "\n";
143-
RetVal = true;
144-
};
145-
146-
// We should have the same debug-format as the parent function.
147-
TestFailure(getParent()->IsNewDbgInfoFormat == IsNewDbgInfoFormat,
148-
"Parent function doesn't have the same debug-info format");
149-
150-
// Only validate if we are using the new format.
151-
if (!IsNewDbgInfoFormat)
152-
return RetVal;
153-
154-
// Match every DPMarker to every Instruction and vice versa, and
155-
// verify that there are no invalid DPValues.
156-
for (auto It = begin(); It != end(); ++It) {
157-
if (!It->DbgMarker)
158-
continue;
159-
160-
// Validate DebugProgramMarkers.
161-
DPMarker *CurrentDebugMarker = It->DbgMarker;
162-
163-
// If this is a marker, it should match the instruction and vice versa.
164-
TestFailure(CurrentDebugMarker->MarkedInstr == &*It,
165-
"Debug Marker points to incorrect instruction?");
166-
167-
// Now validate any DPValues in the marker.
168-
for (DbgRecord &DPR : CurrentDebugMarker->getDbgValueRange()) {
169-
// Validate DebugProgramValues.
170-
TestFailure(DPR.getMarker() == CurrentDebugMarker,
171-
"Not pointing at correct next marker!");
172-
173-
// Verify that no DbgValues appear prior to PHIs.
174-
TestFailure(
175-
!isa<PHINode>(It),
176-
"DebugProgramValues must not appear before PHI nodes in a block!");
177-
}
178-
}
179-
180-
// Except transiently when removing + re-inserting the block terminator, there
181-
// should be no trailing DPValues.
182-
TestFailure(!getTrailingDPValues(), "Trailing DPValues in block");
183-
return RetVal;
184-
}
185-
186125
#ifndef NDEBUG
187126
void BasicBlock::dumpDbgValues() const {
188127
for (auto &Inst : *this) {

‎llvm/lib/IR/DebugProgramInstruction.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,8 @@ DPValue *DPValue::createLinkedDPVAssign(Instruction *LinkedInstr, Value *Val,
175175
return NewDPVAssign;
176176
}
177177

178+
void DPValue::setVariable(DILocalVariable *NewVar) { Variable.reset(NewVar); }
179+
178180
iterator_range<DPValue::location_op_iterator> DPValue::location_ops() const {
179181
auto *MD = getRawLocation();
180182
// If a Value has been deleted, the "location" for this DPValue will be
@@ -313,6 +315,10 @@ bool DPValue::isKillLocation() const {
313315
any_of(location_ops(), [](Value *V) { return isa<UndefValue>(V); });
314316
}
315317

318+
DILocalVariable *DPValue::getVariable() const {
319+
return cast<DILocalVariable>(Variable.get());
320+
}
321+
316322
std::optional<uint64_t> DPValue::getFragmentSizeInBits() const {
317323
if (auto Fragment = getExpression()->getFragmentInfo())
318324
return Fragment->SizeInBits;

‎llvm/lib/IR/Verifier.cpp

Lines changed: 109 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -173,11 +173,36 @@ struct VerifierSupport {
173173
}
174174
}
175175

176+
void Write(const DbgRecord *DR) {
177+
if (DR)
178+
DR->print(*OS, MST, false);
179+
}
180+
176181
void Write(const DPValue *V) {
177182
if (V)
178183
V->print(*OS, MST, false);
179184
}
180185

186+
void Write(DPValue::LocationType Type) {
187+
switch (Type) {
188+
case DPValue::LocationType::Value:
189+
*OS << "value";
190+
break;
191+
case DPValue::LocationType::Declare:
192+
*OS << "declare";
193+
break;
194+
case DPValue::LocationType::Assign:
195+
*OS << "assign";
196+
break;
197+
case DPValue::LocationType::End:
198+
*OS << "end";
199+
break;
200+
case DPValue::LocationType::Any:
201+
*OS << "any";
202+
break;
203+
};
204+
}
205+
181206
void Write(const Metadata *MD) {
182207
if (!MD)
183208
return;
@@ -522,8 +547,10 @@ class Verifier : public InstVisitor<Verifier>, VerifierSupport {
522547

523548
void visitTemplateParams(const MDNode &N, const Metadata &RawParams);
524549

550+
void visit(DPValue &DPV);
525551
// InstVisitor overrides...
526552
using InstVisitor<Verifier>::visit;
553+
void visitDbgRecords(Instruction &I);
527554
void visit(Instruction &I);
528555

529556
void visitTruncInst(TruncInst &I);
@@ -649,7 +676,22 @@ class Verifier : public InstVisitor<Verifier>, VerifierSupport {
649676
} \
650677
} while (false)
651678

679+
void Verifier::visitDbgRecords(Instruction &I) {
680+
if (!I.DbgMarker)
681+
return;
682+
CheckDI(I.DbgMarker->MarkedInstr == &I, "Instruction has invalid DbgMarker",
683+
&I);
684+
CheckDI(!isa<PHINode>(&I) || !I.hasDbgValues(),
685+
"PHI Node must not have any attached DbgRecords", &I);
686+
for (DPValue &DPV : DPValue::filter(I.getDbgValueRange())) {
687+
CheckDI(DPV.getMarker() == I.DbgMarker, "DbgRecord had invalid DbgMarker",
688+
&I, &DPV);
689+
visit(DPV);
690+
}
691+
}
692+
652693
void Verifier::visit(Instruction &I) {
694+
visitDbgRecords(I);
653695
for (unsigned i = 0, e = I.getNumOperands(); i != e; ++i)
654696
Check(I.getOperand(i) != nullptr, "Operand is null", &I);
655697
InstVisitor<Verifier>::visit(I);
@@ -2976,12 +3018,9 @@ void Verifier::visitBasicBlock(BasicBlock &BB) {
29763018
}
29773019

29783020
// Confirm that no issues arise from the debug program.
2979-
if (BB.IsNewDbgInfoFormat) {
2980-
// Configure the validate function to not fire assertions, instead print
2981-
// errors and return true if there's a problem.
2982-
bool RetVal = BB.validateDbgValues(false, true, OS);
2983-
Check(!RetVal, "Invalid configuration of new-debug-info data found");
2984-
}
3021+
if (BB.IsNewDbgInfoFormat)
3022+
CheckDI(!BB.getTrailingDPValues(), "Basic Block has trailing DbgRecords!",
3023+
&BB);
29853024
}
29863025

29873026
void Verifier::visitTerminator(Instruction &I) {
@@ -6148,6 +6187,70 @@ static DISubprogram *getSubprogram(Metadata *LocalScope) {
61486187
return nullptr;
61496188
}
61506189

6190+
void Verifier::visit(DPValue &DPV) {
6191+
CheckDI(DPV.getType() == DPValue::LocationType::Value ||
6192+
DPV.getType() == DPValue::LocationType::Declare ||
6193+
DPV.getType() == DPValue::LocationType::Assign,
6194+
"invalid #dbg record type", &DPV, DPV.getType());
6195+
// The location for a DPValue must be either a ValueAsMetadata, DIArgList, or
6196+
// an empty MDNode (which is a legacy representation for an "undef" location).
6197+
auto *MD = DPV.getRawLocation();
6198+
CheckDI(isa<ValueAsMetadata>(MD) || isa<DIArgList>(MD) ||
6199+
(isa<MDNode>(MD) && !cast<MDNode>(MD)->getNumOperands()),
6200+
"invalid #dbg record address/value", &DPV, MD);
6201+
CheckDI(isa<DILocalVariable>(DPV.getRawVariable()),
6202+
"invalid #dbg record variable", &DPV, DPV.getRawVariable());
6203+
CheckDI(DPV.getExpression(), "missing #dbg record expression", &DPV,
6204+
DPV.getExpression());
6205+
6206+
if (DPV.isDbgAssign()) {
6207+
CheckDI(isa<DIAssignID>(DPV.getRawAssignID()),
6208+
"invalid #dbg_assign DIAssignID", &DPV, DPV.getRawAssignID());
6209+
const auto *RawAddr = DPV.getRawAddress();
6210+
// Similarly to the location above, the address for an assign DPValue must
6211+
// be a ValueAsMetadata or an empty MDNode, which represents an undef
6212+
// address.
6213+
CheckDI(
6214+
isa<ValueAsMetadata>(RawAddr) ||
6215+
(isa<MDNode>(RawAddr) && !cast<MDNode>(RawAddr)->getNumOperands()),
6216+
"invalid #dbg_assign address", &DPV, DPV.getRawAddress());
6217+
CheckDI(DPV.getAddressExpression(),
6218+
"missing #dbg_assign address expression", &DPV,
6219+
DPV.getAddressExpression());
6220+
// All of the linked instructions should be in the same function as DPV.
6221+
for (Instruction *I : at::getAssignmentInsts(&DPV))
6222+
CheckDI(DPV.getFunction() == I->getFunction(),
6223+
"inst not in same function as #dbg_assign", I, &DPV);
6224+
}
6225+
6226+
if (MDNode *N = DPV.getDebugLoc().getAsMDNode()) {
6227+
CheckDI(isa<DILocation>(N), "invalid #dbg record location", &DPV, N);
6228+
visitDILocation(*cast<DILocation>(N));
6229+
}
6230+
6231+
BasicBlock *BB = DPV.getParent();
6232+
Function *F = BB ? BB->getParent() : nullptr;
6233+
6234+
// The scopes for variables and !dbg attachments must agree.
6235+
DILocalVariable *Var = DPV.getVariable();
6236+
DILocation *Loc = DPV.getDebugLoc();
6237+
CheckDI(Loc, "missing #dbg record DILocation", &DPV, BB, F);
6238+
6239+
DISubprogram *VarSP = getSubprogram(Var->getRawScope());
6240+
DISubprogram *LocSP = getSubprogram(Loc->getRawScope());
6241+
if (!VarSP || !LocSP)
6242+
return; // Broken scope chains are checked elsewhere.
6243+
6244+
CheckDI(VarSP == LocSP,
6245+
"mismatched subprogram between #dbg record variable and DILocation",
6246+
&DPV, BB, F, Var, Var->getScope()->getSubprogram(), Loc,
6247+
Loc->getScope()->getSubprogram());
6248+
6249+
// This check is redundant with one in visitLocalVariable().
6250+
CheckDI(isType(Var->getRawType()), "invalid type ref", Var,
6251+
Var->getRawType());
6252+
}
6253+
61516254
void Verifier::visitVPIntrinsic(VPIntrinsic &VPI) {
61526255
if (auto *VPCast = dyn_cast<VPCastIntrinsic>(&VPI)) {
61536256
auto *RetTy = cast<VectorType>(VPCast->getType());

‎llvm/unittests/IR/DebugInfoTest.cpp

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1082,7 +1082,10 @@ TEST(MetadataTest, DPValueConversionRoutines) {
10821082
EXPECT_FALSE(BB1->IsNewDbgInfoFormat);
10831083
// Validating the block for DPValues / DPMarkers shouldn't fail -- there's
10841084
// no data stored right now.
1085-
EXPECT_FALSE(BB1->validateDbgValues(false, false));
1085+
bool BrokenDebugInfo = false;
1086+
bool Error = verifyModule(*M, &errs(), &BrokenDebugInfo);
1087+
EXPECT_FALSE(Error);
1088+
EXPECT_FALSE(BrokenDebugInfo);
10861089

10871090
// Function and module should be marked as not having the new format too.
10881091
EXPECT_FALSE(F->IsNewDbgInfoFormat);
@@ -1135,13 +1138,17 @@ TEST(MetadataTest, DPValueConversionRoutines) {
11351138
EXPECT_TRUE(!Inst.DbgMarker || Inst.DbgMarker->StoredDPValues.empty());
11361139

11371140
// Validating the first block should continue to not be a problem,
1138-
EXPECT_FALSE(BB1->validateDbgValues(false, false));
1141+
Error = verifyModule(*M, &errs(), &BrokenDebugInfo);
1142+
EXPECT_FALSE(Error);
1143+
EXPECT_FALSE(BrokenDebugInfo);
11391144
// But if we were to break something, it should be able to fire. Don't attempt
11401145
// to comprehensively test the validator, it's a smoke-test rather than a
11411146
// "proper" verification pass.
11421147
DPV1->setMarker(nullptr);
11431148
// A marker pointing the wrong way should be an error.
1144-
EXPECT_TRUE(BB1->validateDbgValues(false, false));
1149+
Error = verifyModule(*M, &errs(), &BrokenDebugInfo);
1150+
EXPECT_FALSE(Error);
1151+
EXPECT_TRUE(BrokenDebugInfo);
11451152
DPV1->setMarker(FirstInst->DbgMarker);
11461153

11471154
DILocalVariable *DLV1 = DPV1->getVariable();

0 commit comments

Comments
 (0)