This is an archive of the discontinued LLVM Phabricator instance.

[libunwind][libcxxabi] Use object libraries in the build
AbandonedPublic

Authored by phosek on Jan 5 2022, 12:37 PM.

Details

Reviewers
ldionne
beanz
smeenai
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Summary

This simplifies merging of libunwind and libcxxabi into libcxx since
we don't need to extract and merge archives and can simply use CMake
depednencies. That's also a prerequisite for using OSX_ARCHITECTURES
in CMake to build libcxx as universal library

Diff Detail

Event Timeline

phosek created this revision.Jan 5 2022, 12:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2022, 12:37 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
phosek requested review of this revision.Jan 5 2022, 12:37 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 5 2022, 12:37 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript

Using the TARGET_OBJECTS: generator seems better. However, I also have concerns over using the shared library objects. At least for Windows, reuse of the objects meant for a shared object is not really correct. This is non-portable at best, and incorrect at worse. I would rather that we just introduce a proper target that we use can for embedding into another library (which may or may not expose its interfaces from the library that integrates it).

libcxx/src/CMakeLists.txt
230

Why not use TARGET_OBJECTS:unwind> instead?

The build used to be set up with object libraries, and then @ldionne explicitly changed it to not do so, so that e.g. the shared library is built as PIC by default and the static library isn't. This change would regress that.

That's also a prerequisite for using OSX_ARCHITECTURES in CMake to build libcxx as universal library

I'm especially curious about this -- why does that require using object libraries?

phosek added a comment.Feb 8 2022, 3:09 PM

The build used to be set up with object libraries, and then @ldionne explicitly changed it to not do so, so that e.g. the shared library is built as PIC by default and the static library isn't. This change would regress that.

To clarify, I'm not trying to reuse object library between static and shared library as was the case in the past, that's undesirable for the reason you mentioned. I'm just adding an intermediate step: before we would build static library directly, now we build object library first and then archive it into static library. The reason for doing that is now we can depend on the object library from other targets (for example, libc++ can directly link libc++abi object library).

That's also a prerequisite for using OSX_ARCHITECTURES in CMake to build libcxx as universal library

I'm especially curious about this -- why does that require using object libraries?

https://github.com/llvm/llvm-project/blob/bbddd19ec723a15ea1558cce5e47cb2460fa8e24/libcxx/utils/merge_archives.py script we use for merging archives (this is used when combining libc++abi.a into libc++.a) uses ar to extract archives into a temporary directory and then repackage them into a new combined archive. That doesn't work for universal static libraries since those aren't traditional UNIX archives (at least I haven't found a way to do that with existing tooling).

This change skips the extraction and repackaging step altogether and instead packages all objects that should go into the final library directly (through CMake dependencies). I think that approach is also more efficient (no extraction and repackaging) and explicit (CMake and Ninja know exactly which files form the final archive) at slight increase in CMake code complexity which IMO is a good tradeoff.

phosek added a comment.Feb 8 2022, 3:11 PM

This change skips the extraction and repackaging step altogether and instead packages all objects that should go into the final library directly (through CMake dependencies). I think that approach is also more efficient (no extraction and repackaging) and explicit (CMake and Ninja know exactly which files form the final archive) at slight increase in CMake code complexity which IMO is a good tradeoff.

I also want to point out that I tried to minimize the number of changes in this patch, but I plan to do additional cleanup as a follow up which should simplify the build.

The build used to be set up with object libraries, and then @ldionne explicitly changed it to not do so, so that e.g. the shared library is built as PIC by default and the static library isn't. This change would regress that.

To clarify, I'm not trying to reuse object library between static and shared library as was the case in the past, that's undesirable for the reason you mentioned. I'm just adding an intermediate step: before we would build static library directly, now we build object library first and then archive it into static library. The reason for doing that is now we can depend on the object library from other targets (for example, libc++ can directly link libc++abi object library).

That's also a prerequisite for using OSX_ARCHITECTURES in CMake to build libcxx as universal library

I'm especially curious about this -- why does that require using object libraries?

https://github.com/llvm/llvm-project/blob/bbddd19ec723a15ea1558cce5e47cb2460fa8e24/libcxx/utils/merge_archives.py script we use for merging archives (this is used when combining libc++abi.a into libc++.a) uses ar to extract archives into a temporary directory and then repackage them into a new combined archive. That doesn't work for universal static libraries since those aren't traditional UNIX archives (at least I haven't found a way to do that with existing tooling).

I agree with your reasoning and I'm not suggesting you go this route, but just as a generate note, libtool (and the LLVM version llvm-libtool-darwin) should be able to handle universal archives pretty transparently; in particular, you can tell libtool to construct a static library from multiple existing static libraries directly (and it'll handle universal archives as expected) instead of having to do a manual extraction and repackaging.

