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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
