[libunwind] Try to add -unwindlib=none while configuring and building libunwind

[mstorsjo]

Summary

If Clang is set up to link directly against libunwind (via the
-unwindlib option, or the corresponding builtin default option),
configuring libunwind will fail while bootstrapping (before the
initial libunwind is built), because every cmake test will
fail due to -lunwind not being found, and linking the shared library
will fail similarly.

Prior to 5f9be2c3e37c0428ba56876dd84af04b8d9d8915, the build used
-nodefaultlibs, which skipped both potential C++ standard libraries
and the unwind library, but since that version, -nostdlib++ is
preferred, which omits the C++ standard library, but not any potential
unwind libraries.

Check if -unwindlib=none is supported, and add it in that case.
Using check_c_compiler_flag on its own doesn't work, because that only
adds the tested flag to the compilation command, and if -lunwind is
missing, the linking step would still fail - instead try adding it
to CMAKE_REQUIRED_FLAGS and restore the variable if it doesn't work.

[mstorsjo]

Given how messy this actually turns out, I'm wondering if we should consider using only -nodefaultlibs in libunwind after all, as @hvdijk suggested originally. What does @phosek think?

[mstorsjo]

In D112126#3074610, @mstorsjo wrote:

Given how messy this actually turns out, I'm wondering if we should consider using only -nodefaultlibs in libunwind after all, as @hvdijk suggested originally. What does @phosek think?

Actually, removing -nostdlib++ and just relying on -nodefaultlibs doesn't help either, the same situation regarding check_c_compiler_flag() arises there too - one has to first try to add the flag to CMAKE_REQUIRED_FLAGS to see if it has the desired effect, otherwise all cmake tests fail.

[hvdijk]

With this patch (on top of 13.0.0), I am getting a successful build, but the start of the CMake output makes me doubt whether the result is what is intended:

-- The C compiler identification is Clang 13.0.0
-- The CXX compiler identification is Clang 13.0.0
-- The ASM compiler identification is Clang
-- Found assembler: /usr/bin/clang
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - failed
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - failed
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring for standalone build.
-- Linker detection: LLD
-- Performing Test LIBUNWIND_SUPPORTS_TARGET_EQ_X86_64_UNKNOWN_LINUX_GNU_FLAG
-- Performing Test LIBUNWIND_SUPPORTS_TARGET_EQ_X86_64_UNKNOWN_LINUX_GNU_FLAG - Failed
-- Looking for fopen in c
-- Looking for fopen in c - not found
-- Performing Test LIBUNWIND_SUPPORTS_UNWINDLIB_NONE_FLAG
-- Performing Test LIBUNWIND_SUPPORTS_UNWINDLIB_NONE_FLAG - Success
[...]

Note that tests were run *before* -unwindlib=none got added, which return incorrect results. Is it possible to check for -unwindlib=none as the very first thing, even before ABI detection, or does CMake not support that?

[mstorsjo]

In D112126#3081756, @hvdijk wrote:

With this patch (on top of 13.0.0), I am getting a successful build, but the start of the CMake output makes me doubt whether the result is what is intended:

-- The C compiler identification is Clang 13.0.0
-- The CXX compiler identification is Clang 13.0.0
-- The ASM compiler identification is Clang
-- Found assembler: /usr/bin/clang
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - failed
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - failed
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring for standalone build.
-- Linker detection: LLD
-- Performing Test LIBUNWIND_SUPPORTS_TARGET_EQ_X86_64_UNKNOWN_LINUX_GNU_FLAG
-- Performing Test LIBUNWIND_SUPPORTS_TARGET_EQ_X86_64_UNKNOWN_LINUX_GNU_FLAG - Failed
-- Looking for fopen in c
-- Looking for fopen in c - not found
-- Performing Test LIBUNWIND_SUPPORTS_UNWINDLIB_NONE_FLAG
-- Performing Test LIBUNWIND_SUPPORTS_UNWINDLIB_NONE_FLAG - Success
[...]

