[llvm] 8dfe819 - [Verifier] Constrain where DILocations may be nested
Vedant Kumar via llvm-commits
llvm-commits at lists.llvm.org
Mon May 4 14:02:55 PDT 2020
Author: Vedant Kumar
Date: 2020-05-04T14:02:43-07:00
New Revision: 8dfe819bcd231486db3058e2538fff0f569e3c1a
URL: https://github.com/llvm/llvm-project/commit/8dfe819bcd231486db3058e2538fff0f569e3c1a
DIFF: https://github.com/llvm/llvm-project/commit/8dfe819bcd231486db3058e2538fff0f569e3c1a.diff
LOG: [Verifier] Constrain where DILocations may be nested
Summary:
Constrain which metadata nodes are allowed to be, or contain,
DILocations. This ensures that logic for updating DILocations in a
Module is complete.
Currently, !llvm.loop metadata is the only odd duck which contains
nested DILocations. This has caused problems in the past: some passes
forgot to visit the nested locations, leading to subtly broken debug
info and late verification failures.
If there's a compelling reason for some future metadata to nest
DILocations, we'll need to introduce a generic API for updating the
locations attached to an Instruction before relaxing this check.
Reviewers: aprantl, dsanders
Subscribers: hiraditya, llvm-commits
Tags: #llvm
Differential Revision: https://reviews.llvm.org/D79245
Added:
llvm/test/Verifier/dilocation-in-wrong-place.ll
Modified:
llvm/lib/IR/Verifier.cpp
Removed:
################################################################################
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index df6b2bdc66c4..b1f060737c6f 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -397,6 +397,9 @@ class Verifier : public InstVisitor<Verifier>, VerifierSupport {
}
private:
+ /// Whether a metadata node is allowed to be, or contain, a DILocation.
+ enum class AreDebugLocsAllowed { No, Yes };
+
// Verification methods...
void visitGlobalValue(const GlobalValue &GV);
void visitGlobalVariable(const GlobalVariable &GV);
@@ -405,7 +408,7 @@ class Verifier : public InstVisitor<Verifier>, VerifierSupport {
void visitAliaseeSubExpr(SmallPtrSetImpl<const GlobalAlias *> &Visited,
const GlobalAlias &A, const Constant &C);
void visitNamedMDNode(const NamedMDNode &NMD);
- void visitMDNode(const MDNode &MD);
+ void visitMDNode(const MDNode &MD, AreDebugLocsAllowed AllowLocs);
void visitMetadataAsValue(const MetadataAsValue &MD, Function *F);
void visitValueAsMetadata(const ValueAsMetadata &MD, Function *F);
void visitComdat(const Comdat &C);
@@ -780,11 +783,11 @@ void Verifier::visitNamedMDNode(const NamedMDNode &NMD) {
if (!MD)
continue;
- visitMDNode(*MD);
+ visitMDNode(*MD, AreDebugLocsAllowed::Yes);
}
}
-void Verifier::visitMDNode(const MDNode &MD) {
+void Verifier::visitMDNode(const MDNode &MD, AreDebugLocsAllowed AllowLocs) {
// Only visit each node once. Metadata can be mutually recursive, so this
// avoids infinite recursion here, as well as being an optimization.
if (!MDNodes.insert(&MD).second)
@@ -807,8 +810,10 @@ void Verifier::visitMDNode(const MDNode &MD) {
continue;
Assert(!isa<LocalAsMetadata>(Op), "Invalid operand for global metadata!",
&MD, Op);
+ AssertDI(!isa<DILocation>(Op) || AllowLocs == AreDebugLocsAllowed::Yes,
+ "DILocation not allowed within this metadata node", &MD, Op);
if (auto *N = dyn_cast<MDNode>(Op)) {
- visitMDNode(*N);
+ visitMDNode(*N, AllowLocs);
continue;
}
if (auto *V = dyn_cast<ValueAsMetadata>(Op)) {
@@ -851,7 +856,7 @@ void Verifier::visitValueAsMetadata(const ValueAsMetadata &MD, Function *F) {
void Verifier::visitMetadataAsValue(const MetadataAsValue &MDV, Function *F) {
Metadata *MD = MDV.getMetadata();
if (auto *N = dyn_cast<MDNode>(MD)) {
- visitMDNode(*N);
+ visitMDNode(*N, AreDebugLocsAllowed::No);
return;
}
@@ -2313,7 +2318,7 @@ void Verifier::visitFunction(const Function &F) {
"function declaration may not have a !prof attachment", &F);
// Verify the metadata itself.
- visitMDNode(*I.second);
+ visitMDNode(*I.second, AreDebugLocsAllowed::Yes);
}
Assert(!F.hasPersonalityFn(),
"Function declaration shouldn't have a personality routine", &F);
@@ -2337,6 +2342,7 @@ void Verifier::visitFunction(const Function &F) {
// Visit metadata attachments.
for (const auto &I : MDs) {
// Verify that the attachment is legal.
+ auto AllowLocs = AreDebugLocsAllowed::No;
switch (I.first) {
default:
break;
@@ -2351,6 +2357,7 @@ void Verifier::visitFunction(const Function &F) {
AssertDI(!AttachedTo || AttachedTo == &F,
"DISubprogram attached to more than one function", SP, &F);
AttachedTo = &F;
+ AllowLocs = AreDebugLocsAllowed::Yes;
break;
}
case LLVMContext::MD_prof:
@@ -2361,7 +2368,7 @@ void Verifier::visitFunction(const Function &F) {
}
// Verify the metadata itself.
- visitMDNode(*I.second);
+ visitMDNode(*I.second, AllowLocs);
}
}
@@ -4307,7 +4314,7 @@ void Verifier::visitInstruction(Instruction &I) {
if (MDNode *N = I.getDebugLoc().getAsMDNode()) {
AssertDI(isa<DILocation>(N), "invalid !dbg metadata attachment", &I, N);
- visitMDNode(*N);
+ visitMDNode(*N, AreDebugLocsAllowed::Yes);
}
if (auto *DII = dyn_cast<DbgVariableIntrinsic>(&I)) {
@@ -4315,6 +4322,17 @@ void Verifier::visitInstruction(Instruction &I) {
verifyNotEntryValue(*DII);
}
+ SmallVector<std::pair<unsigned, MDNode *>, 4> MDs;
+ I.getAllMetadata(MDs);
+ for (auto Attachment : MDs) {
+ unsigned Kind = Attachment.first;
+ auto AllowLocs =
+ (Kind == LLVMContext::MD_dbg || Kind == LLVMContext::MD_loop)
+ ? AreDebugLocsAllowed::Yes
+ : AreDebugLocsAllowed::No;
+ visitMDNode(*Attachment.second, AllowLocs);
+ }
+
InstsInThisBlock.insert(&I);
}
diff --git a/llvm/test/Verifier/dilocation-in-wrong-place.ll b/llvm/test/Verifier/dilocation-in-wrong-place.ll
new file mode 100644
index 000000000000..a63af77227e4
--- /dev/null
+++ b/llvm/test/Verifier/dilocation-in-wrong-place.ll
@@ -0,0 +1,26 @@
+; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
+
+; CHECK: DILocation not allowed within this metadata node
+; CHECK-NEXT: [[unknownMD:![0-9]+]] = distinct !{[[unknownMD]], [[dbgMD:![0-9]+]]}
+; CHECK-NEXT: [[dbgMD]] = !DILocation
+
+define void @f() !dbg !5 {
+ ret void, !dbg !10, !unknown_md !11
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.debugify = !{!3, !3}
+!llvm.module.flags = !{!4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "loop.ll", directory: "/")
+!2 = !{}
+!3 = !{i32 1}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = distinct !DISubprogram(name: "f", linkageName: "f", scope: null, file: !1, line: 1, type: !6, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !7)
+!6 = !DISubroutineType(types: !2)
+!7 = !{!8}
+!8 = !DILocalVariable(name: "1", scope: !5, file: !1, line: 1, type: !9)
+!9 = !DIBasicType(name: "ty32", size: 32, encoding: DW_ATE_unsigned)
+!10 = !DILocation(line: 1, column: 1, scope: !5)
+!11 = !{!11, !10}
More information about the llvm-commits
mailing list