phosek added a comment.Feb 8 2022, 3:23 PM

The build used to be set up with object libraries, and then @ldionne explicitly changed it to not do so, so that e.g. the shared library is built as PIC by default and the static library isn't. This change would regress that.

To clarify, I'm not trying to reuse object library between static and shared library as was the case in the past, that's undesirable for the reason you mentioned. I'm just adding an intermediate step: before we would build static library directly, now we build object library first and then archive it into static library. The reason for doing that is now we can depend on the object library from other targets (for example, libc++ can directly link libc++abi object library).

That's also a prerequisite for using OSX_ARCHITECTURES in CMake to build libcxx as universal library

I'm especially curious about this -- why does that require using object libraries?

https://github.com/llvm/llvm-project/blob/bbddd19ec723a15ea1558cce5e47cb2460fa8e24/libcxx/utils/merge_archives.py script we use for merging archives (this is used when combining libc++abi.a into libc++.a) uses ar to extract archives into a temporary directory and then repackage them into a new combined archive. That doesn't work for universal static libraries since those aren't traditional UNIX archives (at least I haven't found a way to do that with existing tooling).

I agree with your reasoning and I'm not suggesting you go this route, but just as a generate note, libtool (and the LLVM version llvm-libtool-darwin) should be able to handle universal archives pretty transparently; in particular, you can tell libtool to construct a static library from multiple existing static libraries directly (and it'll handle universal archives as expected) instead of having to do a manual extraction and repackaging.

Good to know, thanks for pointing that out. I also found out that llvm-ar recently gained L option that allows appending to an archive without having to extract it.

The problem is that these are extensions only supported by LLVM tools, so if we started relying on those unconditionally, we would break compatibility with non-LLVM tools which may undesirable.

We could also have conditional logic: if you're using LLVM tools, use the LLVM-specific options, otherwise use a fallback, but that's going to result in even more complexity.

The build used to be set up with object libraries, and then @ldionne explicitly changed it to not do so, so that e.g. the shared library is built as PIC by default and the static library isn't. This change would regress that.

To clarify, I'm not trying to reuse object library between static and shared library as was the case in the past, that's undesirable for the reason you mentioned. I'm just adding an intermediate step: before we would build static library directly, now we build object library first and then archive it into static library. The reason for doing that is now we can depend on the object library from other targets (for example, libc++ can directly link libc++abi object library).

Ahh, looking at the patch with this in mind, I think I understand what you're doing now. This seems fine at a high level, but I have some questions.

Also, this is going to merge conflict downstream in crazy ways, but that's my problem :-(.

libcxx/src/CMakeLists.txt
230

I have the same question.

303

Do we really mean to use LIBCXXABI_foo variables here?

313

Why do we need this at all now? I must have misunderstood something, but I thought we were handling the merging of archives via those object libraries now.

phosek added inline comments.Feb 10 2022, 3:00 PM
libcxx/src/CMakeLists.txt
230

AFAIK I can only use TARGET_OBJECTS against object library so I need to do $<TARGET_OBJECTS:unwind_shared_objects> which works but doesn't simplify things much. Did you have something else in mind?

313

I left this for other C++ABI libraries such as libcxxrt where we cannot use archives directly as object libraries and have to fallback to archive merging. I'm not sure if that's a use case we want to support, but we do support it now.

ldionne added inline comments.Mar 2 2022, 8:22 AM
libcxx/src/CMakeLists.txt
230

I think this answers my question.

303

Ping on this question.

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 8:22 AM
mstorsjo added inline comments.
libcxx/src/CMakeLists.txt
303

This naming is preexisting; the options for choosing how to link in the unwind library are sorted under libcxxabi (iirc there’s no corresponding option for libcxx), but in practice, the option is acted upon in both libcxxabi and libcxx, depending on what configuration is used for linking them together.

Ping @phosek, do you have time to pick up this patch again? It seems to align very much with something I'd like to do, too.

smeenai resigned from this revision.Apr 28 2022, 1:54 PM

@phosek Do you have time to pick this up and complete it? It'd be essential for a build cleanup for Windows - but the patch itself is a bit too far outside of my cmake-fu.

For the mingw builds, the essential component is that cxx_shared uses cxxabi_objects_shared. However, the tricky thing is that cxxabi_shared isn't built (it's disabled, as it can't be linked as it has inverse dependencies on symbols provided by libcxx), but the cxxabi_objects_shared component still would need to be built).

phosek updated this revision to Diff 427781.May 6 2022, 4:04 PM
phosek marked 2 inline comments as not done.
phosek added a comment.May 6 2022, 4:07 PM

