Author: Balázs Kéri Date: 2021-06-04T14:24:44+02:00 New Revision: ceb62388f2d8bd8deed447ebfed77ac7d9be293d
URL: https://github.com/llvm/llvm-project/commit/ceb62388f2d8bd8deed447ebfed77ac7d9be293d DIFF: https://github.com/llvm/llvm-project/commit/ceb62388f2d8bd8deed447ebfed77ac7d9be293d.diff LOG: [clang][AST] Set correct DeclContext in ASTImporter lookup table for ParmVarDecl. ParmVarDecl is created with translation unit as the parent DeclContext and later moved to the correct DeclContext. ASTImporterLookupTable should be updated at this move. Reviewed By: martong Differential Revision: https://reviews.llvm.org/D103231 Added: Modified: clang/include/clang/AST/ASTImporterLookupTable.h clang/lib/AST/ASTImporter.cpp clang/lib/AST/ASTImporterLookupTable.cpp clang/unittests/AST/ASTImporterTest.cpp Removed: ################################################################################ diff --git a/clang/include/clang/AST/ASTImporterLookupTable.h b/clang/include/clang/AST/ASTImporterLookupTable.h index 407478a51058d..47dca20338393 100644 --- a/clang/include/clang/AST/ASTImporterLookupTable.h +++ b/clang/include/clang/AST/ASTImporterLookupTable.h @@ -63,8 +63,24 @@ class ASTImporterLookupTable { ASTImporterLookupTable(TranslationUnitDecl &TU); void add(NamedDecl *ND); void remove(NamedDecl *ND); + // Sometimes a declaration is created first with a temporarily value of decl + // context (often the translation unit) and later moved to the final context. + // This happens for declarations that are created before the final declaration + // context. In such cases the lookup table needs to be updated. + // (The declaration is in these cases not added to the temporary decl context, + // only its parent is set.) + // FIXME: It would be better to not add the declaration to the temporary + // context at all in the lookup table, but this requires big change in + // ASTImporter. + // The function should be called when the old context is definitely diff erent + // from the new. + void update(NamedDecl *ND, DeclContext *OldDC); using LookupResult = DeclList; LookupResult lookup(DeclContext *DC, DeclarationName Name) const; + // Check if the `ND` is within the lookup table (with its current name) in + // context `DC`. This is intended for debug purposes when the DeclContext of a + // NamedDecl is changed. + bool contains(DeclContext *DC, NamedDecl *ND) const; void dump(DeclContext *DC) const; void dump() const; }; diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp index c0fc376157d2a..66f384ada1485 100644 --- a/clang/lib/AST/ASTImporter.cpp +++ b/clang/lib/AST/ASTImporter.cpp @@ -3503,6 +3503,8 @@ ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) { for (auto *Param : Parameters) { Param->setOwningFunction(ToFunction); ToFunction->addDeclInternal(Param); + if (ASTImporterLookupTable *LT = Importer.SharedState->getLookupTable()) + LT->update(Param, Importer.getToContext().getTranslationUnitDecl()); } ToFunction->setParams(Parameters); diff --git a/clang/lib/AST/ASTImporterLookupTable.cpp b/clang/lib/AST/ASTImporterLookupTable.cpp index e17d6082dcdcc..b78cc0c053f60 100644 --- a/clang/lib/AST/ASTImporterLookupTable.cpp +++ b/clang/lib/AST/ASTImporterLookupTable.cpp @@ -117,6 +117,19 @@ void ASTImporterLookupTable::remove(NamedDecl *ND) { remove(ReDC, ND); } +void ASTImporterLookupTable::update(NamedDecl *ND, DeclContext *OldDC) { + assert(OldDC != ND->getDeclContext() && + "DeclContext should be changed before update"); + if (contains(ND->getDeclContext(), ND)) { + assert(!contains(OldDC, ND) && + "Decl should not be found in the old context if already in the new"); + return; + } + + remove(OldDC, ND); + add(ND); +} + ASTImporterLookupTable::LookupResult ASTImporterLookupTable::lookup(DeclContext *DC, DeclarationName Name) const { auto DCI = LookupTable.find(DC->getPrimaryContext()); @@ -131,6 +144,10 @@ ASTImporterLookupTable::lookup(DeclContext *DC, DeclarationName Name) const { return NamesI->second; } +bool ASTImporterLookupTable::contains(DeclContext *DC, NamedDecl *ND) const { + return 0 < lookup(DC, ND->getDeclName()).count(ND); +} + void ASTImporterLookupTable::dump(DeclContext *DC) const { auto DCI = LookupTable.find(DC->getPrimaryContext()); if (DCI == LookupTable.end()) diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp index db87b2ab5e9c0..4ed4a09b03b9a 100644 --- a/clang/unittests/AST/ASTImporterTest.cpp +++ b/clang/unittests/AST/ASTImporterTest.cpp @@ -5375,6 +5375,33 @@ TEST_P(ErrorHandlingTest, ImportOfOverriddenMethods) { CheckError(FromFooC); } +TEST_P(ErrorHandlingTest, ODRViolationWithinParmVarDecls) { + // Importing of 'f' and parameter 'P' should cause an ODR error. + // The error happens after the ParmVarDecl for 'P' was already created. + // This is a special case because the ParmVarDecl has a temporary DeclContext. + // Expected is no crash at error handling of ASTImporter. + constexpr auto ToTUCode = R"( + struct X { + char A; + }; + )"; + constexpr auto FromTUCode = R"( + struct X { + enum Y { Z }; + }; + void f(int P = X::Z); + )"; + Decl *ToTU = getToTuDecl(ToTUCode, Lang_CXX11); + static_cast<void>(ToTU); + Decl *FromTU = getTuDecl(FromTUCode, Lang_CXX11); + auto *FromF = FirstDeclMatcher<FunctionDecl>().match( + FromTU, functionDecl(hasName("f"))); + ASSERT_TRUE(FromF); + + auto *ImportedF = Import(FromF, Lang_CXX11); + EXPECT_FALSE(ImportedF); +} + TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) { Decl *FromTU = getTuDecl( R"( @@ -6192,6 +6219,21 @@ TEST_P(ImportFunctions, CTADWithLocalTypedef) { ASSERT_TRUE(ToD); } +TEST_P(ImportFunctions, ParmVarDeclDeclContext) { + constexpr auto FromTUCode = R"( + void f(int P); + )"; + Decl *FromTU = getTuDecl(FromTUCode, Lang_CXX11); + auto *FromF = FirstDeclMatcher<FunctionDecl>().match( + FromTU, functionDecl(hasName("f"))); + ASSERT_TRUE(FromF); + + auto *ImportedF = Import(FromF, Lang_CXX11); + EXPECT_TRUE(ImportedF); + EXPECT_TRUE(SharedStatePtr->getLookupTable()->contains( + ImportedF, ImportedF->getParamDecl(0))); +} + // FIXME Move these tests out of ASTImporterTest. For that we need to factor // out the ASTImporter specific pars from ASTImporterOptionSpecificTestBase // into a new test Fixture. Then we should lift up this Fixture to its own _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
