Differential D15895

Use CLANG_INCLUDE_DIRS for clang include dir
ClosedPublic
Actions

Authored by buschinski on Oct 2 2018, 1:37 PM.

Details

Reviewers
kfunk
Group Reviewers
KDevelop
Commits
R32:716372ae2e8d: Use CLANG_INCLUDE_DIRS for clang include dir
Summary

Use CLANG_INCLUDE_DIRS for clang include dir instead of manually building it.

This fixes the error:
kdevplatform.shell: Could not load plugin "kdevclangsupport" , it reported the error: "The clang builtin include path \"/usr/lib64/llvm/7/lib64/clang/7.0.0/include\" is invalid (missing cpuid.h header).\nTry setting the KDEV_CLANG_BUILTIN_DIR environment variable manually to fix this.\nSee also: https://bugs.kde.org/show_bug.cgi?id=393779" Disabling the plugin now.

As CMake with CLANG_INCLUDE_DIRS already tests if this folder is present, it should work across all distros.

CCBUG: 393779

Diff Detail

Repository
R32 KDevelop
Lint
Lint Skipped
Unit
Unit Tests Skipped
buschinski created this revision.Oct 2 2018, 1:37 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptOct 2 2018, 1:37 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
buschinski requested review of this revision.Oct 2 2018, 1:37 PM
buschinski edited the summary of this revision. (Show Details)Oct 2 2018, 1:40 PM
arrowd added a subscriber: arrowd.Oct 2 2018, 2:03 PM

Does it work with clang 6.0?

kfunk added a subscriber: kfunk.Oct 2 2018, 2:16 PM

And what does CLANG_INCLUDE_DIRS resolve to on Gentoo?

kfunk requested changes to this revision.Oct 2 2018, 2:22 PM

Note: This is likely the wrong fix to this problem.

On Ubuntu ...

  • CLANG_INCLUDE_DIRS yields "/usr/lib/llvm-7/include" which contains the clang/ and llvm/ subdirs. This is not the dir we want. No cpuid.h header in here either.
  • @CLANG_LIBRARY_DIRS@/clang/@CLANG_VERSION@/include yields "/usr/lib/llvm-7/lib/clang/7.0.0/include". Okay. Contains the cpuid.h header.
This revision now requires changes to proceed.Oct 2 2018, 2:22 PM
kossebau added a subscriber: kossebau.Oct 2 2018, 2:35 PM

And on openSUSE TW for me:

  • CLANG_INCLUDE_DIRS: /usr/include (no cpuid.h below that)
  • @CLANG_LIBRARY_DIRS@/clang/@CLANG_VERSION@/include: /usr/lib64/clang/6.0.1/include (has cpuid.h in that very dir)
buschinski updated this revision to Diff 42730.Oct 2 2018, 3:17 PM

Sorry, you are right. I forgot the most important part of the patch. Sorry for the noise :(

buschinski added a comment.EditedOct 3 2018, 2:01 PM

"cpuid.h" on my system is in (listing ALL locations):

  • /usr/lib64/clang/7.0.0/include/cpuid.h
  • /usr/lib64/gcc/x86_64-pc-linux-gnu/7.3.0/include/cpuid.h
  • /usr/lib64/gcc/x86_64-pc-linux-gnu/8.2.0/include/cpuid.h
  • and a few in /usr/src/linux-* (linux kernel sources)

And
$ llvm-config --includedir
/usr/lib64/llvm/7/include
$ llvm-config --libdir
/usr/lib64/llvm/7/lib64

buschinski updated this revision to Diff 42794.Oct 3 2018, 2:25 PM

Added additional search PATHS with CLANG_LIBRARY_DIRS, like it was used in the original version (without this patch). This should hopefully work on opensuse as well.

brauch added a subscriber: brauch.Oct 3 2018, 5:12 PM

On Arch Linux, the latest version of the patch seems to work fine.

kossebau added a comment.Oct 3 2018, 6:26 PM

I can tell the updated patch results does not change behaviour on openSUSE TW,,same value for KDEV_CLANG_BUILTIN_DIR before and after (/usr/lib64/clang/6.0.1/include).

Having no clue about clang, I wonder though about the mixing of LLVM and CLANG in the variable names. When would/should LLVM be used, when CLANG?

cmake/modules/FindClang.cmake
95

I would like some comment why "cpuid.h" and some cross-linking between ClangSupport constructor and this.
Unless cpuid.h is a header well known for such compiler/system includes testing, and I yet have to learn much more :)

