Using Clang 10 specifically

[bruntib]

In case multiple LLVM/Clang versions are installed on a machine it is
not determined which one is used. In this commit LLVM 7 is enforced
because cpp plugin compiles with this version.

[mcserep]

This change breaks the CI pipeline on Ubuntu 16.04, where LLVM 7 is installed from the official LLVM repo. (Since the version in the Ubuntu package repository is without RTTI.) Can you take a look at it?

[bruntib]

Yes, I've seen the error. This is a little strange to me because in .travis config file we enforce Clang-7 everywhere. And CodeCompass can't even compile with Clang-6 so this is good that we have Clang-7 in the CI. But I'll check why Travis fails.

[mcserep]

I have pushed a minor modification (see commit aeab84a) to dump the LLVM related information BEFORE locating the Clang package. This uncovered the fact that with the modification of this PR, an incorrect LLVM instance in found.
(The commit can be reverted when this issue is resolved.)

The source of the issue is complex. I will summarize how these Travis CI containers are configured regarding the LLVM/Clang tooling:

• The containers have LLVM/Clang preinstalled, because they are defined with the, cpp language image. While we could also start with a minimal or generic image, this is the "Travis-way" for a mainly C++ project and we can also skip the installation of a couple of dependencies. However, while the preinstalled LLVM/Clang is version 7 (both for Xenial and Bionic), they are compiled without RTTI, so they are not good for CodeCompass. It is installed under /usr/local/ (binaries which are checked in the CI script: /usr/local/clang-7.0.0/bin/clang, /usr/local/clang-7.0.0/bin/llvm-config). This is version 7.0.0.
• For Xenial, LLVM and Clang is installed from the official LLVM repository (llvm-toolchain-xenial-7) with the APT package manager. This is installed under /usr/ (binaries which are checked in the CI script: /usr/bin/clang-7, /usr/bin/llvm-config-7). This is version 7.1.0.
• For Bionic the same is done, but from the official Ubuntu repository, and version 7.0.0 is installed.
• The /usr/local/ path takes precedence when locating LLVM/Clang installations over the /usr/ path, as usual, for that reason cmake is executed for CodeCompass with additional configuration in Travis:
  cmake [...] -DLLVM_DIR=/usr/lib/llvm-7/cmake -DClang_DIR:PATH=/usr/lib/cmake/clang-7

Now, requiring LLVM to be version 7 in the CMakeLists.txt means that version 7.1.0 does not match this requirement. (Which is a surprise for me at least.) So the manual LLVM_DIR configuration is omitted under Xenial and the instance in /usr/local/ is used instead (compiled without RTTI), resulting in this error. (And since for Bionic LLVM with RTTI is also version 7.0.0, there is no error.)

[filbeofITK]

Looking at this I found the root of the problem the same, unfortunately there is a bug in the cmake code which does not accept 7.1 as version 7. Cmake will even give a message that says version 7.1.0.0 was considered but not accepted. (I tested this behavior in an Ubuntu Xenial derived docker image.)

This is a link to the cmake dev forum. They know about this issue, but it's not solved even in the newest version (I tested it).

[mcserep]

I have updated this PR with master, because our Travis configuration has changed since and now we depend on the generic image instead of the cpp language image.

I believed that this would solve the issue of having multiple LLVM/Clang versions on Xenial (7.0.0 without RTTI and 7.1.0 with RTTI) and CMake selecting the incorrect one. My belief was based on the above linked documentation of Travis, which states that Clang is preinstalled (without RTTI) on the cpp image and not on the generic image.

Well, that is not the case, the documentation of Travis is incorrect. So the CI still fails unfortunately for this.

[whisperity]

Superseded by #432. Wrt. the CI, the history shows #342 has added specifying the CMake config paths explicitly even for Clang 7. LLVM 10.0 now also comes with RTTI enabled from the package manager.

[mcserep]

Minimal LLVM/Clang version (now 10) is still not configured in the appropriate CMakeLists.txt file, maybe we can try to update this PR.

[whisperity]

How will this handle 10.0.1 or even 11? Are we sure that there aren't any breaking changes between 10 and 11? (Most likely there won't be between 10 and 10.0.1, but from 10 to 11 there could be...) LLVM 11 isn't released yet (AFAIK it shall be in a few weeks).

