Page MenuHomePhabricator

[runtimes] Fix building initial libunwind+libcxxabi+libcxx with compiler implied -lunwind
ClosedPublic

Authored by mstorsjo on Nov 5 2021, 2:59 AM.

Details

Summary

This does mostly the same as D112126, but for the runtimes cmake files.
Most of that is straightforward, but the interdependency between
libcxx and libunwind is tricky:

Libunwind is built at the same time as libcxx, but libunwind is not
installed yet. LIBCXXABI_USE_LLVM_UNWINDER makes libcxx link directly
against the just-built libunwind, but the compiler implicit -lunwind
isn't found. This patch avoids that by adding --unwindlib=none if
supported, if we are going to link explicitly against a newly built
unwinder anyway.

(There might be a similar interdependency between libcxxabi and
libunwind too - it doesn't show up in my builds targeting Windows,
as libcxxabi is always built statically though. @hvdijk, can you test
using the new build layout by targeting cmake at llvm-project/runtimes
instead of building libunwind/libcxxabi/libcxx individually in your
environment and see how it behaves?)

This is a mutually exclusive alternative to D113252.

Diff Detail

Event Timeline

mstorsjo created this revision.Nov 5 2021, 2:59 AM
mstorsjo requested review of this revision.Nov 5 2021, 2:59 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 5 2021, 2:59 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript

I prefer this version over D113252 because I'd like to avoid linker picking up libraries like libunwind implicitly as that seems more error prone.

phosek accepted this revision.Nov 5 2021, 10:55 AM

LGTM do you also plan to include --unwindlib=none in compiler-rt build in a follow up change?

I prefer this version over D113252 because I'd like to avoid linker picking up libraries like libunwind implicitly as that seems more error prone.

Yeah, this was my second approach, and I like this one better too.

I'll amend it with the change so it doesn't break builds with ASAN/TSAN.

LGTM do you also plan to include --unwindlib=none in compiler-rt build in a follow up change?

Hmm, for building the full compiler-rt at the same time as libunwind, for picking it up for use for the sanitizers etc? I currently build all of those in a separate invocation so it's not on my todo list right now... I guess that'd be needed to fix the full from-scratch cases with the llvm/runtimes ("bootstrap") build with no preexisting libunwind.

I prefer this version over D113252 because I'd like to avoid linker picking up libraries like libunwind implicitly as that seems more error prone.

Yeah, this was my second approach, and I like this one better too.

I'll amend it with the change so it doesn't break builds with ASAN/TSAN.

LGTM do you also plan to include --unwindlib=none in compiler-rt build in a follow up change?

Hmm, for building the full compiler-rt at the same time as libunwind, for picking it up for use for the sanitizers etc? I currently build all of those in a separate invocation so it's not on my todo list right now... I guess that'd be needed to fix the full from-scratch cases with the llvm/runtimes ("bootstrap") build with no preexisting libunwind.

That's our use case. I can look into that after this change lands.

mstorsjo updated this revision to Diff 385142.Nov 5 2021, 11:52 AM

Avoid adding --unwindlib=none unless necessary, fixing the ASAN/TSAN builds.

Hmm, for building the full compiler-rt at the same time as libunwind, for picking it up for use for the sanitizers etc? I currently build all of those in a separate invocation so it's not on my todo list right now... I guess that'd be needed to fix the full from-scratch cases with the llvm/runtimes ("bootstrap") build with no preexisting libunwind.

That's our use case. I can look into that after this change lands.

Thanks!

Btw, what's your opinion on the compiler options added via HandleLLVMOptions - do they end up in the actual builds of runtimes, or are they just tested but left unused? As when building with --unwindlib=none, all of those fail due to the option causing warnings, so such builds would differ from other builds when the option isn't needed.

Hmm, for building the full compiler-rt at the same time as libunwind, for picking it up for use for the sanitizers etc? I currently build all of those in a separate invocation so it's not on my todo list right now... I guess that'd be needed to fix the full from-scratch cases with the llvm/runtimes ("bootstrap") build with no preexisting libunwind.

That's our use case. I can look into that after this change lands.

Thanks!