aaronpuchert added a subscriber: aaronpuchert.Oct 3 2018, 9:32 PM
In D15895#335917, @buschinski wrote:

"cpuid.h" on my system is in:

  • /usr/lib64/clang/7.0.0/include/cpuid.h

$ llvm-config --libdir
/usr/lib64/llvm/7/lib64

It seems that on your system, LLVM is installed into the prefix /usr/lib64/llvm/7, but Clang is installed into /usr. Is that correct? Where are the Clang include headers, /usr/lib64/llvm-7/include/clang or /usr/include/clang?

On @kfunk's Ubuntu the prefix is /usr/lib/llvm-7/ for both, and on openSUSE it's /usr for both. I think the assumption behind this code was that both are installed in the same prefix and we can just use llvm-config. Apparently that is not always the case.

Having no clue about clang, I wonder though about the mixing of LLVM and CLANG in the variable names. When would/should LLVM be used, when CLANG?

Indeed, that's a bit confusing. Technically Clang is a subproject of LLVM, and both have their own include directories. On my system, these are /usr/include/{llvm,clang}{,-c}. They are required for writing applications that link with libLLVM or libclang. But then there are also the headers used at runtime, and that is what you see as /usr/lib64/clang/<version>/include.

I'd probably call the latter CLANG_RUNTIME_INCLUDE_DIR or something like that.

kfunk requested changes to this revision.Oct 4 2018, 6:57 AM
kfunk added a subscriber: mwolff.

But I think in general that approach makes more sense. Thanks for working on it.

Comments from @mwolff would be appreciated, too, since he worked on that feature to begin with.

cmake/modules/FindClang.cmake
92

IIUC then CLANG_INCLUDE_DIRS shouldn't be touched. CLANG_INCLUDE_DIRS is for finding the correct include dirs for building KDevClangSupport against libclang -- we don't need to have things like compiler builtin includes in there.

Rather, leave CLANG_INCLUDE_DIRS as it is and do find_path(CLANG_BUILTIN_DIR ...), in other words: create another variable.

@buschinski Just to double-check: /usr/lib64/llvm/7/include has a clang subfolder on Gentoo, right?

104

Typo: "Unabled"

This revision now requires changes to proceed.Oct 4 2018, 6:57 AM
arrowd added a comment.Oct 4 2018, 9:24 AM

FreeBSD+Clang 6 also works fine with this patch.

buschinski added inline comments.Oct 4 2018, 1:26 PM
cmake/modules/FindClang.cmake
92

Yes, there is a /usr/lib64/llvm/7/include/clang, but it does not contain cpuid.h or such headers.

And just to be clear, the original version looked for
"/usr/lib64/llvm/7/lib64/clang/7.0.0/include", which does not exist.

$ llvm-config --libdir
/usr/lib64/llvm/7/lib64
the double "lib64" sounds wrong, but it is correct, exists and does also contains lots of libclang*.

I will change it to CLANG_BUILTIN_DIR and revert the CLANG_INCLUDE_DIRS changes.

95

"cpuid.h" is common (as far as I know) for compiler (gcc/clang), but not very clang specific. I will add a comment regarding ClangSupport constructor

104

rephrased to match other error messages to:
"Could not find Clang builtin directory"

buschinski updated this revision to Diff 42857.Oct 4 2018, 1:27 PM

Used PATH_SUFFIXES to cleanup the find_path a bit more

kfunk accepted this revision.Oct 4 2018, 1:55 PM

Rest LGTM, feel free to push after fixing my remark.

cmake/modules/FindClang.cmake
103

Needs NO_DEFAULT_PATH otherwise include dirs from e.g. /usr/include may take precedence.

This is important if you (for instance) have a self-built version of LLVM/Clang in $HOME, and a distro-provided version in /usr, with the exact same version.

Your patch as-is returns:

/usr/include/clang/6.0.1/include

... while it should return this for me:

/home/kfunk/devel/build/llvm/lib/clang/6.0.1/include

Adding NO_DEFAULT_PATH fixes the issue for me.

This revision is now accepted and ready to land.Oct 4 2018, 1:55 PM
buschinski closed this revision.Oct 4 2018, 6:36 PM

Revision Contents
Changeset List

Diff 42857

View Options

cmake/modules/FindClang.cmake

Loading...
View Options

plugins/clang/libclang_include_path.h.cmake

Loading...