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

230

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

232

could you also handle isysroot ?

260

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 ↗

(On Diff #240643)

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 ↗

(On Diff #240815)

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<llvm::StringRef> PreservedArgs,
   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"};
+  llvm::copy(PreservedArgs, std::back_inserter(Args));
 
   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}. Driver: {1}, args: [{2}]",
+         llvm::to_string(RC), Driver, 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 {};
   }
 
   auto Includes = parseDriverOutput(BufOrError->get()->getBuffer());
   log("System include extractor: succesfully executed {0}, got includes: "
       "\"{1}\"",
       Driver, llvm::join(Includes, ", "));
   return Includes;
 }
 
 tooling::CompileCommand &
 addSystemIncludes(tooling::CompileCommand &Cmd,
                   llvm::ArrayRef<std::string> SystemIncludes) {
   for (llvm::StringRef Include : SystemIncludes) {
     // FIXME(kadircet): This doesn't work when we have "--driver-mode=cl"
     Cmd.CommandLine.push_back("-isystem");
     Cmd.CommandLine.push_back(Include.str());
public:
 
   llvm::Optional<tooling::CompileCommand>
   getCompileCommand(PathRef File) const override {
     auto Cmd = Base->getCompileCommand(File);
     if (!Cmd || Cmd->CommandLine.empty())
       return Cmd;
 
     llvm::StringRef Lang;
+    llvm::SmallVector<llvm::StringRef, 4> PreservedArgs;
     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();
+      else if (Arg == "-nostdinc")
+        PreservedArgs.push_back(Arg);
+      else if (Arg == "--sysroot" && I + 1 < E) {
+        PreservedArgs.push_back(Arg);
+        PreservedArgs.push_back(Cmd->CommandLine[I + 1]);
+      } else if (Arg.startswith("--sysroot="))
+        PreservedArgs.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());
     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, PreservedArgs, 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