Btw, what's your opinion on the compiler options added via HandleLLVMOptions - do they end up in the actual builds of runtimes, or are they just tested but left unused? As when building with --unwindlib=none, all of those fail due to the option causing warnings, so such builds would differ from other builds when the option isn't needed.

From what I know some of the options are used by individual runtimes but not all of them. Furthermore, not all of the options are defined in HandleLLVMOptions.cmake. For example, LLVM_ENABLE_MODULES is defined in llvm/CMakeLists.txt and is used from both HandleLLVMOptions.cmake as well as libcxx/CMakeLists.txt.

I think we should ideally go over all the LLVM options that are used in runtimes and split those into a separate top-level CMake module and then stop including both AddLLVM.cmake and HandleLLVMOptions.cmake from the runtimes build.

Btw, what's your opinion on the compiler options added via HandleLLVMOptions - do they end up in the actual builds of runtimes, or are they just tested but left unused? As when building with --unwindlib=none, all of those fail due to the option causing warnings, so such builds would differ from other builds when the option isn't needed.

From what I know some of the options are used by individual runtimes but not all of them. Furthermore, not all of the options are defined in HandleLLVMOptions.cmake. For example, LLVM_ENABLE_MODULES is defined in llvm/CMakeLists.txt and is used from both HandleLLVMOptions.cmake as well as libcxx/CMakeLists.txt.

I think we should ideally go over all the LLVM options that are used in runtimes and split those into a separate top-level CMake module and then stop including both AddLLVM.cmake and HandleLLVMOptions.cmake from the runtimes build.

Thanks, that sounds like a good strategy.

@mstorsjo Do you plan on landing this change?

@ldionne Can you take a look if this looks good from your perspective?

@mstorsjo Do you plan on landing this change?

Waiting on the blocking libc++ review first…

ldionne accepted this revision.Nov 16 2021, 6:34 AM

LGTM but I have a question for my understanding.

I think we should ideally go over all the LLVM options that are used in runtimes and split those into a separate top-level CMake module and then stop including both AddLLVM.cmake and HandleLLVMOptions.cmake from the runtimes build.

This x 1000.

runtimes/CMakeLists.txt
94–95

I don't understand how this check works. Since you are disabling runtime linking, I would think that check_c_compiler_flag succeeding does NOT imply that runtime linking works?

This revision is now accepted and ready to land.Nov 16 2021, 6:34 AM
mstorsjo added inline comments.Nov 16 2021, 6:44 AM
runtimes/CMakeLists.txt
94–95

We don’t disable linking here - this is cmake’s standard compile+link test. We do one such test, adding no flags, to see if linking works out of the box.

If it succeeds, we have a complete toolchain and don’t need to do anything. If linking doesn’t seem to work, it could be an incomplete toolchain being built - in those cases, check if we could make it work by adding --unwindlib=none.

The reason for this indirection is that if we have a complete toolchain, adding that option can break things - if we’re setting up building with sanitizers (as in CI), adding that option breaks the sanitizers.

@phosek - You may want to do the same thing in libcxxabi as this patch does in libcxx (if linking to a just built libunwind). I don’t have a setup where I build a shared libcxxabi with an incomplete toolchain, so I’m not touching it here. I’d expect it to work essentially in the same way.

mstorsjo updated this revision to Diff 387614.Nov 16 2021, 6:50 AM

Rebased to get a fresh CI run before pushing.

ldionne added inline comments.Nov 16 2021, 7:03 AM
runtimes/CMakeLists.txt
94–95

Don't you disable runtime linking in compiler checks on line 18 above?

include(EnableLanguageNolink)
project(Runtimes LANGUAGES NONE)
llvm_enable_language_nolink(C CXX ASM)
mstorsjo added inline comments.Nov 16 2021, 7:08 AM
runtimes/CMakeLists.txt
94–95

No, that macro just disables linking temporarily while running the basic compiler sanity checks. We do need to have working linker tests for some later things - but it’s quite fiddly when the toolchain is incomplete.

Temporarily disabling linking in the sanity checks so the tests succeed allows cmake to pick up general features about the target (OS etc). And if not disabled, one has to pass -DCMAKE_C_COMPILER_WORKS=YES to waive the checks.

