[clangd] Support .clangd command line modifications for C++ modules

[petr-polezhaev]

Tunnels Manger object into the ScanningAllProjectModules so it can be used to perform necessary command-line modifications (which also adds --resources path previously added there explicitly). This allows using the experimental C++ modules support with gcc.

This was discussed in the issue with @ChuanqiXu9 and @kadircet

Closes #112635

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clangd

@llvm/pr-subscribers-clang-tools-extra

Author: Petr Polezhaev (petr-polezhaev)

Changes

Tunnels Manger object into the ScanningAllProjectModules so it can be used to perform necessary command-line modifications (which also adds --resources path previously added there explicitly). This allows using the experimental C++ modules support with gcc.

This was discussed in the issue with @ChuanqiXu9 and @kadircet

Closes #112635

---

Full diff: https://github.com/llvm/llvm-project/pull/122606.diff

5 Files Affected:

• (modified) clang-tools-extra/clangd/GlobalCompilationDatabase.cpp (+10-1)
• (modified) clang-tools-extra/clangd/GlobalCompilationDatabase.h (+3)
• (modified) clang-tools-extra/clangd/ProjectModules.h (+7)
• (modified) clang-tools-extra/clangd/ScanningProjectModules.cpp (+24-15)
• (modified) clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp (+46-3)

diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
index 71e97ac4efd673..ea2ff3d604efbc 100644
--- a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
+++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
@@ -830,6 +830,16 @@ bool OverlayCDB::setCompileCommand(PathRef File,
   return true;
 }
 
+std::unique_ptr<ProjectModules>
+OverlayCDB::getProjectModules(PathRef File) const {
+  auto MDB = DelegatingCDB::getProjectModules(File);
+  MDB->setCommandProvider([&Mangler = Mangler](tooling::CompileCommand &Command,
+                                               PathRef CommandPath) {
+    Mangler(Command, CommandPath);
+  });
+  return std::move(MDB);
+}
+
 DelegatingCDB::DelegatingCDB(const GlobalCompilationDatabase *Base)
     : Base(Base) {
   if (Base)
@@ -874,6 +884,5 @@ bool DelegatingCDB::blockUntilIdle(Deadline D) const {
     return true;
   return Base->blockUntilIdle(D);
 }
-
 } // namespace clangd
 } // namespace clang
diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.h b/clang-tools-extra/clangd/GlobalCompilationDatabase.h
index f8349c6efecb01..1d636d73664bee 100644
--- a/clang-tools-extra/clangd/GlobalCompilationDatabase.h
+++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.h
@@ -209,6 +209,9 @@ class OverlayCDB : public DelegatingCDB {
   setCompileCommand(PathRef File,
                     std::optional<tooling::CompileCommand> CompilationCommand);
 
+  std::unique_ptr<ProjectModules>
+  getProjectModules(PathRef File) const override;
+
 private:
   mutable std::mutex Mutex;
   llvm::StringMap<tooling::CompileCommand> Commands; /* GUARDED_BY(Mut) */
diff --git a/clang-tools-extra/clangd/ProjectModules.h b/clang-tools-extra/clangd/ProjectModules.h
index 3b9b564a87da01..6830d44919fa81 100644
--- a/clang-tools-extra/clangd/ProjectModules.h
+++ b/clang-tools-extra/clangd/ProjectModules.h
@@ -9,8 +9,10 @@
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_PROJECTMODULES_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_PROJECTMODULES_H
 
+#include "support/Function.h"
 #include "support/Path.h"
 #include "support/ThreadsafeFS.h"
+#include "clang/Tooling/CompilationDatabase.h"
 
 #include <memory>
 
@@ -36,11 +38,16 @@ namespace clangd {
 /// `<primary-module-name>[:partition-name]`. So module names covers partitions.
 class ProjectModules {
 public:
+  using CommandProvider =
+      llvm::unique_function<void(tooling::CompileCommand &, PathRef) const>;
+
   virtual std::vector<std::string> getRequiredModules(PathRef File) = 0;
   virtual PathRef
   getSourceForModuleName(llvm::StringRef ModuleName,
                          PathRef RequiredSrcFile = PathRef()) = 0;
 
+  virtual void setCommandProvider(CommandProvider Provider) {}
+
   virtual ~ProjectModules() = default;
 };
 
diff --git a/clang-tools-extra/clangd/ScanningProjectModules.cpp b/clang-tools-extra/clangd/ScanningProjectModules.cpp
index 92f75ef7d5c25a..6f098c18cd80a0 100644
--- a/clang-tools-extra/clangd/ScanningProjectModules.cpp
+++ b/clang-tools-extra/clangd/ScanningProjectModules.cpp
@@ -32,6 +32,9 @@ namespace {
 /// interfere with each other.
 class ModuleDependencyScanner {
 public:
+  using CommandProvider =
+      llvm::unique_function<void(tooling::CompileCommand &, PathRef) const>;
+
   ModuleDependencyScanner(
       std::shared_ptr<const clang::tooling::CompilationDatabase> CDB,
       const ThreadsafeFS &TFS)
@@ -48,7 +51,8 @@ class ModuleDependencyScanner {
   };
 
   /// Scanning the single file specified by \param FilePath.
-  std::optional<ModuleDependencyInfo> scan(PathRef FilePath);
+  std::optional<ModuleDependencyInfo> scan(PathRef FilePath,
+                                           CommandProvider const &Provider);
 
   /// Scanning every source file in the current project to get the
   /// <module-name> to <module-unit-source> map.
@@ -57,7 +61,7 @@ class ModuleDependencyScanner {
   /// a global module dependency scanner to monitor every file. Or we
   /// can simply require the build systems (or even the end users)
   /// to provide the map.
-  void globalScan();
+  void globalScan(CommandProvider const &Provider);
 
   /// Get the source file from the module name. Note that the language
   /// guarantees all the module names are unique in a valid program.
@@ -69,7 +73,8 @@ class ModuleDependencyScanner {
 
   /// Return the direct required modules. Indirect required modules are not
   /// included.
-  std::vector<std::string> getRequiredModules(PathRef File);
+  std::vector<std::string> getRequiredModules(PathRef File,
+                                              CommandProvider const &Provider);
 
 private:
   std::shared_ptr<const clang::tooling::CompilationDatabase> CDB;
@@ -87,7 +92,8 @@ class ModuleDependencyScanner {
 };
 
 std::optional<ModuleDependencyScanner::ModuleDependencyInfo>
-ModuleDependencyScanner::scan(PathRef FilePath) {
+ModuleDependencyScanner::scan(PathRef FilePath,
+                              CommandProvider const &Provider) {
   auto Candidates = CDB->getCompileCommands(FilePath);
   if (Candidates.empty())
     return std::nullopt;
@@ -97,10 +103,8 @@ ModuleDependencyScanner::scan(PathRef FilePath) {
   // DirectoryBasedGlobalCompilationDatabase::getCompileCommand.
   tooling::CompileCommand Cmd = std::move(Candidates.front());
 
-  static int StaticForMainAddr; // Just an address in this process.
-  Cmd.CommandLine.push_back("-resource-dir=" +
-                            CompilerInvocation::GetResourcesPath(
-                                "clangd", (void *)&StaticForMainAddr));
+  if (Provider)
+    Provider(Cmd, FilePath);
 
   using namespace clang::tooling::dependencies;
 
@@ -130,9 +134,9 @@ ModuleDependencyScanner::scan(PathRef FilePath) {
   return Result;
 }
 
-void ModuleDependencyScanner::globalScan() {
+void ModuleDependencyScanner::globalScan(CommandProvider const &Provider) {
   for (auto &File : CDB->getAllFiles())
-    scan(File);
+    scan(File, Provider);
 
   GlobalScanned = true;
 }
@@ -150,9 +154,9 @@ PathRef ModuleDependencyScanner::getSourceForModuleName(
   return {};
 }
 
-std::vector<std::string>
-ModuleDependencyScanner::getRequiredModules(PathRef File) {
-  auto ScanningResult = scan(File);
+std::vector<std::string> ModuleDependencyScanner::getRequiredModules(
+    PathRef File, CommandProvider const &CmdProvider) {
+  auto ScanningResult = scan(File, CmdProvider);
   if (!ScanningResult)
     return {};
 
@@ -177,7 +181,11 @@ class ScanningAllProjectModules : public ProjectModules {
   ~ScanningAllProjectModules() override = default;
 
   std::vector<std::string> getRequiredModules(PathRef File) override {
-    return Scanner.getRequiredModules(File);
+    return Scanner.getRequiredModules(File, Provider);
+  }
+
+  void setCommandProvider(CommandProvider Provider) override {
+    this->Provider = std::move(Provider);
   }
 
   /// RequiredSourceFile is not used intentionally. See the comments of
@@ -185,12 +193,13 @@ class ScanningAllProjectModules : public ProjectModules {
   PathRef
   getSourceForModuleName(llvm::StringRef ModuleName,
                          PathRef RequiredSourceFile = PathRef()) override {
-    Scanner.globalScan();
+    Scanner.globalScan(Provider);
     return Scanner.getSourceForModuleName(ModuleName);
   }
 
 private:
   ModuleDependencyScanner Scanner;
+  CommandProvider Provider;
 };
 
 std::unique_ptr<ProjectModules> scanningProjectModules(
diff --git a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp
index 1bb8e19cce23e0..b48ab0bd6ff6f9 100644
--- a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp
+++ b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp
@@ -9,6 +9,7 @@
 
 /// FIXME: Skip testing on windows temporarily due to the different escaping
 /// code mode.
+#include "llvm/ADT/StringRef.h"
 #ifndef _WIN32
 
 #include "ModulesBuilder.h"
@@ -39,7 +40,25 @@ class MockDirectoryCompilationDatabase : public MockCompilationDatabase {
   void addFile(llvm::StringRef Path, llvm::StringRef Contents);
 
   std::unique_ptr<ProjectModules> getProjectModules(PathRef) const override {
-    return scanningProjectModules(MockedCDBPtr, TFS);
+    auto Modules = scanningProjectModules(MockedCDBPtr, TFS);
+    Modules->setCommandProvider(
+        [this](tooling::CompileCommand &Command, PathRef) {
+          for (auto &Flag : ExcludeFlags) {
+            auto const It = std::find(Command.CommandLine.begin(),
+                                      Command.CommandLine.end(), Flag);
+            if (It != Command.CommandLine.end())
+              Command.CommandLine.erase(It);
+          }
+        });
+    return Modules;
+  }
+
+  void addExtraClangFlag(std::string Flag) {
+    this->ExtraClangFlags.push_back(std::move(Flag));
+  }
+
+  void addExcludedFlag(std::string Flag) {
+    this->ExcludeFlags.push_back(std::move(Flag));
   }
 
 private:
@@ -66,6 +85,7 @@ class MockDirectoryCompilationDatabase : public MockCompilationDatabase {
   };
 
   std::shared_ptr<MockClangCompilationDatabase> MockedCDBPtr;
+  std::vector<std::string> ExcludeFlags;
   const ThreadsafeFS &TFS;
 };
 
@@ -191,6 +211,29 @@ export module M;
   EXPECT_TRUE(MInfo->canReuse(*Invocation, FS.view(TestDir)));
 }
 
+TEST_F(PrerequisiteModulesTests, ModuleWithArgumentPatch) {
+  MockDirectoryCompilationDatabase CDB(TestDir, FS);
+
+  CDB.addExtraClangFlag("-invalid-unknown-flag");
+
+  CDB.addFile("Dep.cppm", R"cpp(
+export module Dep;
+  )cpp");
+
+  CDB.addFile("M.cppm", R"cpp(
+export module M;
+import Dep;
+  )cpp");
+
+  auto ProjectModules = CDB.getProjectModules(getFullPath("M.cppm"));
+  EXPECT_TRUE(
+      ProjectModules->getRequiredModules(getFullPath("M.cppm")).empty());
+
+  CDB.addExcludedFlag("-invalid-unknown-flag");
+  EXPECT_FALSE(
+      ProjectModules->getRequiredModules(getFullPath("M.cppm")).empty());
+}
+
 TEST_F(PrerequisiteModulesTests, ModuleWithDepTest) {
   MockDirectoryCompilationDatabase CDB(TestDir, FS);
 
@@ -435,7 +478,7 @@ void func() {
                     /*Callback=*/nullptr);
   EXPECT_TRUE(Preamble);
   EXPECT_TRUE(Preamble->RequiredModules);
-  
+
   auto Result = codeComplete(getFullPath("Use.cpp"), Test.point(),
                              Preamble.get(), Use, {});
   EXPECT_FALSE(Result.Completions.empty());
@@ -474,7 +517,7 @@ void func() {
                     /*Callback=*/nullptr);
   EXPECT_TRUE(Preamble);
   EXPECT_TRUE(Preamble->RequiredModules);
-  
+
   auto Result = signatureHelp(getFullPath("Use.cpp"), Test.point(),
                               *Preamble.get(), Use, MarkupKind::PlainText);
   EXPECT_FALSE(Result.signatures.empty());

[ChuanqiXu9]

Do we need to update buildModuleFile in clang-tools-extra/clangd/ModulesBuilder.cpp to update the commands to build the module interfaces?

[ChuanqiXu9]

Move the include after the #ifndef

[petr-polezhaev]

Do we need to update buildModuleFile in clang-tools-extra/clangd/ModulesBuilder.cpp to update the commands to build the module interfaces?

It uses GlobalCompilationDatabase::getCompileCommand (if I correctly understand what you mean) which is overridden by OverlayCDB::getCompileCommand which does call the Mangler before returning the command, so that should already be covered

[ChuanqiXu9]

Thanks. This looks good to me. And please give @kadircet some time to give it a look. I'll try to land this in the next week if no more comments came in.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[petr-polezhaev]

Ok. I'll wait for review confirmation and then fix the clang-format thing and merge new main

[kadircet]

thanks for working on this!

[petr-polezhaev]

@kadircet this is what I was talking about with unsafe reference capturing. The solution (while keeping lambda compatibility for unit-tests) would be to make the function copyable but pass the Mangler by the shared_ptr into the very first instance.

[kadircet]

thanks!

[petr-polezhaev]

rebased on the latest main squashing all fixes

[ChuanqiXu9]

Given @kadircet approved and @petr-polezhaev is marked as first-time contributor, I'll merge this by assuming you don't have the commit access.

[ChuanqiXu9]

@petr-polezhaev please update the mail address. We don't like github.reply mail address.

[petr-polezhaev]

Didn't knew that's a thing now. Removed the checkbox over the private email, should be ok now. Somehow missed the clang-format issue again (I thought I fixed it) - updated to calm it down.

EDIT: Yes, I don't have commit access. Please merge it, thank you

[github-actions]

@petr-polezhaev Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!