Author: Richard Smith Date: 2023-07-25T15:59:21-07:00 New Revision: 61c7a9140becb19c5b1bc644e54452c6f782f5d5
URL: https://github.com/llvm/llvm-project/commit/61c7a9140becb19c5b1bc644e54452c6f782f5d5 DIFF: https://github.com/llvm/llvm-project/commit/61c7a9140becb19c5b1bc644e54452c6f782f5d5.diff LOG: Commit to a primary definition for a class when we load its first member. Previously, we wouldn't do this if the first member loaded is within a definition that's added to a class via an update record, which happens when template instantiation adds a class definition to a declaration that was imported from an AST file. This would lead to classes having member functions whose getParent returned a class declaration that wasn't the primary definition, which in turn caused the vtable builder to build broken vtables. I don't yet have a reduced testcase for the wrong-code bug here, because the setup required to get us into the broken state is very subtle, but have confirmed that this fixes it. Added: Modified: clang/docs/ReleaseNotes.rst clang/lib/Serialization/ASTReaderDecl.cpp Removed: ################################################################################ diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index d8655e52aa3235..6a762a6e92eb5a 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -98,6 +98,10 @@ Improvements to Clang's diagnostics Bug Fixes in This Version ------------------------- +- Fixed an issue where a class template specialization whose declaration is + instantiated in one module and whose definition is instantiated in another + module may end up with members associated with the wrong declaration of the + class, which can result in miscompiles in some cases. Bug Fixes to Compiler Builtins ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp index 10c92f8d214941..c8cbee14be4f02 100644 --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/lib/Serialization/ASTReaderDecl.cpp @@ -181,6 +181,13 @@ namespace clang { static void setAnonymousDeclForMerging(ASTReader &Reader, DeclContext *DC, unsigned Index, NamedDecl *D); + /// Commit to a primary definition of the class RD, which is known to be + /// a definition of the class. We might not have read the definition data + /// for it yet. If we haven't then allocate placeholder definition data + /// now too. + static CXXRecordDecl *getOrFakePrimaryClassDefinition(ASTReader &Reader, + CXXRecordDecl *RD); + /// Results from loading a RedeclarableDecl. class RedeclarableResult { Decl *MergeWith; @@ -598,7 +605,13 @@ void ASTDeclReader::VisitDecl(Decl *D) { auto *LexicalDC = readDeclAs<DeclContext>(); if (!LexicalDC) LexicalDC = SemaDC; - DeclContext *MergedSemaDC = Reader.MergedDeclContexts.lookup(SemaDC); + // If the context is a class, we might not have actually merged it yet, in + // the case where the definition comes from an update record. + DeclContext *MergedSemaDC; + if (auto *RD = dyn_cast<CXXRecordDecl>(SemaDC)) + MergedSemaDC = getOrFakePrimaryClassDefinition(Reader, RD); + else + MergedSemaDC = Reader.MergedDeclContexts.lookup(SemaDC); // Avoid calling setLexicalDeclContext() directly because it uses // Decl::getASTContext() internally which is unsafe during derialization. D->setDeclContextsImpl(MergedSemaDC ? MergedSemaDC : SemaDC, LexicalDC, @@ -3198,6 +3211,32 @@ uint64_t ASTReader::getGlobalBitOffset(ModuleFile &M, uint64_t LocalOffset) { return LocalOffset + M.GlobalBitOffset; } +CXXRecordDecl * +ASTDeclReader::getOrFakePrimaryClassDefinition(ASTReader &Reader, + CXXRecordDecl *RD) { + // Try to dig out the definition. + auto *DD = RD->DefinitionData; + if (!DD) + DD = RD->getCanonicalDecl()->DefinitionData; + + // If there's no definition yet, then DC's definition is added by an update + // record, but we've not yet loaded that update record. In this case, we + // commit to DC being the canonical definition now, and will fix this when + // we load the update record. + if (!DD) { + DD = new (Reader.getContext()) struct CXXRecordDecl::DefinitionData(RD); + RD->setCompleteDefinition(true); + RD->DefinitionData = DD; + RD->getCanonicalDecl()->DefinitionData = DD; + + // Track that we did this horrible thing so that we can fix it later. + Reader.PendingFakeDefinitionData.insert( + std::make_pair(DD, ASTReader::PendingFakeDefinitionKind::Fake)); + } + + return DD->Definition; +} + /// Find the context in which we should search for previous declarations when /// looking for declarations to merge. DeclContext *ASTDeclReader::getPrimaryContextForMerging(ASTReader &Reader, @@ -3205,29 +3244,8 @@ DeclContext *ASTDeclReader::getPrimaryContextForMerging(ASTReader &Reader, if (auto *ND = dyn_cast<NamespaceDecl>(DC)) return ND->getOriginalNamespace(); - if (auto *RD = dyn_cast<CXXRecordDecl>(DC)) { - // Try to dig out the definition. - auto *DD = RD->DefinitionData; - if (!DD) - DD = RD->getCanonicalDecl()->DefinitionData; - - // If there's no definition yet, then DC's definition is added by an update - // record, but we've not yet loaded that update record. In this case, we - // commit to DC being the canonical definition now, and will fix this when - // we load the update record. - if (!DD) { - DD = new (Reader.getContext()) struct CXXRecordDecl::DefinitionData(RD); - RD->setCompleteDefinition(true); - RD->DefinitionData = DD; - RD->getCanonicalDecl()->DefinitionData = DD; - - // Track that we did this horrible thing so that we can fix it later. - Reader.PendingFakeDefinitionData.insert( - std::make_pair(DD, ASTReader::PendingFakeDefinitionKind::Fake)); - } - - return DD->Definition; - } + if (auto *RD = dyn_cast<CXXRecordDecl>(DC)) + return getOrFakePrimaryClassDefinition(Reader, RD); if (auto *RD = dyn_cast<RecordDecl>(DC)) return RD->getDefinition(); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
