search

jacobdufault / cquery

C/C++ language server supporting multi-million line code base, powered by libclang. Emacs, Vim, VSCode, and others with language server protocol support. Cross references, completion, diagnostics, semantic highlighting and more
MIT License
2.35k stars 163 forks source link

FindSystemIncludeDirectories() thwarts certain options #641

Closed joelhock closed 6 years ago

joelhock commented 6 years ago

77d6ff0da7 enabled the FindSystemIncludeDirectories functionality for mac. My project is perhaps a bit weird, but we are doing cross-platform development for a linux target on a mac editor, using a .cquery file.

FindSystemIncludeDirectories passes only --gcc-toolchain and --sysroot on to the discovery, but other flags also influence the output of the command, namely -nostdinc, -nostdinc++, -nobuiltininc, -resource-dir=xxxxx, and -stdlib=xxxxx.

The FindSystemIncludeDirectories logic seems to undo the -resource-dir handling that is curated through the rest of the code, since the clang command does not receive any -resource-dir flag (or the baked-in hardcode), and its output flags will take precedence over any -resource-dir-induced flags. The result being the flags gotten from the clang binary used for compilation may have an incompatible resource dir wrt the libclang that cquery uses.

I'm happy to make a pull request, but I'm not sure what the philosophy is behind FindSystemIncludeDirectories, so comments are welcome. Perhaps FindSystemIncludeDirectories should always specify -nobuiltininc since resource-dir is handled by other parts of the code. I can just -I the resource dir in my .cquery file, but I feel like the current code exacerbates differences between the cquery libclang and the project clang binary in an unexpected way. I doubt there's a "right" answer here, but perhaps there's something better--certainly cquery is doing an admirable job in getting sane compilation flags, but it's a difficult road to navigate.

jacobdufault commented 6 years ago

but it's a difficult road to navigate.

Yes, parsing the project config is easily the most convoluted part of cquery. libclang does not parse arguments as clang does (it misses some functionality, like system directory resolution), and cquery also supports non-clang style arguments.

The FindSystemIncludeDirectory logic is important because by default libclang will fail to resolve things like <vector>. I'd be very happy to have a PR improving the logic around it.

joelhock commented 6 years ago

It seems libclang looks for the libc++ includes in ../include relative to the realpath of libclang.dylib, which the downloaded tar correctly positions (and brew and xctoolchain from what I can tell), but I've certainly used libclang.dylibs where ../include didn't exist. And, now I remember that I set -target x86_64-unknown-linux-gnu, mainly because we're doing cross-platform work and are targeting linux from a mac dev host (so we need the target to set the correct default linux defines), but that setting the target triple also undoes some of the braindeadness in mac libclang that causes it to not respect -resource-dir and perhaps other oddities, so I'm a bizarre user.

Anyway, here are two suggestions. I can make a PR that does either one or both, and it might be best to just wait to see if anyone else chimes in as to whether these would be helpful.

  1. add the GetSystemIncludes results via -idirafter / --include-directory-after= so that the libclang built-ins are preferred, if available.
  2. add -nostdinc, -nostdinc++, -nobuiltininc, and -stdlib=libstdc++ to GetSystemIncludes's kFlagsToPass
jacobdufault commented 6 years ago

The above sounds reasonable to me. wdyt @bstaletic?

bstaletic commented 6 years ago

@joelhock Thanks for reporting this.

Adding -nostdinc and others mentioned in point 2 sounds good.

As for -idirafter, it shouldn't be necessary because the associated path shall be picked up in IncludeComplete::Rescan(). Note that for this to work the -idirafter needs to exist in kPathArgs in src/project.cc. This is fine for -idirafter, but --include-directory-after is not there.

joelhock commented 6 years ago

yes, i noticed i would have to add --include-directory-after to kPathArgs to use it; i'll do that anyway as a separate PR and also make a PR for point 2 of my list.

@bstaletic Sorry, I'm not following what you're saying about IncludeComplete::Rescan(). Using -idirafter instead of -isystem in ExtractSystemIncludePaths does make a difference for me. The clang binary that gets executed is the system clang, so its (incompatible) resource dir gets added via -isystem/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/9.1.0/include, and this trumps the resource dir that the downloaded libclang has baked in. When that system path is instead added via -idirafter, it is searched after the resource dir that is baked-in to libclang, so the correct one is used. At least "correct" from my perspective; I wouldn't be shocked if someone else expected the reverse to be true, but the current setup does seem much more likely to foist an incompatible resource dir onto libclang.

bstaletic commented 6 years ago

Yes, -idirafter does change system headers. What I'm saying is that it shouldn't matter. The only reason you would want to put something in the FindSystemIncludeDirectories() is to have cquery find "implicit" include paths. -idirafter requires you to write a path as well, which means that that include path will be picked up when cquery scans the directories for include headers, which happens in IncludeComplete::Rescan(). If we place -idirafter in kPathArgs and kFlagsToPass, the path associated with -idirafter will end up in your flags twice, once with -isystem and once with -idirafter.

joelhock commented 6 years ago

ok, put the couple actionable items in referenced PRs. feel free to close this.

joelhock commented 6 years ago

@bstaletic after re-reading your last comment, I think I was miscommunicating. I was ultimately suggesting something like this:

+++ b/src/clang_system_include_extractor.cc
@@ -50,7 +50,7 @@ std::vector<std::string> ExtractSystemIncludePaths(
       line = line.substr(0, line.size() - sizeof(kIncludePostfixToRemove) + 1);
       output.push_back("-iframework" + line);
     } else {
-      output.push_back("-isystem" + line);
+      output.push_back("-isystem-after" + line);
     }
   }

