Details
- Reviewers
phosek mstorsjo - Group Reviewers
Restricted Project Restricted Project Restricted Project - Commits
- rGaa656f6c2dec: [runtimes] Introduce object libraries
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks! Overall, this looks like it can work for me. The same two issues as I still had with D116689 (as discussed with @phosek there) exists here too:
- For mingw, I can't build cxxabi_shared, but I do need cxxabi_shared_objects for cxx_shared. @phosek suggested fixing this by always creating the cxxabi_shared_objects target, but adding the EXCLUDE_FROM_ALL property on it.
- As the line target_link_libraries(cxx_shared PUBLIC unwind_shared) is removed from cxx, builds with LIBCXXABI_USE_LLVM_UNWINDER, without LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY fail. This can be fixed (at least for my case, didn't test other cases) by moving that dependency from cxxabi_shared to cxxabi_shared_objects - so that it propagates to either cxxabi_shared or cxx_shared later.
I.e., to make it work for me, I made this change on top of it:
diff --git a/libcxxabi/src/CMakeLists.txt b/libcxxabi/src/CMakeLists.txt index 6e22432759e6..04bee47deee7 100644 --- a/libcxxabi/src/CMakeLists.txt +++ b/libcxxabi/src/CMakeLists.txt @@ -152,10 +152,13 @@ if (NOT TARGET pstl::ParallelSTL) endif() # Build the shared library. -if (LIBCXXABI_ENABLE_SHARED) add_library(cxxabi_shared_objects OBJECT ${LIBCXXABI_SOURCES} ${LIBCXXABI_HEADERS}) - if (LIBCXXABI_USE_LLVM_UNWINDER AND LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY) - target_sources(cxxabi_shared_objects PUBLIC $<TARGET_OBJECTS:unwind_shared_objects>) + if (LIBCXXABI_USE_LLVM_UNWINDER) + if (LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY) + target_sources(cxxabi_shared_objects PUBLIC $<TARGET_OBJECTS:unwind_shared_objects>) + else() + target_link_libraries(cxxabi_shared_objects PUBLIC unwind_shared) + endif() endif() target_link_libraries(cxxabi_shared_objects PRIVATE cxx-headers ${LIBCXXABI_BUILTINS_LIBRARY} ${LIBCXXABI_SHARED_LIBRARIES} ${LIBCXXABI_LIBRARIES}) target_link_libraries(cxxabi_shared_objects PUBLIC cxxabi-headers) @@ -166,12 +169,11 @@ if (LIBCXXABI_ENABLE_SHARED) CXX_STANDARD_REQUIRED OFF COMPILE_FLAGS "${LIBCXXABI_COMPILE_FLAGS}" DEFINE_SYMBOL "" + EXCLUDE_FROM_ALL ON ) +if (LIBCXXABI_ENABLE_SHARED) add_library(cxxabi_shared SHARED $<TARGET_OBJECTS:cxxabi_shared_objects>) - if(LIBCXXABI_USE_LLVM_UNWINDER AND NOT LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY) - target_link_libraries(cxxabi_shared PUBLIC unwind_shared) - endif() set_target_properties(cxxabi_shared PROPERTIES LINK_FLAGS "${LIBCXXABI_LINK_FLAGS}"
(I guess the same change for always creating the cxxabi_static_objects, but with EXCLUDE_FROM_ALL, should be made too, but I just tested with the bare minimums for making this work for me.)
As mentioned in D116689, the EXCLUDE_FROM_ALL is going to require more cleanup throughout LLVM so I'd suggest making that change as a follow up.
Ah, sorry, I missed to pick up that detail there. As we do have the mingw-dll configuration in libcxx CI (which gets broken by this patch as-is), we need to fix this in one way or another before we can land this though.
libcxx/src/CMakeLists.txt | ||
---|---|---|
309 | 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). |
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.
libcxx/src/CMakeLists.txt | ||
---|---|---|
309 | 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’ll try to have a closer look tonight; until then, the mingw ci configs should test the most important bits at least.
LGTM
See the discussion on 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.
libcxx/src/CMakeLists.txt | ||
---|---|---|
309 | 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. |
Oh, got it. I did read 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.
libcxx/src/CMakeLists.txt | ||
---|---|---|
309 | Awesome, I have a patch ready to go locally once this one lands. |
Try to fix the build on armv8 & friends.
Disregard the CI changes, I'm just trying to target the bots that failed previously.
This seems to work fine for me, thanks! (But please check that the full CI run works of course.) I didn't do a full review in detail, but from what I've looked at, it seems good to me!
Well so far I do build with LIBCXXABI_ENABLE_STATIC=ON (otherwise I wouldn't have a static library to merge into libcxx) - it's just LIBCXXABI_ENABLE_SHARED which I have to disable because it's not possible to link. But as I don't need the static libcxxabi library as a final build product, I guess being able to disable both "product parts" of libcxxabi would be nice!
How can we link against installed libunwind.so now when building libcxxabi stand-alone?
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).