Always use the clang builtin headers for the libclang version we use
ClosedPublic

Authored by mwolff on Apr 18 2018, 9:33 PM.

Details

Summary

When we try to parse GCC builtin headers, or headers from other clang
versions, we can easily end up with dozens of parse errors that
eventually reach the error limit. In such cases, kdev-clang becomes
completely unusable.

To prevent this from happening, we filter out the compiler include
path for the builtin headers. This is done using a simple heuristic
by looking for the varargs.h header. For my system, this is only
available in these three folders:

/usr/lib/clang/6.0.0/include/varargs.h
/usr/lib/gcc/arm-none-eabi/7.3.0/include/varargs.h
/usr/lib/gcc/x86_64-pc-linux-gnu/7.3.1/include/varargs.h

All of them are folders for the compiler builtin headers. So now
we always exclude these folders, and instead find the include path
for the folder containing the builtin headers matching the libclang
version we link against. This is required, since libclang does not
find any builtin headers on its own, cf. [1].

[1]: https://clang.llvm.org/docs/LibTooling.html#libtooling-builtin-includes

In the end, this gives us a seemingly well working GCC emulation layer
without the previous pitfalls. The macro compatibility header kept
breaking, and we never had any support for different clang versions.

BUG: 387005

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mwolff created this revision.Apr 18 2018, 9:33 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptApr 18 2018, 9:33 PM
mwolff requested review of this revision.Apr 18 2018, 9:33 PM
brauch added a subscriber: brauch.Apr 18 2018, 10:04 PM

Given the amount of problems we had with this in the past, I think this change makes perfect sense. I applied the patch here, I'll see if anything breaks in the next few days ...

Solves the problem in https://bugs.kde.org/show_bug.cgi?id=387005 for me. KDevelop properly understands Qt code now, irrespective of the selected compiler (tested gcc, clang-3.8, clang-5.0).

mwolff added a subscriber: apol.Apr 19 2018, 10:02 AM

Very good. Now I'd like to get some feedback from @apol to see how this works with his android toolchain. I'll also test this later with an arm toolchain and see if it works as expected.

apol added a comment.Apr 19 2018, 11:03 AM

+1 from my side, I'm running it locally, didn't really have problems earlier don't seem to have them now. Will get back on the Android work and see how it works there soonish, but it's no reason to stop this patch, especially if it fixes bugs.

plugins/clang/libclang_include_path.h.cmake
23

Wouldn't it make sense to make it possible to point at the include directory of the clang we are interested in?

mwolff added inline comments.Apr 19 2018, 1:00 PM
plugins/clang/libclang_include_path.h.cmake
23

I don't know what you mean? We are interested in the directory that fits to the libclang we link against.

apol added a comment.Apr 19 2018, 2:13 PM

It broke my system... It's looking for all classes in my projects into std::, failing and complaining about it. :'(

In D12331#249727, @apol wrote:

It broke my system... It's looking for all classes in my projects into std::, failing and complaining about it. :'(

can you expand on this?

  • what compiler is used?
  • what do we get from the IADM as include paths, defines and arguments?
  • can you show a minimal test file and the errors reported by clang?
apol added a comment.Apr 19 2018, 11:37 PM
  • what compiler is used?

I'm using clang for all my projects.

  • what do we get from the IADM as include paths, defines and arguments?

Is there an easy way to query it?

  • can you show a minimal test file and the errors reported by clang?

it was litterally plagued with errors such as:

Problem in Semantic analysis
Use of undeclared identifier 'FlatpakNotifier'; did you mean 'std::FlatpakNotifier'?

Hint: 'std::FlatpakNotifier' declared here

I decided to clean my .cache/kdevduchain and now it works properly, so maybe it's a matter of bumping the version. *shrug*

plugins/clang/libclang_include_path.h.cmake
23

Okay, doesn't make a lot of sense from my ignorant perspective. :P

It sounds to me like we should be telling about the system we are targetting. In fact, I'm surprised that sysroot isn't necessary.

To query what include files you got, the easiest really is to define the KDEV_CLANG_DISPLAY_ARGS=1 env var. Then you'll see the command line we use for libclang, which includes the include paths. Defines could be queried by setting KDEV_CLANG_DISPLAY_DEFINES=1.

plugins/clang/libclang_include_path.h.cmake
23

I think your understanding of how it should be is the exact opposite of what's actually required :D

While parsing with libclang, we have to use the builtin headers from clang that exactly match the version of libclang we are using. With builtin headers, I mean e.g. /usr/lib/clang/6.0.0/include/xmmintrin.h - this is what we have to use when we parse with libclang 6.0.0. And this is independent of what compiler we actually use for the project we parse, this really is just related to the libclang version we use for parsing.

The reason is that these headers use builtins that are highly compiler and version specific. Trying to use GCCs builtins, or even those from a newer clang will fail in horrible ways (cf. all the bug reports we got).

This also explains why we don't need to specify the sysroot.

apol accepted this revision.Apr 22 2018, 9:53 AM

Fair enough.
It's working for me anyway, I'd say land this, we land my other patch, then we see how we proceed. Makes sense?

This revision is now accepted and ready to land.Apr 22 2018, 9:53 AM
This revision was automatically updated to reflect the committed changes.