[PATCH] D133757: [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks, i agree that we should make query driver work coherently with `CompileFlags.Compiler`, but I am not sure if we really need to complexity for handling this adjustment logic before resolving the driver. I've got some comments right next to the change but to summarize to possible interactions i can think of:

- User has an absolute path either through compile commands.json or config, which means resolve logic is going to be no-op (well apart form symlink resolution, which I believe is tricky/rare enough that we shouldn't try to special case things for the sake of it).
- We've got just the driver name, and it's generic like `gcc`, I don't think we should do anything custom here (I didn't go through all the bugs you've linked, so PLMK if there are common counter examples to this) because default clang heuristics and gcc heuristics we would get out of a system-installed gcc should be pretty much the same. and if the user has a custom system installed gcc, they can either modify their compile commands.json or update clangd config to provide the absolute path of the compiler. I am saying this is rare, because usually those custom toolchains have the target in the binary name.
- We've got a custom driver name, e.g. `arm-none-eabi-gcc`. I think what we today is again the desired behaviour, we might try to look for it inside the directory mentioned in the compile commands though, when it isn't found inside $PATH.

Another question is actually about performing the driver resolution before/after we apply the user-supplied edits. in theory we might end up overriding a user provided `CompileFlags.Compiler` and I don't really have a good sense of what would be desired here.



================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:310
+  //    that needs to be invoked may come from the CompileFlags->Compiler key
+  //  - BEFORE resolveDriver() because that can mess up the driver path,
+  //    e.g. changing gcc to /path/to/clang/bin/gcc
----------------
why is this necessary? i.e. are there cases where heuristics in clang-driver today cannot identify the same set of includes we would extract from a system installed `gcc` ?

because if we were to get rid of this requirement, we can just run this as an extra arg adjuster after running command mangler, without changing the logic in CommandMangler at all (and also rendering the underlying patch unnecessary (i am still not sure if it's needed, but need to take a closer look there)).


================
Comment at: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp:242
+CompileFlags:
+  Compiler: gcc
+  )yaml";
----------------
nridge wrote:
> To exercise `QueryDriverDatabase`, this test assumes that the machine running the test will have a `gcc` somewhere in its path, and that querying it for includes will result in at least one `-isystem` flag being added to the command line.
> 
> Is this a reasonable assumption for buildbots? Or do we need to introduce some kind of abstraction so that the test doesn't actually try to find and execute gcc?
i think it's better to have this as a lit test rather than a unittest. that way we can set up the temp env in the lit test with appropriate paths and check for the output of `clangd --check=foo.cc`. (we should also have a mock shell script called gcc that'll print out expected lines, we already have this in `clang-tools-extra/clangd/test/system-include-extractor.test`

because this doesn't only rely on existence of `gcc` on buildbots, but also anyone that wants to run clangd-check locally (and might interact weirdly depending on where/which gcc is installed).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133757