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

Tue Feb 14 08:36:01 PST 2023

DmitryPolukhin added a comment.

In D143436#4122594 <https://reviews.llvm.org/D143436#4122594>, @kadircet wrote:

> 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.

Thank you for sharing the context that I didn't know. I think it makes some sense but it requires duplication of this logic in some other places and keeping them up-to-date with driver changes.

> 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 think clangd should be able to make this transformations if they are required even for "fixed" flags and it shouldn't change anything if compilation flags don't require them.

> 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.

I see your point and it does make sense to me. I'll try to move logic to CommandMangler instead so both codepaths will share it. As for keeping up with driver changes, it is real issue and I'm not sure that we can solve this problem without using driver code almost "as is" that is not possible without serious refactoring. I'll ping you when changes are ready for review.

================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1350
+            ->getCompileCommands(File)[0]);
     if (Old != New) {
       CDB->setCompileCommand(File, std::move(New));
----------------
kadircet wrote:
> 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.
I think it won't hurt anything and it was not very useful even before my change. Now it just prevent small extra work when you set the same arguments more than one on arguments are very simple so no transformation actually happening.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436