[clang] ceb6238 - [clang][AST] Set correct DeclContext in ASTImporter lookup table for ParmVarDecl.
Balázs Kéri via cfe-commits
cfe-commits at lists.llvm.org
Fri Jun 4 05:14:22 PDT 2021
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
More information about the cfe-commits
mailing list