This is an archive of the discontinued LLVM Phabricator instance.

Enable `-funwind-tables` flag when building libunwind
ClosedPublic

Authored by broadwaylamb on Nov 28 2019, 5:09 AM.

Details

Summary

Or, rather, don't accidentally forget to pass it.

This is aimed to solve the problem discussed in this thread, and to fix a year old bug.

TL;DR: when building libunwind for ARM Linux, we need libunwind to be built with the -funwind-tables flag, because, well, ARM EHABI needs unwind info produced by this flag. Without the flag all procedures in libunwind are marked .cantunwind, which causes all sorts of bad things. From _Unwind_Backtrace not working, to C++ exceptions not being caught (which the aforementioned bug is about).

Previously, this flag was not added because the CMake check add_compile_flags_if_supported(-funwind-tables) produced a false negative. Why? With this flag, the compiler generates calls to the __aeabi_unwind_cpp_pr0 symbol, which is defined in libunwind itself and obviously is not available at configure time, before libunwind is built. This led to failure at link time during the CMake check. We handle this by disabling the linker for CMake checks in linbunwind.

Also, this patch introduces a lit feature libunwind-arm-ehabi, which is used to mark the signal_frame.pass.cpp test as unsupported (as was advised by @miyuki in D70397).

Diff Detail

Event Timeline

broadwaylamb created this revision.Nov 28 2019, 5:09 AM
broadwaylamb edited the summary of this revision. (Show Details)Nov 28 2019, 5:30 AM

I've put some comments inline. I'm no CMake expert though, so there may be other reviewers that can make alternative suggestions.

libunwind/CMakeLists.txt
311

Assuming we can't set CMAKE_TRY_COMPILE_TARGET_TYPE is to do something like:

if (LIBUNWIND_USES_ARM_EHABI)
  // unconditionally add -funwind-tables

Anyone trying to compile libunwind without a compiler that supports -funwind-tables gets an error message at build rather than configure time, but is there anyone that wouldn't use a form of Clang or GCC?

broadwaylamb added inline comments.Nov 28 2019, 5:46 AM
libunwind/CMakeLists.txt
311

My opinion is that we want to fail as early as possible. Also, we can show a dedicated error message if we're failing at configure time, rather than leaving the user on their own with the obscure 'unsupported flag' message (why do I need this flag, the user may ask then).

I too would like to avoid the CMAKE_TRY_COMPILE_TARGET_TYPE dance, but I'd like to hear more opinions on this from the people that understand things better than me.

miyuki added inline comments.Nov 28 2019, 7:57 AM
libunwind/test/signal_frame.pass.cpp
24–25

#if can be removed

Remove unnecessary conditional compilation in signal_frame.pass.cpp

broadwaylamb marked an inline comment as done.Nov 28 2019, 10:00 AM
phosek added inline comments.Nov 28 2019, 2:25 PM
libunwind/CMakeLists.txt
236

This option is only available since CMake 3.6, but the minimum requirement for LLVM is 3.4. There's a separate discussion on llvm-dev about bumping that version, but until that happens, this should be behind a condition to avoid breaking users and bots that use CMake 3.4 and 3.5.

smeenai added inline comments.
libunwind/CMakeLists.txt
236

Won't it just end up being a no-op on older CMake version?

I believe compiler-rt has some custom machinery set up to run compile-only configuration checks. Idk if that's worth trying to replicate here though if we'll be upgrading CMake versions soon anyway.

broadwaylamb marked an inline comment as done.Nov 28 2019, 11:37 PM
broadwaylamb added inline comments.
libunwind/CMakeLists.txt
236

Won't it just end up being a no-op on older CMake version?

Agreed, we're just setting a variable here, nothing should go wrong with older versions.

compnerd accepted this revision.Nov 29 2019, 10:41 AM
compnerd added inline comments.
libunwind/CMakeLists.txt
312

I tend to prefer SEND_ERROR to help flush out any other issues.

This revision is now accepted and ready to land.Nov 29 2019, 10:41 AM
broadwaylamb marked an inline comment as done.Nov 29 2019, 11:14 AM
broadwaylamb added inline comments.
libunwind/CMakeLists.txt
236

@compnerd I'm curious what do you think about this.

Use message(SEND_ERROR "...") instead of message(FATAL_ERROR "...")

broadwaylamb marked an inline comment as done.Nov 30 2019, 3:57 AM
saugustine accepted this revision.Dec 2 2019, 10:12 AM

I'm not a cmake expert and have very little sense of the mechanics here, but this change is necessary and at least looks correct to me, so accepting.

broadwaylamb added a comment.EditedDec 3 2019, 12:41 PM

Since no one seems to object, I'm gonna commit this. If it breaks anything, I'll revert as soon as possible.

This revision was automatically updated to reflect the committed changes.

I went ahead and reverted this, as it broke how some cmake tests within libunwind work.

As this disables linking, it falsely makes any test succeed that depends on linking, and there's a few such tests in cmake/config-ix.cmake (check_library_exists(c fopen "" LIBUNWIND_HAS_C_LIB), check_library_exists(dl dladdr "" LIBUNWIND_HAS_DL_LIB), etc).

Therefore, set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY) has to be carefully placed at a spot where all such tests that does require linking already has been completed.

raahlb added a subscriber: raahlb.Jun 23 2020, 12:58 AM

I went ahead and reverted this, as it broke how some cmake tests within libunwind work.

As this disables linking, it falsely makes any test succeed that depends on linking, and there's a few such tests in cmake/config-ix.cmake (check_library_exists(c fopen "" LIBUNWIND_HAS_C_LIB), check_library_exists(dl dladdr "" LIBUNWIND_HAS_DL_LIB), etc).

Therefore, set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY) has to be carefully placed at a spot where all such tests that does require linking already has been completed.

This was fixed in a later commit, correct?

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 23 2020, 12:58 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I went ahead and reverted this, as it broke how some cmake tests within libunwind work.

As this disables linking, it falsely makes any test succeed that depends on linking, and there's a few such tests in cmake/config-ix.cmake (check_library_exists(c fopen "" LIBUNWIND_HAS_C_LIB), check_library_exists(dl dladdr "" LIBUNWIND_HAS_DL_LIB), etc).

Therefore, set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY) has to be carefully placed at a spot where all such tests that does require linking already has been completed.

This was fixed in a later commit, correct?

Yep: https://reviews.llvm.org/D71117