[libcxx-commits] [PATCH] D60276: [libc++] Localize CMake code only related to the shared library

Thu Apr 4 12:45:28 PDT 2019

ldionne added inline comments.

================
Comment at: libcxx/lib/CMakeLists.txt:179
+    else()
+      target_link_libraries(cxx_shared PRIVATE "-Wl,--whole-archive" "-Wl,-Bstatic" "$<TARGET_LINKER_FILE:${LIBCXX_CXX_STATIC_ABI_LIBRARY}>" "-Wl,-Bdynamic" "-Wl,--no-whole-archive")
+    endif()
----------------
smeenai wrote:
> Same comment here:
> 
> ```
>       target_link_libraries(cxx_shared PRIVATE "-Wl,--whole-archive,-Bstatic,$<TARGET_LINKER_FILE:${LIBCXX_CXX_STATIC_ABI_LIBRARY}>,-Bdynamic,--no-whole-archive")
> ```
> 
> (or separated out)
> 
> ```
>       target_link_libraries(cxx_shared PRIVATE "-Wl,--whole-archive -Wl,-Bstatic $<TARGET_LINKER_FILE:${LIBCXX_CXX_STATIC_ABI_LIBRARY}> -Wl,-Bdynamic -Wl,--no-whole-archive")
> ```
Note that this requires re-adding the `add_dependencies` call, because CMake doesn't see the implicit dependency anymore.

================
Comment at: libcxx/lib/CMakeLists.txt:184
+  endif()
+  list(APPEND LIBCXX_INTERFACE_LIBRARIES "${LIBCXX_CXX_STATIC_ABI_LIBRARY}")
+
----------------
smeenai wrote:
> I believe this should be part of the `else()` above.
You're right. My intent was that the linker script would work too when statically linking libc++abi into libc++, but actually it's the other way around. When you statically link libc++abi.a into libc++.so, you don't want the linker script to try to link against libc++abi.

================
Comment at: libcxx/lib/CMakeLists.txt:201
+      "-compatibility_version" "1"
+      "-install_name" "/usr/lib/libc++.1.dylib"
+      "-Wl,-unexported_symbols_list,${CMAKE_CURRENT_SOURCE_DIR}/libc++unexp.exp"
----------------
smeenai wrote:
> Same here
Note that both used to be, but I had to change it when I was using `target_link_options` because the quoting was different.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60276