Enable `-funwind-tables` flag when building libunwind

[broadwaylamb]

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).

[peter.smith]

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]

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]

libunwind/test/signal_frame.pass.cpp

24–25

#if can be removed

[broadwaylamb]

Remove unnecessary conditional compilation in signal_frame.pass.cpp

[phosek]

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]

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]

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]

libunwind/CMakeLists.txt

312

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

[broadwaylamb]

libunwind/CMakeLists.txt

236

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

[broadwaylamb]

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

[saugustine]

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]

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

[mstorsjo]

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]

In D70815#1768359, @mstorsjo wrote:

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?

[broadwaylamb]

In D70815#2108406, @raahlb wrote:

In D70815#1768359, @mstorsjo wrote:

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

libunwind/CMakeLists.txt

 
 # Include macros for adding and removing libunwind flags.
 include(HandleLibunwindFlags)
 
 #===============================================================================
 # Setup Compiler Flags
 #===============================================================================
 
+# Don't run the linker in CMake checks.
+#
+# The reason why this was added is that when building libunwind for
+# ARM Linux, we need to pass the -funwind-tables flag in order for it to
+# work properly with ARM EHABI.
+#
+# However, when performing CMake checks, adding this flag causes the check
+# to produce a false negative, because the compiler generates calls
+# to __aeabi_unwind_cpp_pr0, which is defined in libunwind itself,
+# which isn't built yet, so the linker complains about undefined symbols.
+#
+# This leads to libunwind not being built with this flag, which makes
+# libunwind quite useless in this setup.
+set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)
+
 # Get required flags.
 add_target_flags_if(LIBUNWIND_BUILD_32_BITS "-m32")
 
 if(LIBUNWIND_TARGET_TRIPLE)
   add_target_flags("--target=${LIBUNWIND_TARGET_TRIPLE}")
 elseif(CMAKE_CXX_COMPILER_TARGET)
   set(LIBUNWIND_TARGET_TRIPLE "${CMAKE_CXX_COMPILER_TARGET}")
 endif()

 # Get feature flags.
 # Exceptions
 # Catches C++ exceptions only and tells the compiler to assume that extern C
 # functions never throw a C++ exception.
 add_cxx_compile_flags_if_supported(-fstrict-aliasing)
 add_cxx_compile_flags_if_supported(-EHsc)
 
 add_compile_flags_if_supported(-funwind-tables)
+
+if (LIBUNWIND_USES_ARM_EHABI AND NOT LIBUNWIND_SUPPORTS_FUNWIND_TABLES_FLAG)
+  message(SEND_ERROR "The -funwind-tables flag must be supported "
+                     "because this target uses ARM Exception Handling ABI")
+endif()
+
 add_cxx_compile_flags_if_supported(-fno-exceptions)
 add_cxx_compile_flags_if_supported(-fno-rtti)
 
 # Ensure that we don't depend on C++ standard library.
 if (LIBUNWIND_HAS_NOSTDINCXX_FLAG)
   list(APPEND LIBUNWIND_COMPILE_FLAGS -nostdinc++)
   # Remove -stdlib flags to prevent them from causing an unused flag warning.
   string(REPLACE "-stdlib=libc++" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")

libunwind/cmake/config-ix.cmake

 include(CMakePushCheckState)
 include(CheckCCompilerFlag)
 include(CheckCXXCompilerFlag)
 include(CheckLibraryExists)
+include(CheckSymbolExists)
 include(CheckCSourceCompiles)
 
 check_library_exists(c fopen "" LIBUNWIND_HAS_C_LIB)
 
 if (NOT LIBUNWIND_USE_COMPILER_RT)
   check_library_exists(gcc_s __gcc_personality_v0 "" LIBUNWIND_HAS_GCC_S_LIB)
   check_library_exists(gcc __absvdi2 "" LIBUNWIND_HAS_GCC_LIB)
 endif()

 endif()
 
 # Check compiler flags
 check_cxx_compiler_flag(-nostdinc++ LIBUNWIND_HAS_NOSTDINCXX_FLAG)
 
 # Check libraries
 check_library_exists(dl dladdr "" LIBUNWIND_HAS_DL_LIB)
 check_library_exists(pthread pthread_once "" LIBUNWIND_HAS_PTHREAD_LIB)
+
+# Check symbols
+check_symbol_exists(__arm__ "" LIBUNWIND_TARGET_ARM)
+check_symbol_exists(__USING_SJLJ_EXCEPTIONS__ "" LIBUNWIND_USES_SJLJ_EXCEPTIONS)
+check_symbol_exists(__ARM_DWARF_EH__ "" LIBUNWIND_USES_DWARF_EH)
+
+if(LIBUNWIND_TARGET_ARM AND NOT LIBUNWIND_USES_SJLJ_EXCEPTIONS AND NOT LIBUNWIND_USES_DWARF_EH)
+  # This condition is copied from __libunwind_config.h
+  set(LIBUNWIND_USES_ARM_EHABI ON)
+endif()

libunwind/test/CMakeLists.txt

if (NOT DEFINED LIBCXX_ENABLE_SHARED)
   set(LIBCXX_ENABLE_SHARED ON)
 endif()
 
 pythonize_bool(LIBUNWIND_BUILD_32_BITS)
 pythonize_bool(LIBCXX_ENABLE_SHARED)
 pythonize_bool(LIBUNWIND_ENABLE_SHARED)
 pythonize_bool(LIBUNWIND_ENABLE_THREADS)
 pythonize_bool(LIBUNWIND_ENABLE_EXCEPTIONS)
+pythonize_bool(LIBUNWIND_USES_ARM_EHABI)
 pythonize_bool(LIBUNWIND_USE_COMPILER_RT)
 pythonize_bool(LIBUNWIND_BUILD_EXTERNAL_THREAD_LIBRARY)
 set(LIBUNWIND_TARGET_INFO "libcxx.test.target_info.LocalTI" CACHE STRING
     "TargetInfo to use when setting up test environment.")
 set(LIBUNWIND_EXECUTOR "None" CACHE STRING
     "Executor to use when running tests.")
 
 set(AUTO_GEN_COMMENT "## Autogenerated by libunwind configuration.\n# Do not edit!")

libunwind/test/libunwind/test/config.py

class Configuration(LibcxxConfiguration):
 
     def has_cpp_feature(self, feature, required_value):
         return int(self.cxx.dumpMacros().get('__cpp_' + feature, 0)) >= required_value
 
     def configure_features(self):
         super(Configuration, self).configure_features()
         if not self.get_lit_bool('enable_exceptions', True):
             self.config.available_features.add('libcxxabi-no-exceptions')
+        if self.get_lit_bool('arm_ehabi', False):
+            self.config.available_features.add('libunwind-arm-ehabi')
 
     def configure_compile_flags(self):
         self.cxx.compile_flags += ['-DLIBUNWIND_NO_TIMER']
         if not self.get_lit_bool('enable_exceptions', True):
             self.cxx.compile_flags += ['-fno-exceptions', '-DLIBUNWIND_HAS_NO_EXCEPTIONS']
         # Stack unwinding tests need unwinding tables and these are not
         # generated by default on all Targets.
         self.cxx.compile_flags += ['-funwind-tables']
def configure_compile_flags_header_includes(self):
                                   % libunwind_headers)
         self.cxx.compile_flags += ['-I' + libunwind_headers]
 
     def configure_compile_flags_exceptions(self):
         pass
 
     def configure_compile_flags_rtti(self):
         pass
