From: Vedant Kumar Date: Fri, 1 May 2020 04:38:11 +0000 (-0700) Subject: [Verifier] Constrain where DILocations may be nested X-Git-Tag: llvmorg-12-init~7017 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=8dfe819bcd231486db3058e2538fff0f569e3c1a;p=platform%2Fupstream%2Fllvm.git [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 --- diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp index df6b2bd..b1f0607 100644 --- a/llvm/lib/IR/Verifier.cpp +++ b/llvm/lib/IR/Verifier.cpp @@ -397,6 +397,9 @@ public: } 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 @@ private: void visitAliaseeSubExpr(SmallPtrSetImpl &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(Op), "Invalid operand for global metadata!", &MD, Op); + AssertDI(!isa(Op) || AllowLocs == AreDebugLocsAllowed::Yes, + "DILocation not allowed within this metadata node", &MD, Op); if (auto *N = dyn_cast(Op)) { - visitMDNode(*N); + visitMDNode(*N, AllowLocs); continue; } if (auto *V = dyn_cast(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(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(N), "invalid !dbg metadata attachment", &I, N); - visitMDNode(*N); + visitMDNode(*N, AreDebugLocsAllowed::Yes); } if (auto *DII = dyn_cast(&I)) { @@ -4315,6 +4322,17 @@ void Verifier::visitInstruction(Instruction &I) { verifyNotEntryValue(*DII); } + SmallVector, 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 0000000..a63af77 --- /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}