Author: martong Date: Mon Sep 17 05:04:52 2018 New Revision: 342384 URL: http://llvm.org/viewvc/llvm-project?rev=342384&view=rev Log: [ASTImporter] Fix import of VarDecl init
Summary: The init expression of a VarDecl is overwritten in the "To" context if we import a VarDecl without an init expression (and with a definition). Please refer to the added tests, especially InitAndDefinitionAreInDifferentTUs. This patch fixes the malfunction by importing the whole Decl chain similarly as we did that in case of FunctionDecls. We handle the init expression similarly to a definition, alas only one init expression will be in the merged ast. Reviewers: a_sidorin, xazax.hun, r.stahl, a.sidorin Subscribers: rnkovacs, dkrupp, cfe-commits Differential Revision: https://reviews.llvm.org/D51597 Modified: cfe/trunk/lib/AST/ASTImporter.cpp cfe/trunk/unittests/AST/ASTImporterTest.cpp Modified: cfe/trunk/lib/AST/ASTImporter.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTImporter.cpp?rev=342384&r1=342383&r2=342384&view=diff ============================================================================== --- cfe/trunk/lib/AST/ASTImporter.cpp (original) +++ cfe/trunk/lib/AST/ASTImporter.cpp Mon Sep 17 05:04:52 2018 @@ -107,9 +107,11 @@ namespace clang { } SmallVector<Decl*, 2> getCanonicalForwardRedeclChain(Decl* D) { - // Currently only FunctionDecl is supported - auto FD = cast<FunctionDecl>(D); - return getCanonicalForwardRedeclChain<FunctionDecl>(FD); + if (auto *FD = dyn_cast<FunctionDecl>(D)) + return getCanonicalForwardRedeclChain<FunctionDecl>(FD); + if (auto *VD = dyn_cast<VarDecl>(D)) + return getCanonicalForwardRedeclChain<VarDecl>(VD); + llvm_unreachable("Bad declaration kind"); } void updateFlags(const Decl *From, Decl *To) { @@ -280,10 +282,9 @@ namespace clang { (IDK == IDK_Default && !Importer.isMinimalImport()); } + bool ImportInitializer(VarDecl *From, VarDecl *To); bool ImportDefinition(RecordDecl *From, RecordDecl *To, ImportDefinitionKind Kind = IDK_Default); - bool ImportDefinition(VarDecl *From, VarDecl *To, - ImportDefinitionKind Kind = IDK_Default); bool ImportDefinition(EnumDecl *From, EnumDecl *To, ImportDefinitionKind Kind = IDK_Default); bool ImportDefinition(ObjCInterfaceDecl *From, ObjCInterfaceDecl *To, @@ -1424,18 +1425,26 @@ bool ASTNodeImporter::ImportDefinition(R return false; } -bool ASTNodeImporter::ImportDefinition(VarDecl *From, VarDecl *To, - ImportDefinitionKind Kind) { +bool ASTNodeImporter::ImportInitializer(VarDecl *From, VarDecl *To) { if (To->getAnyInitializer()) return false; - // FIXME: Can we really import any initializer? Alternatively, we could force - // ourselves to import every declaration of a variable and then only use - // getInit() here. - To->setInit(Importer.Import(const_cast<Expr *>(From->getAnyInitializer()))); + Expr *FromInit = From->getInit(); + if (!FromInit) + return false; - // FIXME: Other bits to merge? + Expr *ToInit = Importer.Import(const_cast<Expr *>(FromInit)); + if (!ToInit) + return true; + To->setInit(ToInit); + if (From->isInitKnownICE()) { + EvaluatedStmt *Eval = To->ensureEvaluatedStmt(); + Eval->CheckedICE = true; + Eval->IsICE = From->isInitICE(); + } + + // FIXME: Other bits to merge? return false; } @@ -3138,6 +3147,16 @@ Decl *ASTNodeImporter::VisitObjCIvarDecl } Decl *ASTNodeImporter::VisitVarDecl(VarDecl *D) { + + SmallVector<Decl*, 2> Redecls = getCanonicalForwardRedeclChain(D); + auto RedeclIt = Redecls.begin(); + // Import the first part of the decl chain. I.e. import all previous + // declarations starting from the canonical decl. + for (; RedeclIt != Redecls.end() && *RedeclIt != D; ++RedeclIt) + if (!Importer.Import(*RedeclIt)) + return nullptr; + assert(*RedeclIt == D); + // Import the major distinguishing characteristics of a variable. DeclContext *DC, *LexicalDC; DeclarationName Name; @@ -3150,8 +3169,8 @@ Decl *ASTNodeImporter::VisitVarDecl(VarD // Try to find a variable in our own ("to") context with the same name and // in the same context as the variable we're importing. + VarDecl *FoundByLookup = nullptr; if (D->isFileVarDecl()) { - VarDecl *MergeWithVar = nullptr; SmallVector<NamedDecl *, 4> ConflictingDecls; unsigned IDNS = Decl::IDNS_Ordinary; SmallVector<NamedDecl *, 2> FoundDecls; @@ -3166,7 +3185,23 @@ Decl *ASTNodeImporter::VisitVarDecl(VarD D->hasExternalFormalLinkage()) { if (Importer.IsStructurallyEquivalent(D->getType(), FoundVar->getType())) { - MergeWithVar = FoundVar; + + // The VarDecl in the "From" context has a definition, but in the + // "To" context we already have a definition. + VarDecl *FoundDef = FoundVar->getDefinition(); + if (D->isThisDeclarationADefinition() && FoundDef) + // FIXME Check for ODR error if the two definitions have + // different initializers? + return Importer.MapImported(D, FoundDef); + + // The VarDecl in the "From" context has an initializer, but in the + // "To" context we already have an initializer. + const VarDecl *FoundDInit = nullptr; + if (D->getInit() && FoundVar->getAnyInitializer(FoundDInit)) + // FIXME Diagnose ODR error if the two initializers are different? + return Importer.MapImported(D, const_cast<VarDecl*>(FoundDInit)); + + FoundByLookup = FoundVar; break; } @@ -3183,11 +3218,11 @@ Decl *ASTNodeImporter::VisitVarDecl(VarD return nullptr; FoundVar->setType(T); - MergeWithVar = FoundVar; + FoundByLookup = FoundVar; break; } else if (isa<IncompleteArrayType>(TArray) && isa<ConstantArrayType>(FoundArray)) { - MergeWithVar = FoundVar; + FoundByLookup = FoundVar; break; } } @@ -3202,32 +3237,6 @@ Decl *ASTNodeImporter::VisitVarDecl(VarD ConflictingDecls.push_back(FoundDecl); } - if (MergeWithVar) { - // An equivalent variable with external linkage has been found. Link - // the two declarations, then merge them. - Importer.MapImported(D, MergeWithVar); - updateFlags(D, MergeWithVar); - - if (VarDecl *DDef = D->getDefinition()) { - if (VarDecl *ExistingDef = MergeWithVar->getDefinition()) { - Importer.ToDiag(ExistingDef->getLocation(), - diag::err_odr_variable_multiple_def) - << Name; - Importer.FromDiag(DDef->getLocation(), diag::note_odr_defined_here); - } else { - Expr *Init = Importer.Import(DDef->getInit()); - MergeWithVar->setInit(Init); - if (DDef->isInitKnownICE()) { - EvaluatedStmt *Eval = MergeWithVar->ensureEvaluatedStmt(); - Eval->CheckedICE = true; - Eval->IsICE = DDef->isInitICE(); - } - } - } - - return MergeWithVar; - } - if (!ConflictingDecls.empty()) { Name = Importer.HandleNameConflict(Name, DC, IDNS, ConflictingDecls.data(), @@ -3255,17 +3264,27 @@ Decl *ASTNodeImporter::VisitVarDecl(VarD ToVar->setAccess(D->getAccess()); ToVar->setLexicalDeclContext(LexicalDC); - // Templated declarations should never appear in the enclosing DeclContext. - if (!D->getDescribedVarTemplate()) - LexicalDC->addDeclInternal(ToVar); + if (FoundByLookup) { + auto *Recent = const_cast<VarDecl *>(FoundByLookup->getMostRecentDecl()); + ToVar->setPreviousDecl(Recent); + } - // Merge the initializer. - if (ImportDefinition(D, ToVar)) + if (ImportInitializer(D, ToVar)) return nullptr; if (D->isConstexpr()) ToVar->setConstexpr(true); + if (D->getDeclContext()->containsDeclAndLoad(D)) + DC->addDeclInternal(ToVar); + if (DC != LexicalDC && D->getLexicalDeclContext()->containsDeclAndLoad(D)) + LexicalDC->addDeclInternal(ToVar); + + // Import the rest of the chain. I.e. import all subsequent declarations. + for (++RedeclIt; RedeclIt != Redecls.end(); ++RedeclIt) + if (!Importer.Import(*RedeclIt)) + return nullptr; + return ToVar; } @@ -4914,12 +4933,7 @@ Decl *ASTNodeImporter::VisitVarTemplateS D2->setAccess(D->getAccess()); } - // NOTE: isThisDeclarationADefinition() can return DeclarationOnly even if - // declaration has initializer. Should this be fixed in the AST?.. Anyway, - // we have to check the declaration for initializer - otherwise, it won't be - // imported. - if ((D->isThisDeclarationADefinition() || D->hasInit()) && - ImportDefinition(D, D2)) + if (ImportInitializer(D, D2)) return nullptr; return D2; @@ -7084,6 +7098,7 @@ Decl *ASTImporter::Import(Decl *FromD) { // Notify subclasses. Imported(FromD, ToD); + updateFlags(FromD, ToD); return ToD; } Modified: cfe/trunk/unittests/AST/ASTImporterTest.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/ASTImporterTest.cpp?rev=342384&r1=342383&r2=342384&view=diff ============================================================================== --- cfe/trunk/unittests/AST/ASTImporterTest.cpp (original) +++ cfe/trunk/unittests/AST/ASTImporterTest.cpp Mon Sep 17 05:04:52 2018 @@ -1828,13 +1828,62 @@ TEST_P(ASTImporterTestBase, ImportDoesUp { Decl *FromTU = getTuDecl("extern int x; int f() { return x; }", Lang_CXX, "input2.cc"); - auto *FromD = - FirstDeclMatcher<FunctionDecl>().match(FromTU, functionDecl()); + auto *FromD = FirstDeclMatcher<FunctionDecl>().match( + FromTU, functionDecl(hasName("f"))); Import(FromD, Lang_CXX); } EXPECT_TRUE(Imported2->isUsed(false)); } +TEST_P(ASTImporterTestBase, ImportDoesUpdateUsedFlag2) { + auto Pattern = varDecl(hasName("x")); + VarDecl *ExistingD; + { + Decl *ToTU = getToTuDecl("int x = 1;", Lang_CXX); + ExistingD = FirstDeclMatcher<VarDecl>().match(ToTU, Pattern); + } + EXPECT_FALSE(ExistingD->isUsed(false)); + { + Decl *FromTU = getTuDecl( + "int x = 1; int f() { return x; }", Lang_CXX, "input1.cc"); + auto *FromD = FirstDeclMatcher<FunctionDecl>().match( + FromTU, functionDecl(hasName("f"))); + Import(FromD, Lang_CXX); + } + EXPECT_TRUE(ExistingD->isUsed(false)); +} + +TEST_P(ASTImporterTestBase, ImportDoesUpdateUsedFlag3) { + auto Pattern = varDecl(hasName("a")); + VarDecl *ExistingD; + { + Decl *ToTU = getToTuDecl( + R"( + struct A { + static const int a = 1; + }; + )", Lang_CXX); + ExistingD = FirstDeclMatcher<VarDecl>().match(ToTU, Pattern); + } + EXPECT_FALSE(ExistingD->isUsed(false)); + { + Decl *FromTU = getTuDecl( + R"( + struct A { + static const int a = 1; + }; + const int *f() { return &A::a; } // requires storage, + // thus used flag will be set + )", Lang_CXX, "input1.cc"); + auto *FromFunD = FirstDeclMatcher<FunctionDecl>().match( + FromTU, functionDecl(hasName("f"))); + auto *FromD = FirstDeclMatcher<VarDecl>().match(FromTU, Pattern); + ASSERT_TRUE(FromD->isUsed(false)); + Import(FromFunD, Lang_CXX); + } + EXPECT_TRUE(ExistingD->isUsed(false)); +} + TEST_P(ASTImporterTestBase, ReimportWithUsedFlag) { auto Pattern = varDecl(hasName("x")); @@ -3244,6 +3293,94 @@ TEST_P(ASTImporterTestBase, InitListExpr EXPECT_TRUE(ToInitExpr->isGLValue()); } +struct ImportVariables : ASTImporterTestBase {}; + +TEST_P(ImportVariables, ImportOfOneDeclBringsInTheWholeChain) { + Decl *FromTU = getTuDecl( + R"( + struct A { + static const int a = 1 + 2; + }; + const int A::a; + )", Lang_CXX, "input1.cc"); + + auto *FromDWithInit = FirstDeclMatcher<VarDecl>().match( + FromTU, varDecl(hasName("a"))); // Decl with init + auto *FromDWithDef = LastDeclMatcher<VarDecl>().match( + FromTU, varDecl(hasName("a"))); // Decl with definition + ASSERT_NE(FromDWithInit, FromDWithDef); + ASSERT_EQ(FromDWithDef->getPreviousDecl(), FromDWithInit); + + auto *ToD0 = cast<VarDecl>(Import(FromDWithInit, Lang_CXX11)); + auto *ToD1 = cast<VarDecl>(Import(FromDWithDef, Lang_CXX11)); + ASSERT_TRUE(ToD0); + ASSERT_TRUE(ToD1); + EXPECT_NE(ToD0, ToD1); + EXPECT_EQ(ToD1->getPreviousDecl(), ToD0); +} + +TEST_P(ImportVariables, InitAndDefinitionAreInDifferentTUs) { + auto StructA = + R"( + struct A { + static const int a = 1 + 2; + }; + )"; + Decl *ToTU = getToTuDecl(StructA, Lang_CXX); + Decl *FromTU = getTuDecl(std::string(StructA) + "const int A::a;", Lang_CXX, + "input1.cc"); + + auto *FromDWithInit = FirstDeclMatcher<VarDecl>().match( + FromTU, varDecl(hasName("a"))); // Decl with init + auto *FromDWithDef = LastDeclMatcher<VarDecl>().match( + FromTU, varDecl(hasName("a"))); // Decl with definition + ASSERT_EQ(FromDWithInit, FromDWithDef->getPreviousDecl()); + ASSERT_TRUE(FromDWithInit->getInit()); + ASSERT_FALSE(FromDWithInit->isThisDeclarationADefinition()); + ASSERT_TRUE(FromDWithDef->isThisDeclarationADefinition()); + ASSERT_FALSE(FromDWithDef->getInit()); + + auto *ToD = FirstDeclMatcher<VarDecl>().match( + ToTU, varDecl(hasName("a"))); // Decl with init + ASSERT_TRUE(ToD->getInit()); + ASSERT_FALSE(ToD->getDefinition()); + + auto *ImportedD = cast<VarDecl>(Import(FromDWithDef, Lang_CXX11)); + EXPECT_TRUE(ImportedD->getAnyInitializer()); + EXPECT_TRUE(ImportedD->getDefinition()); +} + +TEST_P(ImportVariables, InitAndDefinitionAreInTheFromContext) { + auto StructA = + R"( + struct A { + static const int a; + }; + )"; + Decl *ToTU = getToTuDecl(StructA, Lang_CXX); + Decl *FromTU = getTuDecl(std::string(StructA) + "const int A::a = 1 + 2;", + Lang_CXX, "input1.cc"); + + auto *FromDDeclarationOnly = FirstDeclMatcher<VarDecl>().match( + FromTU, varDecl(hasName("a"))); + auto *FromDWithDef = LastDeclMatcher<VarDecl>().match( + FromTU, varDecl(hasName("a"))); // Decl with definition and with init. + ASSERT_EQ(FromDDeclarationOnly, FromDWithDef->getPreviousDecl()); + ASSERT_FALSE(FromDDeclarationOnly->getInit()); + ASSERT_FALSE(FromDDeclarationOnly->isThisDeclarationADefinition()); + ASSERT_TRUE(FromDWithDef->isThisDeclarationADefinition()); + ASSERT_TRUE(FromDWithDef->getInit()); + + auto *ToD = FirstDeclMatcher<VarDecl>().match( + ToTU, varDecl(hasName("a"))); + ASSERT_FALSE(ToD->getInit()); + ASSERT_FALSE(ToD->getDefinition()); + + auto *ImportedD = cast<VarDecl>(Import(FromDWithDef, Lang_CXX11)); + EXPECT_TRUE(ImportedD->getAnyInitializer()); + EXPECT_TRUE(ImportedD->getDefinition()); +} + struct DeclContextTest : ASTImporterTestBase {}; TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) { @@ -3623,5 +3760,11 @@ INSTANTIATE_TEST_CASE_P(ParameterizedTes ImportFunctionTemplateSpecializations, DefaultTestValuesForRunOptions, ); +INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportImplicitMethods, + DefaultTestValuesForRunOptions, ); + +INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportVariables, + DefaultTestValuesForRunOptions, ); + } // end namespace ast_matchers } // end namespace clang _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