Note that tests were run *before* -unwindlib=none got added, which return incorrect results. Is it possible to check for -unwindlib=none as the very first thing, even before ABI detection, or does CMake not support that?

I don’t think it supports that, unfortunately… FWIW, I’ve run builds for a couple years now with that configuration, and haven’t run into any issue due to that so far. Also, if you have a look at llvm-project/llvm/runtimes/CMakeLists.txt, it also sets -DCMAKE_C_COMPILER_WORKS=ON in a couple of places, for the same reason.

It should be possible to move this before the LIBUNWIND_SUPPORTS_TARGET_EQ_X86_64_UNKNOWN_LINUX_GNU_FLAG check though.

[hvdijk]

In D112126#3081764, @mstorsjo wrote:

I don’t think it supports that, unfortunately… FWIW, I’ve run builds for a couple years now with that configuration, and haven’t run into any issue due to that so far.

I see that the failing check for fopen in libc results in a differently linked libunwind.so, which may cause issues with build reproducibility.

It should be possible to move this before the LIBUNWIND_SUPPORTS_TARGET_EQ_X86_64_UNKNOWN_LINUX_GNU_FLAG check though.

That would be great, thanks, that would also move it before the libc check.

[ychen]

Hit by this issue recently. Not sure it is a better idea but this may also work: create an empty static unwinder lib if Clang is configurated to link unwinder lib by default, and -L to it unconditionally here?

[ychen]

In D112126#3081938, @ychen wrote:

Hit by this issue recently. Not sure it is a better idea but this may also work: create an empty static unwinder lib if Clang is configurated to link unwinder lib by default, and -L to it unconditionally here?

I guess this still could not fix the CMake test failures and CMAKE_C_COMPILER_WORKS is still needed? Oh well...

[mstorsjo]

Moved the tests so that the test for fopen/libc succeeds, and temporarily setting CMAKE_TRY_COMPILE_TARGET_TYPE=STATIC_LIBRARY so that the initial test for --target=<triple> also can succeed.

[hvdijk]

Thanks, LGTM. I have tested the new patch on top of 13.0.0 and confirmed that on my system, with this version of the patch the resulting initial libunwind build is bitwise identical to what I get by building unpatched libunwind with -unwindlib=none forced in through LDFLAGS.

[mstorsjo]

Further small adjustments:

• Prefer --unwindlib instead of -unwindlib. The latter gets silently accepted by GCC (but meaning something entirely different, although harmless), while --unwindlib gets rejected by GCC (as intended by the tests).
• Set CMAKE_TRY_COMPILE_TARGET_TYPE= STATIC_LIBRARY temporarily during the initial compiler sanity checks. This makes sure the checks succeed and lets CMake do the initial ABI detections (for things like CMAKE_SIZEOF_VOID_P etc), and avoids having to set CMAKE_*_COMPILER_WORKS. (The same can be done later for libcxxabi/libcxx too.)

[ldionne]

Honestly, this feels a bit messy to me. At some point I had a patch to set CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC for all runtimes, unconditionally. We ended up not doing it over some concerns about the correctness of the change, however perhaps we should re-consider that?

Also TBH the more I look at it, the more I think that check_c_compiler_flag and the similar utilities provided by CMake out of the box don't work that well for our use case, since we run without a toolchain. Maybe we should consider moving away from those in the runtimes.

[mstorsjo]

In D112126#3084606, @ldionne wrote:

Honestly, this feels a bit messy to me.

I don't deny that, but it's also not entirely possible to make it very simple (given the tools available in cmake) for the bare-bootstrap case when nothing is present to begin with.

At some point I had a patch to set CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC for all runtimes, unconditionally. We ended up not doing it over some concerns about the correctness of the change, however perhaps we should re-consider that?

That's not possible, unfortunately. We have checks like check_library_exists(c fopen "" LIBUNWIND_HAS_C_LIB) - if you set CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC, then this becomes a false positive, succeeding on every platform, everywhere. So because we _do_ want to do linker checks, but in a controlled manner, I'm afraid we do need to do things in a "messy" to detect things in the right order.

