[clangd] Add ability to not use -resource-dir at all

[malaperle]

Summary

Using Clang-cl, I have seen scenarios where the compilation database contains
all flags necessary to find compiler-specific (CL) headers, using /imsvc.
Specifying -resource-dir in this case is detrimental because it seems to have
priority over /imsvc flags. Currently in Clangd, the resource-dir is either
specified as a clangd argument (-resource-dir) or it falls back to
CompilerInvocation::GetResourcePath(). The -resource-dir argument cannot be set
to empty as it triggers the fall back.
This change allows -resource-dir= (nothing)

I'm not familiar enough with clang-cl to know whether or not specifying
-resource-dir is always wrong. If it is always, we could check for clang-cl and
not use the fallback ever but having the ability to nullify -resource-dir seems
useful anyway.

Signed-off-by: Marc-Andre Laperle <[e-mail]>

[ilya-biryukov]

We have to point clangd into the resource dir, corresponding to the version of the headers it was built with. It's important we pick the exact version of the built-in headers that clangd was built for.
Note that resource-dir should only be used for the built-in headers, any compiler-specific libraries should be found using other flags. Could you provide an example clang-cl commands that it breaks with?

[malaperle]

In D54872#1307775, @ilya-biryukov wrote:

We have to point clangd into the resource dir, corresponding to the version of the headers it was built with. It's important we pick the exact version of the built-in headers that clangd was built for.

Can you clarity/confirm "headers it was built with"? For example, if I build Clangd using Xcode's Clang, then running this Clangd, I would have to point to Xcode's Clang headers? This is what I have observed when using Clangd on Clangd's code. What if I compile Clangd with another non-Clang compiler?

Note that resource-dir should only be used for the built-in headers, any compiler-specific libraries should be found using other flags. Could you provide an example clang-cl commands that it breaks with?

The problematic case is a bit different than I originally thought. The CDB contains an include path to the built-in headers of the Clang (windows installer) used to build the project, let's call the compiler Clang1. I also built Clang+Clangd separately from this Clang1, using CL, let's call it Clang2. When running that custom Clangd, the fallback resource-dir is the one from Clang2. I haven't looked deeply to confirm this, but having an include path to Clang1 (windows installer) and resource-dir to Clang2 (custom build) seems like would likely not work. In fact, when running Clangd from Clang2 and setting resource-dir to Clang1, it works properly. Now, I don't know if it's the CDB that's wrongly including the headers of Clang1 or if the CDB should include -resource-dir to Clang1. In any case, I'll spend a bit more time clarifying this.

[ilya-biryukov]

In D54872#1309075, @malaperle wrote:

In D54872#1307775, @ilya-biryukov wrote:

We have to point clangd into the resource dir, corresponding to the version of the headers it was built with. It's important we pick the exact version of the built-in headers that clangd was built for.

Can you clarity/confirm "headers it was built with"?

There are two categories of headers we care about:

• the built-in headers that correspond to the clang-headers build target (they typically live in ./lib/clang/<version>/ if clang lives in ./bin). These headers should be provided alongside the clang-based tools, it's not ok to use the ones from a different version of the compiler (one might get spurious compiler errors or even crashes). We inject -resource-dir in clangd to make sure we pick up the version of built-in headers that we ship.
• the C and C++ library headers, e.g. <vector>, <stdio.h>, etc. Those are not tied to the compiler version, so we just pick up the one based on the compilation database.

For example, if I build Clangd using Xcode's Clang, then running this Clangd, I would have to point to Xcode's Clang headers?

In this example we should point the -resource-dir to pick up the headers from the clangd distribution, not XCode headers.

This is what I have observed when using Clangd on Clangd's code. What if I compile Clangd with another non-Clang compiler?

Users shouldn't point -resource-dir into XCode's headers, this might break if the versions are sufficiently different. D54630 should fix this, it's under review.

The problematic case is a bit different than I originally thought. The CDB contains an include path to the built-in headers of the Clang (windows installer) used to build the project, let's call the compiler Clang1. I also built Clang+Clangd separately from this Clang1, using CL, let's call it Clang2. When running that custom Clangd, the fallback resource-dir is the one from Clang2. I haven't looked deeply to confirm this, but having an include path to Clang1 (windows installer) and resource-dir to Clang2 (custom build) seems like would likely not work. In fact, when running Clangd from Clang2 and setting resource-dir to Clang1, it works properly. Now, I don't know if it's the CDB that's wrongly including the headers of Clang1 or if the CDB should include -resource-dir to Clang1. In any case, I'll spend a bit more time clarifying this.

