[CMake] Split linked libraries for shared and static libc++

[phosek]

Summary

Some linker libraries are only needed for shared libc++, some only
for static libc++, combining these together in LIBCXX_LIBRARIES can
introduce unnecessary dependencies.

This changes splits those up into LIBCXX_SHARED_LIBRARIES and
LIBCXX_STATIC_LIBRARIES matching what libc++abi already does.

[EricWF]

combining these together in LIBCXX_LIBRARIES and LIBCXX_INTERFACE_LIBRARIES can introduce unnecessary dependencies.

Can you provide an example? And elaborate on the introducing unused libraries on the link line?

libcxx/lib/CMakeLists.txt

48 ↗

(On Diff #185692)

These are flags. Not shared libraries.

52 ↗

(On Diff #185692)

Does this make it an interface library? because that's incorrect.

98 ↗

(On Diff #185692)

If you're renaming this, can you double-check there are no buildbot configurations using it on zorg?

Maybe set up a bot or two? You can use the libcxx-cloud workers.

[phosek]

In D57872#1397674, @EricWF wrote:

combining these together in LIBCXX_LIBRARIES and LIBCXX_INTERFACE_LIBRARIES can introduce unnecessary dependencies.

Can you provide an example? And elaborate on the introducing unused libraries on the link line?

Example is change D49587. Currently, this one is failing

-- Configuring done
CMake Error: install(EXPORT "LLVMRuntimes" ...) includes target "cxx_static" which requires target "cxxabi_static" that is not in the export set.
CMake Error: install(EXPORT "LLVMRuntimes" ...) includes target "cxx_static" which requires target "unwind_static" that is not in the export set.
-- Generating done

The reason for this is because we're no installing static libunwind and libc++abi, instead we're merging them into libc++. This shouldn't be a problem, but add_interface_library(unwind_static) and add_interface_library("${LIBCXX_CXX_ABI_LIBRARY}") adds these as dependencies because it doesn't distinguish between static and shared library dependencies. However, there really isn't any such thing as static library dependency since you cannot link static library against another static library. This change is trying to address this issue by separating the notion of shared library dependency and static library "dependency" which is something that both libunwind and libc++abi build already does.

Does this make sense?

libcxx/lib/CMakeLists.txt

48 ↗

(On Diff #185692)

These are still passed to target_link_libraries just like before since add_library_flags simply adds the string to LIBCXX_LIBRARIES which was passed to target_link_libraries, so the original function name was a misnomer. I could change the name to something like LIBCXX_SHARED_LINK_FLAGS, but these should be still passed to target_link_libraries, not as LINK_FLAGS to set_target_properties.

98 ↗

(On Diff #185692)

I'm not renaming this variable, LIBCXXABI_ENABLE_STATIC_UNWINDER implies both LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY and LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_STATIC_LIBRARY ([using cmake_dependent_option](https://github.com/llvm/llvm-project/blob/master/libcxxabi/CMakeLists.txt#L97)), but each of them can be set separately. I'm making this condition more fine grained to account for that since in this branch we're only interested in the shared case.

[smeenai]

libcxx/lib/CMakeLists.txt

54–55 ↗

(On Diff #188438)

The commit message mentions eliminating LIBCXX_INTERFACE_LIBRARIES, but that doesn't appear to be the case. Did you mean getting rid of LIBCXX_LIBRARIES?

This looks equivalent to add_interface_library if you replaced LIBCXX_LIBRARIES with LIBCXX_SHARED_LIBRARIES in that macro, and it looks like you're also appending to LIBCXX_SHARED_LIBRARIES in all the places you append to LIBCXX_INTERFACE_LIBRARIES, so the macro could still be useful.

396 ↗

(On Diff #188438)

What does this change in practice?

[phosek]

libcxx/lib/CMakeLists.txt

396 ↗

(On Diff #188438)

Just a newer syntax, but it's not necessary for this change so removed.

[ldionne]

Is this change still relevant given the recent CMake refactoring?

[phosek]

In D57872#1458908, @ldionne wrote:

Is this change still relevant given the recent CMake refactoring?

Yes although it's simpler now.

[ldionne]

I think LIBCXX_STATIC_LIBRARIES and LIBCXX_SHARED_LIBRARIES would be better renamed LIBCXX_STATIC_LIBRARY_DEPENDENCIES and LIBCXX_SHARED_LIBRARY_DEPENDENCIES , or something similar. What do you think?

[phosek]

In D57872#1468600, @ldionne wrote:

I think LIBCXX_STATIC_LIBRARIES and LIBCXX_SHARED_LIBRARIES would be better renamed LIBCXX_STATIC_LIBRARY_DEPENDENCIES and LIBCXX_SHARED_LIBRARY_DEPENDENCIES , or something similar. What do you think?

I've eliminated that variable altogether by following the pattern we already use for libc++abi.

[ldionne]

Love the cleanup!

cmake/Modules/HandleLibcxxFlags.cmake

 # if 'condition' is true then add the specified list of libraries and flags
 # to 'LIBCXX_LIBRARIES'.
 macro(add_library_flags_if condition)
   if(${condition})
     add_library_flags(${ARGN})
   endif()
 endmacro()
 
-# Add a list of libraries or link flags to 'LIBCXX_LIBRARIES'.
-macro(add_interface_library)
-  foreach(lib ${ARGN})
-    list(APPEND LIBCXX_LIBRARIES ${lib})
-    list(APPEND LIBCXX_INTERFACE_LIBRARIES ${lib})
-  endforeach()
-endmacro()
-
 # Turn a comma separated CMake list into a space separated string.
 macro(split_list listname)
   string(REPLACE ";" " " ${listname} "${${listname}}")
 endmacro()

lib/CMakeLists.txt

if (LIBCXX_USE_COMPILER_RT)
   find_compiler_rt_library(builtins LIBCXX_BUILTINS_LIBRARY)
   add_library_flags_if(LIBCXX_BUILTINS_LIBRARY "${LIBCXX_BUILTINS_LIBRARY}")
 else()
   add_library_flags_if(LIBCXX_HAS_GCC_S_LIB gcc_s)
 endif()
 add_library_flags_if(LIBCXX_HAVE_CXX_ATOMICS_WITH_LIB atomic)
 add_library_flags_if(MINGW "${MINGW_LIBRARIES}")
 
-# Add the unwinder library.
-if (LIBCXXABI_USE_LLVM_UNWINDER)
-  if (NOT LIBCXXABI_ENABLE_STATIC_UNWINDER AND (TARGET unwind_shared OR HAVE_LIBUNWIND))
-    add_interface_library(unwind_shared)
-  elseif (LIBCXXABI_ENABLE_STATIC_UNWINDER AND (TARGET unwind_static OR HAVE_LIBUNWIND))
-    add_interface_library(unwind_static)
-  else()
-    add_interface_library(unwind)
-  endif()
-endif()
-
 # Setup flags.
 add_link_flags_if_supported(-nodefaultlibs)
 
 if (LIBCXX_TARGETING_MSVC)
   if (LIBCXX_DEBUG_BUILD)
     set(LIB_SUFFIX "d")
   else()
     set(LIB_SUFFIX "")
PROPERTIES
       LINK_FLAGS    "${LIBCXX_LINK_FLAGS}"
       OUTPUT_NAME   "c++"
       VERSION       "${LIBCXX_ABI_VERSION}.0"
       SOVERSION     "${LIBCXX_ABI_VERSION}"
       DEFINE_SYMBOL ""
   )
   cxx_set_common_defines(cxx_shared)
 
+  # Link against LLVM libunwind
+  if (LIBCXXABI_USE_LLVM_UNWINDER)
+    if (NOT LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY AND (TARGET unwind_shared OR HAVE_LIBUNWIND))
+      target_link_libraries(cxx_shared PRIVATE unwind_shared)
+      list(APPEND LIBCXX_INTERFACE_LIBRARIES unwind_shared) # For the linker script
+    elseif (LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY AND (TARGET unwind_static OR HAVE_LIBUNWIND))
+      # libunwind is already included in libc++abi
+    else()
+      target_link_libraries(cxx_shared PRIVATE unwind)
+      list(APPEND LIBCXX_INTERFACE_LIBRARIES unwind) # For the linker script
+    endif()
+  endif()
+
   # Link against libc++abi
   if (LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY)
     if (APPLE)
       target_link_libraries(cxx_shared PRIVATE "-Wl,-force_load" "${LIBCXX_CXX_STATIC_ABI_LIBRARY}")
     else()
       target_link_libraries(cxx_shared PRIVATE "-Wl,--whole-archive,-Bstatic" "${LIBCXX_CXX_STATIC_ABI_LIBRARY}" "-Wl,-Bdynamic,--no-whole-archive")
     endif()
   else()