[libc++][CMake] Clean up logic for choosing which unwinder lib to link with tests

[ldionne]

Summary

The main functional change in this patch is that when
LIBCXXABI_USE_LLVM_UNWINDER and LIBCXX_ENABLE_ABI_LINKER_SCRIPT are both
enabled, -lgcc_s and -lgcc will no longer be added to the linker flags
for the test suite.

In this case, linking against libgcc is unnecessary, because the linker
script adds -lunwind.

[ldionne]

I'd love more FIXMEs to be resolved before we move forward.

[phosek]

libcxx/utils/libcxx/test/target_info.py

245

It's needed by [LocalAddressSpace::findFunctionName](https://github.com/llvm/llvm-project/blob/master/libunwind/src/AddressSpace.hpp#L594) (and on Darwin also [_dyld_find_unwind_sections](https://github.com/llvm/llvm-project/blob/master/libunwind/src/AddressSpace.hpp#L68)). AFAIK this function is only used by __unw_get_proc_name which is not needed for C++ exception unwinding (it's not part of the _Unwind_* interface), so one possible solution for eliminating that dependency would be to separate the _Unwind_* and unw_* interfaces into two different libraries which is something I proposed in D59921, but that's likely a non-trivial effort. Alternative would be to avoid dladdr and instead read the symbol information directly from the binary file which is what https://www.nongnu.org/libunwind/ does, that's likely more feasible given the current implementation.

246

Note that libunwind also needs -lpthread which is used by https://github.com/llvm/llvm-project/blob/master/libunwind/src/RWMutex.hpp which is used by https://github.com/llvm/llvm-project/blob/master/libunwind/src/UnwindCursor.hpp when DWARF unwinding is being used. It's also used for sjlj unwinding on Darwin in https://github.com/llvm/llvm-project/blob/master/libunwind/src/Unwind-sjlj.c.

250

libgcc contains the GCC unwinder.

251

-lgcc is libgcc.a, -lgcc_s is libgcc_s.so.1, you probably only want one of those?

[tstellar]

libcxx/utils/libcxx/test/target_info.py

251

This was my question. I wasn't sure why both were included.

[ldionne]

Is this still relevant? You should now be able to specify your own linker flags in the config file by modelling your file on something like libcxx/test/configs/libcxx-trunk-shared.cfg.in.

If not relevant anymore, can this be abandoned to clean up the review queue? If still relevant, let's talk!

[ldionne]

[Github PR transition cleanup]

Abandoning since I think this is no longer relevant now that we have easy-to-create .cfg.in files for the test suite.

libcxx/test/CMakeLists.txt

set(LIBCXX_TEST_COMPILER_FLAGS "" CACHE STRING
     "Additonal linker flags to pass when compiling the tests")
 
 # The tests shouldn't link to any ABI library when it has been linked into
 # libc++ statically or via a linker script.
 if (LIBCXX_ENABLE_STATIC_ABI_LIBRARY OR LIBCXX_ENABLE_ABI_LINKER_SCRIPT)
   set(LIBCXX_CXX_ABI_LIBNAME "none")
 endif()
 
-# The tests shouldn't link to libunwind if we have a linker script which
-# already does so.
-if (LIBCXX_ENABLE_ABI_LINKER_SCRIPT)
-  set(LIBCXXABI_USE_LLVM_UNWINDER OFF)
-endif()
-
 pythonize_bool(LIBCXX_ENABLE_EXCEPTIONS)
 pythonize_bool(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY)
 pythonize_bool(LIBCXX_ENABLE_RTTI)
 pythonize_bool(LIBCXX_ENABLE_SHARED)
 pythonize_bool(LIBCXX_ENABLE_FILESYSTEM)
 pythonize_bool(LIBCXX_BUILD_32_BITS)
 pythonize_bool(LIBCXX_GENERATE_COVERAGE)
 pythonize_bool(LIBCXXABI_ENABLE_SHARED)
 pythonize_bool(LIBCXXABI_USE_LLVM_UNWINDER)
