[PATCH] D49502: [CMake] Support statically linking dependencies only to shared or static library

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.


================
Comment at: libcxx/lib/CMakeLists.txt:269
+        AND (TARGET cxxabi_static OR HAVE_LIBCXXABI))
+    #if ((TARGET ${LIBCXX_CXX_ABI_LIBRARY}) OR
+        #(${LIBCXX_CXX_ABI_LIBRARY} MATCHES "cxxabi(_static|_shared)?" AND HAVE_LIBCXXABI))
----------------
phosek wrote:
> ldionne wrote:
> > phosek wrote:
> > > ldionne wrote:
> > > > I don't understand why any of this needs to change -- can you please explain? Also, you probably didn't mean to leave the commented-out lines.
> > > The reason this change is needed the case when we're linking shared libc++abi into shared libc++ in which case `${LIBCXX_CXX_ABI_LIBRARY}` will be set to `cxxabi_shared` in `HandleLibCXXABI.cmake` but we cannot merge `libc++abi.so` into `libc++.a`, so instead we force the use of `cxxabi_static` here.
> > > 
> > > Alternatively, we could modify `HandleLibCXXABI.cmake` to set two dependencies, one for the static case and one for the shared case and use the former one here.
> > > 
> > > Removed the commented out lines.
> > Thanks. There's something I still don't understand. If you are linking the ABI library dynamically, why would you want to merge it (well, the static version of it) into `libc++.a`? It seems like this is somewhat defeating the purpose of dynamically linking against the ABI library, no?
> In our case, we want to link shared library dynamically and merge all static libraries, so `LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY=OFF` and `LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY=ON`. This means that `LIBCXX_CXX_ABI_LIBRARY` will be set to `cxxabi_shared` and it's what will be used other places throughout this file as the interface library. However, we cannot merge `libc++abi.so` into `libc++.a` so that's why we have to explicitly select `cxxabi_static` here rather than simply using `LIBCXX_CXX_ABI_LIBRARY` as before.
> 
> Like I mentioned in the previous comment, alternative would be to split this into two variables, e.g. `LIBCXX_CXX_SHARED_ABI_LIBRARY` and `LIBCXX_CXX_STATIC_ABI_LIBRARY`, would you prefer that approach?
Yes, I understand why the `TARGET_LINKER_FILE` must also be `cxxabi_static`. What I don't understand is why the conditions need to change, i.e. why you're going from
```
if (TARGET ${LIBCXX_CXX_ABI_LIBRARY} OR
    (${LIBCXX_CXX_ABI_LIBRARY} MATCHES "cxxabi(_static|_shared)?" AND HAVE_LIBCXXABI))
```

to 

```
if (${LIBCXX_CXX_ABI_LIBRARY} MATCHES "cxxabi(_static|_shared)?"
        AND (TARGET cxxabi_static OR HAVE_LIBCXXABI))
```

Note that I do think your new condition make more sense (since if the ABI library is not a target, we should fall back on the `else()` branch). I just want to understand why you're changing it.

> Like I mentioned in the previous comment, alternative would be to split this into two variables, e.g. `LIBCXX_CXX_SHARED_ABI_LIBRARY` and `LIBCXX_CXX_STATIC_ABI_LIBRARY`, would you prefer that approach?

I don't think that is necessary at this point.



================
Comment at: libcxxabi/src/CMakeLists.txt:64
   # FIXME: Is it correct to prefer the static version of libunwind?
   if (NOT LIBCXXABI_ENABLE_STATIC_UNWINDER AND (TARGET unwind_shared OR HAVE_LIBUNWIND))
     list(APPEND LIBCXXABI_LIBRARIES unwind_shared)
----------------
phosek wrote:
> ldionne wrote:
> > Does this not need to change anymore? I think we'd have to set different flags for `cxxabi_shared` and `cxxabi_static`.
> > 
> This is relying on the fact that `LIBCXXABI_LIBRARIES` isn't used for library merging that's done on lines 158-162 and CMake should ignore shared library dependencies passed to `target_link_libraries` when building static library as done on line 163 (since linking dynamic library target against static library doesn't make sense). This seems to be working fine in my testing, but I'm also OK splitting this into two variables if you want it to be explicit.
I would rather see it handled explicitly.


Repository:
  rCXX libc++

https://reviews.llvm.org/D49502