Also TBH the more I look at it, the more I think that check_c_compiler_flag and the similar utilities provided by CMake out of the box don't work that well for our use case, since we run without a toolchain. Maybe we should consider moving away from those in the runtimes.

That's indeed possible - here we could wrap e.g. this

set(CMAKE_REQUIRED_FLAGS_ORIG "${CMAKE_REQUIRED_FLAGS}")
set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} --unwindlib=none")
check_c_compiler_flag("" LIBUNWIND_SUPPORTS_UNWINDLIB_NONE_FLAG)
if (NOT LIBUNWIND_SUPPORTS_UNWINDLIB_NONE_FLAG)
  set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS_ORIG}")
endif()

into a macro - which might be beneficial for some of the other checks (-nostdlib++ and -nodefaultlibs too) in some scenarios.

[hvdijk]

In D112126#3084606, @ldionne wrote:

Honestly, this feels a bit messy to me. At some point I had a patch to set CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC for all runtimes, unconditionally. We ended up not doing it over some concerns about the correctness of the change, however perhaps we should re-consider that?

@mstorsjo wrote earlier that that approach causes tests that rely on linking to pass when they should not (checking whether libraries exist), and I fear that may be an unavoidable problem since the sole purpose of CMAKE_TRY_COMPILE_TARGET_TYPE=STATIC is to avoid having CMake run the linker at configure time.

Also TBH the more I look at it, the more I think that check_c_compiler_flag and the similar utilities provided by CMake out of the box don't work that well for our use case, since we run without a toolchain. Maybe we should consider moving away from those in the runtimes.

compiler-rt builtins & crt do not need linker tests so CMAKE_TRY_COMPILE_TARGET_TYPE=STATIC is fine, and builtins already sets that. crt doesn't, and will need the same handling as builtins, but that should be possible without any issues. With CMAKE_TRY_COMPILE_TARGET_TYPE=STATIC set, check_c_compiler_flag does work.

libcxx and libcxxabi are odd in that they are built without a fully functional C++ compiler, but with a fully functional C compiler, and the C compiler is used to the relevant tests, the C++ compiler is simply not checked by CMake so everything works out. (I have not checked how that is done, I only see from my build logs that it never makes any mention of clang++.)

libunwind is the only special one, it needs to run linker tests before we have a fully functional C compiler, so having special tricks in there to make the C compiler *almost* fully functional, functional enough to pass the remaining CMake tests, should be good enough, I think.

[phosek]

In D112126#3084664, @mstorsjo wrote:

In D112126#3084606, @ldionne wrote:

Also TBH the more I look at it, the more I think that check_c_compiler_flag and the similar utilities provided by CMake out of the box don't work that well for our use case, since we run without a toolchain. Maybe we should consider moving away from those in the runtimes.

That's indeed possible - here we could wrap e.g. this

set(CMAKE_REQUIRED_FLAGS_ORIG "${CMAKE_REQUIRED_FLAGS}")
set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} --unwindlib=none")
check_c_compiler_flag("" LIBUNWIND_SUPPORTS_UNWINDLIB_NONE_FLAG)
if (NOT LIBUNWIND_SUPPORTS_UNWINDLIB_NONE_FLAG)
  set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS_ORIG}")
endif()

into a macro - which might be beneficial for some of the other checks (-nostdlib++ and -nodefaultlibs too) in some scenarios.

That's the strategy I've been considering as well.

libunwind/cmake/config-ix.cmake

20–25

You could also use cmake_push_check_state and cmake_pop_check_state to save and restore the state instead of separate variable, but either way is fine.

[mstorsjo]

libunwind/cmake/config-ix.cmake

20–25

Hmm, thanks - that's maybe nicer - although one ends up duplicating the flag that is added). But if abstracted into a macro/function that doesn't matter.

It's a shame that cmake_push_check_state() doesn't handle CMAKE_TRY_COMPILE_TARGET_TYPE though, because that one is scattered a bit more and would also benefit from a cleaner push/pop.