+pythonize_bool(LIBCXX_ENABLE_ABI_LINKER_SCRIPT)
 pythonize_bool(LIBCXX_USE_COMPILER_RT)
 pythonize_bool(LIBCXX_HAS_ATOMIC_LIB)
 pythonize_bool(LIBCXX_HAVE_CXX_ATOMICS_WITH_LIB)
 pythonize_bool(LIBCXX_BUILD_EXTERNAL_THREAD_LIBRARY)
 pythonize_bool(LIBCXX_DEBUG_BUILD)
 
 # By default, for non-standalone builds, libcxx and libcxxabi share a library
 # directory.

libcxx/test/lit.site.cfg.in

 config.gcc_toolchain            = "@LIBCXX_GCC_TOOLCHAIN@"
 config.generate_coverage        = @LIBCXX_GENERATE_COVERAGE@
 config.target_info              = "@LIBCXX_TARGET_INFO@"
 config.test_linker_flags        = "@LIBCXX_TEST_LINKER_FLAGS@"
 config.test_compiler_flags      = "@LIBCXX_TEST_COMPILER_FLAGS@"
 
 config.executor                 = "@LIBCXX_EXECUTOR@"
 config.llvm_unwinder            = @LIBCXXABI_USE_LLVM_UNWINDER@
+config.libcxx_is_linker_script  = @LIBCXX_ENABLE_ABI_LINKER_SCRIPT@
 config.builtins_library         = "@LIBCXX_BUILTINS_LIBRARY@"
 config.has_libatomic            = @LIBCXX_HAS_ATOMIC_LIB@
 config.use_libatomic            = @LIBCXX_HAVE_CXX_ATOMICS_WITH_LIB@
 config.debug_build              = @LIBCXX_DEBUG_BUILD@
 config.libcxxabi_shared         = @LIBCXXABI_ENABLE_SHARED@
 config.cxx_ext_threads          = @LIBCXX_BUILD_EXTERNAL_THREAD_LIBRARY@
 
 # Let the main config do the real work.
 config.loaded_site_config = True
 lit_config.load_config(config, "@LIBCXX_SOURCE_DIR@/test/lit.cfg")

libcxx/utils/libcxx/test/target_info.py

def add_cxx_compile_flags(self, flags):
         flags += ['-D__STDC_FORMAT_MACROS',
                   '-D__STDC_LIMIT_MACROS',
                   '-D__STDC_CONSTANT_MACROS']
 
     def add_cxx_link_flags(self, flags):
         enable_threads = ('libcpp-has-no-threads' not in
                           self.full_config.config.available_features)
         llvm_unwinder = self.full_config.get_lit_bool('llvm_unwinder', False)
+        libcxx_is_linker_script  = self.full_config.get_lit_bool('libcxx_is_linker_script', False)
         shared_libcxx = self.full_config.get_lit_bool('enable_shared', True)
         flags += ['-lm']
-        if not llvm_unwinder:
-            flags += ['-lgcc_s', '-lgcc']
         if enable_threads:
             flags += ['-lpthread']
             if not shared_libcxx:
                 flags += ['-lrt']
         flags += ['-lc']
-        if llvm_unwinder:
+        if llvm_unwinder and not libcxx_is_linker_script:
+            # FIXME: Why does llvm unwinder need -ldl?
             flags += ['-lunwind', '-ldl']
-        else:
-            flags += ['-lgcc_s']
+        elif not llvm_unwinder:
+            # If we aren't using llvm unwinder, then fallback to using libgcc's
+            # unwind implementation.
+            # FIXME: Why is -lgcc needed for the unwinder?
+            flags += ['-lgcc_s', '-lgcc']
+        # If we are using a linker script and llvm_unwinder is eanbled, then we
+        # don't need to explicitly link against -lunwind, since the linker
+        # script will take care of that for us.
         builtins_lib = self.full_config.get_lit_conf('builtins_library')
         if builtins_lib:
             flags += [builtins_lib]
         else:
             flags += ['-lgcc']
         use_libatomic = self.full_config.get_lit_bool('use_libatomic', False)
         if use_libatomic:
             flags += ['-latomic']