Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CMake] [compiler-rt] fix architecture checks in runtime builds #66374

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sebastianpoeplau
Copy link
Contributor

When Clang is built with libunwind as the default unwinder, the architecture support checks in compiler-rt fail during the runtime build because Clang can't link before libunwind is built. This commit fixes the checks by making them respect CMAKE_REQUIRED_FLAGS, a variable that contains linker arguments to disable the unwinder if necessary.

When Clang is built with libunwind as the default unwinder, the
architecture support checks in compiler-rt fail during the runtime build
because Clang can't link before libunwind is built. This commit fixes
the checks by making them respect CMAKE_REQUIRED_FLAGS, a variable that
contains linker arguments to disable the unwinder if necessary.
@llvm-beanz
Copy link
Collaborator

This is effectively the same problem that the test_compile_only macro was added to solve. There are a bunch of cases in compiler-rt where you can only verify that the compile works not the compile+link.

Can you share more about your configuration that’s leading you to this failure?

CMake has changed and evolved a lot since I wrote a lot of the hacky mess that drives this all, so some of these decisions should probably be revisited.

set(SAVED_CMAKE_EXE_LINKER_FLAGS ${CMAKE_EXE_LINKER_FLAGS})
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${argstring}")
try_compile(CAN_TARGET_${arch} ${CMAKE_BINARY_DIR} ${SIMPLE_SOURCE}
COMPILE_DEFINITIONS "${TARGET_${arch}_CFLAGS} ${FLAG_NO_EXCEPTIONS}"
Copy link
Member

@petrhosek petrhosek Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is already quite fragile so I'd prefer minimizing the changes. Could we just pass ${CMAKE_REQUIRED_FLAGS} here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I tried, mirroring the code in CMake's implementation of check_cxx_source_compiles, which passes the flags as compile definitions. There was some sort of problem, but I can't remember now what it was 🤔

@petrhosek
Copy link
Member

Have you tried using COMPILER_RT_DEFAULT_TARGET_ONLY? The test_target_arch macro is used in test_targets which has a number of issues and ideally we should try to remove it altogether.

@sebastianpoeplau
Copy link
Contributor Author

sebastianpoeplau commented Nov 9, 2023

@llvm-beanz

This is effectively the same problem that the test_compile_only macro was added to solve. There are a bunch of cases in compiler-rt where you can only verify that the compile works not the compile+link.

There is actually code in crt-config-ix.cmake and builtin-config-ix.cmake that sets test_compile_only to true, but if I remember correctly this happens too late in the runtimes build to have an effect (see below for details on my setup).

Can you share more about your configuration that’s leading you to this failure?

Sure. We build Clang and compiler-rt+libunwind via LLVM_ENABLE_RUNTIMES on x86_64 Linux. The toolchain is supposed to be independent of any host GCC installation and therefore configured to use compiler-rt and libunwind by default (via CLANG_DEFAULT_RTLIB/UNWINDLIB), which implies that Clang isn't fully functional yet by the time we build the runtimes. There is code to handle this case in compiler-rt/CMakeLists.txt runtimes/CMakeLists.txt, but I still had the problem that the crt objects weren't built even with explicit COMPILER_RT_BUILD_CRT=ON. It turned out that this was due to the target check failing to link, hence this PR.

CMake has changed and evolved a lot since I wrote a lot of the hacky mess that drives this all, so some of these decisions should probably be revisited.

I'm just a regular CMake user, not an expert 😢 What are the recent changes that would be useful in this context?

@petrhosek

Have you tried using COMPILER_RT_DEFAULT_TARGET_ONLY?

I think I shied away from it because we wanted to build the 32-bit runtimes as well. Is there a good way to achieve this with COMPILER_RT_DEFAULT_TARGET_ONLY, apart from two separate builds?

I should also say that we're hitting the limits of LLVM_ENABLE_RUNTIMES in some other contexts, especially when it comes to bare-metal targets, where we'd like to build an embedded libc between the compiler and the runtimes. So we're considering separate builds anyway, which would also make it easier to build with COMPILER_RT_DEFAULT_TARGET_ONLY.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants