llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Berkay Sahin (berkaysahiin) <details> <summary>Changes</summary> When using `SkipPreambleBuild`, adding a new `import` statement to a file did not invalidate the existing preamble because `isPreambleCompatible` only checked whether existing prerequisite modules were up-to-date, not whether the set of required modules itself had changed. Fixes: #<!-- -->199389 Partially fixes: #<!-- -->126350 --- Full diff: https://github.com/llvm/llvm-project/pull/199460.diff 4 Files Affected: - (modified) clang-tools-extra/clangd/ModulesBuilder.cpp (+24) - (modified) clang-tools-extra/clangd/ModulesBuilder.h (+7) - (modified) clang-tools-extra/clangd/Preamble.cpp (+10-1) - (modified) clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp (+56) ``````````diff diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp index 706fd459e15ec..6e47122bdd82b 100644 --- a/clang-tools-extra/clangd/ModulesBuilder.cpp +++ b/clang-tools-extra/clangd/ModulesBuilder.cpp @@ -16,6 +16,7 @@ #include "clang/Serialization/ModuleCache.h" #include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/StringExtras.h" +#include "llvm/ADT/StringSet.h" #include "llvm/Support/CommandLine.h" #include "llvm/Support/FileSystem.h" #include "llvm/Support/LockFileManager.h" @@ -322,6 +323,8 @@ class FailedPrerequisiteModules : public PrerequisiteModules { llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>) const override { return false; } + + llvm::StringSet<> getRequiredModuleNames() const override { return {}; } }; /// Represents a reference to a module file (*.pcm). @@ -502,10 +505,20 @@ class ReusablePrerequisiteModules : public PrerequisiteModules { RequiredModules.emplace_back(std::move(MF)); } + void setDirectModuleNames(std::vector<std::string> Names) { + DirectModuleNames.insert_range(Names); + } + + llvm::StringSet<> getRequiredModuleNames() const override { + return DirectModuleNames; + } + private: llvm::SmallVector<std::shared_ptr<const ModuleFile>, 8> RequiredModules; // A helper class to speedup the query if a module is built. llvm::StringSet<> BuiltModuleNames; + // The directly required module names as scanned from the source file. + llvm::StringSet<> DirectModuleNames; }; bool IsModuleFileUpToDate(PathRef ModuleFilePath, @@ -1228,6 +1241,16 @@ bool ModulesBuilder::hasRequiredModules(PathRef File) { return !CachedMDB.getRequiredModules(File).empty(); } +std::vector<std::string> ModulesBuilder::getRequiredModuleNames(PathRef File) { + std::unique_ptr<ProjectModules> MDB = Impl->getCDB().getProjectModules(File); + if (!MDB) + return {}; + + CachingProjectModules CachedMDB(std::move(MDB), + Impl->getProjectModulesCache()); + return CachedMDB.getRequiredModules(File); +} + std::unique_ptr<PrerequisiteModules> ModulesBuilder::buildPrerequisiteModulesFor(PathRef File, const ThreadsafeFS &TFS) { @@ -1245,6 +1268,7 @@ ModulesBuilder::buildPrerequisiteModulesFor(PathRef File, return std::make_unique<ReusablePrerequisiteModules>(); auto RequiredModules = std::make_unique<ReusablePrerequisiteModules>(); + RequiredModules->setDirectModuleNames(RequiredModuleNames); for (llvm::StringRef RequiredModuleName : RequiredModuleNames) { // Return early if there is any error. if (llvm::Error Err = Impl->getOrBuildModuleFile( diff --git a/clang-tools-extra/clangd/ModulesBuilder.h b/clang-tools-extra/clangd/ModulesBuilder.h index b0e110b92b6a7..d8c8c446fd592 100644 --- a/clang-tools-extra/clangd/ModulesBuilder.h +++ b/clang-tools-extra/clangd/ModulesBuilder.h @@ -26,6 +26,7 @@ #include "support/ThreadsafeFS.h" #include "clang/Frontend/CompilerInvocation.h" #include "llvm/ADT/SmallString.h" +#include "llvm/ADT/StringSet.h" #include <memory> namespace clang { @@ -76,6 +77,9 @@ class PrerequisiteModules { canReuse(const CompilerInvocation &CI, llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>) const = 0; + /// Returns the set of directly required module names + virtual llvm::StringSet<> getRequiredModuleNames() const = 0; + virtual ~PrerequisiteModules() = default; }; @@ -99,6 +103,9 @@ class ModulesBuilder { bool hasRequiredModules(PathRef File); + /// Returns the list of directly required module names + std::vector<std::string> getRequiredModuleNames(PathRef File); + private: class ModulesBuilderImpl; std::unique_ptr<ModulesBuilderImpl> Impl; diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp index 58da7edcf3b93..f8d05b3dc0ded 100644 --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -728,11 +728,20 @@ bool isPreambleCompatible(const PreambleData &Preamble, auto Bounds = computePreambleBounds(CI.getLangOpts(), *ContentsBuffer, Inputs.Opts.SkipPreambleBuild); auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory); + auto RequiredModulesMatch = [&]() -> bool { + if (!Inputs.ModulesManager || !Preamble.RequiredModules) + return true; + auto NewNames = Inputs.ModulesManager->getRequiredModuleNames(FileName); + llvm::StringSet<> NewNameSet; + NewNameSet.insert_range(NewNames); + return NewNameSet == Preamble.RequiredModules->getRequiredModuleNames(); + }; return compileCommandsAreEqual(Inputs.CompileCommand, Preamble.CompileCommand) && Preamble.Preamble.CanReuse(CI, *ContentsBuffer, Bounds, *VFS) && (!Preamble.RequiredModules || - Preamble.RequiredModules->canReuse(CI, VFS)); + Preamble.RequiredModules->canReuse(CI, VFS)) && + RequiredModulesMatch(); } void escapeBackslashAndQuotes(llvm::StringRef Text, llvm::raw_ostream &OS) { diff --git a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp index b88213decea44..f163e248c383a 100644 --- a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp +++ b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp @@ -15,6 +15,7 @@ #include "CodeComplete.h" #include "Compiler.h" #include "ModulesBuilder.h" +#include "Preamble.h" #include "ProjectModules.h" #include "TestTU.h" #include "support/Path.h" @@ -1613,6 +1614,61 @@ struct TypeFromHeader {}; named("TypeFromHeader"))); } +TEST_F(PrerequisiteModulesTests, + SkipPreambleBuildInvalidatedByNewModuleImport) { + MockDirectoryCompilationDatabase CDB(TestDir, FS); + + CDB.addFile("Dep.cppm", R"cpp( +export module Dep; +)cpp"); + + CDB.addFile("Consumer.cpp", R"cpp( +import Dep; +void use() {} +)cpp"); + + ModulesBuilder Builder(CDB); + + auto Inputs = getInputs("Consumer.cpp", CDB); + Inputs.ModulesManager = &Builder; + Inputs.Opts.SkipPreambleBuild = true; + + auto CI = buildCompilerInvocation(Inputs, DiagConsumer); + ASSERT_TRUE(CI); + + auto Preamble = buildPreamble(getFullPath("Consumer.cpp"), *CI, Inputs, + /*StoreInMemory=*/true, + /*PreambleCallback=*/nullptr); + ASSERT_TRUE(Preamble); + EXPECT_EQ(Preamble->Preamble.getBounds().Size, 0u); + ASSERT_TRUE(Preamble->RequiredModules); + + CDB.addFile("NewDep.cppm", R"cpp( +export module NewDep; +)cpp"); + + // Add a new import. + Inputs.Contents = R"cpp( +import Dep; +import NewDep; +void use() {} +)cpp"; + + { + std::error_code EC; + llvm::raw_fd_ostream OS(getFullPath("Consumer.cpp"), EC, + llvm::sys::fs::OF_None); + ASSERT_FALSE(EC); + OS << Inputs.Contents; + } + + auto NewCI = buildCompilerInvocation(Inputs, DiagConsumer); + ASSERT_TRUE(NewCI); + + EXPECT_FALSE(isPreambleCompatible(*Preamble, Inputs, + getFullPath("Consumer.cpp"), *NewCI)); +} + } // namespace } // namespace clang::clangd `````````` </details> https://github.com/llvm/llvm-project/pull/199460 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
