[llvm-branch-commits] [clang-tools-extra] Release/20.x clangd modules (PR #143647)
Chuanqi Xu via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Tue Jun 10 20:32:52 PDT 2025
https://github.com/ChuanqiXu9 updated https://github.com/llvm/llvm-project/pull/143647
>From eabe027feb43511d2d2709df5b88367a2cb622d5 Mon Sep 17 00:00:00 2001
From: fleeting-xx <bakerdt at gmail.com>
Date: Thu, 5 Jun 2025 20:33:11 -0500
Subject: [PATCH 1/2] [clangd] [Modules] Fix to correctly handle module
dependencies (#142828)
This is a re-application of llvm/llvm-project#142090 without the unit
test changes. A subsequent PR will follow that adds a unit test for
module dependencies.
- Fix dangling string references in the return value of
getAllRequiredModules()
- Change a couple of calls in getOrBuildModuleFile() to use the loop
variable instead of the ModuleName parameter.
---
clang-tools-extra/clangd/ModulesBuilder.cpp | 18 ++--
clang-tools-extra/clangd/ProjectModules.h | 2 +-
.../clangd/ScanningProjectModules.cpp | 6 +-
.../clangd/test/module_dependencies.test | 96 +++++++++++++++++++
4 files changed, 110 insertions(+), 12 deletions(-)
create mode 100644 clang-tools-extra/clangd/test/module_dependencies.test
diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp
index bee31fe51555e..2d2f0f6374486 100644
--- a/clang-tools-extra/clangd/ModulesBuilder.cpp
+++ b/clang-tools-extra/clangd/ModulesBuilder.cpp
@@ -360,9 +360,9 @@ void ModuleFileCache::remove(StringRef ModuleName) {
/// Collect the directly and indirectly required module names for \param
/// ModuleName in topological order. The \param ModuleName is guaranteed to
/// be the last element in \param ModuleNames.
-llvm::SmallVector<StringRef> getAllRequiredModules(ProjectModules &MDB,
+llvm::SmallVector<std::string> getAllRequiredModules(ProjectModules &MDB,
StringRef ModuleName) {
- llvm::SmallVector<llvm::StringRef> ModuleNames;
+ llvm::SmallVector<std::string> ModuleNames;
llvm::StringSet<> ModuleNamesSet;
auto VisitDeps = [&](StringRef ModuleName, auto Visitor) -> void {
@@ -373,7 +373,7 @@ llvm::SmallVector<StringRef> getAllRequiredModules(ProjectModules &MDB,
if (ModuleNamesSet.insert(RequiredModuleName).second)
Visitor(RequiredModuleName, Visitor);
- ModuleNames.push_back(ModuleName);
+ ModuleNames.push_back(ModuleName.str());
};
VisitDeps(ModuleName, VisitDeps);
@@ -418,13 +418,13 @@ llvm::Error ModulesBuilder::ModulesBuilderImpl::getOrBuildModuleFile(
// Get Required modules in topological order.
auto ReqModuleNames = getAllRequiredModules(MDB, ModuleName);
for (llvm::StringRef ReqModuleName : ReqModuleNames) {
- if (BuiltModuleFiles.isModuleUnitBuilt(ModuleName))
+ if (BuiltModuleFiles.isModuleUnitBuilt(ReqModuleName))
continue;
if (auto Cached = Cache.getModule(ReqModuleName)) {
if (IsModuleFileUpToDate(Cached->getModuleFilePath(), BuiltModuleFiles,
TFS.view(std::nullopt))) {
- log("Reusing module {0} from {1}", ModuleName,
+ log("Reusing module {0} from {1}", ReqModuleName,
Cached->getModuleFilePath());
BuiltModuleFiles.addModuleFile(std::move(Cached));
continue;
@@ -432,14 +432,16 @@ llvm::Error ModulesBuilder::ModulesBuilderImpl::getOrBuildModuleFile(
Cache.remove(ReqModuleName);
}
+ std::string ReqFileName =
+ MDB.getSourceForModuleName(ReqModuleName);
llvm::Expected<ModuleFile> MF = buildModuleFile(
- ModuleName, ModuleUnitFileName, getCDB(), TFS, BuiltModuleFiles);
+ ReqModuleName, ReqFileName, getCDB(), TFS, BuiltModuleFiles);
if (llvm::Error Err = MF.takeError())
return Err;
- log("Built module {0} to {1}", ModuleName, MF->getModuleFilePath());
+ log("Built module {0} to {1}", ReqModuleName, MF->getModuleFilePath());
auto BuiltModuleFile = std::make_shared<const ModuleFile>(std::move(*MF));
- Cache.add(ModuleName, BuiltModuleFile);
+ Cache.add(ReqModuleName, BuiltModuleFile);
BuiltModuleFiles.addModuleFile(std::move(BuiltModuleFile));
}
diff --git a/clang-tools-extra/clangd/ProjectModules.h b/clang-tools-extra/clangd/ProjectModules.h
index 48d52ac9deb89..5296508e0584d 100644
--- a/clang-tools-extra/clangd/ProjectModules.h
+++ b/clang-tools-extra/clangd/ProjectModules.h
@@ -42,7 +42,7 @@ class ProjectModules {
llvm::unique_function<void(tooling::CompileCommand &, PathRef) const>;
virtual std::vector<std::string> getRequiredModules(PathRef File) = 0;
- virtual PathRef
+ virtual std::string
getSourceForModuleName(llvm::StringRef ModuleName,
PathRef RequiredSrcFile = PathRef()) = 0;
diff --git a/clang-tools-extra/clangd/ScanningProjectModules.cpp b/clang-tools-extra/clangd/ScanningProjectModules.cpp
index e4dc11c1c2895..859aba3673dc4 100644
--- a/clang-tools-extra/clangd/ScanningProjectModules.cpp
+++ b/clang-tools-extra/clangd/ScanningProjectModules.cpp
@@ -66,7 +66,7 @@ class ModuleDependencyScanner {
///
/// TODO: We should handle the case that there are multiple source files
/// declaring the same module.
- PathRef getSourceForModuleName(llvm::StringRef ModuleName) const;
+ std::string getSourceForModuleName(llvm::StringRef ModuleName) const;
/// Return the direct required modules. Indirect required modules are not
/// included.
@@ -140,7 +140,7 @@ void ModuleDependencyScanner::globalScan(
GlobalScanned = true;
}
-PathRef ModuleDependencyScanner::getSourceForModuleName(
+std::string ModuleDependencyScanner::getSourceForModuleName(
llvm::StringRef ModuleName) const {
assert(
GlobalScanned &&
@@ -189,7 +189,7 @@ class ScanningAllProjectModules : public ProjectModules {
/// RequiredSourceFile is not used intentionally. See the comments of
/// ModuleDependencyScanner for detail.
- PathRef
+ std::string
getSourceForModuleName(llvm::StringRef ModuleName,
PathRef RequiredSourceFile = PathRef()) override {
Scanner.globalScan(Mangler);
diff --git a/clang-tools-extra/clangd/test/module_dependencies.test b/clang-tools-extra/clangd/test/module_dependencies.test
new file mode 100644
index 0000000000000..1023b2363c9fa
--- /dev/null
+++ b/clang-tools-extra/clangd/test/module_dependencies.test
@@ -0,0 +1,96 @@
+# A smoke test to check that a simple dependency chain for modules can work.
+#
+# FIXME: This fails on the Windows ARM64 build server. Not entirely sure why as it has been tested on
+# an ARM64 Windows VM and appears to work there.
+# UNSUPPORTED: host=aarch64-pc-windows-msvc
+#
+# RUN: rm -fr %t
+# RUN: mkdir -p %t
+# RUN: split-file %s %t
+#
+# RUN: sed -e "s|DIR|%/t|g" %t/compile_commands.json.tmpl > %t/compile_commands.json.tmp
+# RUN: sed -e "s|CLANG_CC|%clang|g" %t/compile_commands.json.tmp > %t/compile_commands.json
+# RUN: sed -e "s|DIR|%/t|g" %t/definition.jsonrpc.tmpl > %t/definition.jsonrpc.tmp
+#
+# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
+# (with the extra slash in the front), so we add it here.
+# RUN: sed -E -e 's|"file://([A-Z]):/|"file:///\1:/|g' %/t/definition.jsonrpc.tmp > %/t/definition.jsonrpc
+#
+# RUN: clangd -experimental-modules-support -lit-test < %t/definition.jsonrpc \
+# RUN: | FileCheck -strict-whitespace %t/definition.jsonrpc
+
+#--- A-frag.cppm
+export module A:frag;
+export void printA() {}
+
+#--- A.cppm
+export module A;
+export import :frag;
+
+#--- Use.cpp
+import A;
+void foo() {
+ print
+}
+
+#--- compile_commands.json.tmpl
+[
+ {
+ "directory": "DIR",
+ "command": "CLANG_CC -fprebuilt-module-path=DIR -std=c++20 -o DIR/main.cpp.o -c DIR/Use.cpp",
+ "file": "DIR/Use.cpp"
+ },
+ {
+ "directory": "DIR",
+ "command": "CLANG_CC -std=c++20 DIR/A.cppm --precompile -o DIR/A.pcm",
+ "file": "DIR/A.cppm"
+ },
+ {
+ "directory": "DIR",
+ "command": "CLANG_CC -std=c++20 DIR/A-frag.cppm --precompile -o DIR/A-frag.pcm",
+ "file": "DIR/A-frag.cppm"
+ }
+]
+
+#--- definition.jsonrpc.tmpl
+{
+ "jsonrpc": "2.0",
+ "id": 0,
+ "method": "initialize",
+ "params": {
+ "processId": 123,
+ "rootPath": "clangd",
+ "capabilities": {
+ "textDocument": {
+ "completion": {
+ "completionItem": {
+ "snippetSupport": true
+ }
+ }
+ }
+ },
+ "trace": "off"
+ }
+}
+---
+{
+ "jsonrpc": "2.0",
+ "method": "textDocument/didOpen",
+ "params": {
+ "textDocument": {
+ "uri": "file://DIR/Use.cpp",
+ "languageId": "cpp",
+ "version": 1,
+ "text": "import A;\nvoid foo() {\n print\n}\n"
+ }
+ }
+}
+
+# CHECK: "message"{{.*}}printA{{.*}}(fix available)
+
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/completion","params":{"textDocument":{"uri":"file://DIR/Use.cpp"},"context":{"triggerKind":1},"position":{"line":2,"character":6}}}
+---
+{"jsonrpc":"2.0","id":2,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
>From 529551b9e06b1b41a81d516184d063cbbe1d43bd Mon Sep 17 00:00:00 2001
From: Hans Wennborg <hans at chromium.org>
Date: Tue, 10 Jun 2025 13:04:51 +0200
Subject: [PATCH 2/2] Disable clangd/test/module_dependencies.test on Windows
The test fails (sometimes); see discussion on https://github.com/llvm/llvm-project/pull/142828
---
clang-tools-extra/clangd/test/module_dependencies.test | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/clang-tools-extra/clangd/test/module_dependencies.test b/clang-tools-extra/clangd/test/module_dependencies.test
index 1023b2363c9fa..79306a73da435 100644
--- a/clang-tools-extra/clangd/test/module_dependencies.test
+++ b/clang-tools-extra/clangd/test/module_dependencies.test
@@ -1,8 +1,7 @@
# A smoke test to check that a simple dependency chain for modules can work.
#
-# FIXME: This fails on the Windows ARM64 build server. Not entirely sure why as it has been tested on
-# an ARM64 Windows VM and appears to work there.
-# UNSUPPORTED: host=aarch64-pc-windows-msvc
+# FIXME: The test fails on Windows; see comments on https://github.com/llvm/llvm-project/pull/142828
+# UNSUPPORTED: system-windows
#
# RUN: rm -fr %t
# RUN: mkdir -p %t
More information about the llvm-branch-commits
mailing list