Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ClangCompleter: -include and -include-pch not working #892

Open
nilsdeppe opened this issue Dec 24, 2017 · 13 comments

Comments

Projects
None yet
5 participants
@nilsdeppe
Copy link

commented Dec 24, 2017

Hi,

Thanks for the really awesome package!

I recently switched to using precompiled headers in a project (publicly available here) and I now encounter the issue:

2017-12-24 14:44:31,889 - INFO - Received completion request
2017-12-24 14:44:31,890 - DEBUG - Using filetype completion: True
2017-12-24 14:44:31,893 - ERROR - Exception from semantic completer (using general): Traceback (most recent call last):
  File "/home/nils/Research/ycmd/ycmd/../ycmd/handlers.py", line 103, in GetCompletions
    .ComputeCandidates( request_data ) )
  File "/home/nils/Research/ycmd/ycmd/../ycmd/completers/completer.py", line 218, in ComputeCandidates
    candidates = self._GetCandidatesFromSubclass( request_data )
  File "/home/nils/Research/ycmd/ycmd/../ycmd/completers/completer.py", line 231, in _GetCandidatesFromSubclass
    raw_completions = self.ComputeCandidatesInner( request_data )
  File "/home/nils/Research/ycmd/ycmd/../ycmd/completers/cpp/clang_completer.py", line 175, in ComputeCandidatesInner
    raise RuntimeError( NO_COMPLETIONS_MESSAGE )
RuntimeError: No completions found; errors in the file?

The simplest example that recreates this problem is:

#include "pch.hpp"
int main() {
  std::vector<double> a(10, 1.0);
  a. // Expected completion list doesn't appear and I get the error above
}

I can successfully compile the code with:

clang++ -std=c++14 -include-pch ./pch.hpp.gch ./test_pch.cpp

The pch.hpp file is:

#ifndef MY_PCH_HPP
#define MY_PCH_HPP
#include <vector>
#endif

The flags for the translation unit are:

['-x', 'c++', '-Wall', '-Wextra', '-Werror', '-std=c++14', '-include-pch', '/home/nils/pch.hpp.gch', '-resource-dir=/home/nils/Research/ycmd/ycmd/../clang_includes', '-fspell-checking']

which I obtained by adding print("\n\n%s\n\n" % list(flags)) to the clang_completer.py on line 166.

Things I've tried to resolve the issue:

  1. Change -include-pch to -include and include the non-compiled header
  2. Remove the -include-pch flag (This worked if I added an explicit include in the cpp file!)

Tracking down the issue I was able to get to the ClangCompleter::CandidatesForLocationInFile function in ~/Research/ycmd/cpp/ycm/ClangCompleter/ClangCompleter.cpp. At this point I have not tried debugging things further, but I noticed @micbou's PR #861 which could be useful in diagnosing the issue.

I'm currently using commit 4fa81b5

  • OS: Antergos/Arch Linux
  • System clang that works to compile example provided above: v5.0.1
@micbou

This comment has been minimized.

Copy link
Collaborator

commented Dec 25, 2017

Thanks for reporting the issue. I tried your example with the changes in PR #861 and I am getting the following error:

ClangParseError: An AST deserialization error occurred while parsing the translation unit.

To confirm this is a libclang issue (and not a ycmd one), I wrote a small program that parses a file with the -include-pch flag through the libclang API (version 5.0.1):

#include <iostream>
#include <clang-c/Index.h>

const char *flags[] = {
    "clang",
    "-include-pch", "pch.hpp.gch"
};

int main(int argc, const char *argv[]) {
    if (argc != 2) {
        std::cerr << argv[0] << " file.cpp\n";
        return EXIT_FAILURE;
    }

    CXIndex index = clang_createIndex(0, 0);
    if (!index) {
        std::cerr << "createIndex failed\n";
        return EXIT_FAILURE;
    }

    unsigned options = clang_defaultEditingTranslationUnitOptions();
    CXTranslationUnit tu;
    int code = clang_parseTranslationUnit2FullArgv(
        index, argv[1], flags, sizeof(flags) / sizeof(flags[0]), nullptr, 0,
        options,
        &tu);
    if (code != CXError_Success) {
        std::cerr << "Failed to parse translation unit with error code: "
                  << code << "\n";
        return EXIT_FAILURE;
    }

    return EXIT_SUCCESS;
}

