[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.

In D125393#3512071 <https://reviews.llvm.org/D125393#3512071>, @phosek wrote:

> LGTM
>
> In D125393#3511394 <https://reviews.llvm.org/D125393#3511394>, @ldionne wrote:
>
>> 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`.
>
> See the discussion on D116689 <https://reviews.llvm.org/D116689>. What @mstorsjo would like to do is to build with `LIBCXXABI_ENABLE_SHARED=OFF` and `LIBCXXABI_ENABLE_STATIC=OFF`, but still merge libcxxabi into libcxx. I have since realized that this mode would make most sense for our build as well. Problem is that some parts of the build assume that if `LLVM_ENABLE_RUNTIMES` contains `libcxxabi`, either `LIBCXXABI_ENABLE_SHARED=ON` or `LIBCXXABI_ENABLE_STATIC=ON` is going to be set and fail if both are `OFF`. This is the case even for libcxxabi tests and makes sense, if there's no library being built, then there's nothing to link against. However, if libcxxabi objects are linked into libcxx, then you should be able to use libcxx in place of libcxxabi, but that's not supported today.

Oh, got it. I did read D116689 <https://reviews.llvm.org/D116689> but didn't see that. Anyways, yeah I guess what you describe would make sense. In fact, I think we should define `cxxabi_static` and `cxxabi_shared`, but mark them as `EXCLUDE_FROM_ALL`. Then we can make them dependents of `cxxabi` (which is not `EXCLUDE_FROM_ALL`) only when `LIBCXXABI_ENABLE_SHARED=ON` (etc.). I can look into that once this patch is landed.



================
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:
> ldionne wrote:
> > 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.
> I like that idea, treating the combined libcxxabi+libcxx as a dedicated build-mode is going to be cleaner then using a combination of different flags as is the case today.
Awesome, I have a patch ready to go locally once this one lands.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125393