Page MenuHomePhabricator

[runtimes] Introduce object libraries
ClosedPublic

Authored by ldionne on May 11 2022, 8:25 AM.

Details

Reviewers
phosek
mstorsjo
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rGaa656f6c2dec: [runtimes] Introduce object libraries
Summary

This is a variant of D116689 rebased on top of the new (proposed) ABI
refactoring in D120727. It should conserve the basic properties of the
original patch by @phosek, except it also allows cleaning up the merging
of libc++abi into libc++ from the libc++ side.

Diff Detail

Event Timeline

ldionne created this revision.May 11 2022, 8:25 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 11 2022, 8:25 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested review of this revision.May 11 2022, 8:25 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 11 2022, 8:25 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript

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

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.

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.

phosek added inline comments.May 12 2022, 1:28 AM
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).

ldionne marked an inline comment as done.May 13 2022, 5:58 AM

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.

ldionne updated this revision to Diff 429215.May 13 2022, 6:05 AM
ldionne marked an inline comment as done.

This should address review comments. Please LMK what you think.

ldionne updated this revision to Diff 429229.May 13 2022, 6:58 AM

Fix issue with fPIC not being passed due to the usage of object libraries

ldionne updated this revision to Diff 429246.May 13 2022, 8:13 AM

Hopefully fix libunwind issue.

This should address review comments. Please LMK what you think.

I’ll try to have a closer look tonight; until then, the mingw ci configs should test the most important bits at least.

phosek accepted this revision.May 13 2022, 10:18 AM

LGTM

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

ldionne marked an inline comment as done.May 13 2022, 11:13 AM

LGTM

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

ldionne updated this revision to Diff 429299.May 13 2022, 11:14 AM
ldionne marked an inline comment as done.

Try to fix the build on armv8 & friends.

Disregard the CI changes, I'm just trying to target the bots that failed previously.

ldionne updated this revision to Diff 429329.May 13 2022, 12:40 PM

Try to fix linker script generation.

mstorsjo accepted this revision.May 13 2022, 2:01 PM

This should address review comments. Please LMK what you think.

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!

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.

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!

ldionne accepted this revision as: Restricted Project.May 16 2022, 5:40 AM

AIX failure is a timeout.

This revision is now accepted and ready to land.May 16 2022, 5:40 AM
This revision was landed with ongoing or failed builds.May 16 2022, 5:41 AM
This revision was automatically updated to reflect the committed changes.
mgorny added a comment.EditedJul 28 2022, 4:55 AM

How can we link against installed libunwind.so now when building libcxxabi stand-alone?