[clang][ASTImporter] Not using primary context in lookup table #118466
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`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.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Balázs Kéri (balazske) Changes
Full diff: https://github.com/llvm/llvm-project/pull/118466.diff 3 Files Affected:
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);
|
balazske
commented
Dec 3, 2024
| @@ -3165,6 +3165,7 @@ ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) { | |||
| if (Error Err = ImportImplicitMethods(DCXX, FoundCXX)) | |||
| return std::move(Err); | |||
| } | |||
| return FoundDef; | |||
There was a problem hiding this comment.
I put this probably into a new pull request or omit this change. But in the current code I do not see any reason why we can not return here. If there is no return, the later GetIimportedOrCreateDecl call should return the existing object. But the imports that are done before the call are not necessary.
|
I have tested the change with CTU analysis and did not see significant difference, and no new crashes (in these tests there was one known instance of unreachable at ASTImporterLookupTable.cpp:121 that is fixed by the change). There were some differences in the found reports, probably explored paths are different. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ASTImporterLookupTabledid use thegetPrimaryContextfunction 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.