Running this program on your example prints:

Failed to parse translation unit with error code: 4

This error code corresponds to the AST deserialization error above. In fact, whether the pch.hpp.gch file exists or not, the error code 4 is always returned. Looks like libclang cannot handle the -include-pch flag at all. That's a little surprising given that we added support for the -include-pch flag in ycmd (see PRs #368 and #783) but got no report that it wasn't working (until now).

@nilsdeppe

This comment has been minimized.

Copy link
Author

commented Dec 25, 2017

Thanks for the reply! I figured this was really some issue related to AST parsing in libclang. I just dug around a bit in the libclang documentation (relevant section here) and it looks like the solution is changing the list of flags from:

[..., '-include-pch', 'pch.hpp.gch', ...]

to

[..., '-Xclang', '-include-pch=pch.hpp.gch', ...]

I just tested this by changing my .ycm_extra_conf.py to have a function that goes through all the flags and checks if any are a -include or -include-pch. This resolves the issue for me locally, but it would be good to add support for the -include and -include-pch flags in ycmd. I'm not familiar enough with the ycmd code base to know where this check should be added.

@micbou

This comment has been minimized.

Copy link
Collaborator

commented Dec 25, 2017

I doubt this is doing what you are expecting. -Xclang flags are automatically removed so what you are really doing is removing the -include-pch flag.

The example given in the libclang documentation seems wrong. -include-pch cannot be used this way:

$ clang -include-pch=pch.hpp.gch main.cpp
<built-in>:1:10: fatal error: '-pch=pch.hpp.gch' file not found
#include "-pch=pch.hpp.gch"
         ^~~~~~~~~~~~~~~~~~
1 error generated.
@nilsdeppe

This comment has been minimized.

Copy link
Author

commented Dec 25, 2017

Okay, I just looked through the flags.py file and it was just ignoring the flag. I've played around with your libclang example above some more and the following doesn't emit any errors:

#include <iostream>
#include <clang-c/Index.h>

const char *flags[] = {
    "clang++",
    "-std=c++14",
    "-include-pch",
    "/home/nils/pch.hpp.gch"
};

int main(int argc, const char *argv[]) {
    if (argc != 2) {
        std::cerr << argv[0] << " file.cpp\n";
        return EXIT_FAILURE;
    }

    CXIndex index = clang_createIndex(0, 1);
    if (!index) {
        std::cerr << "createIndex failed\n";
        return EXIT_FAILURE;
    }

    unsigned options = clang_defaultEditingTranslationUnitOptions();
    CXTranslationUnit tu;
    int code = clang_parseTranslationUnit2FullArgv(
        index, argv[1], flags, sizeof(flags) / sizeof(flags[0]), nullptr, 0,
        options,
        &tu);
    if (code != CXError_Success) {
        std::cerr << "Failed to parse translation unit with error code: "
                  << code << "\n";
        return EXIT_FAILURE;
    }

    return EXIT_SUCCESS;
}

With the example you had I got the diagnostic that the PCH and the code were compiled with different -std versions. Unfortunately this doesn't help me make any more sense of why there is an error when calling libclang from ycmd.

@micbou

This comment has been minimized.

Copy link
Collaborator

commented Dec 25, 2017

That's it. The compiler used to generate the pch.hpp.gch file must match the version of libclang in ycmd.
If ycmd is compiled against libclang 5.0.1, the precompiled header must be generated with Clang 5.0.1 or it won't work (I tried with GCC, Clang 3.8, Clang 4.0.0, and Clang 5.0.0). Which version of libclang are you on?

@nilsdeppe

This comment has been minimized.

Copy link
Author

