[clangd] Use all query-driver arguments in cache key

[nridge]

Summary

Rework the cache key construction to consider all the arguments used when invoking the query driver.
https://github.com/clangd/clangd/issues/1403

This also reworks handling for -isysroot
https://github.com/clangd/clangd/issues/1404

And adds support for passing through -specs flags to the query driver
https://github.com/clangd/clangd/issues/1410

[kadircet]

thanks a lot for taking a look at this!

clang-tools-extra/clangd/SystemIncludeExtractor.cpp

301

can we introduce a struct instead?

struct DriverArgs {
  std::string Driver;
  bool StandardIncludes = true;
  bool StandardCXXIncludes = true;
  bool BuiltinIncludes = true;
  llvm::StringRef Lang;
  llvm::StringRef Sysroot;

  DriverArgs(const tooling::CompileCommand &Cmd); // Traverses the Cmd and infers the bits.
  llvm::SmallVector<llvm::StringRef> render() const; // we can use canonical versions.
};

we can also implement hashing based on the struct now and also pass it around.

304

i don't think we gain much by having these 3 different types now, especially considering an arg might need to be placed in multiple categories (-isysroot in both TwoPartArgs and ArgPrefixes).

let's rather have a check for each related bit of struct inside the for loop, e.g. something like:

for(...) {
  // Infer Lang
  if(Arg.startswith("-x")) {
    if (Arg == "-x" && I + 1 < E) Lang = Args[++I];
    else Lang = Arg.drop_front(2).trim();
    continue;
  }

  // Infer Sysroot
  ...
}

310

can we add --specs related changes in a follow up patch instead?

[nridge]

@ehntoo hi! are you planning to update this patch to address Kadir's comments?

[nridge]

This was superseded by D146941.