If I'm not understanding what you're saying, feel free to ignore me. If it's a problem for other people, I'm sure it'll come up again.

bstaletic commented 6 years ago

I couldn't find the the documentation on -isystem-after. Does it "just" add system include paths after -isystem? If so then I'm not sure if it would make any difference.

Note that -isystem flags are parsed "left to right" and that the "discovered" system paths are (or at least should be) appended to the user provided flags. Therefore the discovered paths come after user specified paths.

joelhock commented 6 years ago

@bstaletic, it turns out -isystem-after is supported only on the windows version of clang, although

This concept is actually useful on other targets (e.g. Linux) and would be really easy to support on the core toolchain. http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160613/162393.html

And, actually it seems that -idirafter does everything that I was expecting -isystem-after to do, so just pretend I never mentioned -isystem-after.

I realize the paths discovered by FindSystemIncludeDirectories() will come after the user-provided flags. I was proposing something that would also put them after the directories that are implicitly added by libclang itself, namely the resource dir, since the resource dir found for the system clang (in my case) by FindSystemIncludeDirectories() is not compatible with the libclang.dylib downloaded and used by cquery.

FindSystemIncludeDirectories() for me runs clang++ -E -xc++ - -v -isystem/Volumes/hrt < /dev/null and finds:

#include <...> search starts here:
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1
 /usr/local/include
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/9.1.0/include
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include
 /usr/include
 /System/Library/Frameworks (framework directory)
 /Library/Frameworks (framework directory)
End of search list.

the downloaded libclang is compatible with the resource dir discovered by this (if i copy the included binary to the logical location): /usr/local/lib/clang+llvm-6.0.0-x86_64-apple-darwin/bin/clang -E -xc++ - -v < /dev/null

#include <...> search starts here:
 /usr/local/include
 /usr/local/lib/clang+llvm-6.0.0-x86_64-apple-darwin/lib/clang/6.0.0/include
 /usr/include
 /System/Library/Frameworks (framework directory)
 /Library/Frameworks (framework directory)
End of search list.

so if I have a project where -isystem/Users/joel/myproj-system-includes is a user-provided flag, the effective search path for the current code is: /usr/local/lib/clang+llvm-6.0.0-x86_64-apple-darwin/bin/clang -E -xc++ - -v -isystem/Users/joel/myproj-system-includes -isystem/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1 -isystem/usr/local/include -isystem/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/9.1.0/include -isystem/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include -iframework/System/Library/Frameworks -iframework/Library/Frameworks < /dev/null

#include <...> search starts here:
 /Users/joel/myproj-system-includes
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1
 /usr/local/include
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/9.1.0/include
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include
 /System/Library/Frameworks (framework directory)
 /Library/Frameworks (framework directory)
 /usr/local/lib/clang+llvm-6.0.0-x86_64-apple-darwin/lib/clang/6.0.0/include
 /usr/include
End of search list.

which results in errors like this when parsing a file: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/9.1.0/include/emmintrin.h2261:19 use of undeclared identifier '__builtin_ia32_pavgb128', since the resource dir for the system clang is searched before the resource dir for the downloaded clang.

if instead we use idirafter like: /usr/local/lib/clang+llvm-6.0.0-x86_64-apple-darwin/bin/clang -E -xc++ - -v -isystem/Users/joel/myproj-system-includes -idirafter/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1 -idirafter/usr/local/include -idirafter/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/9.1.0/include -idirafter/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include -iframework/System/Library/Frameworks -iframework/Library/Frameworks < /dev/null

we get:

 /Users/joel/myproj-system-includes
 /System/Library/Frameworks (framework directory)
 /Library/Frameworks (framework directory)
 /usr/local/include
 /usr/local/lib/clang+llvm-6.0.0-x86_64-apple-darwin/lib/clang/6.0.0/include
 /usr/include
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/9.1.0/include
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include
End of search list.

and the resource dir that libclang is compatible with is found first. It would seem to me that many users would run into this; the fact that they haven't suggests maybe something is even weirder with my setup than i thought.

bstaletic commented 6 years ago

I'm on my phone, so excuse my brevity.

MacOS and iOS have been known to cause these issues. The problem is that the downloaded (open source) libclang lacks Apple's special sauce to make it work properly. System includes are just the beginning of the story. The open source libclang won't even be able to parse files in /Applications/Xcode/whatever.

Your best bet is to avoid open source libclang on Apple operating systems.

@jacobdufault I wonder if there is anything we can do to lower the number of "cquery is broken" reports when it is just "Apple broke open source libclang".

jacobdufault commented 6 years ago

I'm not sure, if we get a significant number of bug reports we can try adding more diagnostics.

bstaletic commented 6 years ago

Unfortunately, ycmd did have enough of reports exactly like this one for me to remember that open source libclang is broken on an OS I don't use.

It's rather simple - if the project is an Xcode project, bundled libclang will have problems parsing it.

Examples of this kinds of issues:

Slightly related but not exactly the same (Apple shuffling system include paths, just to screw with people leading projects like cquery and ycmd):

Another thing I noticed while searching through our issue tracker, MacOS users tend to use --system-libclang.

joelhock commented 6 years ago

Yikes! That's a thorough documentation of a hard road ycmd trekked through that cquery would be wise to learn from. I'd appreciate if you didn't have to go through extreme contortions to use the open-source clang download, but it makes sense to me after reading those posts that having system clang be the default on mac has its advantages.