[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
clang/lib/AST/ASTImporter.cpp
Outdated
| @@ -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.
There was a problem hiding this comment.
So currently what happens is that we create a new decl and connect it to FoundDef on a redeclaration chain? LLDB is particularly sensitive to this particular code-path, so I'd say it's best to try do this in a separate PR so we can focus in on this
There was a problem hiding this comment.
The later GetImportedOrCreateDecl returns the "already imported" declaration that was set by the MapImported (line 3157) call. So in this case we should return on line 3227.
|
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. |
Michael137
reviewed
Dec 18, 2024
There was a problem hiding this comment.
LLDB doesn't use the LookupTable infrastructure here so my understanding of this is limited. Generally the idea makes sense to me. Just left some clarification questions for my own understanding.
Does this have any affect on other redeclarable DeclContexts such as class forward declarations?
| @@ -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()); | |||
There was a problem hiding this comment.
So prior to your change this lookup would've found A? Because the LookupTable would unconditionally use the primary context?
There was a problem hiding this comment.
Yes, previously always the primary context was used.
(It is possible to use the primary context in the lookup table only for namespaces, not for other decls. But this exception makes the lookup table more complicated.)
| auto *FromX = FirstDeclMatcher<CXXRecordDecl>().match( | ||
| FromTU, cxxRecordDecl(hasName("X"))); | ||
| auto *ImportedX = Import(FromX, Lang_CXX11); | ||
| EXPECT_FALSE(ImportedX); |
There was a problem hiding this comment.
So this would've succeeded in the past? I guess because the primary context would've just return the empty namespace a of the ToTU, so the importer didn't see any conflicts. Am I understanding this correctly?
There was a problem hiding this comment.
This test should pass in the previous code too. This is because all of the declarations are added to the primary context at the lookup table (before the changes), struct X should be added to a already in the ToTU. This test is added to ensure that it works with the new code too (the change in ASTImporter::findDeclsInToContext is added for this to work).
| auto *ToX = FirstDeclMatcher<CXXRecordDecl>().match( | ||
| ToTU, cxxRecordDecl(hasName("X"))); | ||
| auto *ImportedX = Import(FromX, Lang_CXX11); | ||
| EXPECT_EQ(ImportedX, ToX); |
There was a problem hiding this comment.
Would this have failed in the past?
There was a problem hiding this comment.
I think not, for the same reason as already explained.
There was a problem hiding this comment.
The last test is the ones that fails before the changes. The template parameter objects have the record declaration (of the template) as DeclContext and in that case a definition of it is imported. This results in change of the primary context (even for first struct X in the ToTU) and the lookup table will contain class T template parameter in the wrong place.
In the code of |
Michael137
approved these changes
Jan 10, 2025
Mel-Chen
pushed a commit
to Mel-Chen/llvm-project
that referenced
this pull request
Jan 13, 2025
…118466) `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.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/11589 Here is the relevant piece of the build log for the reference |
kazutakahirata
pushed a commit
to kazutakahirata/llvm-project
that referenced
this pull request
Jan 13, 2025
…118466) `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.
DKLoehr
pushed a commit
to DKLoehr/llvm-project
that referenced
this pull request
Jan 17, 2025
…118466) `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.
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.