+
+    def configure_link_flags_cxx_library(self):
+        # libunwind tests should not link with libc++
+        pass
+
+    def configure_link_flags_abi_library(self):
+        # libunwind tests should not link with libc++abi
+        pass

libunwind/test/lit.site.cfg.in

 config.enable_32bit             = @LIBUNWIND_BUILD_32_BITS@
 config.target_info              = "@LIBUNWIND_TARGET_INFO@"
 config.test_linker_flags        = "@LIBUNWIND_TEST_LINKER_FLAGS@"
 config.test_compiler_flags      = "@LIBUNWIND_TEST_COMPILER_FLAGS@"
 config.executor                 = "@LIBUNWIND_EXECUTOR@"
 config.libunwind_shared         = @LIBUNWIND_ENABLE_SHARED@
 config.enable_shared            = @LIBCXX_ENABLE_SHARED@
 config.enable_exceptions        = @LIBUNWIND_ENABLE_EXCEPTIONS@
+config.arm_ehabi                = @LIBUNWIND_USES_ARM_EHABI@
 config.host_triple              = "@LLVM_HOST_TRIPLE@"
 config.target_triple            = "@TARGET_TRIPLE@"
 config.use_target               = bool("@LIBUNWIND_TARGET_TRIPLE@")
 config.sysroot                  = "@LIBUNWIND_SYSROOT@"
 config.gcc_toolchain            = "@LIBUNWIND_GCC_TOOLCHAIN@"
 config.cxx_ext_threads          = @LIBUNWIND_BUILD_EXTERNAL_THREAD_LIBRARY@
 
 # Let the main config do the real work.
 lit_config.load_config(config, "@LIBUNWIND_SOURCE_DIR@/test/lit.cfg")

libunwind/test/signal_frame.pass.cpp

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // Ensure that functions marked as signal frames are reported as such.
 
+// UNSUPPORTED: libunwind-arm-ehabi
+
 #include <assert.h>
 #include <stdlib.h>
 #include <libunwind.h>
 
 void test() {
   asm(".cfi_signal_frame");
   unw_cursor_t cursor;
   unw_context_t uc;
   unw_getcontext(&uc);
   unw_init_local(&cursor, &uc);
   assert(unw_step(&cursor) > 0);
-#if !defined(_LIBUNWIND_ARM_EHABI)
   assert(unw_is_signal_frame(&cursor));
-#endif
 }
 
 int main() {
   test();
   return 0;
 }