[PATCH] D93600: [clangd] When querying drivers by binary, look in PATH too

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this looks like a good idea to me, but some quibbles with the details:

- for any string, we statically know whether it's path or WD-relative
- we should make sure all the IO gets cached



================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:334
 
     llvm::SmallString<128> Driver(Cmd->CommandLine.front());
     llvm::sys::fs::make_absolute(Cmd->Directory, Driver);
----------------
Oops, this has come up before (in CommandMangler).
Your idea here makes sense (if the driver is "clang" we should look it up on the PATH) and the approach is backwards-compatible.

However when we think more carefully, by allowing the driver to be both WD-relative and dir-relative, we're emulating shell behavior (which makes sense, we have an argv!).
But shells don't generally try one then the other, they instead distinguish three cases:
 - bare name `clang` is looked up on PATH
 - relative path `bin/clang` is resolved against WD
 - absolute path `/usr/bin/clang` is used as-is

So I think we'd rather have something like:
```
if (llvm::none_of(Driver,
                     [](char C) { return llvm::sys::path::is_separator(C); })) {
  // resolve against path
} else {
  make_absolute(Cmd->Directory, Driver);
}
```


================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:337
 
+    if (!llvm::sys::fs::exists(Driver)) {
+      auto DriverProgram =
----------------
I'm not sure it's reasonable to do IO like this before we have a chance to hit the cache (but see above comment, I don't think we need the `exists()` at all)


================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:339
+      auto DriverProgram =
+          llvm::sys::findProgramByName(Cmd->CommandLine.front());
+      if (DriverProgram) {
----------------
hmm, this also does IO, and we obviously do need this.

I think we need to move this into the cached lookup, so:
 - if the path is "bin/clangd" we resolve it to absolute before querying the cache
 - if the path is "/usr/bin/clangd" we pass that absolute path to cache
 - but if the path is "clangd" we pass that exact string into extractSystemIncludesAndTarget(), which notices that it's relative and looks it up on PATH. Fortunately we can assume PATH doesn't change while running, so the cache is still correct. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93600