@phosek Do you have time to pick this up and complete it? It'd be essential for a build cleanup for Windows - but the patch itself is a bit too far outside of my cmake-fu.

I have uploaded an updated version which simplifies the implementation and seems to be working for me locally although I haven't done extensive testing yet.

For the mingw builds, the essential component is that cxx_shared uses cxxabi_objects_shared. However, the tricky thing is that cxxabi_shared isn't built (it's disabled, as it can't be linked as it has inverse dependencies on symbols provided by libcxx), but the cxxabi_objects_shared component still would need to be built).

Is the current implementation sufficient for your needs?

@phosek Do you have time to pick this up and complete it? It'd be essential for a build cleanup for Windows - but the patch itself is a bit too far outside of my cmake-fu.

I have uploaded an updated version which simplifies the implementation and seems to be working for me locally although I haven't done extensive testing yet.

This version breaks things for me; I'm building with LIBCXX_ENABLE_STATIC_ABI_LIBRARY and LIBCXXABI_USE_LLVM_UNWINDER but not LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY. In this case, there's no shared libcxxabi (I build with LIBCXXABI_ENABLE_SHARED=OFF), so libcxx would need to retain the code to link against the libunwind cmake target, it can't be carried transitively via libcxxabi (cxxabi_shared_objects). I.e. this case gets lost:

if(LIBCXXABI_USE_LLVM_UNWINDER AND NOT LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY)
   target_link_libraries(cxxabi_shared PUBLIC unwind_shared)
 endif()

For the mingw builds, the essential component is that cxx_shared uses cxxabi_objects_shared. However, the tricky thing is that cxxabi_shared isn't built (it's disabled, as it can't be linked as it has inverse dependencies on symbols provided by libcxx), but the cxxabi_objects_shared component still would need to be built).

Is the current implementation sufficient for your needs?

No - to make it work, I currently apply this diff:

diff --git a/libcxxabi/src/CMakeLists.txt b/libcxxabi/src/CMakeLists.txt
index 52d9c8046f9a..c2cce23c25b2 100644
--- a/libcxxabi/src/CMakeLists.txt
+++ b/libcxxabi/src/CMakeLists.txt
@@ -176,7 +176,7 @@ if (NOT TARGET pstl::ParallelSTL)
 endif()
 
 # Build the shared library.
-if (LIBCXXABI_ENABLE_SHARED)
+if (LIBCXXABI_ENABLE_SHARED OR ((LIBCXX_ENABLE_SHARED OR NOT DEFINED LIBCXX_ENABLE_SHARED) AND LIBCXX_ENABLE_STATIC_ABI_LIBRARY))
   add_library(cxxabi_shared_objects OBJECT ${LIBCXXABI_SOURCES} ${LIBCXXABI_HEADERS})
   target_link_libraries(cxxabi_shared_objects PRIVATE cxx-headers ${LIBCXXABI_SHARED_LIBRARIES} ${LIBCXXABI_LIBRARIES})
   target_link_libraries(cxxabi_shared_objects PUBLIC cxxabi-headers)
@@ -188,7 +188,9 @@ if (LIBCXXABI_ENABLE_SHARED)
       COMPILE_FLAGS "${LIBCXXABI_COMPILE_FLAGS}"
       DEFINE_SYMBOL ""
   )
+endif()
 
+if (LIBCXXABI_ENABLE_SHARED)
   add_library(cxxabi_shared SHARED $<TARGET_OBJECTS:cxxabi_shared_objects>)
   set_target_properties(cxxabi_shared 
     PROPERTIES

I.e. I need to build cxxabi_shared_objects (as those are needed to be merged into cxx_shared) but I can't build cxxabi_shared (because that's impossible to link when Windows shared libraries can't have undefined references).

Or put differently; the thing that really would simplify the situation on Windows, would be to be able to just omit libcxxabi as its own entity altogether, and just optionally include the cxxabi source files as part of libcxx.

phosek added a comment.May 9 2022, 2:57 PM

This version breaks things for me; I'm building with LIBCXX_ENABLE_STATIC_ABI_LIBRARY and LIBCXXABI_USE_LLVM_UNWINDER but not LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY. In this case, there's no shared libcxxabi (I build with LIBCXXABI_ENABLE_SHARED=OFF), so libcxx would need to retain the code to link against the libunwind cmake target, it can't be carried transitively via libcxxabi (cxxabi_shared_objects). I.e. this case gets lost:

if(LIBCXXABI_USE_LLVM_UNWINDER AND NOT LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY)
   target_link_libraries(cxxabi_shared PUBLIC unwind_shared)
 endif()

The part I don't like is referring to LIBCXXABI_USE_LLVM_UNWINDER from within the libcxx build.

No - to make it work, I currently apply this diff:

diff --git a/libcxxabi/src/CMakeLists.txt b/libcxxabi/src/CMakeLists.txt
index 52d9c8046f9a..c2cce23c25b2 100644
--- a/libcxxabi/src/CMakeLists.txt
+++ b/libcxxabi/src/CMakeLists.txt
@@ -176,7 +176,7 @@ if (NOT TARGET pstl::ParallelSTL)
 endif()
 
 # Build the shared library.
-if (LIBCXXABI_ENABLE_SHARED)
+if (LIBCXXABI_ENABLE_SHARED OR ((LIBCXX_ENABLE_SHARED OR NOT DEFINED LIBCXX_ENABLE_SHARED) AND LIBCXX_ENABLE_STATIC_ABI_LIBRARY))
   add_library(cxxabi_shared_objects OBJECT ${LIBCXXABI_SOURCES} ${LIBCXXABI_HEADERS})
   target_link_libraries(cxxabi_shared_objects PRIVATE cxx-headers ${LIBCXXABI_SHARED_LIBRARIES} ${LIBCXXABI_LIBRARIES})
   target_link_libraries(cxxabi_shared_objects PUBLIC cxxabi-headers)
@@ -188,7 +188,9 @@ if (LIBCXXABI_ENABLE_SHARED)
       COMPILE_FLAGS "${LIBCXXABI_COMPILE_FLAGS}"
       DEFINE_SYMBOL ""
   )