Is there a way CMake could require exact version, and not minimal one?

If the user follows the guide in specifying -DLLVM_DIR and -DClang_DIR, the right versions will be found. Now these directories are made from apt specifically as something that contains -10 in their name.

The two breaking changes from 7 to 10 that we had to tackle happened between different major version releases, IIRC.

[mcserep]

How will this handle 10.0.1 or even 11? Are we sure that there aren't any breaking changes between 10 and 11? (Most likely there won't be between 10 and 10.0.1, but from 10 to 11 there could be...) LLVM 11 isn't released yet (AFAIK it shall be in a few weeks).

According to the documentation the developers of LLVM/Clang have to properly set up compatibility between versions and then CMake will decide if the requested version is compatible with the found one.
See: https://cmake.org/cmake/help/latest/command/find_package.html#version-selection

Is there a way CMake could require exact version, and not minimal one?

There is an EXACT flag for find_package(), but I am not sure that requiring an exact version would be a good idea.

[mcserep]

CI passes with these modifications.

[whisperity]

CI passes with these modifications.

@mcserep Note that the CI script has -DLLVM_DIR and -DClang_DIR hardcoded to be somethingsomething-10... So even though find_package validates that the version in /usr/.../llvm-10 is at least version 10, it does not change which LLVM is loaded. (Or if it is even checked whether multiple LLVMs are installed.)

---

According to the documentation the developers of LLVM/Clang have to properly set up compatibility between versions and then CMake will decide if the requested version is compatible with the found one.
See: https://cmake.org/cmake/help/latest/command/find_package.html#version-selection

The only thing that Clang's found CMake file does, both for version 12 (current master) and version 7 and version 10, etc, is this.

For the package manager version of Clang 7.0 on Ubuntu 20.04:

# This file allows users to call find_package(Clang) and pick up our targets.


# Compute the installation prefix from this LLVMConfig.cmake file location.
get_filename_component(CLANG_INSTALL_PREFIX "${CMAKE_CURRENT_LIST_FILE}" REALPATH)
get_filename_component(CLANG_INSTALL_PREFIX "${CLANG_INSTALL_PREFIX}" PATH)
get_filename_component(CLANG_INSTALL_PREFIX "${CLANG_INSTALL_PREFIX}" PATH)
get_filename_component(CLANG_INSTALL_PREFIX "${CLANG_INSTALL_PREFIX}" PATH)
get_filename_component(CLANG_INSTALL_PREFIX "${CLANG_INSTALL_PREFIX}" PATH)

find_package(LLVM REQUIRED CONFIG
             HINTS "${CLANG_INSTALL_PREFIX}/lib/cmake/llvm")

I guess those get_filename_component calls are basically the equivalent of doing ../../../.., so /usr/lib/llvm-7/lib/cmake/clang/ClangConfig.cmake will become /usr/lib/llvm-7 and then the LLVM file is loaded from /usr/lib/llvm-7/lib/cmake/llvm which seems appropriate as this directory contains a lot of .cmake files, one of which being LLVMConfig.cmake, containing:

set(LLVM_VERSION_MAJOR 7)
set(LLVM_VERSION_MINOR 0)
set(LLVM_VERSION_PATCH 1)
set(LLVM_PACKAGE_VERSION 7.0.1)

For a local compilation of master(-ish) on my computer, the file is a bit easier, as the local CMake most likely hardcoded the full build path, as seen below:

find_package(LLVM REQUIRED CONFIG
             HINTS "/home/w/Work/LLVM/Build/lib/cmake/llvm")

So there isn't any connection between the two, in terms of versions.

---

