[libcxx-commits] [PATCH] D125393: [runtimes] Introduce object libraries

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked an inline comment as done.
ldionne added a comment.

Thanks a lot both @mstorsjo and @phosek for the comments. I've applied @mstorsjo's change (and made a similar change for static libraries). I also did it for libunwind.

@phosek I'm not sure I follow when you say that marking the object libraries as `EXCLUDE_FROM_ALL` is going to require some reworking elsewhere in LLVM. We are only marking the object libraries as `EXCLUDE_FROM_ALL`, and nobody outside the runtimes will be using them. The normal `cxxabi_shared` & friends targets will still be there, and they will still be built by default unless `LIBCXXABI_ENABLE_SHARED=OFF`.



================
Comment at: libcxx/src/CMakeLists.txt:295
   if (LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY)
-    if (APPLE)
-      set(MERGE_ARCHIVES_LIBTOOL "--use-libtool" "--libtool" "${CMAKE_LIBTOOL}")
-    endif()
-    add_custom_command(TARGET cxx_static POST_BUILD
-    COMMAND
-      ${Python3_EXECUTABLE} ${LIBCXX_SOURCE_DIR}/utils/merge_archives.py
-    ARGS
-      -o $<TARGET_LINKER_FILE:cxx_static>
-      --ar "${CMAKE_AR}"
-      ${MERGE_ARCHIVES_LIBTOOL}
-      "$<TARGET_LINKER_FILE:cxx_static>"
-      "$<TARGET_LINKER_FILE:libcxx-abi-static>"
-      ""
-    WORKING_DIRECTORY ${LIBCXX_BUILD_DIR}
-    DEPENDS ${MERGE_ARCHIVES_ABI_TARGET}
-    )
+    target_link_libraries(cxx_static PRIVATE libcxx-abi-static-objects)
   endif()
----------------
phosek wrote:
> Note that this is a potential regression. Specifically, today you can merge static ABI libraries other than libcxxabi, but after this change only libcxxabi will be supported. I don't know if there are any users and if anyone is going to notice, but we should probably give an error when someone tries to use `LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY` with ABI library other than libcxxabi (we might also consider renaming that option to `LIBCXX_STATICALLY_LINK_LIBCXXABI_IN_STATIC_LIBRARY`).
Hmm, good catch. After this change, they would get an error saying that the `libcxx-abi-static-objects` target does not exist, and they would have to go define it in `HandleLibCXXABI.cmake`. Honestly, I think that is reasonable, since I suspect nobody is doing that anyways.

Also, this comment got me thinking that perhaps it would be even better to replace the `LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY ` option by a new "ABI" library choice in `HandleLibCXXABI.cmake` where `libcxx-abi-static` and `libcxx-abi-shared` are the object libraries. So, for example, you'd specify `LIBCXX_CXX_ABI=libcxxabi-objects` instead of `LIBCXX_CXX_ABI=libcxxabi LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY =ON`. We could make that change separately.

Do you have thoughts on removing `LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY` in favour of a new "flavor" of `libcxxabi` in `HandleLibCXXABI.cmake`? I think it might be quite nice since it would remove even more logic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125393