[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

> There is a discrepancy between how clangd processes CDB loaded from JSON file on disk and pushed via LSP.

That's actually because we model compile commands pushed via LSP as a "fixed compilation database" rather than a json compilation database (you can check the code in `parseFixed`, near `parseJson`).
The reason behind the discrepancy between fixed and json compilation databases mostly arises from the fact that the former is written by people specifically to be used by clang-tooling, whereas the latter is usually provided by build system and doesn't necessarily have the same concerns as clang-tooling hence requires certain modifications.

That being said, the two particular transformations (response files & target/mode inference) seem like outliers when it comes to such transformations. These are handled by clang-driver binary, outside of the driver library, in https://github.com/llvm/llvm-project/blob/main/clang/tools/driver/driver.cpp#L426 and https://github.com/llvm/llvm-project/blob/main/clang/tools/driver/driver.cpp#L534. Therefore all the other tools that wants to make use of clang infra tries to simulate this logic, at different layers with slight differences (at the bare minimum, as the logic isn't shared and people mostly care about the driver, logic in other places just gets out-of-date).
I believe the right thing to do here is putting all that arg preparation logic into driver library itself, so that all code paths that tries to create a compiler invocation can have the same behaviour. Unfortunately this is likely too hard to pull at this stage, due to people relying on almost every implementation detail of these interfaces.
So a middle ground would probably involve, moving that logic inside driver binary to a common library, so that future users can at least directly use it rather than trying to come up with their own interpretation (or trying to chose from existing N patterns) and we'll get rid of the issues that result from logic in this place and its duplicates getting out-of-sync. This way we don't need to migrate all the applications that want to create a compiler invocation to a new API and can only migrate clangd (CommandMangler is I believe the right layer for these purposes. as it handles all "string"-like modifications on the compile flags today).

Does that make sense? Is it something you'd like to propose a patch for? Because as things stand I think we're just making the matter worse here by introducing some new code paths that're trying to emulate the logic in driver today and will get out of sync at some point.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1350
+            ->getCompileCommands(File)[0]);
     if (Old != New) {
       CDB->setCompileCommand(File, std::move(New));
----------------
it looks like this comparison is no longer valid.

`OverlayCDB::getCompileCommand` will apply more modifications to the stored flags than just expanding response files and inferring targets. I believe the intent of this interaction was to treat compile flags from the LSP client as-is without any modifications (as I explained more in my main comment).

No action needed right now, just thinking out-loud. I think the proper thing to do here is apply these "common driver transformations" here, and store/use the flags as is going forward inside clangd, without extra mangling.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143436/new/

https://reviews.llvm.org/D143436