https://github.com/lslimani updated https://github.com/llvm/llvm-project/pull/234181
>From a43f9dcbc5248d7d6892ee1645cdc24425123ab9 Mon Sep 17 00:00:00 2001 From: lslimani <[email protected]> Date: Wed, 21 Oct 2026 16:01:30 +0200 Subject: [PATCH] [clangd] Fix prerequisite module stale dependency detection --- clang-tools-extra/clangd/ModulesBuilder.cpp | 55 ++++- .../unittests/PrerequisiteModulesTest.cpp | 192 ++++++++++++++++++ 2 files changed, 244 insertions(+), 3 deletions(-) diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp index 72bb5b836d2012..801f4a8624bba0 100644 --- a/clang-tools-extra/clangd/ModulesBuilder.cpp +++ b/clang-tools-extra/clangd/ModulesBuilder.cpp @@ -1157,6 +1157,14 @@ class ModulesBuilder::ModulesBuilderImpl { /// Runs GC once for the cache root owning a project root. void garbageCollectModuleCacheForProjectRoot(PathRef ProjectRoot); + /// Returns true if the published module file of a direct dependency of + /// `ModuleName` is newer than the published module file of `ModuleName` + /// itself. C++20 named modules don't record the signatures of the modules + /// they import, so the AST reader can't detect this kind of staleness. + bool hasNewerDependency(StringRef ModuleName, PathRef PublishedModuleFilePath, + PathRef RequiredSource, PathRef ModuleUnitFileName, + CachingProjectModules &MDB); + ModuleFileCache Cache; ObservedModuleFiles ObservedFiles; ModuleNameToSourceCache ProjectModulesCache; @@ -1235,6 +1243,39 @@ void ModulesBuilder::ModulesBuilderImpl::getPrebuiltModuleFile( } } +bool ModulesBuilder::ModulesBuilderImpl::hasNewerDependency( + StringRef ModuleName, PathRef PublishedModuleFilePath, + PathRef RequiredSource, PathRef ModuleUnitFileName, + CachingProjectModules &MDB) { + std::vector<std::string> Deps = MDB.getRequiredModules(ModuleUnitFileName); + if (Deps.empty()) + return false; + + llvm::sys::fs::file_status Status; + // Without a published module file there is nothing to compare against, so + // treat any cached copy as stale. + if (llvm::sys::fs::status(PublishedModuleFilePath, Status)) + return true; + const auto ModuleTime = Status.getLastModificationTime(); + + return llvm::any_of(Deps, [&](const std::string &Dep) { + std::string DepSource = MDB.getSourceForModuleName(Dep, RequiredSource); + if (DepSource.empty()) + return false; + auto DepCmd = getCDB().getCompileCommand(DepSource); + if (!DepCmd) + return false; + const std::string DepPublishedPath = getPublishedModuleFilePath( + Dep, getModuleFilesDirectory(DepSource, *DepCmd, getCDB())); + llvm::sys::fs::file_status DepStatus; + // Dependencies that clangd didn't publish, like module files from the + // build system, are validated by the AST reader instead. + if (llvm::sys::fs::status(DepPublishedPath, DepStatus)) + return false; + return DepStatus.getLastModificationTime() > ModuleTime; + }); +} + llvm::Error ModulesBuilder::ModulesBuilderImpl::getOrBuildModuleFile( PathRef RequiredSource, StringRef ModuleName, const ThreadsafeFS &TFS, CachingProjectModules &MDB, ReusablePrerequisiteModules &BuiltModuleFiles) { @@ -1267,6 +1308,7 @@ llvm::Error ModulesBuilder::ModulesBuilderImpl::getOrBuildModuleFile( std::string ReqFileName = MDB.getSourceForModuleName(ReqModuleName, RequiredSource); + auto Cmd = getCDB().getCompileCommand(ReqFileName); if (!Cmd) return llvm::createStringError( @@ -1284,11 +1326,15 @@ llvm::Error ModulesBuilder::ModulesBuilderImpl::getOrBuildModuleFile( auto SourceLock = ScopedModuleSourceLock::acquire(ReqFileName, getCDB()); if (!SourceLock) return SourceLock.takeError(); + + const bool DependencyRebuilt = + hasNewerDependency(ReqModuleName, PublishedModuleFilePath, + RequiredSource, ReqFileName, MDB); std::shared_ptr<const ModuleFile> Cached = Cache.getModule(ReqModuleName, ReqFileName, CommandHash); - if (Cached) { + if (Cached && !DependencyRebuilt) { if (IsModuleFileUpToDate(Cached->getModuleFilePath(), BuiltModuleFiles, TFS.view(std::nullopt))) { log("Reusing module {0} from {1}", ReqModuleName, @@ -1296,11 +1342,14 @@ llvm::Error ModulesBuilder::ModulesBuilderImpl::getOrBuildModuleFile( BuiltModuleFiles.addModuleFile(std::move(Cached)); continue; } - Cache.remove(ReqModuleName, ReqFileName, CommandHash); } + if (Cached) + Cache.remove(ReqModuleName, ReqFileName, CommandHash); + if (llvm::sys::fs::exists(PublishedModuleFilePath)) { - if (IsModuleFileUpToDate(PublishedModuleFilePath, BuiltModuleFiles, + if (!DependencyRebuilt && + IsModuleFileUpToDate(PublishedModuleFilePath, BuiltModuleFiles, TFS.view(std::nullopt))) { log("Reusing persistent module {0} from {1}", ReqModuleName, PublishedModuleFilePath); diff --git a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp index e878e2e0fa637b..9d2e0da4d0dd95 100644 --- a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp +++ b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp @@ -1802,6 +1802,198 @@ export struct TypeFromModule {}; EXPECT_TRUE(HasToken("export", HighlightingKind::Modifier)); } +TEST_F(PrerequisiteModulesTests, DetectsStaleTransitivePrerequisite) { + MockDirectoryCompilationDatabase CDB(TestDir, FS); + + CDB.addFile("A.cppm", R"cpp( +export module A; +export inline constexpr int a = 1; +)cpp"); + + CDB.addFile("M.cppm", R"cpp( +export module M; +import A; +export inline constexpr int m = a + 1; +)cpp"); + + CDB.addFile("Use.cpp", R"cpp( +import M; +int use() { return m; } +)cpp"); + + ModulesBuilder Builder(CDB); + + auto Info = Builder.buildPrerequisiteModulesFor(getFullPath("Use.cpp"), FS); + ASSERT_TRUE(Info); + + auto Inputs = getInputs("Use.cpp", CDB); + auto Invocation = buildCompilerInvocation(Inputs, DiagConsumer); + ASSERT_TRUE(Invocation); + + auto VFS = FS.view(TestDir); + + // Initially, all prerequisite modules are valid. + EXPECT_TRUE(Info->canReuse(*Invocation, VFS)); + + // Repeated checks should not change the result. + EXPECT_TRUE(Info->canReuse(*Invocation, VFS)); + EXPECT_TRUE(Info->canReuse(*Invocation, VFS)); + + // Change the transitive prerequisite without changing M or Use.cpp. + CDB.addFile("A.cppm", R"cpp( +export module A; +export inline constexpr int a = 42; +)cpp"); + + // The existing prerequisite information must not be reused if + // A's module file is now stale, even if M's own AST is readable. + EXPECT_FALSE(Info->canReuse(*Invocation, VFS)); + + // Rebuild the prerequisite information and check that it is reusable + // again after the module dependency graph has been refreshed. + auto RefreshedInfo = + Builder.buildPrerequisiteModulesFor(getFullPath("Use.cpp"), FS); + ASSERT_TRUE(RefreshedInfo); + EXPECT_TRUE(RefreshedInfo->canReuse(*Invocation, VFS)); + + // M depends on A, which was rebuilt, so M must be rebuilt too rather than + // reusing its stale cached module file. + HeaderSearchOptions OldHSOpts, NewHSOpts; + Info->adjustHeaderSearchOptions(OldHSOpts); + RefreshedInfo->adjustHeaderSearchOptions(NewHSOpts); + ASSERT_EQ(OldHSOpts.PrebuiltModuleFiles.count("M"), 1u); + ASSERT_EQ(NewHSOpts.PrebuiltModuleFiles.count("M"), 1u); + EXPECT_NE(OldHSOpts.PrebuiltModuleFiles["M"], + NewHSOpts.PrebuiltModuleFiles["M"]); + + // An unrelated module must not invalidate Use.cpp's prerequisites. + CDB.addFile("Unrelated.cppm", R"cpp( +export module Unrelated; +export inline constexpr int unrelated = 100; +)cpp"); + + EXPECT_TRUE(RefreshedInfo->canReuse(*Invocation, VFS)); +} + +TEST_F(PrerequisiteModulesTests, + RebuiltDependencyInvalidatesDependentAcrossFiles) { + MockDirectoryCompilationDatabase CDB(TestDir, FS); + + CDB.addFile("A.cppm", R"cpp( +export module A; +export inline constexpr int a = 1; +)cpp"); + + CDB.addFile("M.cppm", R"cpp( +export module M; +import A; +export inline constexpr int m = a + 1; +)cpp"); + + CDB.addFile("UseA.cpp", R"cpp( +import A; +int useA() { return a; } +)cpp"); + + CDB.addFile("UseM.cpp", R"cpp( +import M; +int useM() { return m; } +)cpp"); + + ModulesBuilder Builder(CDB); + + auto InfoM = Builder.buildPrerequisiteModulesFor(getFullPath("UseM.cpp"), FS); + ASSERT_TRUE(InfoM); + HeaderSearchOptions OldHSOpts; + InfoM->adjustHeaderSearchOptions(OldHSOpts); + ASSERT_EQ(OldHSOpts.PrebuiltModuleFiles.count("M"), 1u); + + CDB.addFile("A.cppm", R"cpp( +export module A; +export inline constexpr int a = 42; +)cpp"); + + // Opening a file that only imports A rebuilds A, but not M. + auto InfoA = Builder.buildPrerequisiteModulesFor(getFullPath("UseA.cpp"), FS); + ASSERT_TRUE(InfoA); + + // M's cached file was built against the old A, so it must be rebuilt. + auto RefreshedInfoM = + Builder.buildPrerequisiteModulesFor(getFullPath("UseM.cpp"), FS); + ASSERT_TRUE(RefreshedInfoM); + HeaderSearchOptions NewHSOpts; + RefreshedInfoM->adjustHeaderSearchOptions(NewHSOpts); + ASSERT_EQ(NewHSOpts.PrebuiltModuleFiles.count("M"), 1u); + EXPECT_NE(OldHSOpts.PrebuiltModuleFiles["M"], + NewHSOpts.PrebuiltModuleFiles["M"]); +} + +// C++20 named modules don't record the signatures of the modules they import, +// so a persistent BMI of M built against an old A must be detected as stale by +// clangd itself, even from a fresh builder (e.g. a new clangd process). +TEST_F(PrerequisiteModulesTests, + PersistentModuleCacheRebuildsDependentOfRebuiltModuleAcrossBuilders) { + MockDirectoryCompilationDatabase CDB(TestDir, FS); + + CDB.addFile("A.cppm", R"cpp( +export module A; +export inline int a() { return 1; } +)cpp"); + + CDB.addFile("M.cppm", R"cpp( +export module M; +import A; +export inline int m() { return a() + 1; } +)cpp"); + + CDB.addFile("Use.cpp", R"cpp( +import M; +int use() { return m(); } +)cpp"); + + auto ReadModuleFile = [](const PrerequisiteModules &Info, + llvm::StringRef ModuleName) -> std::string { + HeaderSearchOptions HS; + Info.adjustHeaderSearchOptions(HS); + auto It = HS.PrebuiltModuleFiles.find(ModuleName.str()); + if (It == HS.PrebuiltModuleFiles.end()) + return ""; + auto Buf = llvm::MemoryBuffer::getFile(It->second); + return Buf ? (*Buf)->getBuffer().str() : ""; + }; + + // Keep the first builder's module files alive so their contents can be + // compared with the second builder's. + ModulesBuilder FirstBuilder(CDB); + auto FirstInfo = + FirstBuilder.buildPrerequisiteModulesFor(getFullPath("Use.cpp"), FS); + ASSERT_TRUE(FirstInfo); + std::string FirstM = ReadModuleFile(*FirstInfo, "M"); + ASSERT_FALSE(FirstM.empty()); + + CDB.addFile("A.cppm", R"cpp( +export module A; +export inline int a() { return 42; } +)cpp"); + + ModulesBuilder SecondBuilder(CDB); + auto SecondInfo = + SecondBuilder.buildPrerequisiteModulesFor(getFullPath("Use.cpp"), FS); + ASSERT_TRUE(SecondInfo); + std::string SecondM = ReadModuleFile(*SecondInfo, "M"); + ASSERT_FALSE(SecondM.empty()); + + // A reused M would be a byte-for-byte copy of the first published BMI. + EXPECT_NE(FirstM, SecondM); + + // With nothing changed, a third builder reuses the persistent M. + ModulesBuilder ThirdBuilder(CDB); + auto ThirdInfo = + ThirdBuilder.buildPrerequisiteModulesFor(getFullPath("Use.cpp"), FS); + ASSERT_TRUE(ThirdInfo); + EXPECT_EQ(SecondM, ReadModuleFile(*ThirdInfo, "M")); +} + } // namespace } // namespace clang::clangd _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