commented Dec 25, 2017

Aha! This is interesting. My clang --version and the Arch Linux current LLVM is 5.0.1, which naively should be compatible with what ycm downloads (/home/nils/Research/ycmd/cpp/../clang_archives/clang+llvm-5.0.1-x86_64-linux-gnu-ubuntu-14.04.tar.xz). However, that doesn't seem to be the case. If I enable --system-libclang then everything seems to work fine.

@micbou

This comment has been minimized.

Copy link
Collaborator

commented Dec 26, 2017

Yes, I can confirm. This makes it even more difficult to use that flag with libclang. I think we should automatically replace it with a -include flag and remove the .gch (or .pch) extension in the filename. For instance, -include-pch pch.hpp.gch would become -include pch.hpp. Thoughts?

@bstaletic

This comment has been minimized.

Copy link
Collaborator

commented Dec 26, 2017

Doing such a replace would break if the precompiled header was generated like this:

clang -cc1 foo.h -emit-pch -o bar.h.gch

Or even worse, because it looks somewhat reasonable:

clang -cc1 include/foo.h -emit-pch -o pch/foo.h

And if it was my project, it would be:

clang -cc1 foo.h -emit-pch pch/foo.h.pch

Maybe we should just make an FAQ entry about this and call it a day?

@Valloric

This comment has been minimized.

Copy link
Owner

commented Dec 26, 2017

This one is tricky... how about detecting the PCH deser error from libclang and showing an error to the user about using incompatible versions of clang/libclang together? We could have the error message refer to a FAQ entry that would have more info.

@bstaletic

This comment has been minimized.

Copy link
Collaborator

commented Dec 26, 2017

If we can detect that, I think that's a good suggestion.

@nilsdeppe

This comment has been minimized.

Copy link
Author

commented Dec 27, 2017

Yes, I think disabling PCH, even aside from the scenario that @bstaletic described is not ideal. PCHs help reduce parse time dramatically and so I think taking advantage of that for code completion and syntax checking is important. I think detecting that there is an error due to AST parsing and then providing the user an error messaging pointing to the FAQ is the best thing that could be done, if possible. If it's not possible to detect the incompatibility directly, one alternative is when an error occurs here to check if any of the flags are -include-pch and in that case provide the PCH FAQ entry as a suggestion. One way of attempting to check that the PCH AST parsing is the problem, even without relying on such a test inside libclang could be to try this call after the -include-pch flag is stripped. If it succeeds the failure is related to the PCH, if it doesn't then unfortunately the issue could be that the file cannot be compiled correctly without the PCH (I would say this is a flaw in the code base, not ycmd, but it would be trivial to suggest that the PCH may be the issue), parsing the AST, or something else entirely. Even this basic check would've helped me solve the problem right away by reading the to-be-created part of the FAQ.

@puremourning

This comment has been minimized.

Copy link
Collaborator

commented Dec 27, 2017

Please note that libclang always generates a PCH internally irrespective of your options. This is to provide significant speed boost when offering completions.

Personally, I think the manual use of PCH includes is sufficiently niche that we can just provide answers via this GitHub issue. The answer is that either:

  1. You probably don't need it (libclang already creates a preamble)
  2. If you do need it, then it's your responsibility to ensure that the PCH generated is compatible with the version of libclang that ycmd is using.

Otherwise, we're just adding yet-more-special-case code into ycmd which only supports a relatively small number of use cases.

In this case, what is the actual performance difference (in ycmd) of using vs. not using your PCH file?

@nilsdeppe

This comment has been minimized.

Copy link
Author

commented Dec 27, 2017

I have not tried to measure any differences inside ycmd because I've never had any speed troubles with ycmd. Since libclang creates PCHes (which I did not know) then that's not a reason to worry. I don't really have a good feeling for how many projects use PCHes. I think an FAQ entry with a one-liner and linking to this issue would be good. I don't see how this is an overly specialized scenario and that providing a useful error message would be adding code bloat that would not be helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.