intel/llvm
1
0
Fork 0
mirror of https://github.com/intel/llvm.git synced 2026-01-23 07:58:23 +08:00
Code Issues Packages Projects Releases Wiki Activity

Revert "[Modules] Remove unnecessary check when generating name lookup table in ASTWriter"

Browse Source 
This reverts commit bc95f27337, originally db987b9589.
clang/test/Modules/pr61065.cppm is retained to make relands show less diff.

There are other module-related issues that were not caught, related to
false positive errors like
"error: no matching constructor for initialization of 'union (anonymous union at ..."

Reopen #61065
This commit is contained in:
Fangrui Song
2023-04-13 11:17:35 -07:00
parent 42c2fa62c8
commit 363c98b2d6
3 changed files with 72 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
clang/include/clang/Serialization/ASTWriter.h
View File

@@ -514,6 +514,7 @@ private:
void WriteTypeAbbrevs();
void WriteType(QualType T);
bool isLookupResultExternal(StoredDeclsList &Result, DeclContext *DC);
bool isLookupResultEntirelyExternal(StoredDeclsList &Result, DeclContext *DC);
void GenerateNameLookupTable(const DeclContext *DC,

102
clang/lib/Serialization/ASTWriter.cpp
View File

@@ -3849,6 +3849,12 @@ public:
} // namespace
bool ASTWriter::isLookupResultExternal(StoredDeclsList &Result,
DeclContext *DC) {
return Result.hasExternalDecls() &&
DC->hasNeedToReconcileExternalVisibleStorage();
}
bool ASTWriter::isLookupResultEntirelyExternal(StoredDeclsList &Result,
DeclContext *DC) {
for (auto *D : Result.getLookupResult())
@@ -3879,17 +3885,20 @@ ASTWriter::GenerateNameLookupTable(const DeclContext *ConstDC,
// order.
SmallVector<DeclarationName, 16> Names;
// We also track whether we're writing out the DeclarationNameKey for
// constructors or conversion functions.
bool IncludeConstructorNames = false;
bool IncludeConversionNames = false;
// We also build up small sets of the constructor and conversion function
// names which are visible.
llvm::SmallPtrSet<DeclarationName, 8> ConstructorNameSet, ConversionNameSet;
for (auto &Lookup : *DC->buildLookup()) {
auto &Name = Lookup.first;
auto &Result = Lookup.second;
for (auto &[Name, Result] : *DC->buildLookup()) {
// If there are no local declarations in our lookup result, we
// don't need to write an entry for the name at all. If we can't
// write out a lookup set without performing more deserialization,
// just skip this entry.
if (isLookupResultEntirelyExternal(Result, DC))
if (isLookupResultExternal(Result, DC) &&
isLookupResultEntirelyExternal(Result, DC))
continue;
// We also skip empty results. If any of the results could be external and
@@ -3906,20 +3915,24 @@ ASTWriter::GenerateNameLookupTable(const DeclContext *ConstDC,
// results for them. This in almost certainly a bug in Clang's name lookup,
// but that is likely to be hard or impossible to fix and so we tolerate it
// here by omitting lookups with empty results.
if (Result.getLookupResult().empty())
if (Lookup.second.getLookupResult().empty())
continue;
switch (Name.getNameKind()) {
switch (Lookup.first.getNameKind()) {
default:
Names.push_back(Name);
Names.push_back(Lookup.first);
break;
case DeclarationName::CXXConstructorName:
IncludeConstructorNames = true;
assert(isa<CXXRecordDecl>(DC) &&
"Cannot have a constructor name outside of a class!");
ConstructorNameSet.insert(Name);
break;
case DeclarationName::CXXConversionFunctionName:
IncludeConversionNames = true;
assert(isa<CXXRecordDecl>(DC) &&
"Cannot have a conversion function name outside of a class!");
ConversionNameSet.insert(Name);
break;
}
}
@@ -3927,34 +3940,55 @@ ASTWriter::GenerateNameLookupTable(const DeclContext *ConstDC,
// Sort the names into a stable order.
llvm::sort(Names);
if (IncludeConstructorNames || IncludeConversionNames) {
if (auto *D = dyn_cast<CXXRecordDecl>(DC)) {
// We need to establish an ordering of constructor and conversion function
// names, and they don't have an intrinsic ordering. We also need to write
// out all constructor and conversion function results if we write out any
// of them, because they're all tracked under the same lookup key.
llvm::SmallPtrSet<DeclarationName, 8> AddedNames;
for (Decl *ChildD : cast<CXXRecordDecl>(DC)->decls()) {
if (auto *ChildND = dyn_cast<NamedDecl>(ChildD)) {
auto Name = ChildND->getDeclName();
switch (Name.getNameKind()) {
default:
continue;
// names, and they don't have an intrinsic ordering.
case DeclarationName::CXXConstructorName:
if (!IncludeConstructorNames)
continue;
break;
// First we try the easy case by forming the current context's constructor
// name and adding that name first. This is a very useful optimization to
// avoid walking the lexical declarations in many cases, and it also
// handles the only case where a constructor name can come from some other
// lexical context -- when that name is an implicit constructor merged from
// another declaration in the redecl chain. Any non-implicit constructor or
// conversion function which doesn't occur in all the lexical contexts
// would be an ODR violation.
auto ImplicitCtorName = Context->DeclarationNames.getCXXConstructorName(
Context->getCanonicalType(Context->getRecordType(D)));
if (ConstructorNameSet.erase(ImplicitCtorName))
Names.push_back(ImplicitCtorName);
case DeclarationName::CXXConversionFunctionName:
if (!IncludeConversionNames)
// If we still have constructors or conversion functions, we walk all the
// names in the decl and add the constructors and conversion functions
// which are visible in the order they lexically occur within the context.
if (!ConstructorNameSet.empty() || !ConversionNameSet.empty())
for (Decl *ChildD : cast<CXXRecordDecl>(DC)->decls())
if (auto *ChildND = dyn_cast<NamedDecl>(ChildD)) {
auto Name = ChildND->getDeclName();
switch (Name.getNameKind()) {
default:
continue;
break;
case DeclarationName::CXXConstructorName:
if (ConstructorNameSet.erase(Name))
Names.push_back(Name);
break;
case DeclarationName::CXXConversionFunctionName:
if (ConversionNameSet.erase(Name))
Names.push_back(Name);
break;
}
if (ConstructorNameSet.empty() && ConversionNameSet.empty())
break;
}
// We should include lookup results for this name.
if (AddedNames.insert(Name).second)
Names.push_back(Name);
}
}
assert(ConstructorNameSet.empty() && "Failed to find all of the visible "
"constructors by walking all the "
"lexical members of the context.");
assert(ConversionNameSet.empty() && "Failed to find all of the visible "
"conversion functions by walking all "
"the lexical members of the context.");
}
// Next we need to do a lookup with each name into this decl context to fully

6
clang/test/Modules/pr61065.cppm
View File

@@ -6,9 +6,9 @@
// RUN: %clang_cc1 -std=c++20 %t/a.cppm -emit-module-interface -o %t/a.pcm
// RUN: %clang_cc1 -std=c++20 %t/b.cppm -emit-module-interface -o %t/b.pcm \
// RUN: -fprebuilt-module-path=%t
// RUN: %clang_cc1 -std=c++20 %t/c.cppm -emit-module-interface -o %t/c.pcm \
// RUN: -fprebuilt-module-path=%t
// RUN: %clang_cc1 -std=c++20 %t/d.cpp -fsyntax-only -verify -fprebuilt-module-path=%t
// DISABLED: %clang_cc1 -std=c++20 %t/c.cppm -emit-module-interface -o %t/c.pcm \
// DISABLED: -fprebuilt-module-path=%t
// DISABLED: %clang_cc1 -std=c++20 %t/d.cpp -fsyntax-only -verify -fprebuilt-module-path=%t
//--- a.cppm
export module a;