From e6893b5521005193441cd90421cb555bde352b5a Mon Sep 17 00:00:00 2001 From: "Duncan P. N. Exon Smith" Date: Tue, 19 Apr 2016 14:55:09 +0000 Subject: [PATCH] IR: getOrInsertODRUniquedType => DICompositeType::getODRType, NFC Lift the API for debug info ODR type uniquing up a layer. Instead of clients managing the map directly on the LLVMContext, add a static method to DICompositeType called getODRType and handle the map in the background. Also adds DICompositeType::getODRTypeIfExists, so far just for convenience in the unit tests. This simplifies the logic in LLParser and BitcodeReader. Because of argument spam there are actually a few more lines of code now; I'll see if I come up with a reasonable way to clean that up. llvm-svn: 266742 --- include/llvm/IR/DebugInfoMetadata.h | 17 +++++++++++ include/llvm/IR/LLVMContext.h | 11 ------- lib/AsmParser/LLParser.cpp | 18 ++++++------ lib/Bitcode/Reader/BitcodeReader.cpp | 41 +++++++++++++++++---------- lib/IR/DebugInfoMetadata.cpp | 26 +++++++++++++++++ lib/IR/LLVMContext.cpp | 6 ---- test/Linker/dicompositetype-unique.ll | 4 +-- unittests/IR/LLVMContextTest.cpp | 40 +++++++++++++++----------- 8 files changed, 103 insertions(+), 60 deletions(-) diff --git a/include/llvm/IR/DebugInfoMetadata.h b/include/llvm/IR/DebugInfoMetadata.h index 0ed91498281..61bb1334196 100644 --- a/include/llvm/IR/DebugInfoMetadata.h +++ b/include/llvm/IR/DebugInfoMetadata.h @@ -825,6 +825,23 @@ public: TempDICompositeType clone() const { return cloneImpl(); } + /// Get a DICompositeType with the given ODR identifier. + /// + /// If \a LLVMContext::isODRUniquingDebugTypes(), gets the mapped + /// DICompositeType for the given ODR \c Identifier. If none exists, creates + /// a new node. + /// + /// Else, returns \c nullptr. + static DICompositeType * + getODRType(LLVMContext &Context, MDString &Identifier, unsigned Tag, + MDString *Name, Metadata *File, unsigned Line, Metadata *Scope, + Metadata *BaseType, uint64_t SizeInBits, uint64_t AlignInBits, + uint64_t OffsetInBits, unsigned Flags, Metadata *Elements, + unsigned RuntimeLang, Metadata *VTableHolder, + Metadata *TemplateParams); + static DICompositeType *getODRTypeIfExists(LLVMContext &Context, + MDString &Identifier); + DITypeRef getBaseType() const { return DITypeRef(getRawBaseType()); } DINodeArray getElements() const { return cast_or_null(getRawElements()); diff --git a/include/llvm/IR/LLVMContext.h b/include/llvm/IR/LLVMContext.h index 8ef85f8932b..1d05269ab73 100644 --- a/include/llvm/IR/LLVMContext.h +++ b/include/llvm/IR/LLVMContext.h @@ -121,17 +121,6 @@ public: void enableDebugTypeODRUniquing(); void disableDebugTypeODRUniquing(); - /// Get or insert the DICompositeType mapped to the given string. - /// - /// Returns the address of the current \a DICompositeType pointer mapped to - /// \c S, inserting a mapping to \c nullptr if \c S was not previously - /// mapped. This method has no effect (and returns \c nullptr instead of a - /// valid address) if \a isODRUniquingDebugTypes() is \c false. - /// - /// \post If \a isODRUniquingDebugTypes(), \c S will have a (possibly null) - /// mapping. \note The returned address is only valid until the next call. - DICompositeType **getOrInsertODRUniquedType(const MDString &S); - typedef void (*InlineAsmDiagHandlerTy)(const SMDiagnostic&, void *Context, unsigned LocCookie); diff --git a/lib/AsmParser/LLParser.cpp b/lib/AsmParser/LLParser.cpp index c906b7b22db..fb27ab5353f 100644 --- a/lib/AsmParser/LLParser.cpp +++ b/lib/AsmParser/LLParser.cpp @@ -3841,13 +3841,15 @@ bool LLParser::ParseDICompositeType(MDNode *&Result, bool IsDistinct) { // If this isn't a forward declaration and it has a UUID, check for it in the // type map in the context. - DICompositeType **MappedT = nullptr; - if (!(flags.Val & DINode::FlagFwdDecl) && identifier.Val && - (MappedT = Context.getOrInsertODRUniquedType(*identifier.Val)) && - *MappedT) { - Result = *MappedT; - return false; - } + if (!(flags.Val & DINode::FlagFwdDecl) && identifier.Val) + if (auto *CT = DICompositeType::getODRType( + Context, *identifier.Val, tag.Val, name.Val, file.Val, line.Val, + scope.Val, baseType.Val, size.Val, align.Val, offset.Val, flags.Val, + elements.Val, runtimeLang.Val, vtableHolder.Val, + templateParams.Val)) { + Result = CT; + return false; + } // Create a new node, and save it in the context if it belongs in the type // map. @@ -3856,8 +3858,6 @@ bool LLParser::ParseDICompositeType(MDNode *&Result, bool IsDistinct) { (Context, tag.Val, name.Val, file.Val, line.Val, scope.Val, baseType.Val, size.Val, align.Val, offset.Val, flags.Val, elements.Val, runtimeLang.Val, vtableHolder.Val, templateParams.Val, identifier.Val)); - if (MappedT) - *MappedT = cast(Result); return false; } diff --git a/lib/Bitcode/Reader/BitcodeReader.cpp b/lib/Bitcode/Reader/BitcodeReader.cpp index d454eb1fd4c..dba0fd41755 100644 --- a/lib/Bitcode/Reader/BitcodeReader.cpp +++ b/lib/Bitcode/Reader/BitcodeReader.cpp @@ -2190,25 +2190,36 @@ std::error_code BitcodeReader::parseMetadata(bool ModuleLevel) { // If we have a UUID and this is not a forward declaration, lookup the // mapping. + bool IsDistinct = Record[0]; + unsigned Tag = Record[1]; + MDString *Name = getMDString(Record[2]); + Metadata *File = getMDOrNull(Record[3]); + unsigned Line = Record[4]; + Metadata *Scope = getMDOrNull(Record[5]); + Metadata *BaseType = getMDOrNull(Record[6]); + uint64_t SizeInBits = Record[7]; + uint64_t AlignInBits = Record[8]; + uint64_t OffsetInBits = Record[9]; unsigned Flags = Record[10]; + Metadata *Elements = getMDOrNull(Record[11]); + unsigned RuntimeLang = Record[12]; + Metadata *VTableHolder = getMDOrNull(Record[13]); + Metadata *TemplateParams = getMDOrNull(Record[14]); auto *Identifier = getMDString(Record[15]); - DICompositeType **MappedT = nullptr; + DICompositeType *CT = nullptr; if (!(Flags & DINode::FlagFwdDecl) && Identifier) - MappedT = Context.getOrInsertODRUniquedType(*Identifier); + CT = DICompositeType::getODRType( + Context, *Identifier, Tag, Name, File, Line, Scope, BaseType, + SizeInBits, AlignInBits, OffsetInBits, Flags, Elements, RuntimeLang, + VTableHolder, TemplateParams); - // Use the mapped type node, or create a new one if necessary. - DICompositeType *CT = MappedT ? *MappedT : nullptr; - if (!CT) { - CT = GET_OR_DISTINCT( - DICompositeType, Record[0], - (Context, Record[1], getMDString(Record[2]), getMDOrNull(Record[3]), - Record[4], getMDOrNull(Record[5]), getMDOrNull(Record[6]), - Record[7], Record[8], Record[9], Flags, getMDOrNull(Record[11]), - Record[12], getMDOrNull(Record[13]), getMDOrNull(Record[14]), - Identifier)); - if (MappedT) - *MappedT = CT; - } + // Create a node if we didn't get a lazy ODR type. + if (!CT) + CT = GET_OR_DISTINCT(DICompositeType, IsDistinct, + (Context, Tag, Name, File, Line, Scope, BaseType, + SizeInBits, AlignInBits, OffsetInBits, Flags, + Elements, RuntimeLang, VTableHolder, + TemplateParams, Identifier)); MetadataList.assignValue(CT, NextMetadataNo++); break; diff --git a/lib/IR/DebugInfoMetadata.cpp b/lib/IR/DebugInfoMetadata.cpp index 3e6400785fd..37986f73ab1 100644 --- a/lib/IR/DebugInfoMetadata.cpp +++ b/lib/IR/DebugInfoMetadata.cpp @@ -266,6 +266,32 @@ DICompositeType *DICompositeType::getImpl( Ops); } +DICompositeType *DICompositeType::getODRType( + LLVMContext &Context, MDString &Identifier, unsigned Tag, MDString *Name, + Metadata *File, unsigned Line, Metadata *Scope, Metadata *BaseType, + uint64_t SizeInBits, uint64_t AlignInBits, uint64_t OffsetInBits, + unsigned Flags, Metadata *Elements, unsigned RuntimeLang, + Metadata *VTableHolder, Metadata *TemplateParams) { + assert(!Identifier.getString().empty() && "Expected valid identifier"); + if (!Context.isODRUniquingDebugTypes()) + return nullptr; + auto *&CT = (*Context.pImpl->DITypeMap)[&Identifier]; + if (!CT) + CT = DICompositeType::getDistinct( + Context, Tag, Name, File, Line, Scope, BaseType, SizeInBits, + AlignInBits, OffsetInBits, Flags, Elements, RuntimeLang, VTableHolder, + TemplateParams, &Identifier); + return CT; +} + +DICompositeType *DICompositeType::getODRTypeIfExists(LLVMContext &Context, + MDString &Identifier) { + assert(!Identifier.getString().empty() && "Expected valid identifier"); + if (!Context.isODRUniquingDebugTypes()) + return nullptr; + return Context.pImpl->DITypeMap->lookup(&Identifier); +} + DISubroutineType *DISubroutineType::getImpl(LLVMContext &Context, unsigned Flags, Metadata *TypeArray, StorageType Storage, diff --git a/lib/IR/LLVMContext.cpp b/lib/IR/LLVMContext.cpp index 7c0d18d028a..0d4f7242b76 100644 --- a/lib/IR/LLVMContext.cpp +++ b/lib/IR/LLVMContext.cpp @@ -323,12 +323,6 @@ void LLVMContext::enableDebugTypeODRUniquing() { void LLVMContext::disableDebugTypeODRUniquing() { pImpl->DITypeMap.reset(); } -DICompositeType **LLVMContext::getOrInsertODRUniquedType(const MDString &S) { - if (!isODRUniquingDebugTypes()) - return nullptr; - return &(*pImpl->DITypeMap)[&S]; -} - void LLVMContext::setDiscardValueNames(bool Discard) { pImpl->DiscardValueNames = Discard; } diff --git a/test/Linker/dicompositetype-unique.ll b/test/Linker/dicompositetype-unique.ll index 9c8b351ec64..4f3fcec6964 100644 --- a/test/Linker/dicompositetype-unique.ll +++ b/test/Linker/dicompositetype-unique.ll @@ -21,11 +21,11 @@ !named = !{!0, !1} ; Check both directions. -; CHECK: !1 = !DICompositeType( +; CHECK: !1 = distinct !DICompositeType( ; CHECK-SAME: name: "T1" ; CHECK-SAME: identifier: "T" ; CHECK-NOT: identifier: "T" -; REVERSE: !1 = !DICompositeType( +; REVERSE: !1 = distinct !DICompositeType( ; REVERSE-SAME: name: "T2" ; REVERSE-SAME: identifier: "T" ; REVERSE-NOT: identifier: "T" diff --git a/unittests/IR/LLVMContextTest.cpp b/unittests/IR/LLVMContextTest.cpp index 028dc68827b..37656a9b83a 100644 --- a/unittests/IR/LLVMContextTest.cpp +++ b/unittests/IR/LLVMContextTest.cpp @@ -25,35 +25,41 @@ TEST(LLVMContextTest, enableDebugTypeODRUniquing) { TEST(LLVMContextTest, getOrInsertODRUniquedType) { LLVMContext Context; - const MDString &S = *MDString::get(Context, "string"); + MDString &UUID = *MDString::get(Context, "string"); // Without a type map, this should return null. - EXPECT_FALSE(Context.getOrInsertODRUniquedType(S)); + EXPECT_FALSE(DICompositeType::getODRType( + Context, UUID, dwarf::DW_TAG_class_type, nullptr, nullptr, 0, nullptr, + nullptr, 0, 0, 0, 0, nullptr, 0, nullptr, nullptr)); - // Get the mapping. + // Enable the mapping. There still shouldn't be a type. Context.enableDebugTypeODRUniquing(); - DICompositeType **Mapping = Context.getOrInsertODRUniquedType(S); - ASSERT_TRUE(Mapping); + EXPECT_FALSE(DICompositeType::getODRTypeIfExists(Context, UUID)); - // Create some type and add it to the mapping. - auto &CT = *DICompositeType::get(Context, dwarf::DW_TAG_class_type, "name", - nullptr, 0, nullptr, nullptr, 0, 0, 0, 0, - nullptr, 0, nullptr, nullptr, S.getString()); - ASSERT_EQ(S.getString(), CT.getIdentifier()); - *Mapping = &CT; + // Create some ODR-uniqued type. + auto &CT = *DICompositeType::getODRType( + Context, UUID, dwarf::DW_TAG_class_type, nullptr, nullptr, 0, nullptr, + nullptr, 0, 0, 0, 0, nullptr, 0, nullptr, nullptr); + EXPECT_EQ(UUID.getString(), CT.getIdentifier()); - // Check that we get it back. - Mapping = Context.getOrInsertODRUniquedType(S); - ASSERT_TRUE(Mapping); - EXPECT_EQ(&CT, *Mapping); + // Check that we get it back, even if we change a field. + EXPECT_EQ(&CT, DICompositeType::getODRTypeIfExists(Context, UUID)); + EXPECT_EQ( + &CT, DICompositeType::getODRType(Context, UUID, dwarf::DW_TAG_class_type, + nullptr, nullptr, 0, nullptr, nullptr, 0, + 0, 0, 0, nullptr, 0, nullptr, nullptr)); + EXPECT_EQ(&CT, DICompositeType::getODRType( + Context, UUID, dwarf::DW_TAG_class_type, + MDString::get(Context, "name"), nullptr, 0, nullptr, + nullptr, 0, 0, 0, 0, nullptr, 0, nullptr, nullptr)); // Check that it's discarded with the type map. Context.disableDebugTypeODRUniquing(); - EXPECT_FALSE(Context.getOrInsertODRUniquedType(S)); + EXPECT_FALSE(DICompositeType::getODRTypeIfExists(Context, UUID)); // And it shouldn't magically reappear... Context.enableDebugTypeODRUniquing(); - EXPECT_FALSE(*Context.getOrInsertODRUniquedType(S)); + EXPECT_FALSE(DICompositeType::getODRTypeIfExists(Context, UUID)); } } // end namespace