What is the actual error you're getting? It would be nice to have a repro, so that we can look into the details of the issue. I suspect this might be similar to the issues we're running into on Mac (see D54630 for details), where the -resource-dir is used to find the C++ STL headers in addition to the built-in headers.

[malaperle]

Reading D54630, I think that's what might be happening. Here's how to reproduce the problem though:

• Clangd compiled from source with Visual Studio 2017 (i.e. not from win installer)
• LLVM win installer from http://llvm.org/builds/

d:\dev\experiments\resource-dir-problem\test.cpp:

#include <exception>

D:\dev\experiments\resource-dir-problem\compile_commands.json:

[
  {
    "file": "D:\\dev\\experiments\\resource-dir-problem\\test.cpp",
    "directory": "D:\\dev\\experiments\\resource-dir-problem",
    "command": "cl /c /c test.cpp /std:c++14 /imsvc\"C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\Professional\\VC\\Tools\\MSVC\\14.15.26726\\include\" /imsvc\"C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\Professional\\VC\\Tools\\MSVC\\14.15.26726\\atlmfc\\include\" /I\"C:\\Program Files\\LLVM\\lib\\clang\\8.0.0\\include\" -m64 -fmsc-version=1910"
  }
]

Resulting log:

 Updating file d:\dev\experiments\resource-dir-problem\test.cpp with command [D:\dev\experiments\resource-dir-problem] cl /c /c test.cpp /std:c++14 /imsvcC:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\VC\Tools\MSVC\14.15.26726\include /imsvcC:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\VC\Tools\MSVC\14.15.26726\atlmfc\include /IC:\Program Files\LLVM\lib\clang\8.0.0\include -m64 -fmsc-version=1910 -resource-dir=D:\git\llvm\build-vs2017\RelWithDebInfo\bin\..\lib\clang\8.0.0
I[23:39:49.389] <-- textDocument/codeAction(1)
I[23:39:49.389] --> reply:textDocument/codeAction(1) 0 ms
I[23:39:49.481] Dropped diagnostic outside main file: C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\VC\Tools\MSVC\14.15.26726\include\vcruntime.h: unknown type name 'uintptr_t'; did you mean 'intptr_t'?
... some more errors after

[malaperle]

It doesn't seem like there is any difference in how -resource-dir and /imsvc are handled: they are all added as -internal-isystem

In MSVCToolChain::AddClangSystemIncludeArgs (MSCV.cpp):

if (!DriverArgs.hasArg(options::OPT_nobuiltininc)) {
  AddSystemIncludeWithSubfolder(DriverArgs, CC1Args, getDriver().ResourceDir,
                                "include");
}

// Add %INCLUDE%-like directories from the -imsvc flag.
for (const auto &Path : DriverArgs.getAllArgValues(options::OPT__SLASH_imsvc))
  addSystemInclude(DriverArgs, CC1Args, Path);

which in the example above would result in:

-cc1 .... -internal-isystem "D:\\git\\llvm\\build-vs2017\\RelWithDebInfo\\bin\\..\\lib\\clang\\8.0.0\\include" -internal-isystem "C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\Professional\\VC\\Tools\\MSVC\\14.15.26726\\include" -internal-isystem "C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\Professional\\VC\\Tools\\MSVC\\14.15.26726\\atlmfc\\include"

[ilya-biryukov]

In D54872#1319684, @malaperle wrote:

It doesn't seem like there is any difference in how -resource-dir and /imsvc are handled: they are all added as -internal-isystem

Thanks for digging this code up.
I'm a bit confused now, will put up a few clarifying questions to make sure I understand the probem properly.
Does clang-cl work correctly the arguments from compile_commands.json? Does it fail if we add the -resource-dir, similar to how it was done for clangd? The compile-commands.json seems to be generated for the MSVC or hand-crafted, I wonder if it may be a difference between the clang frontend and msvc (unlikely, but just to be sure).
The compile_commands.json does not even mention the -resource-dir, I wonder why would adding it break anything? The only thing that comes to mind is some of the microsoft headers being named the same as the clang headers, but having more stuff in them (the uintptr_t that's missing). Is that the case?

[malaperle]

In D54872#1319958, @ilya-biryukov wrote:

I'm a bit confused now, will put up a few clarifying questions to make sure I understand the probem properly.
Does clang-cl work correctly the arguments from compile_commands.json? Does it fail if we add the -resource-dir, similar to how it was done for clangd?

Yes it does fail the same way if I use clang-cl from the same local build as the clangd with the added -resource-dir. Interestingly, it does also fail if I don't add the -resource-dir, unlike clangd (this patch). For that clang-cl to work on the command-line, I have to use -resource-dir C:\Program Files\LLVM\lib\clang\8.0.0\include. The clang-cl from the installer works without the -resource-dir. Presumably clang-cl might also be adding the resource-dir silently like Clangd?

The compile-commands.json seems to be generated for the MSVC or hand-crafted, I wonder if it may be a difference between the clang frontend and msvc (unlikely, but just to be sure).

Sorry, I confused things because I changed the generator to output "cl" instead of "clang-cl" just as a test. I wrote the generator so you can say that I hand-crafted the generator? ;) Anyway, I've seen no difference between using cl or clang-cl in the compile_commands.json

