Fix libclang's #include search path

[nickhutchinson]

Fixes #809.

It appears libclang.(dylib|so) looks for the builtin compiler includes directory relative to itself at ./clang/3.4/include/. YCM instead feeds libclang its own copy of the builtin headers using the compiler flag -isystem ~/.vim/bundle/YouCompleteMe/python/clang_includes or similar. This means the include search path is messed up: builtin headers now come after /usr/include and friends, when they should really come before. In previous llvm releases this didn't actually matter, but as of llvm 3.4 it interacts badly with their new "modules" feature.

Changes:

• Remove the python/clang_includes/ folder, update flags.py to stop passing it to libclang as a compiler flag
• Update CMake build script to copy the lib/clang directory from the LLVM source tree to PATH_TO_YCM/python/clang.

Seems to work on OS X and Debian. I haven't tested this change on Windows, because it looks truly painful to get YCM running there.

[Valloric]

Remove the python/clang_includes/ folder, update flags.py to stop passing it to libclang as a compiler flag
Update CMake build script to copy the lib/clang directory from the LLVM source tree to PATH_TO_YCM/python/clang.

We can't assume the user has access to the LLVM source tree when building; lots of users just use the system libclang.

The easy fix is to have a copy of the 3.4 includes in python/clang/3.4 and a copy of 3.3 includes in python/clang/3.3 (so we don't break people who still use a 3.3 libclang).

[nickhutchinson]

If you reckon it's easier to just add another set of the builtin includes to YCM every time there's a new LLVM release, then I'll make the change.

However I did try to ensure the CMake finds the builtin headers by looking for the clang directory adjacent to wherever it finds the libclang shared lib, and this works just fine for a system libclang. However I see now there's an annoying niggle: Linux distros don't seem to always package the builtin headers in the core libclang package. e.g. Debian has libclang1-3.3 which doesn't include them, and libclang-3.3-dev which does.

I guess I could update the CMake script to complain if the builtin headers aren't there and tell the user to go install the additional package.

[Valloric]

If you reckon it's easier to just add another set of the builtin includes to YCM every time there's a new LLVM release, then I'll make the change.

Just include them. YCM only officially supports the latest version of libclang anyway, but I know some people are currently using it with 3.3 and I don't want to break them. Next release of libclang we'll probably only have 3.5 headers and be done with it... or we could just leave the old ones in place. It doesn't really matter.

However I did try to ensure the CMake finds the builtin headers by looking for the clang directory adjacent to wherever it finds the libclang shared lib, and this works just fine for a system libclang.

I don't want to depend on the system being set up properly because often it isn't, as you've found.

[nickhutchinson]

Okay, I've updated the pull request. Annoyingly, Xcode's clang uses a different versioning scheme, so I had to include yet another set of headers corresponding to Xcode 5.1, otherwise it's not going to work for OS X users who use --system-libclang. I guess this means that potentially YCM will need to be updated for any new Xcode release as well.

[Valloric]

Annoyingly, Xcode's clang uses a different versioning scheme, so I had to include yet another set of headers corresponding to Xcode 5.1, otherwise it's not going to work for OS X users who use --system-libclang. I guess this means that potentially YCM will need to be updated for any new Xcode release as well.

Ugh... I think we'll just recommend against using the system libclang on Mac OS X then.

[Valloric]

I'll have to try this out on a few machines first, but it looks good to merge. Thanks for working on this!

[Valloric]

This seems to work fine. Crossing my fingers hoping it doesn't break something unexpected.

Thanks for the PR!

[cryptomilk]

With this patch YCM doesn't find stddef.h anymore which is on my system in:

/usr/lib64/clang/3.3/include/

So I need to add this directory to the include path using my ycm config. So YCM need to check several path if it can find the include directory. So relative to itself and also check

libpath + /../lib/clang/
libpath + /../clang/
libpath + /clang/
/usr/lib64/clang
/usr/lib/clang

[oblitum]

I didn't apply this yet but, will this mess anything with my --system-libclang install? Where I properly set my custom clang include directories etc before the YCM build. I use clang from repo, and install it anywhere I please, and still, I was always successful in using it with YCM, avoiding the extra downloads etc.

[cryptomilk]

I also use --system-libclang ...

[nickhutchinson]

The patch should continue work with --system-libclang, and does in my testing. Hmm. @gladiac, what OS are you running on?

Edit: More importantly, what flags are you passing to libclang via YCM? If you have -nobuiltininc or -nostdinc there, take it out.

[cryptomilk]

openSUSE 13.1. I rerun intsall.sh but it doesn't change anything ...

[Valloric]

So YCM need to check several path if it can find the include directory. So relative to itself and also check

YCM is not doing the path lookup, libclang itself is. It looks for Clang-builtin headers in ./clang/<version>/includes/, relative to the location of the libclang.so binary.

I just checked, and the Clang builtin headers that YCM is now shipping do include the stddef.h header in the python/clang/[3.3 and 3.4]/includes folder.

I'm guessing you're using a system libclang instead of the upstream one and your distro changed the way Clang performs include dir lookups. Try to use the upstream libclang and see if that helps.

[cryptomilk]

It worked before and stopped working with this change. I would call this a regression.
It is also working with clang_complete. If clang can successfully compile objects then YCM should be able to do the same, shouldn't it?

So YCM needs a way to correctly find the system standard header files of the installed clang. Installing a non standard package is not the way we work on Linux and distributions :)

