[clang] bc95f27 - Revert "Revert "[Modules] Remove unnecessary check when generating name lookup table in ASTWriter""
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 6 15:42:11 PDT 2023
Author: Richard Smith
Date: 2023-04-06T15:42:00-07:00
New Revision: bc95f27337c7ed77c28e713c855272848f01802a
URL: https://github.com/llvm/llvm-project/commit/bc95f27337c7ed77c28e713c855272848f01802a
DIFF: https://github.com/llvm/llvm-project/commit/bc95f27337c7ed77c28e713c855272848f01802a.diff
LOG: Revert "Revert "[Modules] Remove unnecessary check when generating name lookup table in ASTWriter""
This re-lands commit db987b9589be1eb604fcb74c85b410469e31485f,
previously reverted in commit 1e0709167f5edd330889f51bb203c458bdb5e359,
with a fix for a pre-existing bug in how we write constructor names into
lookup tables that was exposed by that change.
As yet, we've been unable to produce a reduced testcase for this bug,
but we understand the problem: if we write out a lookup table for a
class which has two constructor names, and we only have local lookup
results for one of those constructor names, then we will write out a
lookup table where the "all the constructors" lookup result contains
results for only one of the constructor names.
Added:
clang/test/Modules/pr61065.cppm
Modified:
clang/include/clang/Serialization/ASTWriter.h
clang/lib/Serialization/ASTWriter.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Serialization/ASTWriter.h b/clang/include/clang/Serialization/ASTWriter.h
index 09ee1744e8945..d31fa38b93825 100644
--- a/clang/include/clang/Serialization/ASTWriter.h
+++ b/clang/include/clang/Serialization/ASTWriter.h
@@ -514,7 +514,6 @@ class ASTWriter : public ASTDeserializationListener,
void WriteTypeAbbrevs();
void WriteType(QualType T);
- bool isLookupResultExternal(StoredDeclsList &Result, DeclContext *DC);
bool isLookupResultEntirelyExternal(StoredDeclsList &Result, DeclContext *DC);
void GenerateNameLookupTable(const DeclContext *DC,
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 245304254811a..029f30416d612 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -3849,12 +3849,6 @@ class ASTDeclContextNameLookupTrait {
} // 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())
@@ -3885,20 +3879,17 @@ ASTWriter::GenerateNameLookupTable(const DeclContext *ConstDC,
// order.
SmallVector<DeclarationName, 16> Names;
- // 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;
+ // We also track whether we're writing out the DeclarationNameKey for
+ // constructors or conversion functions.
+ bool IncludeConstructorNames = false;
+ bool IncludeConversionNames = false;
+ 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 (isLookupResultExternal(Result, DC) &&
- isLookupResultEntirelyExternal(Result, DC))
+ if (isLookupResultEntirelyExternal(Result, DC))
continue;
// We also skip empty results. If any of the results could be external and
@@ -3915,24 +3906,20 @@ 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 (Lookup.second.getLookupResult().empty())
+ if (Result.getLookupResult().empty())
continue;
- switch (Lookup.first.getNameKind()) {
+ switch (Name.getNameKind()) {
default:
- Names.push_back(Lookup.first);
+ Names.push_back(Name);
break;
case DeclarationName::CXXConstructorName:
- assert(isa<CXXRecordDecl>(DC) &&
- "Cannot have a constructor name outside of a class!");
- ConstructorNameSet.insert(Name);
+ IncludeConstructorNames = true;
break;
case DeclarationName::CXXConversionFunctionName:
- assert(isa<CXXRecordDecl>(DC) &&
- "Cannot have a conversion function name outside of a class!");
- ConversionNameSet.insert(Name);
+ IncludeConversionNames = true;
break;
}
}
@@ -3940,55 +3927,34 @@ ASTWriter::GenerateNameLookupTable(const DeclContext *ConstDC,
// Sort the names into a stable order.
llvm::sort(Names);
- if (auto *D = dyn_cast<CXXRecordDecl>(DC)) {
+ if (IncludeConstructorNames || IncludeConversionNames) {
// We need to establish an ordering of constructor and conversion function
- // names, and they don't have an intrinsic ordering.
-
- // 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);
-
- // 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;
-
- case DeclarationName::CXXConstructorName:
- if (ConstructorNameSet.erase(Name))
- Names.push_back(Name);
- break;
+ // 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;
- case DeclarationName::CXXConversionFunctionName:
- if (ConversionNameSet.erase(Name))
- Names.push_back(Name);
- break;
- }
+ case DeclarationName::CXXConstructorName:
+ if (!IncludeConstructorNames)
+ continue;
+ break;
- if (ConstructorNameSet.empty() && ConversionNameSet.empty())
- break;
+ case DeclarationName::CXXConversionFunctionName:
+ if (!IncludeConversionNames)
+ continue;
+ break;
}
-
- 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.");
+ // We should include lookup results for this name.
+ if (AddedNames.insert(Name).second)
+ Names.push_back(Name);
+ }
+ }
}
// Next we need to do a lookup with each name into this decl context to fully
diff --git a/clang/test/Modules/pr61065.cppm b/clang/test/Modules/pr61065.cppm
new file mode 100644
index 0000000000000..44fa3679974ad
--- /dev/null
+++ b/clang/test/Modules/pr61065.cppm
@@ -0,0 +1,55 @@
+// From https://github.com/llvm/llvm-project/issues/61065
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// 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
+
+//--- a.cppm
+export module a;
+
+struct base {
+ base(int) {}
+};
+
+export struct a : base {
+ using base::base;
+};
+
+//--- b.cppm
+export module b;
+
+import a;
+
+a b() {
+ return a(1);
+}
+
+//--- c.cppm
+export module c;
+
+import a;
+import b;
+
+struct noncopyable {
+ noncopyable(noncopyable const &) = delete;
+ noncopyable() = default;
+};
+
+export struct c {
+ noncopyable c0;
+ a c1 = 43;
+ c() = default;
+};
+
+//--- d.cpp
+// expected-no-diagnostics
+import c;
+void d() {
+ c _;
+}
More information about the cfe-commits
mailing list