[libcxx-commits] [PATCH] D57872: [CMake] Split linked libraries for shared and static libc++

[Petr Hosek via Phabricator via libcxx-commits]

phosek added a comment.

In D57872#1397674 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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?



================
Comment at: libcxx/lib/CMakeLists.txt:48
 if (LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY)
-  add_library_flags("-Wl,--whole-archive" "-Wl,-Bstatic")
-  add_library_flags("${LIBCXX_CXX_ABI_LIBRARY}")
-  add_library_flags("-Wl,-Bdynamic" "-Wl,--no-whole-archive")
+  list(APPEND LIBCXX_SHARED_LIBRARIES "-Wl,--whole-archive" "-Wl,-Bstatic")
+  list(APPEND LIBCXX_SHARED_LIBRARIES "${LIBCXX_CXX_ABI_LIBRARY}")
----------------
EricWF wrote:
> These are flags. Not shared libraries.
> 
>  
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`.


================
Comment at: libcxx/lib/CMakeLists.txt:98
 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)
+  if (NOT LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY AND (TARGET unwind_shared OR HAVE_LIBUNWIND))
+    list(APPEND LIBCXX_SHARED_LIBRARIES unwind_shared)
----------------
EricWF wrote:
> 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.
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.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57872/new/

https://reviews.llvm.org/D57872