From c4eafd24f28b676ed7f605cda1fe42da6d34b81f Mon Sep 17 00:00:00 2001 From: "Duncan P. N. Exon Smith" Date: Thu, 26 Mar 2015 22:05:04 +0000 Subject: [PATCH] Verifier: Check accessors of MDLocation Check accessors of `MDLocation`, and change them to `cast<>` down to the right types. Also add type-safe factory functions. All the callers that handle broken code need to use the new versions of the accessors (`getRawScope()` instead of `getScope()`) that still return `Metadata*`. This is also necessary for things like `MDNodeKeyImpl` (in LLVMContextImpl.h) that need to unique the nodes when their operands might still be forward references of the wrong type. In the `Value` hierarchy, consumers that handle broken code use `getOperand()` directly. However, debug info nodes have a ton of operands, and their order (even their existence) isn't stable yet. It's safer and more maintainable to add an explicit "raw" accessor on the class itself. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@233322 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/IR/DebugInfoMetadata.h | 22 ++++++++++++++++++++-- lib/AsmParser/LLParser.cpp | 4 ++-- lib/Bitcode/Reader/BitcodeReader.cpp | 7 ++++--- lib/IR/AsmWriter.cpp | 7 +++---- lib/IR/LLVMContextImpl.h | 6 +++--- lib/IR/Verifier.cpp | 5 +++-- test/Assembler/mdlocation.ll | 4 ++-- test/Assembler/metadata.ll | 2 +- test/Linker/Inputs/mdlocation.ll | 4 ++-- test/Linker/mdlocation.ll | 10 +++++----- unittests/IR/MetadataTest.cpp | 10 ++++++++-- 11 files changed, 53 insertions(+), 28 deletions(-) diff --git a/include/llvm/IR/DebugInfoMetadata.h b/include/llvm/IR/DebugInfoMetadata.h index 5d8df102c19..d7563fce940 100644 --- a/include/llvm/IR/DebugInfoMetadata.h +++ b/include/llvm/IR/DebugInfoMetadata.h @@ -882,6 +882,13 @@ class MDLocation : public MDNode { unsigned Column, Metadata *Scope, Metadata *InlinedAt, StorageType Storage, bool ShouldCreate = true); + static MDLocation *getImpl(LLVMContext &Context, unsigned Line, + unsigned Column, MDLocalScope *Scope, + MDLocation *InlinedAt, StorageType Storage, + bool ShouldCreate = true) { + return getImpl(Context, Line, Column, static_cast(Scope), + static_cast(InlinedAt), Storage, ShouldCreate); + } TempMDLocation cloneImpl() const { return getTemporary(getContext(), getLine(), getColumn(), getScope(), @@ -896,14 +903,25 @@ public: (unsigned Line, unsigned Column, Metadata *Scope, Metadata *InlinedAt = nullptr), (Line, Column, Scope, InlinedAt)) + DEFINE_MDNODE_GET(MDLocation, + (unsigned Line, unsigned Column, MDLocalScope *Scope, + MDLocation *InlinedAt = nullptr), + (Line, Column, Scope, InlinedAt)) /// \brief Return a (temporary) clone of this. TempMDLocation clone() const { return cloneImpl(); } unsigned getLine() const { return SubclassData32; } unsigned getColumn() const { return SubclassData16; } - Metadata *getScope() const { return getOperand(0); } - Metadata *getInlinedAt() const { + MDLocalScope *getScope() const { + return cast_or_null(getRawScope()); + } + MDLocation *getInlinedAt() const { + return cast_or_null(getRawInlinedAt()); + } + + Metadata *getRawScope() const { return getOperand(0); } + Metadata *getRawInlinedAt() const { if (getNumOperands() == 2) return getOperand(1); return nullptr; diff --git a/lib/AsmParser/LLParser.cpp b/lib/AsmParser/LLParser.cpp index 0e98692e11c..103c8c48a5e 100644 --- a/lib/AsmParser/LLParser.cpp +++ b/lib/AsmParser/LLParser.cpp @@ -3348,8 +3348,8 @@ bool LLParser::ParseMDLocation(MDNode *&Result, bool IsDistinct) { PARSE_MD_FIELDS(); #undef VISIT_MD_FIELDS - auto get = (IsDistinct ? MDLocation::getDistinct : MDLocation::get); - Result = get(Context, line.Val, column.Val, scope.Val, inlinedAt.Val); + Result = GET_OR_DISTINCT( + MDLocation, (Context, line.Val, column.Val, scope.Val, inlinedAt.Val)); return false; } diff --git a/lib/Bitcode/Reader/BitcodeReader.cpp b/lib/Bitcode/Reader/BitcodeReader.cpp index 66831cced41..84753ffd181 100644 --- a/lib/Bitcode/Reader/BitcodeReader.cpp +++ b/lib/Bitcode/Reader/BitcodeReader.cpp @@ -1691,14 +1691,15 @@ std::error_code BitcodeReader::ParseMetadata() { if (Record.size() != 5) return Error("Invalid record"); - auto get = Record[0] ? MDLocation::getDistinct : MDLocation::get; unsigned Line = Record[1]; unsigned Column = Record[2]; MDNode *Scope = cast(MDValueList.getValueFwdRef(Record[3])); Metadata *InlinedAt = Record[4] ? MDValueList.getValueFwdRef(Record[4] - 1) : nullptr; - MDValueList.AssignValue(get(Context, Line, Column, Scope, InlinedAt), - NextMDValueNo++); + MDValueList.AssignValue( + GET_OR_DISTINCT(MDLocation, Record[0], + (Context, Line, Column, Scope, InlinedAt)), + NextMDValueNo++); break; } case bitc::METADATA_GENERIC_DEBUG: { diff --git a/lib/IR/AsmWriter.cpp b/lib/IR/AsmWriter.cpp index 4eba3212e2b..d77c3447705 100644 --- a/lib/IR/AsmWriter.cpp +++ b/lib/IR/AsmWriter.cpp @@ -1419,11 +1419,10 @@ static void writeMDLocation(raw_ostream &Out, const MDLocation *DL, if (DL->getColumn()) Out << FS << "column: " << DL->getColumn(); Out << FS << "scope: "; - WriteAsOperandInternal(Out, DL->getScope(), TypePrinter, Machine, Context); - if (DL->getInlinedAt()) { + WriteAsOperandInternal(Out, DL->getRawScope(), TypePrinter, Machine, Context); + if (auto *IA = DL->getRawInlinedAt()) { Out << FS << "inlinedAt: "; - WriteAsOperandInternal(Out, DL->getInlinedAt(), TypePrinter, Machine, - Context); + WriteAsOperandInternal(Out, IA, TypePrinter, Machine, Context); } Out << ")"; } diff --git a/lib/IR/LLVMContextImpl.h b/lib/IR/LLVMContextImpl.h index 4631246d77a..e3806652830 100644 --- a/lib/IR/LLVMContextImpl.h +++ b/lib/IR/LLVMContextImpl.h @@ -240,12 +240,12 @@ template <> struct MDNodeKeyImpl { : Line(Line), Column(Column), Scope(Scope), InlinedAt(InlinedAt) {} MDNodeKeyImpl(const MDLocation *L) - : Line(L->getLine()), Column(L->getColumn()), Scope(L->getScope()), - InlinedAt(L->getInlinedAt()) {} + : Line(L->getLine()), Column(L->getColumn()), Scope(L->getRawScope()), + InlinedAt(L->getRawInlinedAt()) {} bool isKeyOf(const MDLocation *RHS) const { return Line == RHS->getLine() && Column == RHS->getColumn() && - Scope == RHS->getScope() && InlinedAt == RHS->getInlinedAt(); + Scope == RHS->getRawScope() && InlinedAt == RHS->getRawInlinedAt(); } unsigned getHashValue() const { return hash_combine(Line, Column, Scope, InlinedAt); diff --git a/lib/IR/Verifier.cpp b/lib/IR/Verifier.cpp index ee3582d00ca..fcf48c452e2 100644 --- a/lib/IR/Verifier.cpp +++ b/lib/IR/Verifier.cpp @@ -659,8 +659,9 @@ void Verifier::visitMetadataAsValue(const MetadataAsValue &MDV, Function *F) { } void Verifier::visitMDLocation(const MDLocation &N) { - Assert(N.getScope(), "location requires a valid scope", &N); - if (auto *IA = N.getInlinedAt()) + Assert(N.getRawScope() && isa(N.getRawScope()), + "location requires a valid scope", &N, N.getRawScope()); + if (auto *IA = N.getRawInlinedAt()) Assert(isa(IA), "inlined-at should be a location", &N, IA); } diff --git a/test/Assembler/mdlocation.ll b/test/Assembler/mdlocation.ll index e095d900d64..47e6e983d42 100644 --- a/test/Assembler/mdlocation.ll +++ b/test/Assembler/mdlocation.ll @@ -4,8 +4,8 @@ ; CHECK: !named = !{!0, !1, !1, !2, !2, !3, !3, !4} !named = !{!0, !1, !2, !3, !4, !5, !6, !7} -; CHECK: !0 = !{} -!0 = !{} +; CHECK: !0 = !MDSubprogram( +!0 = !MDSubprogram() ; CHECK-NEXT: !1 = !MDLocation(line: 3, column: 7, scope: !0) !1 = !MDLocation(line: 3, column: 7, scope: !0) diff --git a/test/Assembler/metadata.ll b/test/Assembler/metadata.ll index e2c59237981..b483fc3f4cf 100644 --- a/test/Assembler/metadata.ll +++ b/test/Assembler/metadata.ll @@ -12,7 +12,7 @@ define void @test() { } !0 = !MDLocation(line: 662302, column: 26, scope: !1) -!1 = !{i32 4, !"foo"} +!1 = !MDSubprogram(name: "foo") declare void @llvm.dbg.func.start(metadata) nounwind readnone diff --git a/test/Linker/Inputs/mdlocation.ll b/test/Linker/Inputs/mdlocation.ll index f85c1dc7475..69353866b63 100644 --- a/test/Linker/Inputs/mdlocation.ll +++ b/test/Linker/Inputs/mdlocation.ll @@ -1,10 +1,10 @@ !named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8, !9} -!0 = !{} ; Use this as a scope. +!0 = !MDSubprogram() ; Use this as a scope. !1 = !MDLocation(line: 3, column: 7, scope: !0) !2 = !MDLocation(line: 3, column: 7, scope: !0, inlinedAt: !1) !3 = !MDLocation(line: 3, column: 7, scope: !0, inlinedAt: !2) -!4 = distinct !{} ; Test actual remapping. +!4 = distinct !MDSubprogram() ; Test actual remapping. !5 = !MDLocation(line: 3, column: 7, scope: !4) !6 = !MDLocation(line: 3, column: 7, scope: !4, inlinedAt: !5) !7 = !MDLocation(line: 3, column: 7, scope: !4, inlinedAt: !6) diff --git a/test/Linker/mdlocation.ll b/test/Linker/mdlocation.ll index 5ee366cbb43..9aea4b9027d 100644 --- a/test/Linker/mdlocation.ll +++ b/test/Linker/mdlocation.ll @@ -5,27 +5,27 @@ ; CHECK: !named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8, !9, !0, !1, !2, !3, !10, !11, !12, !13, !14, !15} !named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8, !9} -; CHECK: !0 = !{} +; CHECK: !0 = !MDSubprogram( ; CHECK-NEXT: !1 = !MDLocation(line: 3, column: 7, scope: !0) ; CHECK-NEXT: !2 = !MDLocation(line: 3, column: 7, scope: !0, inlinedAt: !1) ; CHECK-NEXT: !3 = !MDLocation(line: 3, column: 7, scope: !0, inlinedAt: !2) -; CHECK-NEXT: !4 = distinct !{} +; CHECK-NEXT: !4 = distinct !MDSubprogram( ; CHECK-NEXT: !5 = !MDLocation(line: 3, column: 7, scope: !4) ; CHECK-NEXT: !6 = !MDLocation(line: 3, column: 7, scope: !4, inlinedAt: !5) ; CHECK-NEXT: !7 = !MDLocation(line: 3, column: 7, scope: !4, inlinedAt: !6) ; CHECK-NEXT: !8 = distinct !MDLocation(line: 3, column: 7, scope: !0) ; CHECK-NEXT: !9 = distinct !MDLocation(line: 3, column: 7, scope: !0, inlinedAt: !8) -; CHECK-NEXT: !10 = distinct !{} +; CHECK-NEXT: !10 = distinct !MDSubprogram( ; CHECK-NEXT: !11 = !MDLocation(line: 3, column: 7, scope: !10) ; CHECK-NEXT: !12 = !MDLocation(line: 3, column: 7, scope: !10, inlinedAt: !11) ; CHECK-NEXT: !13 = !MDLocation(line: 3, column: 7, scope: !10, inlinedAt: !12) ; CHECK-NEXT: !14 = distinct !MDLocation(line: 3, column: 7, scope: !0) ; CHECK-NEXT: !15 = distinct !MDLocation(line: 3, column: 7, scope: !0, inlinedAt: !14) -!0 = !{} ; Use this as a scope. +!0 = !MDSubprogram() ; Use this as a scope. !1 = !MDLocation(line: 3, column: 7, scope: !0) !2 = !MDLocation(line: 3, column: 7, scope: !0, inlinedAt: !1) !3 = !MDLocation(line: 3, column: 7, scope: !0, inlinedAt: !2) -!4 = distinct !{} ; Test actual remapping. +!4 = distinct !MDSubprogram() ; Test actual remapping. !5 = !MDLocation(line: 3, column: 7, scope: !4) !6 = !MDLocation(line: 3, column: 7, scope: !4, inlinedAt: !5) !7 = !MDLocation(line: 3, column: 7, scope: !4, inlinedAt: !6) diff --git a/unittests/IR/MetadataTest.cpp b/unittests/IR/MetadataTest.cpp index 70ce2946af1..51a9e0bfc12 100644 --- a/unittests/IR/MetadataTest.cpp +++ b/unittests/IR/MetadataTest.cpp @@ -69,6 +69,12 @@ protected: Metadata *MDs[] = {MD1, MD2}; return MDNode::get(Context, MDs); } + + MDSubprogram *getSubprogram() { + return MDSubprogram::getDistinct(Context, nullptr, "", "", nullptr, 0, + nullptr, false, false, 0, nullptr, 0, 0, 0, + 0); + } }; typedef MetadataTest MDStringTest; @@ -671,7 +677,7 @@ TEST_F(MDNodeTest, deleteTemporaryWithTrackingRef) { typedef MetadataTest MDLocationTest; TEST_F(MDLocationTest, Overflow) { - MDNode *N = MDNode::get(Context, None); + MDSubprogram *N = getSubprogram(); { MDLocation *L = MDLocation::get(Context, 2, 7, N); EXPECT_EQ(2u, L->getLine()); @@ -696,7 +702,7 @@ TEST_F(MDLocationTest, Overflow) { } TEST_F(MDLocationTest, getDistinct) { - MDNode *N = MDNode::get(Context, None); + MDNode *N = getSubprogram(); MDLocation *L0 = MDLocation::getDistinct(Context, 2, 7, N); EXPECT_TRUE(L0->isDistinct()); MDLocation *L1 = MDLocation::get(Context, 2, 7, N); -- 2.34.1