The compile_commands.json does not even mention the -resource-dir, I wonder why would adding it break anything? The only thing that comes to mind is some of the microsoft headers being named the same as the clang headers, but having more stuff in them (the uintptr_t that's missing). Is that the case?

Here is the preprocessor output when I run the "win installer" clang-cl (no -resource-dir):

# 48 "C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\Professional\\VC\\Tools\\MSVC\\14.15.26726\\include\\vcruntime.h" 2 3
# 1 "C:\\Program Files\\LLVM\\lib\\clang\\8.0.0\\include\\vadefs.h" 1 3
# 32 "C:\\Program Files\\LLVM\\lib\\clang\\8.0.0\\include\\vadefs.h" 3  !!!! #include_next <vadefs.h> !!!!
# 1 "C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\Professional\\VC\\Tools\\MSVC\\14.15.26726\\include\\vadefs.h" 1 3
...
typedef unsigned __int64 uintptr_t;

Here is the preprocessor output when I run the custom-built clang-cl (no -resource-dir, but with /I"C:\Program Files\LLVM\lib\clang\8.0.0\include"):

# 48 "C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\Professional\\VC\\Tools\\MSVC\\14.15.26726\\include\\vcruntime.h" 2 3
# 1 "C:\\Program Files\\LLVM\\lib\\clang\\8.0.0\\include\\vadefs.h" 1 3
# 32 "C:\\Program Files\\LLVM\\lib\\clang\\8.0.0\\include\\vadefs.h" 3 !!!! #include_next <vadefs.h> !!!!
# 1 "D:\\git\\llvm\\build-vs2017\\RelWithDebInfo\\lib\\clang\\8.0.0\\include\\vadefs.h" 1 3
# 33 "C:\\Program Files\\LLVM\\lib\\clang\\8.0.0\\include\\vadefs.h" 2 3
# 49 "C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\Professional\\VC\\Tools\\MSVC\\14.15.26726\\include\\vcruntime.h" 2 3
...
(MSVC's vadefs.h never included)

So the #include_next in the presence of two vadefs.h from different Clang messes it up. It seems wrong to have this include in the compile commands: /I"C:\Program Files\LLVM\lib\clang\8.0.0\include", right? Because it will always clash with any other Clang's internal headers. I'll check if we can remove that flag in the build or if there was some special reason it was there that could impact the solution for the problem we're trying to solve here.

clangd/ClangdServer.cpp

 tooling::CompileCommand ClangdServer::getCompileCommand(PathRef File) {
   trace::Span Span("GetCompileCommand");
   Optional<tooling::CompileCommand> C = CDB.getCompileCommand(File);
   if (!C) // FIXME: Suppress diagnostics? Let the user know?
     C = CDB.getFallbackCommand(File);
 
   // Inject the resource dir.
   // FIXME: Don't overwrite it if it's already there.
+  if (!ResourceDir.empty())
     C->CommandLine.push_back("-resource-dir=" + ResourceDir);
   return std::move(*C);
 }
 
 void ClangdServer::onFileEvent(const DidChangeWatchedFilesParams &Params) {
   // FIXME: Do nothing for now. This will be used for indexing and potentially
   // invalidating other caches.
 }
 

clangd/tool/ClangdMain.cpp

int main(int argc, char *argv[]) {
   switch (PCHStorage) {
   case PCHStorageFlag::Memory:
     Opts.StorePreamblesInMemory = true;
     break;
   case PCHStorageFlag::Disk:
     Opts.StorePreamblesInMemory = false;
     break;
   }
-  if (!ResourceDir.empty())
+  if (ResourceDir.getNumOccurrences() > 0)
     Opts.ResourceDir = ResourceDir;
   Opts.BuildDynamicSymbolIndex = EnableIndex;
   Opts.HeavyweightDynamicSymbolIndex = UseDex;
   std::unique_ptr<SymbolIndex> StaticIdx;
   std::future<void> AsyncIndexLoad; // Block exit while loading the index.
   if (EnableIndex && !IndexFile.empty()) {
     // Load the index asynchronously. Meanwhile SwapIndex returns no results.
     SwapIndex *Placeholder;