I need a bit more information for understanding this. What will happen, if someone somehow has installed LLVM version X, and Clang version Y. Namely, if X > 10 it will pass the requirements, however, could it be that find_package(Clang ... (without the version tag, as the patch stands currently) will find an old Clang Y < 10, and thus there will be some breaking?

I have done the following in Docker:

• Go through full install of CodeCompass, but do llvm-10-dev clang-7 libclang-7-dev instead of clang-10 libclang-10-dev.
• As expected, this will install llvm-7 llvm-7-dev. But weirdly enough, this also results in installing only libclang-cpp10 in addition to a bunch of clang-7 packages like libclang1-7 libclang-common-7-dev. No "clang-10" or "libclang-10-dev" is installed just because we've installed llvm-10.
• Due to llvm-7 being installed, we have most of the LLVM-7 tools and configuration available.
• The rest of the configuration continues as normal (GTest, git clone, etc...)

While I understand this particular environment isn't up to the documentation, this shows the discrepancy.

CodeCompass's CMake happily eats the broken environment, giving me no errors, if I omit the -DLLVM_DIR -DClang_DIR from the cmake call.

Loading plugin cpp
-- Found LLVM 10.0.0
-- Using LLVMConfig.cmake in: /usr/lib/llvm-10/cmake
-- Using RTTI for LLVM: ON
-- Using ClangConfig.cmake in: /usr/lib/cmake/clang-7

But of course, doing the build fails, with incredibly weird errors. Weird because this output has zero indication that we are dealing with a version discrepancy. How could an include chain of only llvm-7 files be so broken that LLVM can't use its own generics correctly?!

root@507ea7a0ddc9:/usr/src/CodeCompass/Build# make cppparser -j1
[ 28%] Built target model
[ 84%] Built target cppmodel
[ 88%] Building CXX object plugins/cpp/parser/CMakeFiles/cppparser.dir/src/cppparser.cpp.o
In file included from /usr/lib/llvm-7/include/clang/AST/Type.h:22,
                 from /usr/lib/llvm-7/include/clang/AST/Decl.h:23,
                 from /usr/lib/llvm-7/include/clang/AST/ASTTypeTraits.h:20,
                 from /usr/lib/llvm-7/include/clang/AST/ASTContext.h:18,
                 from /usr/lib/llvm-7/include/clang/Frontend/ASTUnit.h:18,
                 from /usr/lib/llvm-7/include/clang/Frontend/FrontendAction.h:24,
                 from /usr/src/CodeCompass/plugins/cpp/parser/src/cppparser.cpp:16:
/usr/lib/llvm-7/include/clang/AST/TemplateName.h:180:13: error: 'PointerUnion4' in namespace 'llvm' does not name a template type; did you mean 'PointerUnion'?
  180 |       llvm::PointerUnion4<TemplateDecl *, UncommonTemplateNameStorage *,
      |             ^~~~~~~~~~~~~
      |             PointerUnion

---

All in all, I believe it is not a good idea to pose restrictions on requiring a particular LLVM version, while we let the user loose with "any" Clang version. Au contraire, perhaps we should first require the proper version of Clang, considering the dependency is that we (CodeCompass) depend on Clang mainly (and not "on LLVM" per se), which in turn depends (as seen, on the package manager and CMake level!) on the right "version" (not expressed as version tags but rather the install directory of the right version of LLVM baked into the config file) of LLVM.

[whisperity]

Another funny thing: due to having both LLVM 7 and 10 installed in the CI, another discrepancy is present: the .travis.yml calls llvm-config as an audit line, which results in PATH picking up Clang 7.0 and LLVM 7.0 (see https://travis-ci.com/github/Ericsson/CodeCompass/jobs/372731716#L3449 from the latest run of this patch).

[mcserep]

CI passes with these modifications.

@mcserep Note that the CI script has -DLLVM_DIR and -DClang_DIR hardcoded to be somethingsomething-10... So even though find_package validates that the version in /usr/.../llvm-10 is at least version 10, it does not change which LLVM is loaded. (Or if it is even checked whether multiple LLVMs are installed.)

It was only left there, because previously there was two LLVM 7 versions in the Travis containers, so the precise path was required to be defined. This should be removable now, and we can also specify the Clang version in the CMakeLists.txt file.

I have done that in 8720f8b, but unfortunately it seems that the CMake support for Clang is incomplete, it does not provide version information:

/usr/lib/cmake/clang-10/ClangConfig.cmake, version: unknown

So ultimately yes: we cannot specify the Clang version, because Clang's CMake support is deficient. Therefore we either:

• Specify the LLVM version, but not the Clang version (at least until they fix this bug). In that case if someone has multiple Clang version installed, specifying Clang_DIR will be required.
• Close this PR without merging it. In this case both LLVM_DIR and Clang_DIR must be specified if multiple LLVM/Clang versions are installed.

[whisperity]

Bl... blimey.

Yeah, this is a bummer, thank you for checking this out. I'm not terribly opposed to this patch, just want to make sure we don't do anything that gives users a false sense of security if it results in hard to grasp errors.

For the sake of clarity, could you add two lines to the CMakeList? Basically, just make sure that during configure time, we at least give the user a message that uses llvm-config --version and clang --version. Just put these two string out there, in addition to the current message( calls that dump CMake variables. I think both Clang's and LLVM's CMake set up some variable like LLVM_BINDIR or something that you can do ${LLVM_BINDIR}/llvm-config* --version as a shell command (* needed because nowadays the version is concatenated into the binary's name...). Just so we can see what the found binaries think about themselves.

So in case of us getting a bug report about compilation issues we can be explicit about "Hey, you definitely have the wrong version installed, see Clang said it's 7.0.1!".

Although if we want this to be extra pedantic, a preprocessor magic could also work, Clang defines the CLANG_VERSION_MAJOR macro:

$ cat /usr/lib/llvm-10/include/clang/Basic/Version.inc
───────┬───────────────────────────────────────────────────────────────────────────────────────
       │ File: Version.inc
───────┼───────────────────────────────────────────────────────────────────────────────────────
   1   │ #define CLANG_VERSION 10.0.0
   2   │ #define CLANG_VERSION_STRING "10.0.0"
   3   │ #define CLANG_VERSION_MAJOR 10
   4   │ #define CLANG_VERSION_MINOR 0
   5   │ #define CLANG_VERSION_PATCHLEVEL 0
───────┴───────────────────────────────────────────────────────────────────────────────────────

(This file should trickle down into every header whatsoever, but if not, including clang/Basic/Version.h should give access.)

I vaguely remember something about CMake's find_package( preferring newer versions, so this shouldn't pose a big issue unless the users are keen on putting together surgically broken environments.

[mcserep]

Unfortunately while there is a LLVM_BINARY_DIR variable (but even better to get the version: LLVM_PACKAGE_VERSION), there are no such things for Clang. The variables Clang_VERSION_MAJOR and others exist, but all of them the value 0.

So there is really no way to get the Clang version in CMake. Note, that even if the binary folder would be known, we still wouldn't know the binary's name! E.g. on Bionic it is clang-10 and not simply clang, when installed from the default Ubuntu APT repository.

I think this is the best we can do for now:

# Find LLVM
find_package(LLVM REQUIRED CONFIG)
message(STATUS "Found LLVM ${LLVM_PACKAGE_VERSION}")
message(STATUS "Using LLVMConfig.cmake in: ${LLVM_DIR}")

# Check whether RTTI is on for LLVM
message(STATUS "Using RTTI for LLVM: ${LLVM_ENABLE_RTTI}")
if(NOT LLVM_ENABLE_RTTI)
    message(SEND_ERROR "RTTI is required for LLVM")
endif()

# Check LLVM version
if (${LLVM_PACKAGE_VERSION} VERSION_LESS "10")
    message(SEND_ERROR "Loaded LLVM version must be at least 10.0, ${LLVM_PACKAGE_VERSION} found.")
endif()

# Add LLVM to the library path
mark_as_run_env_path(LD_LIBRARY_PATH "${LLVM_BUILD_LIBRARY_DIR}")

# Find Clang
find_package(Clang REQUIRED CONFIG)
message(STATUS "Using ClangConfig.cmake in: ${Clang_DIR}")

And then LLVM_DIR and Clang_DIR shall be specified if multiple versions are available on the computer.

[mcserep]

Can we merge this @whisperity , @intjftw ?
I cannot request a review from you @bruntib , because you are the original author, but since I have modified this PR heavily, what is your opinion on this?

[whisperity]

@mcserep Yes, sorry, I realise my previous letter was full of thoughts but also very vague. I think it's okay... I think I'll try getting around to fixing the CMake parts of Clang to support this, surely we aren't the only ones out there who get broken over this.