Reland "Enable `-funwind-tables` flag when building libunwind"

[broadwaylamb]

Summary

Relands https://reviews.llvm.org/D70815.

The original commit set CMAKE_TRY_COMPILE_TARGET_TYPE to
STATIC_LIBRARY globally in libunwind/CMakeLists.txt, which effectively
disabled the linking step in CMake checks.

This broke some builds (see 938c70b86c7d2165f8c28d5700e9c1ac1263307e).

Here we set CMAKE_TRY_COMPILE_TARGET_TYPE to
STATIC_LIBRARY only when checking for presence of the -funwind-tables
flag, and then set it back to the original value so it doesn't affect
other checks.

[mstorsjo]

Thanks - this one seems to work fine for my use case at least.

I'm not sure if it might be good to keep the flag set for at least a bit wider range of tests, at least the other add_cxx_compile_flags_if_supported tests right next to this one. Not sure if they are triggering issues like references to __aeabi_unwind_cpp_pr0, but it still might be good for consistency in any case?

[broadwaylamb]

@mstorsjo thanks for verifying!

When preparing this patch I've looked through the flags being added and couldn't find those that would benefit from this.

1. Obviously, this won't affect any -W flags.
2. Same for -fstrict-aliasing.
3. -EHsc is MSVC-specific and won't generate any new calls (will it?).
4. -fno-exceptions and -fno-rtti won't add any new calls either.
5. Same for -nostdinc++.
6. -D and -U are only for the preprocessor.

So, it seems like here we only want -funwind-tables to be guarded with this CMake setting.

but it still might be good for consistency in any case?

Now that you've demonstrated the issues that this flag might uncover, my opinion is we probably only want this flag to be set in places we know for sure it is needed, with a clear explanation why it is needed.

[mstorsjo]

In D71117#1774395, @broadwaylamb wrote:

@mstorsjo thanks for verifying!

When preparing this patch I've looked through the flags being added and couldn't find those that would benefit from this.

1. Obviously, this won't affect any -W flags.
2. Same for -fstrict-aliasing.
3. -EHsc is MSVC-specific and won't generate any new calls (will it?).
4. -fno-exceptions and -fno-rtti won't add any new calls either.
5. Same for -nostdinc++.
6. -D and -U are only for the preprocessor.

So, it seems like here we only want -funwind-tables to be guarded with this CMake setting.

but it still might be good for consistency in any case?

Now that you've demonstrated the issues that this flag might uncover, my opinion is we probably only want this flag to be set in places we know for sure it is needed, with a clear explanation why it is needed.

Well I guess it's whether you aim to be as safe as possible from this kind of issue, or consistency/simplicity with broader groupings. I'd say in general this flag should be rather safe for anything which is a test for a compiler feature (instead of trying to dissect exactly which compiler options might add symbol references), but clearly not for tests for linker features.

But I won't insist, if you prefer keeping it this narrow.

[broadwaylamb]

Since the original patch has been approved and the bug because of which this has been reverted is fixed, I'm going to commit this.

libunwind/CMakeLists.txt

 
 # 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)
 
+# Don't run the linker in this CMake check.
+#
+# 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(_previous_CMAKE_TRY_COMPILE_TARGET_TYPE ${CMAKE_TRY_COMPILE_TARGET_TYPE})
+set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)
 add_compile_flags_if_supported(-funwind-tables)
+set(CMAKE_TRY_COMPILE_TARGET_TYPE ${_previous_CMAKE_TRY_COMPILE_TARGET_TYPE})
+
+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;
 }