Skip to content

Commit

Permalink
[clang][ASTImporter] Not using primary context in lookup table (#118466)
Browse files Browse the repository at this point in the history 
`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.
  • Loading branch information
@balazske @Mel-Chen
balazske authored and Mel-Chen committed Jan 13, 2025
1 parent 8a0117d commit 41a42d6
Show file tree
Hide file tree
Showing 3 changed files with 181 additions and 15 deletions.
24 changes: 21 additions & 3 deletions clang/lib/AST/ASTImporter.cpp
View file
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -6052,7 +6052,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 @@ -6062,7 +6062,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 @@ -6071,6 +6073,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,
Expand Down Expand Up @@ -10170,6 +10173,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));
}

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

0 comments on commit 41a42d6

Please sign in to comment.