From f7ac5a44796bbb164c285e2c914f3261e7a1ecef Mon Sep 17 00:00:00 2001 From: Gabor Marton Date: Wed, 23 May 2018 13:53:36 +0000 Subject: [PATCH] Fix duplicate class template definitions problem Summary: We fail to import a `ClassTemplateDecl` if the "To" context already contains a definition and then a forward decl. This is because `localUncachedLookup` does not find the definition. This is not a lookup error, the parser behaves differently than assumed in the importer code. A `DeclContext` contains one DenseMap (`LookupPtr`) which maps names to lists. The list is a special list `StoredDeclsList` which is optimized to have one element. During building the initial AST, the parser first adds the definition to the `DeclContext`. Then during parsing the second declaration (the forward decl) the parser again calls `DeclContext::addDecl` but that will not add a new element to the `StoredDeclsList` rarther it simply overwrites the old element with the most recent one. This patch fixes the error by finding the definition in the redecl chain. Added tests for the same issue with `CXXRecordDecl` and with `ClassTemplateSpecializationDecl`. These tests pass and they pass because in `VisitRecordDecl` and in `VisitClassTemplateSpecializationDecl` we already use `D->getDefinition()` after the lookup. Reviewers: a.sidorin, xazax.hun, szepet Subscribers: rnkovacs, dkrupp, cfe-commits Differential Revision: https://reviews.llvm.org/D46950 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@333082 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/AST/ASTImporter.cpp | 46 +++++++---- unittests/AST/ASTImporterTest.cpp | 130 +++++++++++++++++++++++++++++- unittests/AST/DeclMatcher.h | 19 ++++- 3 files changed, 174 insertions(+), 21 deletions(-) diff --git a/lib/AST/ASTImporter.cpp b/lib/AST/ASTImporter.cpp index 6fa4617cf0..3c69897436 100644 --- a/lib/AST/ASTImporter.cpp +++ b/lib/AST/ASTImporter.cpp @@ -4070,6 +4070,17 @@ ASTNodeImporter::VisitTemplateTemplateParmDecl(TemplateTemplateParmDecl *D) { TemplateParams); } +// Returns the definition for a (forward) declaration of a ClassTemplateDecl, if +// it has any definition in the redecl chain. +static ClassTemplateDecl *getDefinition(ClassTemplateDecl *D) { + CXXRecordDecl *ToTemplatedDef = D->getTemplatedDecl()->getDefinition(); + if (!ToTemplatedDef) + return nullptr; + ClassTemplateDecl *TemplateWithDef = + ToTemplatedDef->getDescribedClassTemplate(); + return TemplateWithDef; +} + Decl *ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) { // If this record has a definition in the translation unit we're coming from, // but this particular declaration is not that definition, import the @@ -4084,7 +4095,7 @@ Decl *ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) { return Importer.Imported(D, ImportedDef); } - + // Import the major distinguishing characteristics of this class template. DeclContext *DC, *LexicalDC; DeclarationName Name; @@ -4103,34 +4114,39 @@ Decl *ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) { for (auto *FoundDecl : FoundDecls) { if (!FoundDecl->isInIdentifierNamespace(Decl::IDNS_Ordinary)) continue; - + Decl *Found = FoundDecl; if (auto *FoundTemplate = dyn_cast(Found)) { - if (IsStructuralMatch(D, FoundTemplate)) { - // The class templates structurally match; call it the same template. - // We found a forward declaration but the class to be imported has a - // definition. - // FIXME Add this forward declaration to the redeclaration chain. - if (D->isThisDeclarationADefinition() && - !FoundTemplate->isThisDeclarationADefinition()) + // The class to be imported is a definition. + if (D->isThisDeclarationADefinition()) { + // Lookup will find the fwd decl only if that is more recent than the + // definition. So, try to get the definition if that is available in + // the redecl chain. + ClassTemplateDecl *TemplateWithDef = getDefinition(FoundTemplate); + if (!TemplateWithDef) continue; + FoundTemplate = TemplateWithDef; // Continue with the definition. + } + + if (IsStructuralMatch(D, FoundTemplate)) { + // The class templates structurally match; call it the same template. - Importer.Imported(D->getTemplatedDecl(), + Importer.Imported(D->getTemplatedDecl(), FoundTemplate->getTemplatedDecl()); return Importer.Imported(D, FoundTemplate); - } + } } - + ConflictingDecls.push_back(FoundDecl); } - + if (!ConflictingDecls.empty()) { Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Ordinary, - ConflictingDecls.data(), + ConflictingDecls.data(), ConflictingDecls.size()); } - + if (!Name) return nullptr; } diff --git a/unittests/AST/ASTImporterTest.cpp b/unittests/AST/ASTImporterTest.cpp index 7d6748e550..18c8f4cc02 100644 --- a/unittests/AST/ASTImporterTest.cpp +++ b/unittests/AST/ASTImporterTest.cpp @@ -281,8 +281,9 @@ public: return std::make_tuple(*FoundDecls.begin(), Imported); } - // Creates a TU decl for the given source code. - // May be called several times in a given test. + // Creates a TU decl for the given source code which can be used as a From + // context. May be called several times in a given test (with different file + // name). TranslationUnitDecl *getTuDecl(StringRef SrcCode, Language Lang, StringRef FileName = "input.cc") { assert( @@ -297,6 +298,16 @@ public: return Tu.TUDecl; } + // Creates the To context with the given source code and returns the TU decl. + TranslationUnitDecl *getToTuDecl(StringRef ToSrcCode, Language ToLang) { + ArgVector ToArgs = getArgVectorForLanguage(ToLang); + ToCode = ToSrcCode; + assert(!ToAST); + ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName); + + return ToAST->getASTContext().getTranslationUnitDecl(); + } + // Import the given Decl into the ToCtx. // May be called several times in a given test. // The different instances of the param From may have different ASTContext. @@ -1464,6 +1475,121 @@ TEST_P(ASTImporterTestBase, ImportDefinitionOfClassTemplateAfterFwdDecl) { } } +TEST_P(ASTImporterTestBase, + ImportDefinitionOfClassTemplateIfThereIsAnExistingFwdDeclAndDefinition) { + Decl *ToTU = getToTuDecl( + R"( + template + struct B { + void f(); + }; + + template + struct B; + )", + Lang_CXX); + ASSERT_EQ(1u, DeclCounterWithPredicate( + [](const ClassTemplateDecl *T) { + return T->isThisDeclarationADefinition(); + }) + .match(ToTU, classTemplateDecl())); + + Decl *FromTU = getTuDecl( + R"( + template + struct B { + void f(); + }; + )", + Lang_CXX, "input1.cc"); + ClassTemplateDecl *FromD = FirstDeclMatcher().match( + FromTU, classTemplateDecl(hasName("B"))); + + Import(FromD, Lang_CXX); + + // We should have only one definition. + EXPECT_EQ(1u, DeclCounterWithPredicate( + [](const ClassTemplateDecl *T) { + return T->isThisDeclarationADefinition(); + }) + .match(ToTU, classTemplateDecl())); +} + +TEST_P(ASTImporterTestBase, + ImportDefinitionOfClassIfThereIsAnExistingFwdDeclAndDefinition) { + Decl *ToTU = getToTuDecl( + R"( + struct B { + void f(); + }; + + struct B; + )", + Lang_CXX); + ASSERT_EQ(2u, DeclCounter().match( + ToTU, cxxRecordDecl(hasParent(translationUnitDecl())))); + + Decl *FromTU = getTuDecl( + R"( + struct B { + void f(); + }; + )", + Lang_CXX, "input1.cc"); + auto *FromD = FirstDeclMatcher().match( + FromTU, cxxRecordDecl(hasName("B"))); + + Import(FromD, Lang_CXX); + + EXPECT_EQ(2u, DeclCounter().match( + ToTU, cxxRecordDecl(hasParent(translationUnitDecl())))); +} + +TEST_P( + ASTImporterTestBase, + ImportDefinitionOfClassTemplateSpecIfThereIsAnExistingFwdDeclAndDefinition) +{ + Decl *ToTU = getToTuDecl( + R"( + template + struct B; + + template <> + struct B {}; + + template <> + struct B; + )", + Lang_CXX); + // We should have only one definition. + ASSERT_EQ(1u, DeclCounterWithPredicate( + [](const ClassTemplateSpecializationDecl *T) { + return T->isThisDeclarationADefinition(); + }) + .match(ToTU, classTemplateSpecializationDecl())); + + Decl *FromTU = getTuDecl( + R"( + template + struct B; + + template <> + struct B {}; + )", + Lang_CXX, "input1.cc"); + auto *FromD = FirstDeclMatcher().match( + FromTU, classTemplateSpecializationDecl(hasName("B"))); + + Import(FromD, Lang_CXX); + + // We should have only one definition. + EXPECT_EQ(1u, DeclCounterWithPredicate( + [](const ClassTemplateSpecializationDecl *T) { + return T->isThisDeclarationADefinition(); + }) + .match(ToTU, classTemplateSpecializationDecl())); +} + INSTANTIATE_TEST_CASE_P( ParameterizedTests, ASTImporterTestBase, ::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),); diff --git a/unittests/AST/DeclMatcher.h b/unittests/AST/DeclMatcher.h index c73a4cb49f..602f8dff07 100644 --- a/unittests/AST/DeclMatcher.h +++ b/unittests/AST/DeclMatcher.h @@ -44,15 +44,23 @@ template using FirstDeclMatcher = DeclMatcher; template -class DeclCounter : public MatchFinder::MatchCallback { +class DeclCounterWithPredicate : public MatchFinder::MatchCallback { + using UnaryPredicate = std::function; + UnaryPredicate Predicate; unsigned Count = 0; void run(const MatchFinder::MatchResult &Result) override { - if(Result.Nodes.getNodeAs("")) { + if (auto N = Result.Nodes.getNodeAs("")) { + if (Predicate(N)) ++Count; - } + } } + public: - // Returns the number of matched nodes under the tree rooted in `D`. + DeclCounterWithPredicate() + : Predicate([](const NodeType *) { return true; }) {} + DeclCounterWithPredicate(UnaryPredicate P) : Predicate(P) {} + // Returns the number of matched nodes which satisfy the predicate under the + // tree rooted in `D`. template unsigned match(const Decl *D, const MatcherType &AMatcher) { MatchFinder Finder; @@ -62,6 +70,9 @@ public: } }; +template +using DeclCounter = DeclCounterWithPredicate; + } // end namespace ast_matchers } // end namespace clang -- 2.40.0