diff --git a/clang/include/clang/Serialization/ASTWriter.h b/clang/include/clang/Serialization/ASTWriter.h index d3029373ed2f7..1c4025f7482d9 100644 --- a/clang/include/clang/Serialization/ASTWriter.h +++ b/clang/include/clang/Serialization/ASTWriter.h @@ -773,13 +773,18 @@ class ASTWriter : public ASTDeserializationListener, /// Is this a local declaration (that is, one that will be written to /// our AST file)? This is the case for declarations that are neither imported /// from another AST file nor predefined. - bool IsLocalDecl(const Decl *D) { + bool IsLocalDecl(const Decl *D) const { if (D->isFromASTFile()) return false; auto I = DeclIDs.find(D); return (I == DeclIDs.end() || I->second >= clang::NUM_PREDEF_DECL_IDS); }; + /// Collect the first declaration from each module file that provides a + /// declaration of D. + llvm::MapVector + CollectFirstDeclFromEachModule(const Decl *D, bool IncludeLocal); + void AddLookupOffsets(const LookupBlockOffsets &Offsets, RecordDataImpl &Record); diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index 17801b59963a0..f3902d57e3d1f 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -555,7 +555,25 @@ namespace { using MacroDefinitionsMap = llvm::StringMap>; -using DeclsMap = llvm::DenseMap>; + +class DeclsSet { + SmallVector Decls; + llvm::SmallPtrSet Found; + +public: + operator ArrayRef() const { return Decls; } + + bool empty() const { return Decls.empty(); } + + bool insert(NamedDecl *ND) { + auto [_, Inserted] = Found.insert(ND); + if (Inserted) + Decls.push_back(ND); + return Inserted; + } +}; + +using DeclsMap = llvm::DenseMap; } // namespace @@ -8729,14 +8747,23 @@ bool ASTReader::FindExternalVisibleDeclsByName(const DeclContext *DC, return false; // Load the list of declarations. - SmallVector Decls; - llvm::SmallPtrSet Found; + DeclsSet DS; auto Find = [&, this](auto &&Table, auto &&Key) { for (GlobalDeclID ID : Table.find(Key)) { NamedDecl *ND = cast(GetDecl(ID)); - if (ND->getDeclName() == Name && Found.insert(ND).second) - Decls.push_back(ND); + if (ND->getDeclName() != Name) + continue; + // Special case for namespaces: There can be a lot of redeclarations of + // some namespaces, and we import a "key declaration" per imported module. + // Since all declarations of a namespace are essentially interchangeable, + // we can optimize namespace look-up by only storing the key declaration + // of the current TU, rather than storing N key declarations where N is + // the # of imported modules that declare that namespace. + // TODO: Try to generalize this optimization to other redeclarable decls. + if (isa(ND)) + ND = cast(getKeyDeclaration(ND)); + DS.insert(ND); } }; @@ -8771,8 +8798,8 @@ bool ASTReader::FindExternalVisibleDeclsByName(const DeclContext *DC, Find(It->second.Table, Name); } - SetExternalVisibleDeclsForName(DC, Name, Decls); - return !Decls.empty(); + SetExternalVisibleDeclsForName(DC, Name, DS); + return !DS.empty(); } void ASTReader::completeVisibleDeclsMap(const DeclContext *DC) { @@ -8790,7 +8817,16 @@ void ASTReader::completeVisibleDeclsMap(const DeclContext *DC) { for (GlobalDeclID ID : It->second.Table.findAll()) { NamedDecl *ND = cast(GetDecl(ID)); - Decls[ND->getDeclName()].push_back(ND); + // Special case for namespaces: There can be a lot of redeclarations of + // some namespaces, and we import a "key declaration" per imported module. + // Since all declarations of a namespace are essentially interchangeable, + // we can optimize namespace look-up by only storing the key declaration + // of the current TU, rather than storing N key declarations where N is + // the # of imported modules that declare that namespace. + // TODO: Try to generalize this optimization to other redeclarable decls. + if (isa(ND)) + ND = cast(getKeyDeclaration(ND)); + Decls[ND->getDeclName()].insert(ND); } // FIXME: Why a PCH test is failing if we remove the iterator after findAll? @@ -8800,9 +8836,9 @@ void ASTReader::completeVisibleDeclsMap(const DeclContext *DC) { findAll(ModuleLocalLookups, NumModuleLocalVisibleDeclContexts); findAll(TULocalLookups, NumTULocalVisibleDeclContexts); - for (DeclsMap::iterator I = Decls.begin(), E = Decls.end(); I != E; ++I) { - SetExternalVisibleDeclsForName(DC, I->first, I->second); - } + for (auto &[Name, DS] : Decls) + SetExternalVisibleDeclsForName(DC, Name, DS); + const_cast(DC)->setHasExternalVisibleStorage(false); } diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 2e9f19eb0f51d..3e10bbfedfe65 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -4399,20 +4399,20 @@ class ASTDeclContextNameLookupTrait template data_type getData(const Coll &Decls) { unsigned Start = DeclIDs.size(); - for (NamedDecl *D : Decls) { + auto AddDecl = [this](NamedDecl *D) { NamedDecl *DeclForLocalLookup = getDeclForLocalLookup(Writer.getLangOpts(), D); if (Writer.getDoneWritingDeclsAndTypes() && !Writer.wasDeclEmitted(DeclForLocalLookup)) - continue; + return; // Try to avoid writing internal decls to reduced BMI. // See comments in ASTWriter::WriteDeclContextLexicalBlock for details. if (Writer.isGeneratingReducedBMI() && !DeclForLocalLookup->isFromExplicitGlobalModule() && IsInternalDeclFromFileContext(DeclForLocalLookup)) - continue; + return; auto ID = Writer.GetDeclRef(DeclForLocalLookup); @@ -4426,7 +4426,7 @@ class ASTDeclContextNameLookupTrait ModuleLocalDeclsMap.insert({Key, DeclIDsTy{ID}}); else Iter->second.push_back(ID); - continue; + return; } break; case LookupVisibility::TULocal: { @@ -4435,7 +4435,7 @@ class ASTDeclContextNameLookupTrait TULocalDeclsMap.insert({D->getDeclName(), DeclIDsTy{ID}}); else Iter->second.push_back(ID); - continue; + return; } case LookupVisibility::GenerallyVisibile: // Generally visible decls go into the general lookup table. @@ -4443,6 +4443,24 @@ class ASTDeclContextNameLookupTrait } DeclIDs.push_back(ID); + }; + ASTReader *Chain = Writer.getChain(); + for (NamedDecl *D : Decls) { + if (Chain && isa(D) && D->isFromASTFile() && + D == Chain->getKeyDeclaration(D)) { + // In ASTReader, we stored only the key declaration of a namespace decl + // for this TU. If we have an external namespace decl, this is that + // key declaration and we need to re-expand it to write out the first + // decl from each module. + // + // See comment 'ASTReader::FindExternalVisibleDeclsByName' for details. + auto Firsts = + Writer.CollectFirstDeclFromEachModule(D, /*IncludeLocal=*/false); + for (const auto &[_, First] : Firsts) + AddDecl(cast(const_cast(First))); + } else { + AddDecl(D); + } } return std::make_pair(Start, DeclIDs.size()); } @@ -6916,6 +6934,19 @@ TypeID ASTWriter::GetOrCreateTypeID(ASTContext &Context, QualType T) { }); } +llvm::MapVector +ASTWriter::CollectFirstDeclFromEachModule(const Decl *D, bool IncludeLocal) { + llvm::MapVector Firsts; + // FIXME: We can skip entries that we know are implied by others. + for (const Decl *R = D->getMostRecentDecl(); R; R = R->getPreviousDecl()) { + if (R->isFromASTFile()) + Firsts[Chain->getOwningModuleFile(R)] = R; + else if (IncludeLocal) + Firsts[nullptr] = R; + } + return Firsts; +} + void ASTWriter::AddLookupOffsets(const LookupBlockOffsets &Offsets, RecordDataImpl &Record) { Record.push_back(Offsets.LexicalOffset); diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp index df24a12271a16..7646d5d5efe00 100644 --- a/clang/lib/Serialization/ASTWriterDecl.cpp +++ b/clang/lib/Serialization/ASTWriterDecl.cpp @@ -194,30 +194,13 @@ namespace clang { Record.AddSourceLocation(typeParams->getRAngleLoc()); } - /// Collect the first declaration from each module file that provides a - /// declaration of D. - void CollectFirstDeclFromEachModule( - const Decl *D, bool IncludeLocal, - llvm::MapVector &Firsts) { - - // FIXME: We can skip entries that we know are implied by others. - for (const Decl *R = D->getMostRecentDecl(); R; R = R->getPreviousDecl()) { - if (R->isFromASTFile()) - Firsts[Writer.Chain->getOwningModuleFile(R)] = R; - else if (IncludeLocal) - Firsts[nullptr] = R; - } - } - /// Add to the record the first declaration from each module file that /// provides a declaration of D. The intent is to provide a sufficient /// set such that reloading this set will load all current redeclarations. void AddFirstDeclFromEachModule(const Decl *D, bool IncludeLocal) { - llvm::MapVector Firsts; - CollectFirstDeclFromEachModule(D, IncludeLocal, Firsts); - - for (const auto &F : Firsts) - Record.AddDeclRef(F.second); + auto Firsts = Writer.CollectFirstDeclFromEachModule(D, IncludeLocal); + for (const auto &[_, First] : Firsts) + Record.AddDeclRef(First); } template bool shouldSkipWritingSpecializations(T *Spec) { @@ -272,18 +255,17 @@ namespace clang { assert((isa(D) || isa(D) || isa(D)) && "Must not be called with other decls"); - llvm::MapVector Firsts; - CollectFirstDeclFromEachModule(D, /*IncludeLocal*/ true, Firsts); - - for (const auto &F : Firsts) { - if (shouldSkipWritingSpecializations(F.second)) + auto Firsts = + Writer.CollectFirstDeclFromEachModule(D, /*IncludeLocal=*/true); + for (const auto &[_, First] : Firsts) { + if (shouldSkipWritingSpecializations(First)) continue; if (isa(F.second)) - PartialSpecsInMap.push_back(F.second); + VarTemplatePartialSpecializationDecl>(First)) + PartialSpecsInMap.push_back(First); else - SpecsInMap.push_back(F.second); + SpecsInMap.push_back(First); } } diff --git a/clang/test/Modules/pr171769.cpp b/clang/test/Modules/pr171769.cpp new file mode 100644 index 0000000000000..a8b9a40aada4b --- /dev/null +++ b/clang/test/Modules/pr171769.cpp @@ -0,0 +1,63 @@ +// RUN: rm -rf %t +// RUN: mkdir -p %t +// RUN: split-file %s %t +// RUN: cd %t +// + +// RUN: %clang_cc1 -fmodule-name=A -fno-cxx-modules -xc++ -emit-module \ +// RUN: -fmodules A.cppmap -o A.pcm +// RUN: %clang_cc1 -fmodule-name=B -fno-cxx-modules -xc++ -emit-module \ +// RUN: -fmodules -fmodule-file=A.pcm B.cppmap -o B.pcm +// RUN: %clang_cc1 -fmodule-name=C -fno-cxx-modules -xc++ -emit-module \ +// RUN: -fmodules C.cppmap -o C.pcm +// RUN: %clang_cc1 -fmodule-name=D -fno-cxx-modules -xc++ -emit-module \ +// RUN: -fmodules -fmodule-file=C.pcm D.cppmap -o D.pcm +// RUN: %clang_cc1 -fmodule-name=E -fno-cxx-modules -xc++ -emit-module \ +// RUN: -fmodules -fmodule-file=B.pcm E.cppmap -o E.pcm +// RUN: %clang_cc1 -fmodule-name=F -fno-cxx-modules -xc++ -emit-module \ +// RUN: -fmodules -fmodule-file=D.pcm F.cppmap -o F.pcm +// RUN: %clang_cc1 -fmodule-name=G -fno-cxx-modules -xc++ -emit-module \ +// RUN: -fmodules -fmodule-file=E.pcm -fmodule-file=F.pcm G.cppmap -o G.pcm +// RUN: %clang_cc1 -fno-cxx-modules -fmodules -fmodule-file=G.pcm src.cpp \ +// RUN: -o /dev/null + +//--- A.cppmap +module "A" { header "A.h" } + +//--- A.h +int x; + +//--- B.cppmap +module "B" {} + +//--- C.cppmap +module "C" { header "C.h" } + +//--- C.h +namespace xyz {} + +//--- D.cppmap +module "D" {} + +//--- E.cppmap +module "E" {} + +//--- F.cppmap +module "F" { header "F.h" } + +//--- F.h +namespace xyz { inline void func() {} } + +//--- G.cppmap +module "G" { header "G.h" } + +//--- G.h +#include "F.h" +namespace { void func2() { xyz::func(); } } + +//--- hdr.h +#include "F.h" +namespace xyz_ns = xyz; + +//--- src.cpp +#include "hdr.h" diff --git a/clang/unittests/Serialization/CMakeLists.txt b/clang/unittests/Serialization/CMakeLists.txt index 6782e6b4d7330..a5cc1ed83af49 100644 --- a/clang/unittests/Serialization/CMakeLists.txt +++ b/clang/unittests/Serialization/CMakeLists.txt @@ -2,6 +2,7 @@ add_clang_unittest(SerializationTests ForceCheckFileInputTest.cpp InMemoryModuleCacheTest.cpp ModuleCacheTest.cpp + NamespaceLookupTest.cpp NoCommentsTest.cpp PreambleInNamedModulesTest.cpp LoadSpecLazilyTest.cpp diff --git a/clang/unittests/Serialization/NamespaceLookupTest.cpp b/clang/unittests/Serialization/NamespaceLookupTest.cpp new file mode 100644 index 0000000000000..eefa4be9fbee5 --- /dev/null +++ b/clang/unittests/Serialization/NamespaceLookupTest.cpp @@ -0,0 +1,247 @@ +//== unittests/Serialization/NamespaceLookupOptimizationTest.cpp =======// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "clang/Driver/CreateInvocationFromArgs.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Frontend/FrontendAction.h" +#include "clang/Frontend/FrontendActions.h" +#include "clang/Parse/ParseAST.h" +#include "clang/Serialization/ASTReader.h" +#include "clang/Tooling/Tooling.h" +#include "gtest/gtest.h" + +using namespace llvm; +using namespace clang; +using namespace clang::tooling; + +namespace { + +class NamespaceLookupTest : public ::testing::Test { + void SetUp() override { + ASSERT_FALSE( + sys::fs::createUniqueDirectory("namespace-lookup-test", TestDir)); + } + + void TearDown() override { sys::fs::remove_directories(TestDir); } + +public: + SmallString<256> TestDir; + + void addFile(StringRef Path, StringRef Contents) { + ASSERT_FALSE(sys::path::is_absolute(Path)); + + SmallString<256> AbsPath(TestDir); + sys::path::append(AbsPath, Path); + + ASSERT_FALSE( + sys::fs::create_directories(llvm::sys::path::parent_path(AbsPath))); + + std::error_code EC; + llvm::raw_fd_ostream OS(AbsPath, EC); + ASSERT_FALSE(EC); + OS << Contents; + } + + std::string GenerateModuleInterface(StringRef ModuleName, + StringRef Contents) { + std::string FileName = llvm::Twine(ModuleName + ".cppm").str(); + addFile(FileName, Contents); + + IntrusiveRefCntPtr VFS = + llvm::vfs::createPhysicalFileSystem(); + DiagnosticOptions DiagOpts; + IntrusiveRefCntPtr Diags = + CompilerInstance::createDiagnostics(*VFS, DiagOpts); + CreateInvocationOptions CIOpts; + CIOpts.Diags = Diags; + CIOpts.VFS = VFS; + + std::string CacheBMIPath = + llvm::Twine(TestDir + "/" + ModuleName + ".pcm").str(); + std::string PrebuiltModulePath = + "-fprebuilt-module-path=" + TestDir.str().str(); + const char *Args[] = {"clang++", + "-std=c++20", + "--precompile", + PrebuiltModulePath.c_str(), + "-working-directory", + TestDir.c_str(), + "-I", + TestDir.c_str(), + FileName.c_str(), + "-o", + CacheBMIPath.c_str()}; + std::shared_ptr Invocation = + createInvocation(Args, CIOpts); + EXPECT_TRUE(Invocation); + + CompilerInstance Instance(std::move(Invocation)); + Instance.setDiagnostics(Diags); + Instance.getFrontendOpts().OutputFile = CacheBMIPath; + // Avoid memory leaks. + Instance.getFrontendOpts().DisableFree = false; + GenerateModuleInterfaceAction Action; + EXPECT_TRUE(Instance.ExecuteAction(Action)); + EXPECT_FALSE(Diags->hasErrorOccurred()); + + return CacheBMIPath; + } +}; + +struct NamespaceLookupResult { + int NumLocalNamespaces = 0; + int NumExternalNamespaces = 0; +}; + +class NamespaceLookupConsumer : public ASTConsumer { + NamespaceLookupResult &Result; + +public: + explicit NamespaceLookupConsumer(NamespaceLookupResult &Result) + : Result(Result) {} + + void HandleTranslationUnit(ASTContext &Context) override { + TranslationUnitDecl *TU = Context.getTranslationUnitDecl(); + ASSERT_TRUE(TU); + ASTReader *Chain = dyn_cast_or_null(Context.getExternalSource()); + ASSERT_TRUE(Chain); + for (const Decl *D : + TU->lookup(DeclarationName(&Context.Idents.get("N")))) { + if (!isa(D)) + continue; + if (!D->isFromASTFile()) { + ++Result.NumLocalNamespaces; + } else { + ++Result.NumExternalNamespaces; + EXPECT_EQ(D, Chain->getKeyDeclaration(D)); + } + } + } +}; + +class NamespaceLookupAction : public ASTFrontendAction { + NamespaceLookupResult &Result; + +public: + explicit NamespaceLookupAction(NamespaceLookupResult &Result) + : Result(Result) {} + + std::unique_ptr + CreateASTConsumer(CompilerInstance &CI, StringRef /*Unused*/) override { + return std::make_unique(Result); + } +}; + +TEST_F(NamespaceLookupTest, ExternalNamespacesOnly) { + GenerateModuleInterface("M1", R"cpp( +export module M1; +namespace N {} + )cpp"); + GenerateModuleInterface("M2", R"cpp( +export module M2; +namespace N {} + )cpp"); + GenerateModuleInterface("M3", R"cpp( +export module M3; +namespace N {} + )cpp"); + const char *test_file_contents = R"cpp( +import M1; +import M2; +import M3; + )cpp"; + std::string DepArg = "-fprebuilt-module-path=" + TestDir.str().str(); + NamespaceLookupResult Result; + EXPECT_TRUE(runToolOnCodeWithArgs( + std::make_unique(Result), test_file_contents, + { + "-std=c++20", + DepArg.c_str(), + "-I", + TestDir.c_str(), + }, + "main.cpp")); + + EXPECT_EQ(0, Result.NumLocalNamespaces); + EXPECT_EQ(1, Result.NumExternalNamespaces); +} + +TEST_F(NamespaceLookupTest, ExternalReplacedByLocal) { + GenerateModuleInterface("M1", R"cpp( +export module M1; +namespace N {} + )cpp"); + GenerateModuleInterface("M2", R"cpp( +export module M2; +namespace N {} + )cpp"); + GenerateModuleInterface("M3", R"cpp( +export module M3; +namespace N {} + )cpp"); + const char *test_file_contents = R"cpp( +import M1; +import M2; +import M3; + +namespace N {} + )cpp"; + std::string DepArg = "-fprebuilt-module-path=" + TestDir.str().str(); + NamespaceLookupResult Result; + EXPECT_TRUE(runToolOnCodeWithArgs( + std::make_unique(Result), test_file_contents, + { + "-std=c++20", + DepArg.c_str(), + "-I", + TestDir.c_str(), + }, + "main.cpp")); + + EXPECT_EQ(1, Result.NumLocalNamespaces); + EXPECT_EQ(0, Result.NumExternalNamespaces); +} + +TEST_F(NamespaceLookupTest, LocalAndExternalInterleaved) { + GenerateModuleInterface("M1", R"cpp( +export module M1; +namespace N {} + )cpp"); + GenerateModuleInterface("M2", R"cpp( +export module M2; +namespace N {} + )cpp"); + GenerateModuleInterface("M3", R"cpp( +export module M3; +namespace N {} + )cpp"); + const char *test_file_contents = R"cpp( +import M1; + +namespace N {} + +import M2; +import M3; + )cpp"; + std::string DepArg = "-fprebuilt-module-path=" + TestDir.str().str(); + NamespaceLookupResult Result; + EXPECT_TRUE(runToolOnCodeWithArgs( + std::make_unique(Result), test_file_contents, + { + "-std=c++20", + DepArg.c_str(), + "-I", + TestDir.c_str(), + }, + "main.cpp")); + + EXPECT_EQ(1, Result.NumLocalNamespaces); + EXPECT_EQ(1, Result.NumExternalNamespaces); +} + +} // namespace