[clangd] Introduce reusable modules builder #73483
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Alternatives to https://reviews.llvm.org/D153114. Try to address clangd/clangd#1293. See the links for design ideas. We want to have some initial support in clang18. This is the initial support for C++20 Modules in clangd. As suggested by sammccall in https://reviews.llvm.org/D153114, we should minimize the scope of the initial patch to make it easier to review and understand so that every one are in the same page: > Don't attempt any cross-file or cross-version coordination: i.e. don't > try to reuse BMIs between different files, don't try to reuse BMIs > between (preamble) reparses of the same file, don't try to persist the > module graph. Instead, when building a preamble, synchronously scan > for the module graph, build the required PCMs on the single preamble > thread with filenames private to that preamble, and then proceed to > build the preamble. And this patch reflects the above opinions.
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clangd Author: Chuanqi Xu (ChuanqiXu9) ChangesThis is a draft based on #66462. The main goal of the patch is to implement the TODO step 1: "reuse module files" across source files. In my mind, the modules in clangd will become relatively usable after this patch. I hope we can land this in clang18 too. A big assumption for this patch is that there is exactly one source file declaring the same module (unit) in the project. This is not technically true since we can multiple unrelated modules in the same project. I think this is a understandable assumption for users too. To fully address the underlying issue, we need input from build systems as @mathstuf mentioned in #66462 (comment). But I don't think we have to wait for that. The core ideas for this patch are:
The model can be improved in future by introducing a thread pool for building modules. The difference is that now when we only open a source file which imports a lot of modules (indirectly), we would only build the modules one by one. However, if we have a thread pool for building modules specially, we can try to build the modules in parallel. To achieve this, we need an explicit module graph. I did this in the first patch: https://reviews.llvm.org/D153114. I think we can try to introduce that thread pool later than sooner since it may be better to keep the changes small instead of large. (This is what required in the first review page.) I tested this patch with real workloads. I'll add in-tree test once #66462 landed (Otherwise refactoring interfaces may be painful) Patch is 92.78 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/73483.diff 31 Files Affected:
diff --git a/clang-tools-extra/clangd/CMakeLists.txt b/clang-tools-extra/clangd/CMakeLists.txt
index 3911fb6c6c746a8..242a8ad2e350be7 100644
--- a/clang-tools-extra/clangd/CMakeLists.txt
+++ b/clang-tools-extra/clangd/CMakeLists.txt
@@ -97,7 +97,10 @@ add_clang_library(clangDaemon
IncludeFixer.cpp
InlayHints.cpp
JSONTransport.cpp
+ ModuleDependencyScanner.cpp
+ ModulesBuilder.cpp
PathMapping.cpp
+ ProjectModules.cpp
Protocol.cpp
Quality.cpp
ParsedAST.cpp
@@ -161,6 +164,7 @@ clang_target_link_libraries(clangDaemon
clangAST
clangASTMatchers
clangBasic
+ clangDependencyScanning
clangDriver
clangFormat
clangFrontend
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 13d788162817fb4..8ba4b38c420ab6a 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -202,6 +202,7 @@ ClangdServer::Options::operator TUScheduler::Options() const {
Opts.UpdateDebounce = UpdateDebounce;
Opts.ContextProvider = ContextProvider;
Opts.PreambleThrottler = PreambleThrottler;
+ Opts.ExperimentalModulesSupport = ExperimentalModulesSupport;
return Opts;
}
@@ -222,6 +223,7 @@ ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
DirtyFS(std::make_unique<DraftStoreFS>(TFS, DraftMgr)) {
if (Opts.AsyncThreadsCount != 0)
IndexTasks.emplace();
+
// Pass a callback into `WorkScheduler` to extract symbols from a newly
// parsed file and rebuild the file index synchronously each time an AST
// is parsed.
diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index a416602251428b0..dc546b118cb8f5e 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -112,6 +112,9 @@ class ClangdServer {
/// This throttler controls which preambles may be built at a given time.
clangd::PreambleThrottler *PreambleThrottler = nullptr;
+ /// Enable experimental support for modules.
+ bool ExperimentalModulesSupport = false;
+
/// If true, ClangdServer builds a dynamic in-memory index for symbols in
/// opened files and uses the index to augment code completion results.
bool BuildDynamicSymbolIndex = false;
diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
index d1833759917a30f..3a1931041eb0c56 100644
--- a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
+++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
@@ -9,6 +9,7 @@
#include "GlobalCompilationDatabase.h"
#include "Config.h"
#include "FS.h"
+#include "ProjectModules.h"
#include "SourceCode.h"
#include "support/Logger.h"
#include "support/Path.h"
@@ -729,6 +730,21 @@ DirectoryBasedGlobalCompilationDatabase::getProjectInfo(PathRef File) const {
return Res->PI;
}
+std::shared_ptr<ProjectModules>
+DirectoryBasedGlobalCompilationDatabase::getProjectModules(PathRef File) const {
+ CDBLookupRequest Req;
+ Req.FileName = File;
+ Req.ShouldBroadcast = false;
+ Req.FreshTime = Req.FreshTimeMissing =
+ std::chrono::steady_clock::time_point::min();
+ auto Res = lookupCDB(Req);
+ if (!Res)
+ return {};
+ return ProjectModules::create(
+ ProjectModules::ProjectModulesKind::ScanningAllFiles,
+ Res->CDB->getAllFiles(), *this, Opts.TFS);
+}
+
OverlayCDB::OverlayCDB(const GlobalCompilationDatabase *Base,
std::vector<std::string> FallbackFlags,
CommandMangler Mangler)
@@ -805,6 +821,13 @@ std::optional<ProjectInfo> DelegatingCDB::getProjectInfo(PathRef File) const {
return Base->getProjectInfo(File);
}
+std::shared_ptr<ProjectModules>
+DelegatingCDB::getProjectModules(PathRef File) const {
+ if (!Base)
+ return nullptr;
+ return Base->getProjectModules(File);
+}
+
tooling::CompileCommand DelegatingCDB::getFallbackCommand(PathRef File) const {
if (!Base)
return GlobalCompilationDatabase::getFallbackCommand(File);
diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.h b/clang-tools-extra/clangd/GlobalCompilationDatabase.h
index 2bf8c973c534c6f..1bd6324c4cd8e4d 100644
--- a/clang-tools-extra/clangd/GlobalCompilationDatabase.h
+++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.h
@@ -25,6 +25,8 @@
namespace clang {
namespace clangd {
+class ProjectModules;
+
struct ProjectInfo {
// The directory in which the compilation database was discovered.
// Empty if directory-based compilation database discovery was not used.
@@ -45,6 +47,12 @@ class GlobalCompilationDatabase {
return std::nullopt;
}
+ /// Get the modules in the closest project to \p File
+ virtual std::shared_ptr<ProjectModules>
+ getProjectModules(PathRef File) const {
+ return nullptr;
+ }
+
/// Makes a guess at how to build a file.
/// The default implementation just runs clang on the file.
/// Clangd should treat the results as unreliable.
@@ -76,6 +84,9 @@ class DelegatingCDB : public GlobalCompilationDatabase {
std::optional<ProjectInfo> getProjectInfo(PathRef File) const override;
+ std::shared_ptr<ProjectModules>
+ getProjectModules(PathRef File) const override;
+
tooling::CompileCommand getFallbackCommand(PathRef File) const override;
bool blockUntilIdle(Deadline D) const override;
@@ -122,6 +133,9 @@ class DirectoryBasedGlobalCompilationDatabase
/// \p File's parents.
std::optional<ProjectInfo> getProjectInfo(PathRef File) const override;
+ std::shared_ptr<ProjectModules>
+ getProjectModules(PathRef File) const override;
+
bool blockUntilIdle(Deadline Timeout) const override;
private:
diff --git a/clang-tools-extra/clangd/ModuleDependencyScanner.cpp b/clang-tools-extra/clangd/ModuleDependencyScanner.cpp
new file mode 100644
index 000000000000000..78403861a8cfdd0
--- /dev/null
+++ b/clang-tools-extra/clangd/ModuleDependencyScanner.cpp
@@ -0,0 +1,90 @@
+//===---------------- ModuleDependencyScanner.cpp ----------------*- C++-*-===//
+//
+// 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 "ModuleDependencyScanner.h"
+#include "support/Logger.h"
+
+namespace clang {
+namespace clangd {
+
+std::optional<ModuleDependencyScanner::ModuleDependencyInfo>
+ModuleDependencyScanner::scan(PathRef FilePath) {
+ std::optional<tooling::CompileCommand> Cmd = CDB.getCompileCommand(FilePath);
+
+ if (!Cmd)
+ return std::nullopt;
+
+ using namespace clang::tooling::dependencies;
+
+ llvm::SmallString<128> FilePathDir(FilePath);
+ llvm::sys::path::remove_filename(FilePathDir);
+ DependencyScanningTool ScanningTool(Service, TFS.view(FilePathDir));
+
+ llvm::Expected<P1689Rule> ScanningResult =
+ ScanningTool.getP1689ModuleDependencyFile(*Cmd, Cmd->Directory);
+
+ if (auto E = ScanningResult.takeError()) {
+ log("Scanning modules dependencies for {0} failed: {1}", FilePath,
+ llvm::toString(std::move(E)));
+ return std::nullopt;
+ }
+
+ ModuleDependencyInfo Result;
+
+ if (ScanningResult->Provides) {
+ ModuleNameToSource[ScanningResult->Provides->ModuleName] = FilePath;
+ Result.ModuleName = ScanningResult->Provides->ModuleName;
+ }
+
+ for (auto &Required : ScanningResult->Requires)
+ Result.RequiredModules.push_back(Required.ModuleName);
+
+ return Result;
+}
+
+void ModuleDependencyScanner::globalScan(
+ const std::vector<std::string> &AllFiles) {
+ for (auto &File : AllFiles)
+ scan(File);
+
+ GlobalScanned = true;
+}
+
+PathRef
+ModuleDependencyScanner::getSourceForModuleName(StringRef ModuleName) const {
+ assert(
+ GlobalScanned &&
+ "We should only call getSourceForModuleName after calling globalScan()");
+
+ if (auto It = ModuleNameToSource.find(ModuleName);
+ It != ModuleNameToSource.end())
+ return It->second;
+
+ return {};
+}
+
+std::vector<std::string>
+ModuleDependencyScanner::getRequiredModules(PathRef File) {
+ auto ScanningResult = scan(File);
+ if (!ScanningResult)
+ return {};
+
+ return ScanningResult->RequiredModules;
+}
+
+std::optional<std::string>
+ModuleDependencyScanner::getModuleName(PathRef File) {
+ auto ScanningResult = scan(File);
+ if (!ScanningResult)
+ return std::nullopt;
+
+ return ScanningResult->ModuleName;
+}
+
+} // namespace clangd
+} // namespace clang
diff --git a/clang-tools-extra/clangd/ModuleDependencyScanner.h b/clang-tools-extra/clangd/ModuleDependencyScanner.h
new file mode 100644
index 000000000000000..177e599f4b1e55b
--- /dev/null
+++ b/clang-tools-extra/clangd/ModuleDependencyScanner.h
@@ -0,0 +1,108 @@
+//===-------------- ModuleDependencyScanner.h --------------------*- C++-*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_MODULEDEPENDENCYSCANNER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_MODULEDEPENDENCYSCANNER_H
+
+#include "GlobalCompilationDatabase.h"
+#include "support/Path.h"
+#include "support/ThreadsafeFS.h"
+
+#include "clang/Tooling/DependencyScanning/DependencyScanningService.h"
+#include "clang/Tooling/DependencyScanning/DependencyScanningTool.h"
+
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringMap.h"
+
+namespace clang {
+namespace clangd {
+
+/// A scanner to query the dependency information for C++20 Modules.
+///
+/// The scanner can scan a single file with `scan(PathRef)` member function
+/// or scan the whole project with `globalScan(PathRef)` member function. See
+/// the comments of `globalScan` to see the details.
+///
+/// ModuleDependencyScanner should only be used via ScanningAllProjectModules.
+///
+/// The ModuleDependencyScanner can get the directly required module name for a
+/// specific source file. Also the ModuleDependencyScanner can get the source
+/// file declaring a specific module name.
+///
+/// IMPORTANT NOTE: we assume that every module unit is only declared once in a
+/// source file in the project. But the assumption is not strictly true even
+/// besides the invalid projects. The language specification requires that every
+/// module unit should be unique in a valid program. But a project can contain
+/// multiple programs. Then it is valid that we can have multiple source files
+/// declaring the same module in a project as long as these source files don't
+/// interere with each other.`
+class ModuleDependencyScanner {
+public:
+ ModuleDependencyScanner(const GlobalCompilationDatabase &CDB,
+ const ThreadsafeFS &TFS)
+ : CDB(CDB), TFS(TFS),
+ Service(tooling::dependencies::ScanningMode::CanonicalPreprocessing,
+ tooling::dependencies::ScanningOutputFormat::P1689) {}
+
+ // The scanned modules dependency information for a specific source file.
+ struct ModuleDependencyInfo {
+ // The name of the module if the file is a module unit.
+ std::optional<std::string> ModuleName;
+ // A list of names for the modules that the file directly depends.
+ std::vector<std::string> RequiredModules;
+ };
+
+ /// Scanning the single file specified by \param FilePath.
+ /// NOTE: This is only used by unittests for external uses.
+ std::optional<ModuleDependencyInfo> scan(PathRef FilePath);
+
+ /// Scanning every source file in the current project to get the
+ /// <module-name> to <module-unit-source> map.
+ /// It looks unefficiency to scan the whole project especially for
+ /// every version of every file!
+ /// TODO: We should find an efficient method to get the <module-name>
+ /// to <module-unit-source> map. We can make it either by providing
+ /// a global module dependency scanner to monitor every file. Or we
+ /// can simply require the build systems (or even if the end users)
+ /// to provide the map.
+ void globalScan(const std::vector<std::string> &AllFiles);
+ bool isGlobalScanned() const { return GlobalScanned; }
+
+ /// Get the source file from the module name. Note that the language
+ /// guarantees all the module names are unique in a valid program.
+ /// This function should only be called after globalScan.
+ ///
+ /// FIXME: We should handle the case that there are multiple source files
+ /// declaring the same module.
+ PathRef getSourceForModuleName(StringRef ModuleName) const;
+
+ /// Return the direct required modules. Indirect required modules are not
+ /// included.
+ std::vector<std::string> getRequiredModules(PathRef File);
+
+ std::optional<std::string> getModuleName(PathRef File);
+
+private:
+ const GlobalCompilationDatabase &CDB;
+ const ThreadsafeFS &TFS;
+
+ // Whether the scanner has scanned the project globally.
+ bool GlobalScanned = false;
+
+ clang::tooling::dependencies::DependencyScanningService Service;
+
+ // TODO: Add a scanning cache.
+
+ // Map module name to source file path.
+ llvm::StringMap<std::string> ModuleNameToSource;
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif
diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp
new file mode 100644
index 000000000000000..c3a2ba89e944572
--- /dev/null
+++ b/clang-tools-extra/clangd/ModulesBuilder.cpp
@@ -0,0 +1,706 @@
+//===----------------- ModulesBuilder.cpp ------------------------*- C++-*-===//
+//
+// 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 "ModulesBuilder.h"
+#include "PrerequisiteModules.h"
+#include "support/Logger.h"
+
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Frontend/FrontendActions.h"
+
+namespace clang {
+namespace clangd {
+
+namespace {
+
+/// Get or create a path to store module files. Generally it should be:
+///
+/// project_root/.cache/clangd/module_files/{RequiredPrefixDir}/.
+///
+/// \param MainFile is used to get the root of the project from global
+/// compilation database. \param RequiredPrefixDir is used to get the user
+/// defined prefix for module files. This is useful when we want to seperate
+/// module files. e.g., we want to build module files for the same module unit
+/// `a.cppm` with 2 different users `b.cpp` and `c.cpp` and we don't want the
+/// module file for `b.cpp` be conflict with the module files for `c.cpp`. Then
+/// we can put the 2 module files into different dirs like:
+///
+/// project_root/.cache/clangd/module_files/b.cpp/a.pcm
+/// project_root/.cache/clangd/module_files/c.cpp/a.pcm
+llvm::SmallString<256> getModuleFilesPath(PathRef MainFile,
+ const GlobalCompilationDatabase &CDB,
+ StringRef RequiredPrefixDir) {
+ std::optional<ProjectInfo> PI = CDB.getProjectInfo(MainFile);
+ if (!PI)
+ return {};
+
+ // FIXME: PI->SourceRoot may be empty, depending on the CDB strategy.
+ llvm::SmallString<256> Result(PI->SourceRoot);
+
+ llvm::sys::path::append(Result, ".cache");
+ llvm::sys::path::append(Result, "clangd");
+ llvm::sys::path::append(Result, "module_files");
+
+ llvm::sys::path::append(Result, RequiredPrefixDir);
+
+ llvm::sys::fs::create_directories(Result, /*IgnoreExisting=*/true);
+
+ return Result;
+}
+
+/// Get the absolute path for the filename from the compile command.
+llvm::SmallString<128> getAbsolutePath(const tooling::CompileCommand &Cmd) {
+ llvm::SmallString<128> AbsolutePath;
+ if (llvm::sys::path::is_absolute(Cmd.Filename)) {
+ AbsolutePath = Cmd.Filename;
+ } else {
+ AbsolutePath = Cmd.Directory;
+ llvm::sys::path::append(AbsolutePath, Cmd.Filename);
+ llvm::sys::path::remove_dots(AbsolutePath, true);
+ }
+ return AbsolutePath;
+}
+
+/// Get a unique module file path under \param ModuleFilesPrefix.
+std::string getUniqueModuleFilePath(StringRef ModuleName,
+ PathRef ModuleFilesPrefix) {
+ llvm::SmallString<256> ModuleFilePattern(ModuleFilesPrefix);
+ auto [PrimaryModuleName, PartitionName] = ModuleName.split(':');
+ llvm::sys::path::append(ModuleFilePattern, PrimaryModuleName);
+ if (!PartitionName.empty()) {
+ ModuleFilePattern.append("-");
+ ModuleFilePattern.append(PartitionName);
+ }
+
+ ModuleFilePattern.append("-%%-%%-%%-%%-%%-%%");
+ ModuleFilePattern.append(".pcm");
+
+ llvm::SmallString<256> ModuleFilePath;
+ llvm::sys::fs::createUniquePath(ModuleFilePattern, ModuleFilePath,
+ /*MakeAbsolute=*/false);
+
+ return (std::string)ModuleFilePath;
+}
+
+struct ModuleFile {
+ ModuleFile(StringRef ModuleName, PathRef ModuleFilePath)
+ : ModuleName(ModuleName.str()), ModuleFilePath(ModuleFilePath.str()) {}
+
+ ModuleFile() = delete;
+
+ ModuleFile(const ModuleFile &) = delete;
+ ModuleFile operator=(const ModuleFile &) = delete;
+
+ // The move constructor is needed for llvm::SmallVector.
+ ModuleFile(ModuleFile &&Other)
+ : ModuleName(std::move(Other.ModuleName)),
+ ModuleFilePath(std::move(Other.ModuleFilePath)) {
+ Other.ModuleName.clear();
+ Other.ModuleFilePath.clear();
+ }
+
+ ModuleFile &operator=(ModuleFile &&Other) = delete;
+
+ ~ModuleFile() {
+ if (!ModuleFilePath.empty())
+ llvm::sys::fs::remove(ModuleFilePath);
+ }
+
+ std::string ModuleName;
+ std::string ModuleFilePath;
+};
+
+/// All the required module should be included in BuiltModuleFiles.
+std::optional<ModuleFile>
+buildModuleFile(StringRef ModuleName, PathRef ModuleUnitFile,
+ const GlobalCompilationDatabase &CDB,
+ const PrerequisiteModules &BuiltModuleFiles,
+ const ThreadsafeFS *TFS, PathRef ModuleFilesPrefix) {
+ auto Cmd = CDB.getCompileCommand(ModuleUnitFile);
+ if (!Cmd)
+ return std::nullopt;
+
+ std::string ModuleFileName =
+ getUniqueModuleFilePath(ModuleName, ModuleFilesPrefix);
+ Cmd->Output = ModuleFileName;
+
+ std::string CommandLine;
+ for (auto &Arg : Cmd->CommandLine)
+ CommandLine += Arg + " ";
+
+ ParseInputs Inputs;
+ Inputs.TFS = TFS;
+ Inputs.CompileCommand = std::move(*Cmd);
+
+ IgnoreDiagnostics IgnoreDiags;
+ auto CI = buildCompilerInvocation(Inputs, IgnoreDiags);
+ if (!CI) {
+ log("Failed to build module {0} since build Compiler invocation failed", ModuleName);
+ return std::nullopt;
+ }
+
+ auto FS = Inputs.TFS->view(Inputs.CompileCommand.Directory);
+ auto AbsolutePath = getAbsolutePath(Inputs.CompileCommand);
+ auto Buf = FS->getBufferForFile(AbsolutePath);
+ if (!Buf) {
+ log("Failed to build module {0} since get buffer failed", ModuleName);
+ return std::nullopt;
+ }
+
+ // Try to use the built module files from clangd first.
+ BuiltModuleFiles.adjustHeaderSearchOptions(CI->getHeaderSearchOpts());
+
+ // Hash the contents of input files and store the hash value to the BMI files.
+ // So that we can check if the files are still valid when we want to reuse the
+ // BMI files.
+ CI->getHeaderSearchOpts().ValidateASTInputFilesContent = true;
+
+ BuiltModuleFiles.adjustHeaderSearchOptions(CI->getHeaderSearchOpts());
+
+ CI->getFrontendOpts().OutputFile = Inputs.CompileCommand.Output;
+ auto Clang =
+ prepareCompilerInstance(std::move(CI), /*Preamble=*/nullptr,
+ std::move(*Buf), std::move(FS), IgnoreDiags);
+ if (!Clang) {
+ log("Failed to build module {0} since build compiler instance failed", ModuleName);
+ return std::nullopt;
+ }
+
+ GenerateModuleInterfaceAction Action;
+ Clang->ExecuteAction(Action);
+
+ if (Clang->getDiagnostics().hasErrorOccurred()) {
+ log("Failed to build module {0} since error occurred failed", ModuleName);
+ log("Failing Command line {0}", CommandLine);
+ return std::nullopt;
+ }
+
+ return ModuleFile{ModuleName, ModuleFileName};
+}
+} // namespace
+
+/// FailedPrerequisiteModules - stands for the PrerequisiteModules which has
+/// errors happened during the...
[truncated]
|
You can test this locally with the following command:git-clang-format --diff e89324219acf3d799a86fed5651e492bbab4867c 052e2da0ede8cc72e22ad9ba75ddf2868e5fffe1 -- clang-tools-extra/clangd/ModuleDependencyScanner.cpp clang-tools-extra/clangd/ModuleDependencyScanner.h clang-tools-extra/clangd/ModulesBuilder.cpp clang-tools-extra/clangd/ModulesBuilder.h clang-tools-extra/clangd/PrerequisiteModules.h clang-tools-extra/clangd/ProjectModules.cpp clang-tools-extra/clangd/ProjectModules.h clang-tools-extra/clangd/unittests/ModuleDependencyScannerTest.cpp clang-tools-extra/clangd/unittests/ModulesTestSetup.h clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp clang-tools-extra/clangd/ClangdServer.cpp clang-tools-extra/clangd/ClangdServer.h clang-tools-extra/clangd/GlobalCompilationDatabase.cpp clang-tools-extra/clangd/GlobalCompilationDatabase.h clang-tools-extra/clangd/ParsedAST.cpp clang-tools-extra/clangd/Preamble.cpp clang-tools-extra/clangd/Preamble.h clang-tools-extra/clangd/TUScheduler.cpp clang-tools-extra/clangd/TUScheduler.h clang-tools-extra/clangd/tool/Check.cpp clang-tools-extra/clangd/tool/ClangdMain.cpp clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp clang-tools-extra/clangd/unittests/FileIndexTests.cpp clang-tools-extra/clangd/unittests/ParsedASTTests.cpp clang-tools-extra/clangd/unittests/PreambleTests.cpp clang-tools-extra/clangd/unittests/TestTU.cppView the diff from clang-format here.diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp
index c3a2ba89e9..b7c71bb31e 100644
--- a/clang-tools-extra/clangd/ModulesBuilder.cpp
+++ b/clang-tools-extra/clangd/ModulesBuilder.cpp
@@ -100,9 +100,9 @@ struct ModuleFile {
ModuleFile(ModuleFile &&Other)
: ModuleName(std::move(Other.ModuleName)),
ModuleFilePath(std::move(Other.ModuleFilePath)) {
- Other.ModuleName.clear();
- Other.ModuleFilePath.clear();
- }
+ Other.ModuleName.clear();
+ Other.ModuleFilePath.clear();
+ }
ModuleFile &operator=(ModuleFile &&Other) = delete;
@@ -140,7 +140,8 @@ buildModuleFile(StringRef ModuleName, PathRef ModuleUnitFile,
IgnoreDiagnostics IgnoreDiags;
auto CI = buildCompilerInvocation(Inputs, IgnoreDiags);
if (!CI) {
- log("Failed to build module {0} since build Compiler invocation failed", ModuleName);
+ log("Failed to build module {0} since build Compiler invocation failed",
+ ModuleName);
return std::nullopt;
}
@@ -151,7 +152,7 @@ buildModuleFile(StringRef ModuleName, PathRef ModuleUnitFile,
log("Failed to build module {0} since get buffer failed", ModuleName);
return std::nullopt;
}
-
+
// Try to use the built module files from clangd first.
BuiltModuleFiles.adjustHeaderSearchOptions(CI->getHeaderSearchOpts());
@@ -167,7 +168,8 @@ buildModuleFile(StringRef ModuleName, PathRef ModuleUnitFile,
prepareCompilerInstance(std::move(CI), /*Preamble=*/nullptr,
std::move(*Buf), std::move(FS), IgnoreDiags);
if (!Clang) {
- log("Failed to build module {0} since build compiler instance failed", ModuleName);
+ log("Failed to build module {0} since build compiler instance failed",
+ ModuleName);
return std::nullopt;
}
@@ -192,8 +194,7 @@ public:
/// We shouldn't adjust the compilation commands based on
/// FailedPrerequisiteModules.
- void adjustHeaderSearchOptions(HeaderSearchOptions &Options) const override {
- }
+ void adjustHeaderSearchOptions(HeaderSearchOptions &Options) const override {}
/// FailedPrerequisiteModules can never be reused.
bool
canReuse(const CompilerInvocation &CI,
@@ -492,8 +493,8 @@ private:
std::shared_ptr<std::condition_variable> &CV,
ReusableModulesBuilder &Builder)
: ModuleName(ModuleName), Mutex(Mutex), CV(CV), Builder(Builder) {
- IsFirstTask = (Mutex.use_count() == 2);
- }
+ IsFirstTask = (Mutex.use_count() == 2);
+ }
~ModuleBuildingSharedOwner();
@@ -641,7 +642,8 @@ bool ReusableModulesBuilder::getOrBuildModuleFile(
if (std::shared_ptr<ModuleFile> Cached =
getValidModuleFile(ModuleName, MDB)) {
- log("Reusing Built Module {0} with {1}", Cached->ModuleName, Cached->ModuleFilePath);
+ log("Reusing Built Module {0} with {1}", Cached->ModuleName,
+ Cached->ModuleFilePath);
BuiltModuleFiles.addModuleFile(Cached);
return true;
}
diff --git a/clang-tools-extra/clangd/ProjectModules.h b/clang-tools-extra/clangd/ProjectModules.h
index 3c47863cf8..5a3ca3a05f 100644
--- a/clang-tools-extra/clangd/ProjectModules.h
+++ b/clang-tools-extra/clangd/ProjectModules.h
@@ -46,7 +46,7 @@ public:
virtual PathRef
getSourceForModuleName(StringRef ModuleName,
PathRef RequiredSrcFile = PathRef()) = 0;
-
+
virtual ~ProjectModules() = default;
};
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a draft based on #66462.
The main goal of the patch is to implement the TODO step 1: "reuse module files" across source files. In my mind, the modules in clangd will become relatively usable after this patch. I hope we can land this in clang18 too.
A big assumption for this patch is that there is exactly one source file declaring the same module (unit) in the project. This is not technically true since we can multiple unrelated modules in the same project. I think this is a understandable assumption for users too. To fully address the underlying issue, we need input from build systems as @mathstuf mentioned in #66462 (comment). But I don't think we have to wait for that.
The core ideas for this patch are:
I tested this patch with real workloads. I'll add in-tree test once #66462 landed (Otherwise refactoring interfaces may be painful)
Future TODO
The model can be improved in future by introducing a thread pool for building modules. The difference is that now when we only open a source file which imports a lot of modules (indirectly), we would only build the modules one by one. However, if we have a thread pool for building modules specially, we can try to build the modules in parallel. To achieve this, we need an explicit module graph. I did this in the first patch: https://reviews.llvm.org/D153114. I think we can try to introduce that thread pool later than sooner since it may be better to keep the changes small instead of large. (This is what required in the first review page.)