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

[rapgenic]

Summary

Sometimes compile_commands.json databases are created without an
absolute path for the driver in the command field. By default the driver
name is appended to the current directory, however if no driver is found
in that location assume it was in the default PATH and try finding it
there

[rapgenic]

Just wanted to say that this is my first patch submission to the LLVM/clangd project, so please call me out on any mistake!

The arc tool caught me by surprise by opening the revision before I could double check it, hence the rename and edits...

[sammccall]

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

clang-tools-extra/clangd/QueryDriverDatabase.cpp

350

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);
}

357

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)

359

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.

[rapgenic]

I think I have fixed what you asked!

About the test, I thought it would be reasonable to test the three cases separately, however I cannot get it to work with a single test file (I think it's some problem with the generated files). Should I duplicate it or is there any way to do it better?

Sorry for my trivial question, but I'm still learning to navigate the codebase...

[sammccall]

Thanks, this looks good, just a couple of nits.

I guess you don't have commit access, I'm happy to land this for you when you're happy with it. Just need an email address to associate the commit with.

In D93600#2467393, @rapgenic wrote:

About the test, I thought it would be reasonable to test the three cases separately, however I cannot get it to work with a single test file (I think it's some problem with the generated files). Should I duplicate it or is there any way to do it better?

Yeah, these tests are a pain. I think you'd have to duplicate it, but honestly that's a maintenance trap, and it's really hard to factor out common logic in lit tests.
I think folding in the complicated case into the existing test as you've done is probably best.

clang-tools-extra/clangd/QueryDriverDatabase.cpp

145

maybe add an assert inside this if that there are no separators?

151

else log and return none?
We don't want to continue if we couldn't find clang on the path.

351–355

nit: !llvm::none_of -> llvm::any_of

[rapgenic]

Done! My email is:

Giulio Girardi <[e-mail]>

Thank you for your help!

[sammccall]

Sorry for the delay getting this landed, and thanks for the patch!
It'll be part of the clangd 12 release.

clang-tools-extra/clangd/QueryDriverDatabase.cpp

llvm::Optional<DriverInfo> parseDriverOutput(llvm::StringRef Output) {
   if (State == IncludesExtracting) {
     elog("System include extraction: end marker missing: {0}", Output);
     return llvm::None;
   }
   return std::move(Info);
 }
 
 llvm::Optional<DriverInfo>
-extractSystemIncludesAndTarget(PathRef Driver, llvm::StringRef Lang,
+extractSystemIncludesAndTarget(llvm::SmallString<128> Driver,
+                               llvm::StringRef Lang,
                                llvm::ArrayRef<std::string> CommandLine,
                                const llvm::Regex &QueryDriverRegex) {
   trace::Span Tracer("Extract system includes and target");
+
+  if (!llvm::sys::path::is_absolute(Driver)) {
+    assert(llvm::none_of(
+        Driver, [](char C) { return llvm::sys::path::is_separator(C); }));
+    auto DriverProgram = llvm::sys::findProgramByName(Driver);
+    if (DriverProgram) {
+      vlog("System include extraction: driver {0} expanded to {1}", Driver,
+           *DriverProgram);
+      Driver = *DriverProgram;
+    } else {
+      elog("System include extraction: driver {0} not found in PATH", Driver);
+      return llvm::None;
+    }
+  }
+
   SPAN_ATTACH(Tracer, "driver", Driver);
   SPAN_ATTACH(Tracer, "lang", Lang);
 
   if (!QueryDriverRegex.match(Driver)) {
     vlog("System include extraction: not allowed driver {0}", Driver);
     return llvm::None;
   }
 
if (Lang.empty()) {
       auto Type = driver::types::lookupTypeForExtension(Ext);
       if (Type == driver::types::TY_INVALID) {
         elog("System include extraction: invalid file type for {0}", Ext);
         return {};
       }
       Lang = driver::types::getTypeName(Type);
     }
 
     llvm::SmallString<128> Driver(Cmd->CommandLine.front());
+    if (llvm::any_of(Driver,
+                       [](char C) { return llvm::sys::path::is_separator(C); }))
+      // Driver is a not a single executable name but instead a path (either
+      // relative or absolute).
       llvm::sys::fs::make_absolute(Cmd->Directory, Driver);
 
     if (auto Info =
             QueriedDrivers.get(/*Key=*/(Driver + ":" + Lang).str(), [&] {
               return extractSystemIncludesAndTarget(
                   Driver, Lang, Cmd->CommandLine, QueryDriverRegex);
             })) {
       setTarget(addSystemIncludes(*Cmd, Info->SystemIncludes), Info->Target);
     }
     return Cmd;
   }
 
   llvm::Optional<ProjectInfo> getProjectInfo(PathRef File) const override {

clang-tools-extra/clangd/test/system-include-extractor.test

 # RUN: rm -rf %t.dir && mkdir -p %t.dir
 
 # The mock driver below is a shell script:
 # REQUIRES: shell
 
+# Create a bin directory to store the mock-driver and add it to the path
+# RUN: mkdir -p %t.dir/bin
+# RUN: export PATH=%t.dir/bin:$PATH
 # Generate a mock-driver that will print %temp_dir%/my/dir and
 # %temp_dir%/my/dir2 as include search paths.
-# RUN: echo '#!/bin/sh' >> %t.dir/my_driver.sh
-# RUN: echo '[ "$0" = "%t.dir/my_driver.sh" ] || exit' >> %t.dir/my_driver.sh
-# RUN: echo 'args="$*"' >> %t.dir/my_driver.sh
-# RUN: echo '[ -z "${args##*"-nostdinc"*}" ] || exit' >> %t.dir/my_driver.sh
-# RUN: echo '[ -z "${args##*"-isysroot=/isysroot"*}" ] || exit' >> %t.dir/my_driver.sh
-# RUN: echo 'echo " $* " | grep " --sysroot /my/sysroot/path " || exit' >> %t.dir/my_driver.sh
-# RUN: echo 'echo line to ignore >&2' >> %t.dir/my_driver.sh
-# RUN: echo 'printf "Target: arm-linux-gnueabihf\r\n" >&2' >> %t.dir/my_driver.sh
-# RUN: echo 'printf "#include <...> search starts here:\r\n" >&2' >> %t.dir/my_driver.sh
-# RUN: echo 'echo %t.dir/my/dir/ >&2' >> %t.dir/my_driver.sh
-# RUN: echo 'echo %t.dir/my/dir2/ >&2' >> %t.dir/my_driver.sh
-# RUN: echo 'printf "End of search list.\r\n" >&2' >> %t.dir/my_driver.sh
-# RUN: chmod +x %t.dir/my_driver.sh
+# RUN: echo '#!/bin/sh' >> %t.dir/bin/my_driver.sh
+# RUN: echo '[ "$0" = "%t.dir/bin/my_driver.sh" ] || exit' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'args="$*"' >> %t.dir/bin/my_driver.sh
+# RUN: echo '[ -z "${args##*"-nostdinc"*}" ] || exit' >> %t.dir/bin/my_driver.sh
+# RUN: echo '[ -z "${args##*"-isysroot=/isysroot"*}" ] || exit' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'echo " $* " | grep " --sysroot /my/sysroot/path " || exit' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'echo line to ignore >&2' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'printf "Target: arm-linux-gnueabihf\r\n" >&2' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'printf "#include <...> search starts here:\r\n" >&2' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'echo %t.dir/my/dir/ >&2' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'echo %t.dir/my/dir2/ >&2' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'printf "End of search list.\r\n" >&2' >> %t.dir/bin/my_driver.sh
+# RUN: chmod +x %t.dir/bin/my_driver.sh
 
 # Create header files my/dir/a.h and my/dir2/b.h
 # RUN: mkdir -p %t.dir/my/dir
 # RUN: touch %t.dir/my/dir/a.h
 # RUN: mkdir -p %t.dir/my/dir2
 # RUN: touch %t.dir/my/dir2/b.h
 
 # Generate a compile_commands.json that will query the mock driver we've
 # created. Which should add a.h and b.h into include search path.
-# RUN: echo '[{"directory": "%/t.dir", "command": "%/t.dir/my_driver.sh the-file.cpp -nostdinc --sysroot /my/sysroot/path -isysroot=/isysroot", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir", "command": "my_driver.sh the-file.cpp -nostdinc --sysroot /my/sysroot/path -isysroot=/isysroot", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
 
 # RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
 # On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
 # (with the extra slash in the front), so we add it here.
 # RUN: sed -E -e 's|"file://([A-Z]):/|"file:///\1:/|g' %t.test.1 > %t.test
 
 # Bless the mock driver we've just created so that clangd can execute it.
 # RUN: clangd -lit-test -query-driver="**.test,**.sh" < %t.test | FileCheck -strict-whitespace %t.test