[clangd][query-driver] Extract target

[ArcsinX]

Summary

In some cases system includes extractions is not enough, we also need target specific defines.
The problems appears when clang default target is not the same as toolchain's one (GCC cross-compiler, MinGW on Windows).
After this patch query-driver also extracts target and adds --target=<extracted target> compile option.

[ArcsinX]

I'm not sure if this solution is elegant enough. I will be happy to hear advices.

[sammccall]

Thanks, this seems really useful!

clang-tools-extra/clangd/QueryDriverDatabase.cpp

62

define a little struct holding these two, for clarity?

92

This part seems pretty confusing :-)
You could just search for Target: independently of the current scanning - I'm not terribly worried about it appearing in the middle of the include list!

If we want to be strict though, it might be time to rewrite to be more stateful e.g. a loop consuming chunks from an ArrayRef<StringRef>, or a state machine, or something.

130

this changes the error behavior: intentional?

245

clang -target foo test.cc seems to be a hard error in the driver if the target is unknown.
(vs likely *some* functionality if we just didn't set the driver)

so this could regress some scenarios. Can we mitigate that?
(It's possible that we're running the driver in a mode where we proceed anyway, but I can't remember :-()

338

nit: while here, add std::move?

[kadircet]

clang-tools-extra/clangd/QueryDriverDatabase.cpp

245

what if target already exists in Cmd?

also it would be nice to use --target=X format to be consistent with target inference from invocation name as in https://github.com/llvm/llvm-project/blob/master/clang/lib/Tooling/Tooling.cpp#L278.

[ArcsinX]

Add structure to hold system includes and target.
Use state machine to go though lines of the driver output.
Handle invalid target
Use '--target=X' instead of '-target X'

[ArcsinX]

clang-tools-extra/clangd/QueryDriverDatabase.cpp

134–135

Unsure about this.
Should we toss all collected data if end markes was not found?

245

I failed to find options to process in case of invalid target, so added target validation.

245

We could specify several --target options, the last one will be used. But I am not sure should we override existing or not.

[kadircet]

clang-tools-extra/clangd/QueryDriverDatabase.cpp

67

i think you can just do TargetRegistry::lookupTarget

229

nit: I would fold these two into a single log statement.

245

We could specify several --target options, the last one will be used. But I am not sure should we override existing or not.

Having multiple targets sounds confusing. I would prefer keeping a target specifically mentioned by the user but it is also possible for the target to be inferred by clang (and that might be wrong). This is similar to our stand against predefined macros though, i think we should fix clang's inference logic if it has any false positives. So i am slightly leaning towards not overriding the target if it exists.

336–337

nit:

if(auto Info = ...)
  setTarget(addSystemIncludes(...), ...);
return std::move(Cmd);

[sammccall]

Thanks! just nits, apart from the "-target already explicitly set" issue Kadir raised.

clang-tools-extra/clangd/QueryDriverDatabase.cpp

99

I'm not sure these early returns are worth the complexity

102

also not sure whether initial vs includesextracted is worth distinguishing - this doesn't scale well if we add a third thing we're tracking (do we need a state for each combination that we could have seen already?)

Maybe just a bool for SeenIncludes or something to detect duplicate/missing sections would be simpler

106

this mostly echoes the code: "Only detect targets that clang understands"?

108

I'd prefix with "System include extraction: " and bump up to elog - this is fairly likely to break things (and we'll only log it ~once thanks to caching)

112

also add a prefix here so it's clear what the context is in the log

226

we want to log this even if the set is empty, so the reader can tell the extraction worked
(it's fine to only optionally log target)

230

The target extractor isn't a separate component from the system includes extractor, so the log message seems a bit misleading.
I'd suggest just using "System includes extractor" for both, even if it's not totally accurate

245

I think the question of "target already explicitly set" should probably be answered.
Checking for an arg equal to "-target" or beginning with "--target" is probably enough.

[sammccall]

Sorry, code reviews are racy :-)

clang-tools-extra/clangd/QueryDriverDatabase.cpp

134–135

Yup, at least for this patch (this is the existing behavior).
I think the logic is "this is a fragile text protocol, we've never seen this happen, so don't really know what it would mean".

245

I think we should definitely not override the target if it already exists.

The scenario is: compile_commands.json has /my/driver --target=bar foo.cc.
/my/driver reports "Target: foo"

So foo is the default target, but it's being overridden on the command-line as part of the build.

Now if we add the extracted target to the end of the compile command, to produce /my/driver --target=bar foo.cc --target=foo, we're effectively reversing the precedence: now the default target is used even if the compile command overrode it.

[ArcsinX]

Do not override existing target.
Fix clang-tidy warning.
Address review comments.

[ArcsinX]

clang-tools-extra/clangd/QueryDriverDatabase.cpp

67

Seems we can't. TargetRegistry::lookupTarget returns nullptr for arm-linux-gnueabihf with error message: No available targets are compatible with triple "arm-linux-gnueabihf"

336–337

I used your advice except std::move(Cmd), seems we do not need std::move here

[kadircet]

clang-tools-extra/clangd/QueryDriverDatabase.cpp

344

ofc we don't need it. but the thing is we are returning an Optional<tooling::Command> hence the return statement needs to invoke a constructor for Optional and without std::move it would invoke a copy constructor rather than a move-based one.

And even though rest of the calculations are cached per driver&language name, this copy is going to happen for every translation unit. So in theory it would be nice to prevent that.

[kadircet]

clang-tools-extra/clangd/QueryDriverDatabase.cpp

344

oh actually never mind, Cmd itself is also an Optional<tooling::Command>, i thought it was a naked tooling::Command

[sammccall]

Still LG, thanks!

clang-tools-extra/clangd/QueryDriverDatabase.cpp

344

It's moot here, but this isn't the case anymore, return x can call a converting move constructor: http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1579

This is part of C++14, and so we should be able to rely on it in LLVM. It wasn't true in the original C++11 spec, but since it was a defect report, reasonably-modern compilers allow the optimization even in C++11 mode.

Buggy compilers certainly exist, but I don't think we need to work around them for small performance issues. (If the code fails to compile on old-gcc without std::move, that's another story...)