ldionne accepted this revision.Nov 16 2021, 7:23 AM
ldionne added inline comments.
runtimes/CMakeLists.txt
94–95

Ah, I re-read the definition of llvm_enable_language_nolink and it all became very obvious. Sorry for the naive questions, and thanks for explaining. 🚢 it!

mstorsjo updated this revision to Diff 387640.Nov 16 2021, 7:43 AM

Reuploading, with the right version of the patch now (where asan/tsan don't fail).

mstorsjo updated this revision to Diff 387651.Nov 16 2021, 8:14 AM

Restart the CI run again, the previous attempt failed for intermittent reasons.

Hi, this patch is producing cmake errors in our amdgpu buildbot: https://lab.llvm.org/buildbot/#/builders/193. Initially I thought there was something wrong with our configuration, but git bisecting revealed that this commit is causing the issue. If I remove the following line, the build is fine.

project(Runtimes LANGUAGES NONE)

CC: @estewart08 @dpalermo

Hi, this patch is producing cmake errors in our amdgpu buildbot: https://lab.llvm.org/buildbot/#/builders/193. Initially I thought there was something wrong with our configuration, but git bisecting revealed that this commit is causing the issue. If I remove the following line, the build is fine.

project(Runtimes LANGUAGES NONE)

CC: @estewart08 @dpalermo

Ok, sorry about that - I reverted it now, and trying to see how I can reproduce and understand the issue.

If its helpful, following is the cmake command I used (cmake version: 3.20)

cmake ../llvm -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_TARGETS_TO_BUILD="X86;AMDGPU" -DLLVM_ENABLE_ASSERTIONS=On -DLLVM_ENABLE_RUNTIMES=openmp -DCLANG_DEFAULT_LINKER=lld -DLLVM_ENABLE_PROJECTS="lld;clang"

I managed to reproduce the issue. @phosek, can you advise how to proceed from here?

One part of the issue, is that setting CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC_LIBRARY makes the initial detection step detect less things. In particular, if doing full link tests, cmake sets the CMAKE_LIBRARY_ARCHITECTURE variable to x86_64-linux-gnu. This seems to be vital for some parts of library detection. To show off the issue, one can execute this minimal standalone cmake snippet:

set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)
project(Runtimes)
cmake_minimum_required(VERSION 3.10)
find_package(ZLIB)

On Ubuntu, it fails to find zlib, but if the set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY) line is uncommented, it finds it as expected.

When runtimes include parts of the llvm cmake files from the main host build, from find_package(LLVM PATHS "${LLVM_BINARY_DIR}" NO_DEFAULT_PATH NO_CMAKE_FIND_ROOT_PATH), it executes this check which now failed, while it succeeded as part of the host build.

Secondly, in openmp/libomptarget/src/CMakeLists.txt, we have this bit:

if (OPENMP_ENABLE_LIBOMPTARGET_PROFILING)
  # Add LLVMSupport dependency if profiling is enabled.
  # Linking with LLVM component libraries also requires
  # aligning the compile flags.
  llvm_update_compile_flags(omptarget)
  target_compile_definitions(omptarget PUBLIC OMPTARGET_PROFILE_ENABLED)
  target_link_libraries(omptarget PRIVATE LLVMSupport)
endif()

This seems to be enabled in the default case. Thus, the runtime build of libopenmp (which could be targeting any foreign target) tries to link against the host-built LLVMSupport. As LLVMSupport is set to link against zlib, but zlib wasn't found, the build breaks.

I guess the right course of action would be to properly untangle openmp from the main host llvm build. Should OPENMP_ENABLE_LIBOMPTARGET_PROFILING be disabled when built as part of the runtimes build (where openmp might be built for a wildly different target anyway), as one can't expect to link against LLVMSupport in such a setup?

mstorsjo reopened this revision.Nov 17 2021, 4:35 AM
This revision is now accepted and ready to land.Nov 17 2021, 4:35 AM
mstorsjo updated this revision to Diff 392501.Dec 7 2021, 12:31 PM

Rerun CI after landing D114083; will reapply once CI is passing.

Hi, this is again causing issues on amdgpu buildbot (https://lab.llvm.org/buildbot/#/builders/193). In the logs, I see that cmake was unable to find libelf dependency which is necessary for openmp plugins to be built and tested. In absence of libelf, the bot, currently, silently ignores and run non-amdgpu tests. I locally reverted this patch and found that tests are again working fine.

Hi, this is again causing issues on amdgpu buildbot (https://lab.llvm.org/buildbot/#/builders/193). In the logs, I see that cmake was unable to find libelf dependency which is necessary for openmp plugins to be built and tested. In absence of libelf, the bot, currently, silently ignores and run non-amdgpu tests. I locally reverted this patch and found that tests are again working fine.

Sorry about that - I reverted this now again.

I think the best way forward with this is to just skip the changes to project()/enable_languages (as setting CMAKE_TRY_COMPILE_TARGET_TYPE=STATIC_LIBRARY affects what the compiler can detect when the setup is a full working toolchain), and just require callers to set CMAKE_{C,CXX}_COMPILER_WORKS=YES when building with an incomplete toolchain. I'll update the patch in that form, and reapply again after CI completes.

mstorsjo reopened this revision.Dec 9 2021, 4:43 AM
This revision is now accepted and ready to land.Dec 9 2021, 4:43 AM
mstorsjo updated this revision to Diff 393116.Dec 9 2021, 4:54 AM

Removed the parts of the patch that touch project(), not using llvm_enable_language_nolink(). This form shouldn't affect how libraries are detected any longer.

This is less nice for the incomplete toolchain case as this now requires setting CMAKE_{C,CXX}_COMPILER_WORKS=YES while building, but that's not a regression (that has pretty much always been needed anyway) and a tolerable compromise.

I managed to reproduce the issue. @phosek, can you advise how to proceed from here?

One part of the issue, is that setting CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC_LIBRARY makes the initial detection step detect less things. In particular, if doing full link tests, cmake sets the CMAKE_LIBRARY_ARCHITECTURE variable to x86_64-linux-gnu. This seems to be vital for some parts of library detection. To show off the issue, one can execute this minimal standalone cmake snippet:

set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)
project(Runtimes)
cmake_minimum_required(VERSION 3.10)
find_package(ZLIB)

On Ubuntu, it fails to find zlib, but if the set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY) line is uncommented, it finds it as expected.

I don't know the answer but I did some experiments.

This fails:

set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)
project(Runtimes)
set(CMAKE_TRY_COMPILE_TARGET_TYPE EXECUTABLE)
cmake_minimum_required(VERSION 3.10)
find_package(ZLIB)

This succeeds:

project(Runtimes)
cmake_minimum_required(VERSION 3.10)
set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)
find_package(ZLIB)
set(CMAKE_TRY_COMPILE_TARGET_TYPE EXECUTABLE)

So it seems like this is somehow related to the project configuration rather than the find_package implementation.

I'm looking into CMake implementation to see if I can figure what's going on.

This fails:

set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)
project(Runtimes)
set(CMAKE_TRY_COMPILE_TARGET_TYPE EXECUTABLE)
cmake_minimum_required(VERSION 3.10)
find_package(ZLIB)

This succeeds:

project(Runtimes)
cmake_minimum_required(VERSION 3.10)
set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)
find_package(ZLIB)
set(CMAKE_TRY_COMPILE_TARGET_TYPE EXECUTABLE)

So it seems like this is somehow related to the project configuration rather than the find_package implementation.

Yeah - or even more precisely, it's related to enable_languages, as this snippet also should reproduce the same issue based on my experience so far:

project(Runtimes LANGUAGES NONE)
set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)
enable_language(C)
set(CMAKE_TRY_COMPILE_TARGET_TYPE EXECUTABLE)
find_package(ZLIB)

I think the reason is that when probing the compiler, it normally detects things like the target triple and directories that the compiler normally looks in when linking (e.g. /usr/lib/x86_64-linux-gnu) which then are used for finding libraries.

Anyway, I'll go ahead and reland this patch without those bits, to move forward somewhat. I can live without them, even if would be neat if we could avoid needing things like setting CMAKE_C_COMPILER_WORKS=YES.

It looks like the amdgpu buildbot at https://lab.llvm.org/buildbot/#/builders/193 runs (and passes) the same amount of openmp tests now.