[CMake] Differentiate between static and shared libc++abi

[phosek]

Summary

This addresses the issue introduced in r354212 which broke the case when
static libc++abi is merged into static libc++, but shared libc++ is
linked against shared libc++. There are 4 different possible
combinations which is difficult to capture using a single variable. This
change splits LIBCXX_CXX_ABI_LIBRARY into two:
LIBCXX_CXX_SHARED_ABI_LIBRARY and LIBCXX_CXX_STATIC_ABI_LIBRARY to
handle the shared and static cases. This in turn allows simplification
of some of the logic around merging of static archives.

[Hahnfeld]

I guess that change makes sense overall, some comments from reading the code inline. I didn't apply the patch locally so I might be missing some context here...

libcxx/cmake/Modules/HandleLibCXXABI.cmake

111 ↗

(On Diff #193238)

This should be CXXABI_STATIC_LIBNAME, no?

libcxx/lib/CMakeLists.txt

62–65 ↗

(On Diff #193238)

LIBCXX_CXX_SHARED_ABI_LIBRARY?

[ldionne]

This makes sense given the issue at hand, but we should really really move towards an approach based on find_package to find our dependencies.

[phosek]

In D60114#1451607, @ldionne wrote:

This makes sense given the issue at hand, but we should really really move towards an approach based on find_package to find our dependencies.

I think that's the best approach long-term, but I'd like to land this one first and make that change as a follow up, since right now our ASan builds that use libc++ are broken because of this issue so I'd like to unbreak them.

libcxx/cmake/Modules/HandleLibCXXABI.cmake

111 ↗

(On Diff #193238)

I've removed the condition altogether to simplify the logic.

libcxx/lib/CMakeLists.txt

62–65 ↗

(On Diff #193238)

I've changed the logic so now it's actually correct.

[smeenai]

LGTM, though I agree with @ldionne, and I'll wait for him (or another libc++ maintainer) to accept.

libcxx/lib/CMakeLists.txt

64 ↗

(On Diff #193333)

Not your diff, but CMake reorders static libraries on the link line in dependency order, so I've seen libraries get reordered out of the whole-archive sandwich. I don't know of a good way to handle that generically though ... making the entire sandwich be one giant argument (e.g. "-Wl,--whole-archive,libfoo,--no-whole-archive") breaks when libfoo is a target, since CMake doesn't replace it with the path to the actual library, and using a generator expression breaks when libfoo isn't a target.

(On the same note, if you ever need to debug CMake's library reordering, CMAKE_LINK_DEPENDS_DEBUG_MODE is super useful.)

[smeenai]

(to be clear I also think it's fine to unbreak this first and do the find_package change later, but the latter should hopefully end up being much nicer)

[Hahnfeld]

libcxx/cmake/Modules/HandleLibCXXABI.cmake

110–114 ↗

(On Diff #193333)

Does it make sense to move these lines into the above if (LIBCXX_CXX_ABI_INTREE)? If I understand the code correctly, LIBCXX_CXX_ABI_INTREE and LIBCXX_CXX_ABI_SYSTEM are mutually exclusive, no?

[phosek]

libcxx/cmake/Modules/HandleLibCXXABI.cmake

110–114 ↗

(On Diff #193333)

I'm not sure if that was the intention, but it's not how things are set up right now, e.g. for FreeBSD or default we set neither of these variables. This is probably something we should also consider cleaning up in the future.

libcxx/trunk/cmake/Modules/HandleLibCXXABI.cmake

 
 #===============================================================================
 # Add an ABI library if appropriate
 #===============================================================================
 
 #
 # _setup_abi: Set up the build to use an ABI library
 #
 # Parameters:
 #   abidefines: A list of defines needed to compile libc++ with the ABI library
-#   abilib    : The ABI library to link against.
+#   abishared : The shared ABI library to link against.
+#   abistatic : The static ABI library to link against.
 #   abifiles  : A list of files (which may be relative paths) to copy into the
 #               libc++ build tree for the build.  These files will be copied
 #               twice: once into include/, so the libc++ build itself can find
 #               them, and once into include/c++/v1, so that a clang built into
 #               the same build area will find them.  These files will also be
 #               installed alongside the libc++ headers.
 #   abidirs   : A list of relative paths to create under an include directory
 #               in the libc++ build directory.
 #
 
-macro(setup_abi_lib abidefines abilib abifiles abidirs)
+macro(setup_abi_lib abidefines abishared abistatic abifiles abidirs)
   list(APPEND LIBCXX_COMPILE_FLAGS ${abidefines})
   set(LIBCXX_CXX_ABI_INCLUDE_PATHS "${LIBCXX_CXX_ABI_INCLUDE_PATHS}"
     CACHE PATH
     "Paths to C++ ABI header directories separated by ';'." FORCE
     )
   set(LIBCXX_CXX_ABI_LIBRARY_PATH "${LIBCXX_CXX_ABI_LIBRARY_PATH}"
     CACHE PATH
     "Paths to C++ ABI library directory"
     )
-  set(LIBCXX_CXX_ABI_LIBRARY ${abilib})
+  set(LIBCXX_CXX_SHARED_ABI_LIBRARY ${abishared})
+  set(LIBCXX_CXX_STATIC_ABI_LIBRARY ${abistatic})
   set(LIBCXX_ABILIB_FILES ${abifiles})
 
   foreach(fpath ${LIBCXX_ABILIB_FILES})
     set(found FALSE)
     foreach(incpath ${LIBCXX_CXX_ABI_INCLUDE_PATHS})
       if (EXISTS "${incpath}/${fpath}")
         set(found TRUE)
         get_filename_component(dstdir ${fpath} PATH)
if ("${LIBCXX_CXX_ABI_LIBNAME}" STREQUAL "libstdc++")
     set(_LIBSUPCXX_DEFINES "-DLIBSTDCXX")
     set(_LIBSUPCXX_LIBNAME stdc++)
   else()
     set(_LIBSUPCXX_DEFINES "")
     set(_LIBSUPCXX_LIBNAME supc++)
   endif()
   setup_abi_lib(
     "-D__GLIBCXX__ ${_LIBSUPCXX_DEFINES}"
-    "${_LIBSUPCXX_LIBNAME}" "${_LIBSUPCXX_INCLUDE_FILES}" "bits"
+    "${_LIBSUPCXX_LIBNAME}" "${_LIBSUPCXX_LIBNAME}" "${_LIBSUPCXX_INCLUDE_FILES}" "bits"
     )
 elseif ("${LIBCXX_CXX_ABI_LIBNAME}" STREQUAL "libcxxabi")
   if (LIBCXX_CXX_ABI_INTREE)
     # Link against just-built "cxxabi" target.
-    if (LIBCXX_ENABLE_STATIC_ABI_LIBRARY)
-      set(CXXABI_LIBNAME cxxabi_static)
-    else()
-      set(CXXABI_LIBNAME cxxabi_shared)
-    endif()
+    set(CXXABI_SHARED_LIBNAME cxxabi_shared)
+    set(CXXABI_STATIC_LIBNAME cxxabi_static)
   else()
     # Assume c++abi is installed in the system, rely on -lc++abi link flag.
-    set(CXXABI_LIBNAME "c++abi")
+    set(CXXABI_SHARED_LIBNAME "c++abi")
+    set(CXXABI_STATIC_LIBNAME "c++abi")
   endif()
-  set(HEADERS "cxxabi.h;__cxxabi_config.h")
   if (LIBCXX_CXX_ABI_SYSTEM)
     set(HEADERS "")
+  else()
+    set(HEADERS "cxxabi.h;__cxxabi_config.h")
   endif()
-  setup_abi_lib("-DLIBCXX_BUILDING_LIBCXXABI" ${CXXABI_LIBNAME} "${HEADERS}" "")
+  setup_abi_lib(
+    "-DLIBCXX_BUILDING_LIBCXXABI"
+    "${CXXABI_SHARED_LIBNAME}" "${CXXABI_STATIC_LIBNAME}" "${HEADERS}" "")
 elseif ("${LIBCXX_CXX_ABI_LIBNAME}" STREQUAL "libcxxrt")
-  setup_abi_lib("-DLIBCXXRT"
-    "cxxrt" "cxxabi.h;unwind.h;unwind-arm.h;unwind-itanium.h" ""
+  setup_abi_lib(
+    "-DLIBCXXRT"
+    "cxxrt" "cxxrt" "cxxabi.h;unwind.h;unwind-arm.h;unwind-itanium.h" ""
     )
 elseif ("${LIBCXX_CXX_ABI_LIBNAME}" STREQUAL "vcruntime")
  # Nothing TODO
 elseif ("${LIBCXX_CXX_ABI_LIBNAME}" STREQUAL "none")
   list(APPEND LIBCXX_COMPILE_FLAGS "-D_LIBCPP_BUILDING_HAS_NO_ABI_LIBRARY")
 elseif ("${LIBCXX_CXX_ABI_LIBNAME}" STREQUAL "default")
   # Nothing TODO
 else()
   message(FATAL_ERROR
     "Unsupported c++ abi: '${LIBCXX_CXX_ABI_LIBNAME}'. \
      Currently libstdc++, libsupc++, libcxxabi, libcxxrt, default and none are
      supported for c++ abi."
     )
 endif ()

libcxx/trunk/lib/CMakeLists.txt

 if (APPLE AND (LIBCXX_CXX_ABI_LIBNAME STREQUAL "libcxxabi" OR
                LIBCXX_CXX_ABI_LIBNAME STREQUAL "default")
           AND NOT DEFINED LIBCXX_OSX_REEXPORT_LIBCXXABI_SYMBOLS)
   set(LIBCXX_OSX_REEXPORT_LIBCXXABI_SYMBOLS ON)
 endif()
 
 if (LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY)
   if (APPLE)
-    add_library_flags("-Wl,-force_load" "${LIBCXX_CXX_ABI_LIBRARY}")
+    add_library_flags("-Wl,-force_load" "${LIBCXX_CXX_STATIC_ABI_LIBRARY}")
   else()
     add_library_flags("-Wl,--whole-archive" "-Wl,-Bstatic")
-    add_library_flags("${LIBCXX_CXX_ABI_LIBRARY}")
+    add_library_flags("${LIBCXX_CXX_STATIC_ABI_LIBRARY}")
     add_library_flags("-Wl,-Bdynamic" "-Wl,--no-whole-archive")
   endif()
 elseif (LIBCXX_OSX_REEXPORT_LIBCXXABI_SYMBOLS)
-  add_library_flags("${LIBCXX_CXX_ABI_LIBRARY}")
+  add_library_flags("${LIBCXX_CXX_SHARED_ABI_LIBRARY}")
 else ()
- add_interface_library("${LIBCXX_CXX_ABI_LIBRARY}")
+  add_interface_library("${LIBCXX_CXX_SHARED_ABI_LIBRARY}")
 endif()
 
 if (APPLE AND LLVM_USE_SANITIZER)
   if (("${LLVM_USE_SANITIZER}" STREQUAL "Address") OR
       ("${LLVM_USE_SANITIZER}" STREQUAL "Address;Undefined") OR
       ("${LLVM_USE_SANITIZER}" STREQUAL "Undefined;Address"))
     set(LIBFILE "libclang_rt.asan_osx_dynamic.dylib")
   elseif("${LLVM_USE_SANITIZER}" STREQUAL "Undefined")
if (LIBCXX_INSTALL_STATIC_LIBRARY)
     list(APPEND LIBCXX_INSTALL_TARGETS "cxx_static")
   endif()
   # Attempt to merge the libc++.a archive and the ABI library archive into one.
   if (LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY)
     set(MERGE_ARCHIVES_SEARCH_PATHS "")
     if (LIBCXX_CXX_ABI_LIBRARY_PATH)
       set(MERGE_ARCHIVES_SEARCH_PATHS "-L${LIBCXX_CXX_ABI_LIBRARY_PATH}")
     endif()
-    if ((TARGET ${LIBCXX_CXX_ABI_LIBRARY}) OR
-        (${LIBCXX_CXX_ABI_LIBRARY} MATCHES "cxxabi(_static|_shared)?" AND HAVE_LIBCXXABI))
-      set(MERGE_ARCHIVES_ABI_TARGET "$<TARGET_LINKER_FILE:cxxabi_static>")
+    if (TARGET "${LIBCXX_CXX_STATIC_ABI_LIBRARY}" OR HAVE_LIBCXXABI)
+      set(MERGE_ARCHIVES_ABI_TARGET "$<TARGET_LINKER_FILE:${LIBCXX_CXX_STATIC_ABI_LIBRARY}>")
     else()
       set(MERGE_ARCHIVES_ABI_TARGET
-          "${CMAKE_STATIC_LIBRARY_PREFIX}${LIBCXX_CXX_ABI_LIBRARY}${CMAKE_STATIC_LIBRARY_SUFFIX}")
+        "${CMAKE_STATIC_LIBRARY_PREFIX}${LIBCXX_CXX_STATIC_ABI_LIBRARY}${CMAKE_STATIC_LIBRARY_SUFFIX}")
     endif()
     add_custom_command(TARGET cxx_static POST_BUILD
     COMMAND
       ${PYTHON_EXECUTABLE} ${LIBCXX_SOURCE_DIR}/utils/merge_archives.py
     ARGS
       -o $<TARGET_LINKER_FILE:cxx_static>
       --ar "${CMAKE_AR}"
       "$<TARGET_LINKER_FILE:cxx_static>"