[ArcsinX]

Thanks for review!

clang-tools-extra/clangd/QueryDriverDatabase.cpp

 // database is used as compiler driver path. Due to this arbitrary binary
 // execution, this mechanism is not used by default and only executes binaries
 // in the paths that are explicitly included by the user.
 
 #include "GlobalCompilationDatabase.h"
 #include "support/Logger.h"
 #include "support/Path.h"
 #include "support/Trace.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/TargetOptions.h"
 #include "clang/Driver/Types.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include <algorithm>
 #include <map>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 
-std::vector<std::string> parseDriverOutput(llvm::StringRef Output) {
+struct DriverInfo {
   std::vector<std::string> SystemIncludes;
+  std::string Target;
+};
+
+bool isValidTarget(llvm::StringRef Triple) {
+  std::shared_ptr<TargetOptions> TargetOpts(new TargetOptions);
+  TargetOpts->Triple = Triple.str();
+  DiagnosticsEngine Diags(new DiagnosticIDs, new DiagnosticOptions,
+                          new IgnoringDiagConsumer);
+  IntrusiveRefCntPtr<TargetInfo> Target =
+      TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+  return bool(Target);
+}
+
+llvm::Optional<DriverInfo> parseDriverOutput(llvm::StringRef Output) {
+  DriverInfo Info;
   const char SIS[] = "#include <...> search starts here:";
   const char SIE[] = "End of search list.";
+  const char TS[] = "Target: ";
   llvm::SmallVector<llvm::StringRef, 8> Lines;
   Output.split(Lines, '\n', /*MaxSplit=*/-1, /*KeepEmpty=*/false);
 
-  auto StartIt = llvm::find_if(
-      Lines, [SIS](llvm::StringRef Line) { return Line.trim() == SIS; });
-  if (StartIt == Lines.end()) {
-    elog("System include extraction: start marker not found: {0}", Output);
-    return {};
+  enum {
+    Initial,            // Initial state: searching for target or includes list.
+    IncludesExtracting, // Includes extracting.
+    Done                // Includes and target extraction done.
+  } State = Initial;
+  bool SeenIncludes = false;
+  bool SeenTarget = false;
+  for (auto *It = Lines.begin(); State != Done && It != Lines.end(); ++It) {
+    auto Line = *It;
+    switch (State) {
+    case Initial:
+      if (!SeenIncludes && Line.trim() == SIS) {
+        SeenIncludes = true;
+        State = IncludesExtracting;
+      } else if (!SeenTarget && Line.trim().startswith(TS)) {
+        SeenTarget = true;
+        llvm::StringRef TargetLine = Line.trim();
+        TargetLine.consume_front(TS);
+        // Only detect targets that clang understands
+        if (!isValidTarget(TargetLine)) {
+          elog("System include extraction: invalid target \"{0}\", ignoring",
+               TargetLine);
+        } else {
+          Info.Target = TargetLine.str();
+          vlog("System include extraction: target extracted: \"{0}\"",
+               TargetLine);
         }
-  ++StartIt;
-  const auto EndIt =
-      llvm::find_if(llvm::make_range(StartIt, Lines.end()),
-                    [SIE](llvm::StringRef Line) { return Line.trim() == SIE; });
-  if (EndIt == Lines.end()) {
-    elog("System include extraction: end marker missing: {0}", Output);
-    return {};
       }
-
-  for (llvm::StringRef Line : llvm::make_range(StartIt, EndIt)) {
-    SystemIncludes.push_back(Line.trim().str());
+      break;
+    case IncludesExtracting:
+      if (Line.trim() == SIE) {
+        State = SeenTarget ? Done : Initial;
+      } else {
+        Info.SystemIncludes.push_back(Line.trim().str());
         vlog("System include extraction: adding {0}", Line);
       }
-  return SystemIncludes;
+      break;
+    default:
+      llvm_unreachable("Impossible state of the driver output parser");
+      break;
+    }
+  }
+  if (!SeenIncludes) {
+    elog("System include extraction: start marker not found: {0}", Output);
+    return llvm::None;
+  }
+  if (State == IncludesExtracting) {
+    elog("System include extraction: end marker missing: {0}", Output);
+    return llvm::None;
+  }
+  return std::move(Info);
 }
 
-std::vector<std::string>
-extractSystemIncludes(PathRef Driver, llvm::StringRef Lang,
+llvm::Optional<DriverInfo>
+extractSystemIncludesAndTarget(PathRef Driver, llvm::StringRef Lang,
                                llvm::ArrayRef<std::string> CommandLine,
                                const llvm::Regex &QueryDriverRegex) {
-  trace::Span Tracer("Extract system includes");
+  trace::Span Tracer("Extract system includes and target");
   SPAN_ATTACH(Tracer, "driver", Driver);
   SPAN_ATTACH(Tracer, "lang", Lang);
 
   if (!QueryDriverRegex.match(Driver)) {
     vlog("System include extraction: not allowed driver {0}", Driver);
-    return {};
+    return llvm::None;
   }
 
   if (!llvm::sys::fs::exists(Driver)) {
     elog("System include extraction: {0} does not exist.", Driver);
-    return {};
+    return llvm::None;
   }
   if (!llvm::sys::fs::can_execute(Driver)) {
     elog("System include extraction: {0} is not executable.", Driver);
-    return {};
+    return llvm::None;
   }
 
   llvm::SmallString<128> StdErrPath;
   if (auto EC = llvm::sys::fs::createTemporaryFile("system-includes", "clangd",
                                                    StdErrPath)) {
     elog("System include extraction: failed to create temporary file with "
          "error {0}",
          EC.message());
-    return {};
+    return llvm::None;
   }
   auto CleanUp = llvm::make_scope_exit(
       [&StdErrPath]() { llvm::sys::fs::remove(StdErrPath); });
 
   llvm::Optional<llvm::StringRef> Redirects[] = {
       {""}, {""}, llvm::StringRef(StdErrPath)};
 
   llvm::SmallVector<llvm::StringRef, 12> Args = {Driver, "-E", "-x",
for (size_t I = 0, E = CommandLine.size(); I < E; ++I) {
     }
   }
 
   if (int RC = llvm::sys::ExecuteAndWait(Driver, Args, /*Env=*/llvm::None,
                                          Redirects)) {
     elog("System include extraction: driver execution failed with return code: "
          "{0}. Args: ['{1}']",
          llvm::to_string(RC), llvm::join(Args, "', '"));
-    return {};
+    return llvm::None;
   }
 
   auto BufOrError = llvm::MemoryBuffer::getFile(StdErrPath);
   if (!BufOrError) {
     elog("System include extraction: failed to read {0} with error {1}",
          StdErrPath, BufOrError.getError().message());
-    return {};
+    return llvm::None;
   }
 
-  auto Includes = parseDriverOutput(BufOrError->get()->getBuffer());
-  log("System include extractor: successfully executed {0}, got includes: "
-      "\"{1}\"",
-      Driver, llvm::join(Includes, ", "));
-  return Includes;
+  llvm::Optional<DriverInfo> Info =
+      parseDriverOutput(BufOrError->get()->getBuffer());
+  if (!Info)
+    return llvm::None;
+  log("System includes extractor: successfully executed {0}\n\tgot includes: "
+      "\"{1}\"\n\tgot target: \"{2}\"",
+      Driver, llvm::join(Info->SystemIncludes, ", "), Info->Target);
+  return Info;
 }
 
 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());
   }
   return Cmd;
 }
 
+tooling::CompileCommand &setTarget(tooling::CompileCommand &Cmd,
+                                   const std::string &Target) {
+  if (!Target.empty()) {
+    // We do not want to override existing target with extracted one.
+    for (llvm::StringRef Arg : Cmd.CommandLine) {
+      if (Arg == "-target" || Arg.startswith("--target="))
+        return Cmd;
+    }
+    Cmd.CommandLine.push_back("--target=" + Target);
+  }
+  return Cmd;
+}
+
 /// Converts a glob containing only ** or * into a regex.
 std::string convertGlobToRegex(llvm::StringRef Glob) {
   std::string RegText;
   llvm::raw_string_ostream RegStream(RegText);
   RegStream << '^';
   for (size_t I = 0, E = Glob.size(); I < E; ++I) {
     if (Glob[I] == '*') {
       if (I + 1 < E && Glob[I + 1] == '*') {
if (Lang.empty()) {
         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);
 
-    std::vector<std::string> SystemIncludes =
+    if (auto Info =
             QueriedDrivers.get(/*Key=*/(Driver + ":" + Lang).str(), [&] {
-          return extractSystemIncludes(Driver, Lang, Cmd->CommandLine,
-                                       QueryDriverRegex);
-        });
-
-    return addSystemIncludes(*Cmd, SystemIncludes);
+              return extractSystemIncludesAndTarget(
+                  Driver, Lang, Cmd->CommandLine, QueryDriverRegex);
+            })) {
+      setTarget(addSystemIncludes(*Cmd, Info->SystemIncludes), Info->Target);
+    }
+    return Cmd;
   }
 
   llvm::Optional<ProjectInfo> getProjectInfo(PathRef File) const override {
     return Base->getProjectInfo(File);
   }
 
 private:
   // Caches includes extracted from a driver. Key is driver:lang.
-  Memoize<llvm::StringMap<std::vector<std::string>>> QueriedDrivers;
+  Memoize<llvm::StringMap<llvm::Optional<DriverInfo>>> QueriedDrivers;
   llvm::Regex QueryDriverRegex;
 
   std::unique_ptr<GlobalCompilationDatabase> Base;
   CommandChanged::Subscription BaseChanged;
 };
 } // namespace
 
 std::unique_ptr<GlobalCompilationDatabase>

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/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
 
 # Create header files my/dir/a.h and my/dir2/b.h
 # RUN: mkdir -p %t.dir/my/dir

 {
   "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>"
+      "text":"#include <a.h>\n#include <b.h>\n#if !defined(__ARM_ARCH) || !defined(__gnu_linux__)\n#error \"Invalid target\"\n#endif"
     }
   }
 }
 # CHECK:   "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:   "params": {
 # CHECK-NEXT:     "diagnostics": [],
 ---
 {"jsonrpc":"2.0","id":10000,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}