[clangd] Link and initialize target infos

[kadircet]

Summary

So that clang can infer target from argv[0] of command line invocation

[ilya-biryukov]

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

36 ↗

(On Diff #204271)

Do we actually need this if we provide a proper Argv[0] to the driver?
My understanding is that the driver should figure out the targets from custom binary names on its own.

clang-tools-extra/clangd/test/target_info.test

2

Could you add a short human-readable description of the test setup here?
Reading the bash scripts is not fun. Something like

Try to mock 'compile_commnands.json' for fuchsia-based builds and check the corresponding target flags
are being passed into clang.

9

This is added unconditionally on all platforms, right?
Why not just add this extra slash directly into the input file?

[kadircet]

• Add comments to the test

[kadircet]

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

36 ↗

(On Diff #204271)

unfortunately yes.

because if the "-target" is missing, driver tries to deduce it from the "running binary" which is "clangd" in our case.

clang-tools-extra/clangd/test/target_info.test

9

no it is not, this checks whether we have a drive letter in the file uri ("[A-Z]:") and adds the slash in that case.

[ilya-biryukov]

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

36 ↗

(On Diff #204271)

I wonder if it would sense to change this? Where does this code live?

That's exactly what argv[0] is for in compile_commands.json. Fixing that at the driver level would "fix" all libclang- and libtooling-based binaries, like clang-tidy, etc.

[ilya-biryukov]

clang-tools-extra/clangd/test/target_info.test

9

Ah, ok, LGTM. Missed that.

[kadircet]

• Move target name deduction into Driver

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

36 ↗

(On Diff #204271)

ah sorry, I've mixed that one actually. the behavior is: if the "-target" is missing, we always use the default target(depends on the host). even though ClangExecutable is available in the Driver and set correctly to argv[0].
tools that care about target deduction needs to pass "-target" to driver explicitly. currently the driver only deduces "driver-mode" from the executable name.

adding this to rather driver side, looks like nothing is broken.

[ilya-biryukov]

Nice, thanks! The clangd parts obviously LGTM, but let's be more conservative with the driver bits (see the relevant comment)

clang-tools-extra/clangd/test/target_info.test

29

This is a marvellous hack!

clang/lib/Driver/Driver.cpp

1055 ↗

(On Diff #204484)

I strongly think the driver is the right place to do this, but we have to watch out for breakages coming from this. The drivers code has way more uses than clangd code, so we should expect that people rely on all kinds of things there.

Could you separate the driver pieces into a separate change and add driver tests for them? To make it more focused and easier to revert in case of breakages.

[kadircet]

clang/lib/Driver/Driver.cpp

1055 ↗

(On Diff #204484)

sent out D63264

[kadircet]

ping

[ilya-biryukov]

Sorry about the confusion, I can now see why the original version of the patch was actually simpler.
I was put off by the fact that we override by adding command-line arguments instead of passing things around in the code, but it appears that's actually simpler than adding yet another mechanism to override target tripple in the driver.

The only suggestion from me would be maybe try moving this to a layer that would also make also benefit other compilation-db-based things like clang-tidy, etc?
Any ideas on how to do this? Would this potentially break anything?

[kadircet]

In D63194#1553649, @ilya-biryukov wrote:

The only suggestion from me would be maybe try moving this to a layer that would also make also benefit other compilation-db-based things like clang-tidy, etc?
Any ideas on how to do this? Would this potentially break anything?

I've made it a wrapper for JSON-based compilation database(as inferring cdb does), there were no breakages in tests.
Hopefully all clang-based tools can benefit from it by linking in target infos and enabling them at startup.

[kadircet]

• Introduce a new wrapper CDB that adds target and mode info to returned compile commands

[ilya-biryukov]

Thanks, this looks very good!
Leaving a few nitpicky comments, but nothing important really.

Two more general NITs:

• could we update the title and description of this revision to mirror that it focuses on code in tooling rather than in clangd?
• maybe land the clangd parts into a separate change for nicer VCS history?

clang/include/clang/Tooling/CompilationDatabase.h

220 ↗

(On Diff #206193)

NIT: in the spirit of a previous name, we could shorten the name inferTargetAndDriverMode(…)

clang/lib/Tooling/CMakeLists.txt

25 ↗

(On Diff #206193)

NIT: GuessTargetAndDriverModeCompilationDatabase would align better with the names of other files that define compilation databases.

It looks a little long, though, maybe there's a better word to capture TargetAndMode

clang/lib/Tooling/TargetAndModeAdderDatabase.cpp

39 ↗

(On Diff #206193)

NIT: maybe accept by value to simplify the code?
A cost of an extra move constructor here is negligible.

[kadircet]

• Move clang related changes to a different patch

[ilya-biryukov]

LGTM

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

14 ↗

(On Diff #206378)

looks like accidental leftover from a previous revision. Remove?

[kadircet]

• revert leftover change

[kadircet]

Landed as rL364387

clang-tools-extra/clangd/CMakeLists.txt

 
 configure_file(
   ${CMAKE_CURRENT_SOURCE_DIR}/Features.inc.in
   ${CMAKE_CURRENT_BINARY_DIR}/Features.inc
 )
 
 set(LLVM_LINK_COMPONENTS
   Support
+  AllTargetsInfos
   )
 
 if(CLANG_BUILT_STANDALONE)
   # needed to get HAVE_CXX_ATOMICS64_WITHOUT_LIB defined
   include(CheckAtomic)
 endif()
 
 set(CLANGD_ATOMIC_LIB "")

clang-tools-extra/clangd/test/target_info.test

+# Mock 'compile_commands.json' to contain a driver name targeting fuchsia OS.
+# Afterwards check that correct target is passed into clang.
+
+# RUN: rm -rf %t.dir && mkdir -p %t.dir
+
+# RUN: echo '[{"directory": "%/t.dir", "command": "%/t.dir/x86_64-fuchsia-clang -x c++ the-file.cpp -v", "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 "s|file://\([A-Z]\):/|file:///\1:/|g" %t.test.1 > %t.test
+
+# RUN: clangd -lit-test < %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":""
+    }
+  }
+}
+# Make sure we have target passed into cc1 driver, which is printed due to -v in
+# the compile_commands.json
+# CHECK: Target: x86_64-unknown-fuchsia
+---
+{"jsonrpc":"2.0","id":10000,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}

clang-tools-extra/clangd/tool/ClangdMain.cpp

 #include "clang/Format/Format.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/Signals.h"
+#include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cstdlib>
 #include <iostream>
 #include <memory>
 #include <mutex>
 #include <string>
 #include <thread>
 
enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
   CantRunAsXPCService = 2
 };
 
 int main(int argc, char *argv[]) {
   using namespace clang;
   using namespace clang::clangd;
 
+  llvm::InitializeAllTargetInfos();
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   llvm::cl::SetVersionPrinter([](llvm::raw_ostream &OS) {
     OS << clang::getClangToolFullVersion("clangd") << "\n";
   });
   llvm::cl::ParseCommandLineOptions(
       argc, argv,
       "clangd is a language server that provides IDE-like features to editors. "
       "\n\nIt should be used via an editor plugin rather than invoked "