@llvm/issue-subscribers-clang-modules

Author: Petr Polezhaev (petr-polezhaev)

[
    {
      "directory": "/path/to/example",
      "command": "/usr/bin/clang++ -std=c++23 -ffoobar test.cppm",
      "file": "test.cppm",
      "output": "build/test.cppm.o"
    },
    {
        "directory": "/path/to/example",
        "command": "/usr/bin/clang++ -std=c++23 -ffoobar main.cpp",
        "file": "main.cpp",
        "output": "build/main.cpp.o"
    }
  ]

CompileFlags:
    CompilationDatabase: "."
    Remove:
    - "-ffoobar"

export module test;

export void test() {}

import test;

int main() { return 0; }

E[02:40:54.668] Scanning modules dependencies for /home/test/clangd_modules_test/test.cppm failed: error: unknown argument: '-ffoobar'

I[02:40:54.668] Built prerequisite modules for file /home/test/clangd_modules_test/test.cppm in 0.00 seconds
E[02:40:54.668] Scanning modules dependencies for /home/test/clangd_modules_test/main.cpp failed: error: unknown argument: '-ffoobar'

I[02:40:54.668] Built prerequisite modules for file /home/test/clangd_modules_test/main.cpp in 0.00 seconds

@llvm/issue-subscribers-clangd

Author: Petr Polezhaev (petr-polezhaev)

[
    {
      "directory": "/path/to/example",
      "command": "/usr/bin/clang++ -std=c++23 -ffoobar test.cppm",
      "file": "test.cppm",
      "output": "build/test.cppm.o"
    },
    {
        "directory": "/path/to/example",
        "command": "/usr/bin/clang++ -std=c++23 -ffoobar main.cpp",
        "file": "main.cpp",
        "output": "build/main.cpp.o"
    }
  ]

CompileFlags:
    CompilationDatabase: "."
    Remove:
    - "-ffoobar"

export module test;

export void test() {}

import test;

int main() { return 0; }

E[02:40:54.668] Scanning modules dependencies for /home/test/clangd_modules_test/test.cppm failed: error: unknown argument: '-ffoobar'

I[02:40:54.668] Built prerequisite modules for file /home/test/clangd_modules_test/test.cppm in 0.00 seconds
E[02:40:54.668] Scanning modules dependencies for /home/test/clangd_modules_test/main.cpp failed: error: unknown argument: '-ffoobar'

I[02:40:54.668] Built prerequisite modules for file /home/test/clangd_modules_test/main.cpp in 0.00 seconds

I can fix this (and did so in a hack locally) but I would like to know what was the intent in code to know hohw to fix.

As far as I can tell, in the codebase you are meant to use GlobalCompilationDatabase that does caching/mangling/etc. Yet ScanningAllProjectModules and ModuleDependencyScanner use tooling::CompilationDatabase directly and thus are missing on the mangling (the thing that adds the resource path and applies argument patching).

I see three options for fixing this:

1. Move the Mangler (and potentially other modificators) from OverlayCDB into a new CompilationDatabase wrapper that will perform the mangling on the level similar to the tooling::CompilationDatabase and will provide same API as tooling::CompilationDatabase (including getAllFiles()). This will probably break a lot of unit tests and will require fixes all over the codebase.
2. Add the Mangler (shared with OverlayCDB) to the ScanningAllProjectModules's version of CDB. I.e. do the same as 1 but localized to just the module scanning. The Mangler will be passed by the OverlayCDB's override of getProjectModules (new) and the normal version in GlobalCompilationDatabase will create it without the Mangler (nullptr)
3. Decouple GlobalCompilationDatabase and ProjectModules and instead have GlobalProjectModules (to mimic the name) with a similar setup and potentially similar caching logic later on. Make ScanningAllProjectModules use GlobalCompilationDatabase instead of a tooling::CompilationDatabase directly and expose a method to replicate getAllFiles for global scanning

I think 2 is the easiest, 1 is probably goes against the codebase intents and 3 is (IMO) the most correct one but I am not sure why it was coupled with the compilation database in the first place.

@ChuanqiXu9 since you are the author of the implementation in the first place, what is your preference?

Pretty appreciated to see someone else to take part in this area! I prefer 3. Maybe it worths to take a look at https://github.com/ChuanqiXu9/clangd-for-modules for my following plan.

Previously, in the first step, we hope the compilation of different TUs won't interact with each other so that we can avoid some thread safe problems. Then we tried to create ProjectModules from GlobalCompilationDatabase given we don't track a global state of modules. But this may be more or less a mistake. To make different TUs can share BMI of modules, (this is necessary, or the performance of modules in clangd will be ridiculous), I think we have to make the ProjectModules to track the global state of modules. Then we should be able to create ProjectModules without creating GlobalCompilationDatabase. But we need information from GlobalCompilationDatabase, so I tried to introduce GlobalCompilationDatabase::updateProjectModules(PathRef File, ProjectModules &) . Maybe this can save some coding for what you said with a similar setup and potentially similar caching logic later on.

I'd actually recommend something like 2).

Coupling of GlobalCompilationDatabase and ProjectModules is very much intentional. Because at the end of the day it's the underlying compilation-database that provides modules graph. Trying to mimic something like 3) will also achieve this, but I am afraid it'll come at the cost of code duplication and divergence overtime. I believe we should have a single mechanism for going from a file path to its governing compilation-database structure.

Moreover getAllFiles is a detail needed specifically by ScanningAllProjectModules, it won't scale to big monorepos that clangd supports today (e.g. chromium). Hence we shouldn't rely on it in any abstract interface, in the long run we should be implementing ProjectModules interfaces via point queries to build systems (or other information sources) rather than performing global scans (e.g. something like module-map files in @ChuanqiXu9's branch).

A similar argument goes for needing compile flags, ScanningAllProjectModules needs to run a clang-action on each source file to figure out module provided by that source and its module dependencies. In an ideal world, this operation shouldn't require running clang either. But asking for compile flags of a particular modulemap/sourcefile is less controversial than asking for them all, during the initial review the need wasn't obvious and layering was problematic (and #66462 was getting quite long already). So I asked to go with the simplest approach.

So I'd rather introduce:

struct ProjectModules {
  ...
  using CmdProvider = std::function<std::optional<tooling::CompileCommand>(PathRef)>
  virtual void setCmdProvider(CmdProvider C) {} 
  ...
};

and then call this in OverlayCDB::getProjectModules, with a lambda dispatching to OverlayCDB::getCompileCommand to make sure we're using the exact same logic.

Sorry for disappearing - I missed the notification in mail and only saw your replies now. virtual void setCmdProvider(CmdProvider C) {} approach is pretty much what I did locally to fix this issue. I can clean it up and will send a PR.