@nickhutchinson If you want to know which flags I pass, take a look here https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/heads/ycm

[cryptomilk]

YCM is not doing the path lookup, libclang itself is. It looks for Clang-builtin headers in
./clang//includes/, relative to the location of the libclang.so binary.

Well, that's exactly where my headers are:

The library is:
/usr/lib64/libclang.so.3.3

The headers are at:
/usr/lib64/clang/3.3/include/

So it should work, but it doesn't. The clang binary also detects them.

echo | clang -v -E -x c -
clang version 3.3 (branches/release_33 183898)
Target: x86_64-suse-linux
Thread model: posix
 "/usr/bin/clang-3.3" -cc1 -triple x86_64-suse-linux -E -disable-free -disable-llvm-verifier -main-file-name - -mrelocation-model static -mdisable-fp-elim -fmath-errno -masm-verbose -mconstructor-aliases -munwind-tables -fuse-init-array -target-cpu x86-64 -v -resource-dir /usr/bin/../lib64/clang/3.3 -internal-isystem /usr/local/include -internal-isystem /usr/bin/../lib64/clang/3.3/include -internal-externc-isystem /include -internal-externc-isystem /usr/include -fdebug-compilation-dir /home/asn/workspace/projects/samba/git -ferror-limit 19 -fmessage-length 184 -mstackrealign -fobjc-runtime=gcc -fobjc-default-synthesize-properties -fdiagnostics-show-option -fcolor-diagnostics -backend-option -vectorize-loops -o - -x c -
clang -cc1 version 3.3 based upon LLVM 3.3svn default target x86_64-suse-linux
ignoring nonexistent directory "/include"
#include "..." search starts here:
#include <...> search starts here:
 /usr/local/include
 /usr/bin/../lib64/clang/3.3/include
 /usr/include
End of search list.
# 1 "<stdin>"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 159 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "<stdin>" 2

[Valloric]

It worked before and stopped working with this change. I would call this a regression.

We're actually doing things in a more standard way than before; the previous hack of forcing a new include path broke tons of stuff over the years.

If clang can successfully compile objects then YCM should be able to do the same, shouldn't it?

There's no "YCM" here, you're actually comparing clang-the-binary with libclang-the-library (which YCM uses). And if you're saying "libclang should compile stuff the same way that clang does", I completely agree with you... except unfortunately that's not the way it actually works. I've complained about this before to Clang devs. See issue #303 to learn more about unfortunate differences between libclang and clang compilation.

So YCM needs a way to correctly find the system standard header files of the installed clang.

It's more complicated than that. Every copy of libclang needs to find its own corresponding builtin headers, and the way it looks for them is hard-coded into libclang (and clang) itself. Again, note there's no "YCM" here. We used to do something terribly hacky by trying to add search paths to a copy of builtin headers we'd ship, but like I said, that caused tons of problems and would cause even more problems in the future because of the upcoming Clang support for C++ modules.

Installing a non standard package is not the way we work on Linux and distributions :)

Sigh... there's no installation of "a non standard package." The only recommended way to get a libclang that works with YCM is to use the upstream LLVM pre-built binaries. YCM's install.sh script doesn't install them to the system, it downloads and then copies them to the YouCompleteMe/python folder. If you choose to use the system libclang (which is NOT recommended, and never has been; see the docs), then we copy that system libclang to the python folder so that other YCM libs can find it.

