[libcxx-commits] [PATCH] D116689: [libunwind][libcxxabi] Use object libraries in the build

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D116689#3306286 <https://reviews.llvm.org/D116689#3306286>, @phosek wrote:

> In D116689#3223513 <https://reviews.llvm.org/D116689#3223513>, @smeenai wrote:
>
>> 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 :-(.



================
Comment at: libcxx/src/CMakeLists.txt:233
+    elseif (LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY AND (TARGET unwind_shared_objects OR HAVE_LIBUNWIND))
+      target_link_libraries(cxx_shared PUBLIC unwind_shared_objects)
     else()
----------------
compnerd wrote:
> Why not use `TARGET_OBJECTS:unwind>` instead?
I have the same question.


================
Comment at: libcxx/src/CMakeLists.txt:319
+  if (LIBCXXABI_USE_LLVM_UNWINDER)
+    if (NOT LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_STATIC_LIBRARY AND (TARGET unwind_shared OR HAVE_LIBUNWIND))
+      target_link_libraries(cxx_static PUBLIC unwind_shared)
----------------
Do we really mean to use `LIBCXXABI_foo` variables here?


================
Comment at: libcxx/src/CMakeLists.txt:340
+
+      set(MERGE_ARCHIVES_SEARCH_PATHS "")
+      if (LIBCXX_CXX_ABI_LIBRARY_PATH)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116689