[PATCH] D62804: [clangd] Enable extraction of system includes from custom toolchains

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D62804#1533710 <https://reviews.llvm.org/D62804#1533710>, @ilya-biryukov wrote:

> Could you give more context on what the custom toolchains are?
>  One feasible alternative is to move this detection to clang's driver (where the rest of include path detection lives), why won't that work?


Moving this logic into clang's driver should also work there is nothing specific to clangd. This patch just fetches those headers and adds them to clang invocation command line with a "-isystem" in front of them.
But we execute an arbitrary binary to fetch system includes coming from the custom toolchain drivers. Therefore we decided to rather keep it contained in clangd hidden behind a flag during offline discussions.

For example a gcc cross compiling to arm comes with its own system includes and has some mechanisms to discover that implicitly, without requiring any "-I" flags.
Hence when used with clangd, we make use of system includes instead of the target's include library. This patch aims to solve that issue, tools like cquery also handles that problem in a similar way.



================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:82
+
+    if (!llvm::sys::fs::exists(Line))
+      elog("Parsed non-existing system include: {0}", Line);
----------------
sammccall wrote:
> Is this check important? (What happens if non-existent dirs are on the include path?)
> 
> If it is, maybe we should pass in the VFS (factory?) here.
Existence of the directory doesn't really matter, I've put this as a debug measure, in case we get a "non-path"(malformed) entry.
But I suppose we can already see this in the "Updating a.cc with command ...." log.

So removing it.


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:90
+
+std::vector<std::string> extractSystemIncludes(PathRef Driver, PathRef File) {
+  trace::Span Tracer("Extract system includes");
----------------
sammccall wrote:
> some explanation of what this is doing?
> 
> e.g. "Many compilers have default system includes... these are known by the driver and passed to cc1 ... gcc and compilers with compatible CLI can dump the cc1 invocation information with this syntax... we do so and parse the result."
I've aleady mentioned this in file comment, do you think it is necessary to mention that in here as well?


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:276
+                   "for non-sequence match."),
+    llvm::cl::init(""));
+
----------------
sammccall wrote:
> is there something useful we should set this to by default? like `/usr/bin/gcc,/usr/bin/g++`?
> 
> Or is the assumption that the standard/system compilers will never have weird information to extract?
I believe this is not an issue with default gcc/g++ since we never heard such complaints. I would rather leave the default empty until we observe some wide usage, due to security concerns


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62804/new/

https://reviews.llvm.org/D62804