Remember LLVM_ENABLE_LIBCXX setting in installed configuration

[Meinersbur]

Updated version of #134990 which was reverted because of the buildbot openmp-offload-amdgpu-runtime-2 failing. Its configuration has LLVM_ENABLE_LIBCXX=ON set although it does not even have libc++ installed. In addition to #134990, this PR adds a check whether C++ libraries are actually available. #include <chrono> was chosen because this is the library that openmp-offload-amdgpu-runtime-2 failed with.

Original summary:

The buidbot flang-aarch64-libcxx is currently failing with an ABI issue. The suspected reason is that LLVMSupport.a is built using libc++, but the unittests are using the default C++ standard library, libstdc++ in this case. This predefined llvm_gtest target uses the LLVMSupport from find_package(LLVM), which finds the libc++-built LLVMSupport.

To fix, store the LLVM_ENABLE_LIBCXX setting in the LLVMConfig.cmake such that everything that links to LLVM libraries use the same standard library. In this case discussed in llvm/llvm-zorg#387 it was the flang-rt unittests, but other runtimes with GTest unittests should have the same issue (e.g. offload), and any external project that uses find_package(LLVM). This patch fixed the problem for me locally.

llvm_gtest defined here:

llvm-project/third-party/unittest/CMakeLists.txt

Line 51 in 74f69c4

