Skip to content

[clang][ASTImporter] Not using primary context in lookup table #118466

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 13, 2025
Merged

[clang][ASTImporter] Not using primary context in lookup table #118466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e97972c
[clang][ASTImporter] Not using primary context in lookup table
balazske Nov 29, 2024
3034b98
Merge branch 'main' into astimporter_lookuptable_noprimary
balazske Dec 18, 2024
d5cc73a
remove 'return FoundDef' from code
balazske Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions clang/lib/AST/ASTImporter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3165,6 +3165,7 @@ ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
if (Error Err = ImportImplicitMethods(DCXX, FoundCXX))
return std::move(Err);
}
// FIXME: We can return FoundDef here.
}
PrevDecl = FoundRecord->getMostRecentDecl();
break;
Expand Down Expand Up @@ -9064,9 +9065,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());
Expand Down
20 changes: 10 additions & 10 deletions clang/lib/AST/ASTImporterLookupTable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand All @@ -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);
}
Expand All @@ -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 {};

Expand All @@ -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;
Expand All @@ -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);
}
}
Expand Down
152 changes: 150 additions & 2 deletions clang/unittests/AST/ASTImporterTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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();
Expand All @@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So prior to your change this lookup would've found A? Because the LookupTable would unconditionally use the primary context?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

}

TEST_P(ASTImporterOptionSpecificTestBase,
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@balazske balazske Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this have failed in the past?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not, for the same reason as already explained.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

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));
}

TEST_P(ASTImporterOptionSpecificTestBase,
ExistingUndeclaredImportDeclaredFriend) {
Decl *ToTU = getToTuDecl(
Expand Down
Loading