[mstorsjo]

Factorized the tricky testing of --unwindlib=none to a separate cmake function, use this for the other similar-ish linker flags too.

I tried to mimic the naming and behaviour of the check_c_compiler_flag function, but should it be named more distinctly away from conflicting with possible future cmake core functions? And we could have the function append the tested flag directly to CMAKE_REQUIRED_FLAGS if succeeded too, instead of letting the caller do it.

[compnerd]

@mstorsjo I think that it might be interesting to consider a wrapper for enable_languages and to split that out from project.

[mstorsjo]

In D112126#3088270, @compnerd wrote:

@mstorsjo I think that it might be interesting to consider a wrapper for enable_languages and to split that out from project.

Hmm, so project with LANGUAGES NONE? Maybe… but https://cmake.org/cmake/help/latest/command/enable_language.html says “This command must be called in file scope, not in a function call.” But maybe macro scope would work?

[mstorsjo]

Extract a macro for doing enable_languages() while temporarily setting CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC_LIBRARY, simplifying the main cmake file.

Both of the newly created cmake files could probably be moved to the new cmake subdirectory in the monorepo root, for neater sharing across runtimes.

[mstorsjo]

@phosek - Do you have further ideas on this one, or is this as tolerable as it gets? (I have a follow up later for doing similar things in the runtimes directory.)

[phosek]

libunwind/CMakeLists.txt

28 ↗