The root of your problem is almost certainly caused by your linux distro changing their local system libclang to use different search paths instead of sending patches upstream. This has always been a bad idea and it's exactly why YCM recommends against using the system libclang: we don't know what custom changes have been done to it. Using it might randomly break things, as you've discovered.

[cryptomilk]

So it is broken by design from the clang developers ...

http://comments.gmane.org/gmane.comp.compilers.clang.devel/34192

[Valloric]

So it is broken by design from the clang developers ...

"Broken by design" is a bit strong; more like "broken because currently we don't care enough to fix it and fixing it would be non-trivial". See this clang-dev thread.

Neither of us like their position on this, but it is what it is.

[Valloric]

will this mess anything with my --system-libclang install

The answer to that is maybe. It depends on what your distro did to your system libclang (they often change the default header search paths). Using the system libclang is strongly NOT recommended (and always has been); I've added stronger language to the docs discouraging such a configuration.

[cryptomilk]

According to http://clang-developers.42468.n3.nabble.com/How-to-determine-clang-s-system-include-dirs-to-set-in-ASTVisitor-td4029080.html the executable needs to be next to the clang executable to be able to find the correct path. As vim is here in /bin/vim it probably will not find it cause /bin/../lib64/clang/3.3/include doesn't exist ...

[Valloric]

the executable needs to be next to the clang executable to be able to find the correct path. As vim is here in /bin/vim it probably will not find it cause /bin/../lib64/clang/3.3/include doesn't exist ...

No, the lookup is relative to the libclang.so location. I know because I've hooked up dtrace on Mac OS X and strace on Linux to binaries that load libclang and have seen the behavior with my own eyes. I did this yesterday again to confirm everything was working on my machines.

[cryptomilk]

27292 00:11:45 stat("../lib64/clang/3.3/include", 0x7f65155f5738) = -1 ENOENT (No such file or directory)
27292 00:11:45 stat("../lib64/clang/3.3", 0x7f65155f5608) = -1 ENOENT (No such file or directory)

I think you're wrong, else I wouldn't get ENOENT in my strace here :)

[oblitum]

If you choose to use the system libclang (which is NOT recommended, and never has been;
see the docs), then we copy that system libclang to the python folder so that other YCM libs
can find it.

Relieved, since what I'm looking for is to have coding assistance being able to assist with coding for my environment (which now, at my machine, it's to try every feature I'm interested in on tip of LLVM/Clang) and not just be able to assist through the feature set of some point release (which may lack features from time to time).

Using a point release is not the recommend way in this case. It may be the recommended way so that YCM can work, but it's not from the POV of assisting me.

Anyway, I think it's still working without problems, since I don't have any header lookup patch over the repository. I just hope --system-libclang doesn't get deprecated.

[Valloric]

@nickhutchinson I forgot to ask you to sign the Google CLA when you sent the PR initially. As per CONTRIBUTING.md, please sign the Google CLA so that I don't have to revert this PR.

@gladiac This only goes to show that your system libclang has been messed with.

@oblitum --system-libclang might actually be deprecated in the future. It's causing a lot of pain... I'm inclined to keep it as long as people are aware that stuff might break for them and that they're on their own then.

[cryptomilk]

After a closer look, it does it relative to the path I execute vim at.

I did ln -s /usr/lib64 ../lib64 and now it finds stddef.h. There is no difference if I build with --system-libclang or without. Maybe the system one is used first ...

[Valloric]

After a closer look, it does it relative to the path I execute vim at.

That's really weird... Vim isn't the binary that's loading libclang at all, ycmd does it (which is started by Vim though).

Maybe the system one is used first ...

Possibly, but that shouldn't be the case. YCM builds ycm_core.so (which does the loading of libclang.so) so that it always first looks for libclang.so in the same folder.

[Valloric]

Ok, this PR seems to be breaking a lot of users... we need to come up with something between the old approach and the new one. Something like using the extra header search path hack for some systems and not for others.

I'm reverting this until we come up with such a solution.

[Valloric]

Now reverted.

[nickhutchinson]

@Valloric I've now signed the CLA.
@gladiac That OpenSUSE is apparently messing with libclang is fairly annoying; libclang should be determining the builtin include path according to https://github.com/llvm-mirror/clang/blob/release_34/tools/libclang/CIndexer.cpp or https://github.com/llvm-mirror/clang/blob/release_33/tools/libclang/CIndexer.cpp.

There might be a way out that works for everyone. Perusing the clang source code I see there's an (undocumented?) command line flag called -resource-dir that takes the path to the clang resource directory containing the builtin includes.

[cryptomilk]

@nickhutchinson openSUSE sets CLANG_RESOURCE_DIR.

%if "%{_lib}" == "lib64"
-DLLVM_LIBDIR_SUFFIX=64
-DCLANG_RESOURCE_DIR=../%{_lib}/clang/%{_release_version}
%endif

The CMakeLists.txt has
set(CLANG_RESOURCE_DIR "" CACHE STRING
"Relative directory from the Clang binary to its resource files.")

I think the issue here is simply that it is a relative directory ...

If you look at:
https://github.com/llvm-mirror/clang/blob/7683f196ea8fdc42b0ff9705f9770e3424f9e593/lib/Driver/Driver.cpp

You can see that the default is ../lib/clang/. As it is in lib64 it is correctly set. I think the code should check if the string starts with / and then use an absolute path and all would be fine ...

[cryptomilk]

 CMakeLists.txt        | 2 +-
 lib/Driver/Driver.cpp | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2d7bb6f..205c061 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -164,7 +164,7 @@ if( CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR )
 endif()

 set(CLANG_RESOURCE_DIR "" CACHE STRING
-  "Relative directory from the Clang binary to its resource files.")
+  "Relative directory from the Clang binary to its resource files or an absolute path.")

 set(C_INCLUDE_DIRS "" CACHE STRING
   "Colon separated list of directories clang will search for headers.")
diff --git a/lib/Driver/Driver.cpp b/lib/Driver/Driver.cpp
index 317b822..92105be 100644
--- a/lib/Driver/Driver.cpp
+++ b/lib/Driver/Driver.cpp
@@ -68,7 +68,10 @@ Driver::Driver(StringRef ClangExecutable,
   StringRef ClangResourceDir(CLANG_RESOURCE_DIR);
   SmallString<128> P(Dir);
   if (ClangResourceDir != "")
-    llvm::sys::path::append(P, ClangResourceDir);
+    if (ClangResourceDir.startswith("/"))
+      P = ClangResourceDir;
+    else
+      llvm::sys::path::append(P, ClangResourceDir);
   else
     llvm::sys::path::append(P, "..", "lib", "clang", CLANG_VERSION_STRING);
   ResourceDir = P.str();
´´´

[cryptomilk]

I've talked to Ismail. ResourceDir is a relative path so the binary finds the include directory for the tests which are run in source. We need ResourceDir to be a list which is checked if it exists and has e.g. stddef.h in it. So when installed it will use an absolute path and not guessing the directory.

[cryptomilk]

Ok, after fixing LLVM on openSUSE it works now, but I had to delete the provided header files. I think you should rely on a working libclang.so of the system. But bugs need to be reported to the distributions. I will look into Fedora next and make sure the resource dir is detected correctly using libclang.so.

I think the pull request is correct and valid. We just need to fix libclang.so in all distributions.

[Valloric]

We just need to fix libclang.so in all distributions.

"Just"? And then "just" ask everyone to update their system?

That isn't feasible. This is why YCM isn't a fan of the system libclang. We need something that works well with upstream libclang code (of which there's one version instead of N versions across distros).

[cryptomilk]

FYI: It works fine on Fedora, openSUSE is fixed but not in 13.1. Do you know if there exists an upstream bug which reports the problem?

[oblitum]

@Valloric Since you mentioned --system-libclang might actually be deprecated in the future and it's being now strongly not recommend, I suggest instead of completely banishing such option, if it gets buried, at the same time to create a new one more targetted at custom LLVM installation tree. Because, even though using a system libclang from apt etc may be indeed a headache because of the additional patches not from upstream, using --system-libclang is a feature for me by now, because I just happen to have a clone of LLVM in my machine, and have an installation based on it.

[Valloric]

@oblitum --system-libclang is unlikely to be outright removed; the "worst" I'd do would be to undocument it and add a comment in the install scripts that using it is experimental and unsupported.

I haven't decided yet; we'll see how much of an issue it continues to be.