add_llvm_library(llvm_gtest

Used by flang-rt unittests here:

llvm-project/flang-rt/unittests/CMakeLists.txt

Line 20 in 74f69c4

add_subdirectory("${LLVM_THIRD_PARTY_DIR}/unittest" "${CMAKE_CURRENT_BINARY_DIR}/third-party/unittest")

Used by offload here:

llvm-project/offload/CMakeLists.txt

Line 391 in 74f69c4

add_subdirectory(${LLVM_THIRD_PARTY_DIR}/unittest ${CMAKE_CURRENT_BINARY_DIR}/third-party/unittest)

Use by libc here (but I don't see where it is added):

llvm-project/libc/benchmarks/CMakeLists.txt

Line 42 in 74f69c4

llvm_gtest

[DavidTruby]

This doesn't fix the issue for me, on the same system that #139569 does. My cmake line is:

CC=clang CXX=clang++ cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_LIBCXX=On -DLLVM_ENABLE_PROJECTS="flang;clang" -DLLVM_ENABLE_RUNTIMES=openmp -DLLVM_TARGETS_TO_BUILD=host -DLLVM_CCACHE_BUILD=On -DLLVM_ENABLE_LLD=On ../llvm

and then ninja && ninja check-flang-rt fails with the same linker errors. I definitely have libc++ installed, but the tests in check-flang-rt are not adding -stdlib=libc++.

[DavidTruby]

CC=clang CXX=clang++ cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_LIBCXX=On -DLLVM_ENABLE_PROJECTS="flang;clang" -DLLVM_ENABLE_RUNTIMES=openmp -DLLVM_TARGETS_TO_BUILD=host -DLLVM_CCACHE_BUILD=On -DLLVM_ENABLE_LLD=On ../llvm

Note: It makes no difference if I add libcxx;libcxxabi to LLVM_ENABLE_RUNTIMES, although I believe it should work either way (and just use my system libc++ if there's not one in the LLVM build).

[Meinersbur]

Why doesn't it add -stdlib=libc++? What is the CMake output for CXX_COMPILER_SUPPORTS_STDLIB_CHRONO? What does the CMakeConfigureLog.yaml log say? Does re-applying #134990 work for you? Where is libc++ installed?

[pawosm-arm]

@Meinersbur we're trying to build it with this script: https://github.com/arm/arm-toolchain/blob/arm-software/arm-software/linux/build.sh, it used to work for months before problem started a week ago. After my failed attempt to fix the building issue with applying this PR, @DavidTruby has pointed me at some issue with this scipt: building libc++ in a separate step is not a recommended way, it should be built at all steps and the separate step should be dropped. I'll try that later.

EDIT: I don't like the proposed idea. The bootstrap compiler is being dropped after the final compiler is ready. If we start building libc++ at that step, the final compiler will be linked against the (shared) libs that will be gone later.

EDIT: another proposed idea was to use the -stdlib++-isystem ... CXX flag (instead of -I...), I'll try that.

[DavidTruby]

My issue here is separate to the script; I'm just trying to get a simple case working outside of the script.
I don't appear to get any output for CXX_COMPILER_SUPPORTS_STDLIB_CHRONO. All I do see is:

CMake Warning at .../llvm-project/llvm/cmake/modules/HandleLLVMStdlib.cmake:33 (message):
  Can't specify libc++ with '-stdlib='
Call Stack (most recent call first):
  .../llvm-project/llvm/cmake/modules/HandleLLVMOptions.cmake:11 (include)
  CMakeLists.txt:175 (include)

EDIT:
in CMakeConfigureLog.yaml, CXX_COMPILER_SUPPORTS_STDLIB, CXX_LINKER_SUPPORTS_STDLIB and CXX_COMPILER_SUPPORTS_STDLIB_CHRONO are all true for the main build. But CXX_COMPILER_SUPPORTS_STDLIB_CHRONO is not true for the runtimes build.
bin/clang++ -stdlib=libc++ -c test.cpp works fine for me from the build directory, where test.cpp only has #include <chrono>

[DavidTruby]

Looking at the output of CXX_COMPILER_SUPPORTS_STDLIB_CHRONO in the runtimes folder:
the check passes -nostdlibc++ -nostdinc++. This will mean the test always returns false, as it's telling clang not to even look for C++ headers. So it will never find <chrono> regardless if it's there or not.

[DavidTruby]

Regardless I'm not convinced this check is correct anyway. If there's a mismatch between the requested C++ runtime and the one actually being used, we should error out with a cmake message, not just allow it to continue and silently do the wrong thing.

[DavidTruby]

Even with the comment I've added, which does make this work, this seems to cause a lot of extra warnings in the other runtimes builds for me. It looks like -stdlib=libc++ is being passed there too, and is then giving the not used at compile time warning because -nostdinc++ is also being passed.

I believe we only want to add -stdlib=libc++ for runtimes that don't pass -nostdinc++.

[DavidTruby]

When this is processed in the runtimes cmake sub-command, CMAKE_REQUIRED_FLAGS contains -nostdinc++. You could fix this with:

string(REPLACE "-nostdinc++" " " CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS}")

Which makes the PR work for me.

[DavidTruby]

Just saw your updated PR; I think we do need to still inherit CMAKE_REQUIRED_FLAGS, for cases where the path to the c++ lib is being added as a flag etc. We just need to specifically remove -nostdinc++.

[DavidTruby]

Ah, never mind, that's not how CMAKE_REQUIRED_FLAGS works :). It will still be populated with CMAKE_CXX_FLAGS here. I think what you have now is fine.

[DavidTruby]

This works for me now, except for the extra warnings during the compiler-rt build. Could we change the append("-stdlib=libc++ ...) to something like set(LLVM_ENABLE_LIBCXX_SUPPORTED ON CACHE BOOL "")
and then use that in the runtimes that remove -nostdinc++ (which might only be flang-rt?) to add the flag?

[Meinersbur]

Your #139569 will fail some buildbots for the same reasons that #134990 was reverted for and would need the same code to check whether -stdlib=libc++ is actually working. It also passes LLVM_ENABLE_LIBCXX to compiler-rt in a bootstrapping builds and therefore will cause the very same warnings you don't like about this PR. IMHO it is an issue with compiler-rt, the same warning you would get if building compiler-rt in a standalone build with LLVM_ENABLE_LIBCXX=ON. In #139569 you were still arguing that

most likely what the user wanted when passing -DLLVM_ENABLE_LIBCXX".

The -nostdinc++ in CMAKE_REQUIRED_FLAGS issue is already the second time I fell over it. See #139771.

Whether LLVM_ENABLE_LIBCXX=ON should fail if libc++ is unavailable is not my decision; the current implementation just emits a warning. That could be changed and is what I would also prefer, but it will also cause many additional build failures, including of some buildbots that add LLVM_ENABLE_LIBCXX=ON without having libc++ available on the system.

If you like I can withdraw this PR and we iterate in #139569.

[DavidTruby]

I think this is probably better than what I was doing in #139569 so we should stick with this.
I'm not sure what your reference to my comment about what the user probably wanted is meant to be attached to; is it in reference to the next line about nostdinc++ or the previous line about compiler-rt? I'm not really sure how it relates to either.

All I'm saying about compiler-rt is that because this patch unconditionally adds -stdlib=libc++ you get loads of warnings, where you didn't before, because compiler-rt doesn't link to a C++ library so the flag ends up unsed. Maybe it would be easier to have compiler-rt strip that flag, in which case it'd fix it for a standalone compiler-rt build too?

[DavidTruby]

Just to clarify, I'm happy with this PR now except that it introduces a bunch of warnings to my builds that weren't there before. This was true of my attempt as well, for what it's worth, I just hadn't clocked it at the time.

If you'd rather that gets fixed on the compiler-rt side I can post a fix to compiler-rt to strip -stdlib=libc++ in there so the warnings go away. It might also be necessary for the other runtimes though. Probably all of them except flang-rt?