(On Diff #382947)

When reading through CMake modules, I noticed that some CMake platform modules set _CMAKE_FEATURE_DETECTION_TARGET_TYPE (see https://github.com/Kitware/CMake/blob/master/Modules/Platform/iOS-Initialize.cmake#L9), this is then used by C and C++ compiler test, see https://github.com/Kitware/CMake/blob/4e84a4763d702590fb06d62540e35a614dcd5133/Modules/CMakeTestCCompiler.cmake#L17 and https://github.com/Kitware/CMake/blob/4e84a4763d702590fb06d62540e35a614dcd5133/Modules/CMakeTestCXXCompiler.cmake#L17.

I wonder if we can use _CMAKE_FEATURE_DETECTION_TARGET_TYPE here as well instead of wrapping enable_language.

libunwind/cmake/Modules/CheckCLinkerFlag.cmake

4 ↗

(On Diff #382947)

The same function already exists in compiler-rt, see https://github.com/llvm/llvm-project/blob/2d3953499c8ca73c12e9417f5c4516c8a930a689/compiler-rt/cmake/config-ix.cmake#L9

I'd prefer choosing a name that's unlikely to conflict, for example libunwind_check_linker_flag or llvm_check_linker_flag. The latter might be preferable if we decide to move this file to a shared location later.

libunwind/cmake/Modules/EnableLanguageNolink.cmake

6 ↗

(On Diff #382947)

This is just a nit, but I'd prefer using a name that starts with __ to make it clear that this is an internal variable. CMake uses __CMAKE_SAVED_TRY_COMPILE_TARGET_TYPE in its own macros which might be a good choice.

9 ↗

(On Diff #382947)

Can you also do unset(CMAKE_TRY_COMPILE_TARGET_TYPE_ORIG) to avoid potential errors if someone else tries to use this variable elsewhere?

[mstorsjo]

libunwind/CMakeLists.txt

28 ↗

(On Diff #382947)

I guess we might be able to, but I guess _CMAKE_FEATURE_DETECTION_TARGET_TYPE is undocumented and shouldn't be used outside of cmake's own modules?

libunwind/cmake/Modules/CheckCLinkerFlag.cmake

4 ↗

(On Diff #382947)

Oh, nice.

Hmm, I see that the new shared files in llvm-project/cmake seems to have stuck around without being reverted for a while now, so I guess I could add the new ones directly there, with llvm_ prefixes, as I'd need to use them in llvm-project/runtimes directly in the next patch anyway.

libunwind/cmake/Modules/EnableLanguageNolink.cmake

6 ↗

(On Diff #382947)

Sounds good, especially as this is a macro. I'm wondering if it'd be better to use a different name to avoid potential clashes with the internal variable though...

9 ↗

(On Diff #382947)

Yes, that's a good idea.

[mstorsjo]

Moved the newly added cmake modules (which may be needed for the runtimes directory later) to the toplevel llvm-project/cmake dir, added llvm_ prefixes for those cmake functions/macros, added unset() and an underscore prefix for the internal variable in the llvm_enable_language_nolink macro.

[phosek]

LGTM

libunwind/CMakeLists.txt

28 ↗

(On Diff #382947)

I've filed https://gitlab.kitware.com/cmake/cmake/-/issues/22859, we'll see what CMake maintainers think.

libunwind/cmake/config-ix.cmake

if (NOT LIBUNWIND_USE_COMPILER_RT)
   if (ANDROID)
     check_library_exists(gcc __gcc_personality_v0 "" LIBUNWIND_HAS_GCC_LIB)
   else ()
     check_library_exists(gcc_s __gcc_personality_v0 "" LIBUNWIND_HAS_GCC_S_LIB)
     check_library_exists(gcc __absvdi2 "" LIBUNWIND_HAS_GCC_LIB)
   endif ()
 endif()
 
+# The compiler driver may be implicitly trying to link against libunwind,
+# which obviously might not work when building libunwind itself. Try to
+# check if -unwindlib=none is supported, and use that if possible.
+#
+# However, a plain check_c_compiler_flag(-unwindlib=none) would also fail, as
+# the tested flag is only added to the compilation command (letting the
+# compiler error out if it is unsupported), but not included in the linking
+# command (which still fails due to implicitly trying to link against the
+# possibly missing libunwind). Therefore, we must add it directly to
+# CMAKE_REQUIRED_FLAGS (which is included both when compiling and linking),
+# then do a dummy test with check_c_compiler_flag, and revert the change to
+# CMAKE_REQUIRED_FLAGS if it didn't work out.
+set(CMAKE_REQUIRED_FLAGS_ORIG "${CMAKE_REQUIRED_FLAGS}")
+set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -unwindlib=none")
+check_c_compiler_flag("" LIBUNWIND_SUPPORTS_UNWINDLIB_NONE_FLAG)
+if (NOT LIBUNWIND_SUPPORTS_UNWINDLIB_NONE_FLAG)
+  set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS_ORIG}")
+endif()
+
 # libunwind is using -nostdlib++ at the link step when available,
 # otherwise -nodefaultlibs is used. We want all our checks to also
 # use one of these options, otherwise we may end up with an inconsistency between
 # the flags we think we require during configuration (if the checks are
 # performed without one of those options) and the flags that are actually
 # required during compilation (which has the -nostdlib++ or -nodefaultlibs). libc is
 # required for the link to go through. We remove sanitizers from the
 # configuration checks to avoid spurious link errors.

libunwind/src/CMakeLists.txt

 endif()
 add_library_flags_if(LIBUNWIND_HAS_DL_LIB dl)
 if (LIBUNWIND_ENABLE_THREADS)
   add_library_flags_if(LIBUNWIND_HAS_PTHREAD_LIB pthread)
   add_compile_flags_if(LIBUNWIND_WEAK_PTHREAD_LIB -DLIBUNWIND_USE_WEAK_PTHREAD=1)
 endif()
 
 # Setup flags.
+add_link_flags_if(LIBUNWIND_SUPPORTS_UNWINDLIB_NONE_FLAG -unwindlib=none)
 if (LIBUNWIND_SUPPORTS_NOSTDLIBXX_FLAG)
   add_link_flags_if_supported(-nostdlib++)
 else()
   add_link_flags_if_supported(-nodefaultlibs)
 endif()
 
 # MINGW_LIBRARIES is defined in config-ix.cmake
 add_library_flags_if(MINGW "${MINGW_LIBRARIES}")