Preserve -nostdinc and --sysroot when calling query driver

[topisani]

Summary

Solves this issue: https://github.com/clangd/clangd/issues/157

This is my first contribution to an llvm project, so I hope I'm doing it right!

[kadircet]

Hi @topisani, thanks for working on clangd!

Yes that bug fell through the cracks since the logs proposed in the comments were not making use of -query-driver option. Do you mind adding some logs to the bug report, without your patch so that we can clearly see its effect.

Initially we left it out, because it is always hairy to parse command line options textually, and also kind of unclear why clang wouldn't pick it up itself once you have isysroot or sysroot set, with extra logs we can be sure there are
no bugs in clang/clangd side, and it is just about a custom logic in the driver you are using.

We also need some tests, could you add some to test/system-include-extractor.test ?

clang-tools-extra/clangd/QueryDriverDatabase.cpp

257

we should also handle -nobuiltininc, --no-standard-includes and -nostdinc++.

259

could you also handle isysroot ?

280–281

let's pass the Cmd->CommandLine to extractSystemIncludes and perform the preservation logic there, so that we only do it once.

[topisani]

Addressed CR comments

I'm not sure about the tests, I basically just added to the one test that already exists that it passes one of each "type" of preservable argument.

[kadircet]

tests look good, but please upload the logs to the bug report for investigation.

clang-tools-extra/clangd/QueryDriverDatabase.cpp

137

s/auto/llvm::StringRef

same for the other two lambdas below

139

instead of having the third case, you can check whether the Arg.startswith(S), then call Arg.consume_front(S) to drop the prefix and check for emptiness or starting with equals.

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

10

could you rather make use of [ -z "${args##"-nostdinc"}" ] to be compatible with non-bash shells ?

[topisani]

Address CR Comments

I couldn't think of a good way to check for --sysroot /my/sysroot in the test in posix shell, so i hope the grep usage is OK.

[kadircet]

clang-tools-extra/clangd/QueryDriverDatabase.cpp

144

nit: early exit with

if(Found == std::end(ArgsToPreserve))
  continue;

148

nit: also increment I

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

10

sorry I forgot to include *s, it should've been [ -z "${args##*"-nostdinc"*}" ].

Also you need to populate args with $* or $@ first.

[kadircet]

it seems like the last diff is same as the previous one, maybe you've uploaded the wrong diff ?

[topisani]

Woops yeah, still getting used to using git like this

[kadircet]

LGTM, thanks

let me know if you need me to commit this.

[topisani]

In D73453#1857095, @kadircet wrote:

let me know if you need me to commit this.

Yes please, thanks for your time!

[kadircet]

closed by 6e8d6bc9ec8739ec22b73a23f740f171f452e234

clang-tools-extra/clangd/QueryDriverDatabase.cpp

std::vector<std::string> parseDriverOutput(llvm::StringRef Output) {
 
   for (llvm::StringRef Line : llvm::make_range(StartIt, EndIt)) {
     SystemIncludes.push_back(Line.trim().str());
     vlog("System include extraction: adding {0}", Line);
   }
   return SystemIncludes;
 }
 
-std::vector<std::string> extractSystemIncludes(PathRef Driver,
-                                               llvm::StringRef Lang,
-                                               llvm::Regex &QueryDriverRegex) {
+std::vector<std::string>
+extractSystemIncludes(PathRef Driver, llvm::StringRef Lang,
+                      llvm::ArrayRef<std::string> CommandLine,
   trace::Span Tracer("Extract system includes");
   SPAN_ATTACH(Tracer, "driver", Driver);
   SPAN_ATTACH(Tracer, "lang", Lang);
 
   if (!QueryDriverRegex.match(Driver)) {
     vlog("System include extraction: not whitelisted driver {0}", Driver);
     return {};
   }
if (auto EC = llvm::sys::fs::createTemporaryFile("system-includes", "clangd",
     return {};
   }
   auto CleanUp = llvm::make_scope_exit(
       [&StdErrPath]() { llvm::sys::fs::remove(StdErrPath); });
 
   llvm::Optional<llvm::StringRef> Redirects[] = {
       {""}, {""}, llvm::StringRef(StdErrPath)};
 
-  // Should we also preserve flags like "-sysroot", "-nostdinc" ?
-  const llvm::StringRef Args[] = {Driver, "-E", "-x", Lang, "-", "-v"};
+  llvm::SmallVector<llvm::StringRef, 12> 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::any_of(FlagsToPreserve,
+                     [&Arg](llvm::StringRef S) { return S == 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.consume_front(*Found);
+      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]);
+      }
+    }
+  }
 
   if (int RC = llvm::sys::ExecuteAndWait(Driver, Args, /*Env=*/llvm::None,
                                          Redirects)) {
     elog("System include extraction: driver execution failed with return code: "
-         "{0}",
-         llvm::to_string(RC));
+         "{0}. Args: ['{1}']",
+         llvm::to_string(RC), llvm::join(Args, "', '"));
     return {};
   }
 
   auto BufOrError = llvm::MemoryBuffer::getFile(StdErrPath);
   if (!BufOrError) {
     elog("System include extraction: failed to read {0} with error {1}",
          StdErrPath, BufOrError.getError().message());
     return {};
getCompileCommand(PathRef File) const override {
 
     llvm::StringRef Lang;
     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"))
         Lang = Arg.drop_front(2).trim();
     }
     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());
     llvm::sys::fs::make_absolute(Cmd->Directory, Driver);
     auto Key = std::make_pair(Driver.str(), Lang);
 
     std::vector<std::string> SystemIncludes;
     {
       std::lock_guard<std::mutex> Lock(Mu);
 
       auto It = DriverToIncludesCache.find(Key);
       if (It != DriverToIncludesCache.end())
         SystemIncludes = It->second;
       else
-        DriverToIncludesCache[Key] = SystemIncludes =
-            extractSystemIncludes(Key.first, Key.second, QueryDriverRegex);
+        DriverToIncludesCache[Key] = SystemIncludes = extractSystemIncludes(
+            Key.first, Key.second, Cmd->CommandLine, QueryDriverRegex);
     }
 
     return addSystemIncludes(*Cmd, SystemIncludes);
   }
 
   llvm::Optional<ProjectInfo> getProjectInfo(PathRef File) const override {
     return Base->getProjectInfo(File);
   }

 
 std::unique_ptr<GlobalCompilationDatabase>
 getQueryDriverDatabase(llvm::ArrayRef<std::string> QueryDriverGlobs,
                        std::unique_ptr<GlobalCompilationDatabase> Base) {
   assert(Base && "Null base to SystemIncludeExtractor");
   if (QueryDriverGlobs.empty())
     return Base;
   return std::make_unique<QueryDriverDatabase>(QueryDriverGlobs,
-                                                std::move(Base));
 }
 
 } // namespace clangd
 } // namespace clang

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
 
 # Generate a mock-driver that will print %temp_dir%/my/dir and
 # %temp_dir%/my/dir2 as include search paths.
-# RUN: echo '#!/bin/bash' >> %t.dir/my_driver.sh
+# 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 'echo -e "#include <...> search starts here:\r" >&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 'echo -e "End of search list.\r" >&2' >> %t.dir/my_driver.sh
 # RUN: chmod +x %t.dir/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", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
+# 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: 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