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

[Petr Hosek via Phabricator via cfe-commits]

phosek 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))
----------------
ldionne wrote:
> 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.
> 
You're right, that's a leftover from previous iterations.


Repository:
  rL LLVM

https://reviews.llvm.org/D49502