clang-tools-extra/clangd/SystemIncludeExtractor.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(llvm::SmallString<128> Driver,
-                               llvm::StringRef Lang,
-                               llvm::ArrayRef<std::string> CommandLine,
-                               const llvm::Regex &QueryDriverRegex) {
+llvm::Optional<DriverInfo> extractSystemIncludesAndTarget(
+    llvm::SmallString<128> Driver, llvm::StringRef Lang,
+    llvm::ArrayRef<llvm::StringRef> AdditionalDriverArgs,
   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,
llvm::Optional<DriverInfo> extractSystemIncludesAndTarget(
   auto CleanUp = llvm::make_scope_exit(
       [&StdErrPath]() { llvm::sys::fs::remove(StdErrPath); });
 
   std::optional<llvm::StringRef> Redirects[] = {{""}, {""}, StdErrPath.str()};
 
   llvm::SmallVector<llvm::StringRef> Args = {Driver, "-E", "-x",
                                              Lang,   "-",  "-v"};
 
-  // These flags will be preserved
-  const llvm::StringRef FlagsToPreserve[] = {
-      "-nostdinc", "--no-standard-includes", "-nostdinc++", "-nobuiltininc"};
-  // Preserves these flags and their values, either as separate args or with an
-  // equalsbetween them
-  const llvm::StringRef ArgsToPreserve[] = {"--sysroot", "-isysroot"};
-
-  for (size_t I = 0, E = CommandLine.size(); I < E; ++I) {
-    llvm::StringRef Arg = CommandLine[I];
-    if (llvm::is_contained(FlagsToPreserve, Arg)) {
-      Args.push_back(Arg);
-    } else {
-      const auto *Found =
-          llvm::find_if(ArgsToPreserve, [&Arg](llvm::StringRef S) {
-            return Arg.startswith(S);
-          });
-      if (Found == std::end(ArgsToPreserve))
-        continue;
-      Arg = Arg.drop_front(Found->size());
-      if (Arg.empty() && I + 1 < E) {
-        Args.push_back(CommandLine[I]);
-        Args.push_back(CommandLine[++I]);
-      } else if (Arg.startswith("=")) {
-        Args.push_back(CommandLine[I]);
-      }
-    }
-  }
+  llvm::append_range(Args, AdditionalDriverArgs);
 
   std::string ErrMsg;
   if (int RC = llvm::sys::ExecuteAndWait(Driver, Args, /*Env=*/llvm::None,
                                          Redirects, /*SecondsToWait=*/0,
                                          /*MemoryLimit=*/0, &ErrMsg)) {
     elog("System include extraction: driver execution failed with return code: "
          "{0} - '{1}'. Args: [{2}]",
          llvm::to_string(RC), ErrMsg, printArgv(Args));
public:
   SystemIncludeExtractor(llvm::ArrayRef<std::string> QueryDriverGlobs)
       : QueryDriverRegex(convertGlobsToRegex(QueryDriverGlobs)) {}
 
   void operator()(tooling::CompileCommand &Cmd, llvm::StringRef File) const {
     if (Cmd.CommandLine.empty())
       return;
 
     llvm::StringRef Lang;
+    llvm::SmallVector<llvm::StringRef> AdditionalDriverArgs;
+
+    // These flags will be preserved
+    const llvm::StringRef FlagsToPreserve[] = {
+        "-nostdinc", "--no-standard-includes", "-nostdinc++", "-nobuiltininc"};
+    // Preserve these flags and their values when provided as separate args
+    const llvm::StringRef TwoPartArgsToPreserve[] = {"--sysroot", "-isysroot"};
+    // Preserve these flags and their values when provided as a single arg
+    const llvm::StringRef ArgPrefixesToPreserve[] = {
+        "-isysroot", "--sysroot=", "-specs=", "--specs="};
+
     for (size_t I = 0, E = Cmd.CommandLine.size(); I < E; ++I) {
       llvm::StringRef Arg = Cmd.CommandLine[I];
-      if (Arg == "-x" && I + 1 < E)
-        Lang = Cmd.CommandLine[I + 1];
-      else if (Arg.startswith("-x"))
+      if (Arg == "-x" && I + 1 < E) {
+        Lang = Cmd.CommandLine[++I];
+      } else if (Arg.startswith("-x")) {
         Lang = Arg.drop_front(2).trim();
+      } else if (llvm::is_contained(FlagsToPreserve, Arg)) {
+        AdditionalDriverArgs.push_back(Arg);
+      } else if (llvm::is_contained(TwoPartArgsToPreserve, Arg) && I + 1 < E) {
+        AdditionalDriverArgs.push_back(Arg);
+        AdditionalDriverArgs.push_back(Cmd.CommandLine[++I]);
+      } else if (llvm::any_of(ArgPrefixesToPreserve,
+                              [&Arg](const llvm::StringRef Prefix) {
+                                return Arg.starts_with(Prefix);
+                              })) {
+        AdditionalDriverArgs.push_back(Arg);
+      }
     }
     if (Lang.empty()) {
       llvm::StringRef Ext = llvm::sys::path::extension(File).trim('.');
       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);
-            })) {
+    std::string CacheKey = (Driver + ":" + Lang).str();
+    if (!AdditionalDriverArgs.empty())
+      CacheKey += ":" + llvm::join(AdditionalDriverArgs, ":");
+
+    if (auto Info = QueriedDrivers.get(/*Key=*/CacheKey, [&] {
+              Driver, Lang, AdditionalDriverArgs, QueryDriverRegex);
       setTarget(addSystemIncludes(Cmd, Info->SystemIncludes), Info->Target);
     }
   }
 
 private:
   // Caches includes extracted from a driver. Key is driver:lang.
   Memoize<llvm::StringMap<llvm::Optional<DriverInfo>>> QueriedDrivers;
   llvm::Regex QueryDriverRegex;

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/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
 # Check that clangd preserves certain flags like `-nostdinc` from
 # original invocation in compile_commands.json.
 # 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 '[ -z "${args##*"--sysroot /my/sysroot/path"*}" ] || exit' >> %t.dir/bin/my_driver.sh
+# RUN: echo '[ -z "${args##*"-isysroot/isysroot1"*}" ] || exit' >> %t.dir/bin/my_driver.sh
+# RUN: echo '[ -z "${args##*"-isysroot /isysroot2"*}" ] || exit' >> %t.dir/bin/my_driver.sh
+# RUN: echo '[ -z "${args##*"--sysroot /my/sysroot1/path"*}" ] || exit' >> %t.dir/bin/my_driver.sh
+# RUN: echo '[ -z "${args##*"--sysroot=/my/sysroot2/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
+# Append an alternate includes path iff we were given a specs file
+# RUN: echo '[ -z "${args##*"-specs=nano.specs"*}" ] && echo %t.dir/my/dir3/ >&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
+# RUN: mkdir -p %t.dir/my/dir3
+# RUN: touch %t.dir/my/dir3/c.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": "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/sysroot1/path --sysroot=/my/sysroot2/path -isysroot/isysroot1 -isysroot /isysroot2 -specs=nano.specs", "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.
 # Note: include clangd's stderr in the FileCheck input with "2>&1" so that we
 # can match output lines like "ASTWorker building file"
 # RUN: clangd -lit-test -query-driver="**.test,**.sh" < %t.test 2>&1 | FileCheck -strict-whitespace %t.test
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
 ---
 {
   "jsonrpc":"2.0",
   "method":"textDocument/didOpen",
   "params": {
     "textDocument": {
       "uri": "file://INPUT_DIR/the-file.cpp",
       "languageId":"cpp",
       "version":1,
-      "text":"#include <a.h>\n#include <b.h>\n#if !defined(__ARM_ARCH) || !defined(__gnu_linux__)\n#error \"Invalid target\"\n#endif"
+      "text":"#include <a.h>\n#include <b.h>\n#include <c.h>\n#if !defined(__ARM_ARCH) || !defined(__gnu_linux__)\n#error \"Invalid target\"\n#endif"
     }
   }
 }
 # Look for the "ASTWorker building file" line so that the subsequent diagnostics
 # that are matches are for the C++ source file and not a config file.
 # CHECK: ASTWorker building file
 # CHECK:   "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:   "params": {
 # CHECK-NEXT:     "diagnostics": [],
 # CHECK-NEXT:     "uri": "file://{{.*}}/the-file.cpp",
 ---
 {"jsonrpc":"2.0","id":10000,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}
 
 # Generate a different compile_commands.json which does not point to the mock driver
-# RUN: echo '[{"directory": "%/t.dir", "command": "gcc 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": "gcc the-file.cpp -nostdinc --sysroot /my/sysroot1/path --sysroot=/my/sysroot2/path -isysroot/isysroot1 -isysroot /isysroot2 -specs=nano.specs", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
 
 # Generate a clangd config file which points to the mock driver instead
 # RUN: echo 'CompileFlags:' > %t.dir/.clangd
 # RUN: echo '  Compiler: my_driver.sh' >> %t.dir/.clangd
 
 # Run clangd a second time, to make sure it picks up the driver name from the config file
 # Note, we need to pass -enable-config because -lit-test otherwise disables it
 # RUN: clangd -lit-test -enable-config -query-driver="**.test,**.sh" < %t.test 2>&1 | FileCheck -strict-whitespace %t.test