[clang] [clang][ASTImporter] Not using primary context in lookup table (PR #118466)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 3 02:43:59 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Balázs Kéri (balazske)
<details>
<summary>Changes</summary>
`ASTImporterLookupTable` did use the `getPrimaryContext` function to get the declaration context of the inserted items. This is problematic because the primary context can change during import of AST items, most likely if a definition of a previously not defined class is imported. (For any record the primary context is the definition if there is one.) The use of primary context is really not important, only for namespaces because these can be re-opened and lookup in one namespace block is not enough. This special search is now moved into ASTImporter instead of relying on the lookup table.
---
Full diff: https://github.com/llvm/llvm-project/pull/118466.diff
3 Files Affected:
- (modified) clang/lib/AST/ASTImporter.cpp (+21-3)
- (modified) clang/lib/AST/ASTImporterLookupTable.cpp (+10-10)
- (modified) clang/unittests/AST/ASTImporterTest.cpp (+150-2)
``````````diff
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index a0cd57e2e5ee0d..34b2d8e3696df5 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -3165,6 +3165,7 @@ ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
if (Error Err = ImportImplicitMethods(DCXX, FoundCXX))
return std::move(Err);
}
+ return FoundDef;
}
PrevDecl = FoundRecord->getMostRecentDecl();
break;
@@ -9052,9 +9053,26 @@ ASTImporter::findDeclsInToCtx(DeclContext *DC, DeclarationName Name) {
// We can diagnose this only if we search in the redecl context.
DeclContext *ReDC = DC->getRedeclContext();
if (SharedState->getLookupTable()) {
- ASTImporterLookupTable::LookupResult LookupResult =
- SharedState->getLookupTable()->lookup(ReDC, Name);
- return FoundDeclsTy(LookupResult.begin(), LookupResult.end());
+ if (ReDC->isNamespace()) {
+ // Namespaces can be reopened.
+ // Lookup table does not handle this, we must search here in all linked
+ // namespaces.
+ FoundDeclsTy Result;
+ SmallVector<Decl *, 2> NSChain =
+ getCanonicalForwardRedeclChain<NamespaceDecl>(
+ dyn_cast<NamespaceDecl>(ReDC));
+ for (auto *D : NSChain) {
+ ASTImporterLookupTable::LookupResult LookupResult =
+ SharedState->getLookupTable()->lookup(dyn_cast<NamespaceDecl>(D),
+ Name);
+ Result.append(LookupResult.begin(), LookupResult.end());
+ }
+ return Result;
+ } else {
+ ASTImporterLookupTable::LookupResult LookupResult =
+ SharedState->getLookupTable()->lookup(ReDC, Name);
+ return FoundDeclsTy(LookupResult.begin(), LookupResult.end());
+ }
} else {
DeclContext::lookup_result NoloadLookupResult = ReDC->noload_lookup(Name);
FoundDeclsTy Result(NoloadLookupResult.begin(), NoloadLookupResult.end());
diff --git a/clang/lib/AST/ASTImporterLookupTable.cpp b/clang/lib/AST/ASTImporterLookupTable.cpp
index 07d39dcee2583a..4ed3198d7ea62b 100644
--- a/clang/lib/AST/ASTImporterLookupTable.cpp
+++ b/clang/lib/AST/ASTImporterLookupTable.cpp
@@ -115,8 +115,9 @@ void ASTImporterLookupTable::remove(DeclContext *DC, NamedDecl *ND) {
#ifndef NDEBUG
if (!EraseResult) {
std::string Message =
- llvm::formatv("Trying to remove not contained Decl '{0}' of type {1}",
- Name.getAsString(), DC->getDeclKindName())
+ llvm::formatv(
+ "Trying to remove not contained Decl '{0}' of type {1} from a {2}",
+ Name.getAsString(), ND->getDeclKindName(), DC->getDeclKindName())
.str();
llvm_unreachable(Message.c_str());
}
@@ -125,18 +126,18 @@ void ASTImporterLookupTable::remove(DeclContext *DC, NamedDecl *ND) {
void ASTImporterLookupTable::add(NamedDecl *ND) {
assert(ND);
- DeclContext *DC = ND->getDeclContext()->getPrimaryContext();
+ DeclContext *DC = ND->getDeclContext();
add(DC, ND);
- DeclContext *ReDC = DC->getRedeclContext()->getPrimaryContext();
+ DeclContext *ReDC = DC->getRedeclContext();
if (DC != ReDC)
add(ReDC, ND);
}
void ASTImporterLookupTable::remove(NamedDecl *ND) {
assert(ND);
- DeclContext *DC = ND->getDeclContext()->getPrimaryContext();
+ DeclContext *DC = ND->getDeclContext();
remove(DC, ND);
- DeclContext *ReDC = DC->getRedeclContext()->getPrimaryContext();
+ DeclContext *ReDC = DC->getRedeclContext();
if (DC != ReDC)
remove(ReDC, ND);
}
@@ -161,7 +162,7 @@ void ASTImporterLookupTable::updateForced(NamedDecl *ND, DeclContext *OldDC) {
ASTImporterLookupTable::LookupResult
ASTImporterLookupTable::lookup(DeclContext *DC, DeclarationName Name) const {
- auto DCI = LookupTable.find(DC->getPrimaryContext());
+ auto DCI = LookupTable.find(DC);
if (DCI == LookupTable.end())
return {};
@@ -178,7 +179,7 @@ bool ASTImporterLookupTable::contains(DeclContext *DC, NamedDecl *ND) const {
}
void ASTImporterLookupTable::dump(DeclContext *DC) const {
- auto DCI = LookupTable.find(DC->getPrimaryContext());
+ auto DCI = LookupTable.find(DC);
if (DCI == LookupTable.end())
llvm::errs() << "empty\n";
const auto &FoundNameMap = DCI->second;
@@ -196,8 +197,7 @@ void ASTImporterLookupTable::dump(DeclContext *DC) const {
void ASTImporterLookupTable::dump() const {
for (const auto &Entry : LookupTable) {
DeclContext *DC = Entry.first;
- StringRef Primary = DC->getPrimaryContext() ? " primary" : "";
- llvm::errs() << "== DC:" << cast<Decl>(DC) << Primary << "\n";
+ llvm::errs() << "== DC:" << cast<Decl>(DC) << "\n";
dump(DC);
}
}
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index bf7313f882e455..ed93856a27b41c 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -6063,7 +6063,7 @@ TEST_P(ASTImporterLookupTableTest, EnumConstantDecl) {
EXPECT_EQ(*Res.begin(), A);
}
-TEST_P(ASTImporterLookupTableTest, LookupSearchesInTheWholeRedeclChain) {
+TEST_P(ASTImporterLookupTableTest, LookupSearchesInActualNamespaceOnly) {
TranslationUnitDecl *ToTU = getToTuDecl(
R"(
namespace N {
@@ -6073,7 +6073,9 @@ TEST_P(ASTImporterLookupTableTest, LookupSearchesInTheWholeRedeclChain) {
}
)",
Lang_CXX03);
- auto *N1 =
+ auto *N1 = FirstDeclMatcher<NamespaceDecl>().match(
+ ToTU, namespaceDecl(hasName("N")));
+ auto *N2 =
LastDeclMatcher<NamespaceDecl>().match(ToTU, namespaceDecl(hasName("N")));
auto *A = FirstDeclMatcher<VarDecl>().match(ToTU, varDecl(hasName("A")));
DeclarationName Name = A->getDeclName();
@@ -6082,6 +6084,7 @@ TEST_P(ASTImporterLookupTableTest, LookupSearchesInTheWholeRedeclChain) {
auto Res = LT.lookup(N1, Name);
ASSERT_EQ(Res.size(), 1u);
EXPECT_EQ(*Res.begin(), A);
+ EXPECT_TRUE(LT.lookup(N2, Name).empty());
}
TEST_P(ASTImporterOptionSpecificTestBase,
@@ -10181,6 +10184,151 @@ TEST_P(ImportTemplateParmDeclDefaultValue,
FromD, FromDInherited);
}
+TEST_P(ASTImporterOptionSpecificTestBase, ImportIntoReopenedNamespaceNoMatch1) {
+ const char *ToCode =
+ R"(
+ namespace a {
+ }
+ namespace a {
+ struct X { int A; };
+ }
+ )";
+ Decl *ToTU = getToTuDecl(ToCode, Lang_CXX11);
+ const char *Code =
+ R"(
+ namespace a {
+ struct X { char A; };
+ }
+ )";
+ Decl *FromTU = getTuDecl(Code, Lang_CXX11);
+ auto *FromX = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("X")));
+ auto *ImportedX = Import(FromX, Lang_CXX11);
+ EXPECT_FALSE(ImportedX);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, ImportIntoReopenedNamespaceNoMatch2) {
+ const char *ToCode =
+ R"(
+ namespace a {
+ struct X { int A; };
+ }
+ namespace a {
+ }
+ )";
+ Decl *ToTU = getToTuDecl(ToCode, Lang_CXX11);
+ const char *Code =
+ R"(
+ namespace a {
+ struct X { char A; };
+ }
+ )";
+ Decl *FromTU = getTuDecl(Code, Lang_CXX11);
+ auto *FromX = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("X")));
+ auto *ImportedX = Import(FromX, Lang_CXX11);
+ EXPECT_FALSE(ImportedX);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, ImportIntoReopenedNamespaceMatch1) {
+ const char *ToCode =
+ R"(
+ namespace a {
+ }
+ namespace a {
+ struct X { int A; };
+ }
+ )";
+ Decl *ToTU = getToTuDecl(ToCode, Lang_CXX11);
+ const char *Code =
+ R"(
+ namespace a {
+ struct X { int A; };
+ }
+ )";
+ Decl *FromTU = getTuDecl(Code, Lang_CXX11);
+ auto *FromX = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("X")));
+ auto *ToX = FirstDeclMatcher<CXXRecordDecl>().match(
+ ToTU, cxxRecordDecl(hasName("X")));
+ auto *ImportedX = Import(FromX, Lang_CXX11);
+ EXPECT_EQ(ImportedX, ToX);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, ImportIntoReopenedNamespaceMatch2) {
+ const char *ToCode =
+ R"(
+ namespace a {
+ struct X { int A; };
+ }
+ namespace a {
+ }
+ )";
+ Decl *ToTU = getToTuDecl(ToCode, Lang_CXX11);
+ const char *Code =
+ R"(
+ namespace a {
+ struct X { int A; };
+ }
+ )";
+ Decl *FromTU = getTuDecl(Code, Lang_CXX11);
+ auto *FromX = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("X")));
+ auto *ToX = FirstDeclMatcher<CXXRecordDecl>().match(
+ ToTU, cxxRecordDecl(hasName("X")));
+ auto *ImportedX = Import(FromX, Lang_CXX11);
+ EXPECT_EQ(ImportedX, ToX);
+}
+
+TEST_P(ASTImporterLookupTableTest, PrimaryDCChangeAtImport) {
+ const char *ToCode =
+ R"(
+ template <class T>
+ struct X;
+ )";
+ Decl *ToTU = getToTuDecl(ToCode, Lang_CXX11);
+ auto *ToX = FirstDeclMatcher<ClassTemplateDecl>().match(
+ ToTU, classTemplateDecl(hasName("X")));
+ NamedDecl *ToParm = ToX->getTemplateParameters()->getParam(0);
+ DeclContext *OldPrimaryDC = ToX->getTemplatedDecl()->getPrimaryContext();
+ ASSERT_EQ(ToParm->getDeclContext(), ToX->getTemplatedDecl());
+ ASSERT_EQ(SharedStatePtr->getLookupTable()
+ ->lookup(ToX->getTemplatedDecl(), ToParm->getDeclName())
+ .size(),
+ 1u);
+ ASSERT_TRUE(SharedStatePtr->getLookupTable()->contains(
+ ToX->getTemplatedDecl(), ToParm));
+
+ const char *Code =
+ R"(
+ template <class T>
+ struct X;
+ template <class T>
+ struct X {};
+ )";
+ Decl *FromTU = getTuDecl(Code, Lang_CXX11);
+ auto *FromX = LastDeclMatcher<ClassTemplateDecl>().match(
+ FromTU, classTemplateDecl(hasName("X")));
+
+ auto *ImportedX = Import(FromX, Lang_CXX11);
+
+ EXPECT_TRUE(ImportedX);
+ EXPECT_EQ(ImportedX->getTemplateParameters()->getParam(0)->getDeclContext(),
+ ImportedX->getTemplatedDecl());
+
+ // ToX did not change at the import.
+ // Verify that primary context has changed after import of class definition.
+ DeclContext *NewPrimaryDC = ToX->getTemplatedDecl()->getPrimaryContext();
+ EXPECT_NE(OldPrimaryDC, NewPrimaryDC);
+ // The lookup table should not be different than it was before.
+ EXPECT_EQ(SharedStatePtr->getLookupTable()
+ ->lookup(ToX->getTemplatedDecl(), ToParm->getDeclName())
+ .size(),
+ 1u);
+ EXPECT_TRUE(SharedStatePtr->getLookupTable()->contains(
+ ToX->getTemplatedDecl(), ToParm));
+}
+
INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
DefaultTestValuesForRunOptions);
``````````
</details>
https://github.com/llvm/llvm-project/pull/118466
More information about the cfe-commits
mailing list