+endif()
 
+if (LIBCXXABI_ENABLE_SHARED)
   add_library(cxxabi_shared SHARED $<TARGET_OBJECTS:cxxabi_shared_objects>)
   set_target_properties(cxxabi_shared 
     PROPERTIES

I.e. I need to build cxxabi_shared_objects (as those are needed to be merged into cxx_shared) but I can't build cxxabi_shared (because that's impossible to link when Windows shared libraries can't have undefined references).

I wonder if we could simplify things by always building cxxabi_static_objects and cxxabi_shared_objects but marking them as EXCLUDE_FROM_ALL?

This version breaks things for me; I'm building with LIBCXX_ENABLE_STATIC_ABI_LIBRARY and LIBCXXABI_USE_LLVM_UNWINDER but not LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY. In this case, there's no shared libcxxabi (I build with LIBCXXABI_ENABLE_SHARED=OFF), so libcxx would need to retain the code to link against the libunwind cmake target, it can't be carried transitively via libcxxabi (cxxabi_shared_objects). I.e. this case gets lost:

if(LIBCXXABI_USE_LLVM_UNWINDER AND NOT LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY)
   target_link_libraries(cxxabi_shared PUBLIC unwind_shared)
 endif()

The part I don't like is referring to LIBCXXABI_USE_LLVM_UNWINDER from within the libcxx build.

Yep. But apparently it does work to pass such dependencies along for an object library, within cmake, like this:

if(LIBCXXABI_USE_LLVM_UNWINDER AND NOT LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY)
  target_link_libraries(cxxabi_shared_objects PUBLIC unwind_shared)
endif()

I.e. I need to build cxxabi_shared_objects (as those are needed to be merged into cxx_shared) but I can't build cxxabi_shared (because that's impossible to link when Windows shared libraries can't have undefined references).

I wonder if we could simplify things by always building cxxabi_static_objects and cxxabi_shared_objects but marking them as EXCLUDE_FROM_ALL?

That's probably the best solution for that - thanks!

I wonder if we could simplify things by always building cxxabi_static_objects and cxxabi_shared_objects but marking them as EXCLUDE_FROM_ALL?

That's probably the best solution for that - thanks!

I've realized that this also going to simplify our build so I like that direction. It's not entirely straightforward though because there are currently various parts of the build that assume that either static or shared libcxxabi is being built which need to be reworked.

Since this conflicts heavily with D120727, I proposed a way forward that implements this patch on top of D120727. See D125393

I wonder if we could simplify things by always building cxxabi_static_objects and cxxabi_shared_objects but marking them as EXCLUDE_FROM_ALL?

That's probably the best solution for that - thanks!

I've realized that this also going to simplify our build so I like that direction. It's not entirely straightforward though because there are currently various parts of the build that assume that either static or shared libcxxabi is being built which need to be reworked.

I got everything to build, but there are some open questions. For example, if we only build libcxxabi objects and neither of the libraries, should we skip libcxxabi tests, or shall we run them but use libcxx (if libcxxabi objects are being linked into libcxx)?

Since this conflicts heavily with D120727, I proposed a way forward that implements this patch on top of D120727. See D125393

I'm fine going with your change if we can land it shortly after D120727 since there's other follow up work I'd like to do that I mentioned above.

phosek abandoned this revision.May 13 2022, 10:26 AM

Superseded by D125393.