ldionne added inline comments.
================ Comment at: clang/cmake/caches/Fuchsia-stage2.cmake:124 set(RUNTIMES_${target}_LIBCXX_ENABLE_SHARED OFF CACHE BOOL "") - set(RUNTIMES_${target}_LIBCXX_ENABLE_STATIC_ABI_LIBRARY ON CACHE BOOL "") set(RUNTIMES_${target}_LIBCXX_ABI_VERSION 2 CACHE STRING "") ---------------- phosek wrote: > ldionne wrote: > > phosek wrote: > > > ldionne wrote: > > > > ldionne wrote: > > > > > phosek wrote: > > > > > > ldionne wrote: > > > > > > > phosek wrote: > > > > > > > > ldionne wrote: > > > > > > > > > Note that I am removing these options here because I don't > > > > > > > > > think they are required -- since we specify > > > > > > > > > `LIBCXXABI_ENABLE_SHARED=OFF`, there is no shared libc++abi > > > > > > > > > to link against, so we should already be linking against > > > > > > > > > `libc++abi.a` regardless of this change. > > > > > > > > This option was set to merge `libc++abi.a` into `libc++.a` so > > > > > > > > to achieve the same effect, presumably we would need to set > > > > > > > > `-DLIBCXX_CXX_ABI=libcxxabi-objects`? > > > > > > > I agree this is suspicious, but why is there no > > > > > > > `LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY` specified here > > > > > > > then? I can add `-DLIBCXX_CXX_ABI=libcxxabi-objects`, I just want > > > > > > > to make sure we both understand what's going on. > > > > > > This is intentional. We are merging `libc++abi.a` into `libc++.a` > > > > > > but we ship `libc++abi.so` and `libc++.so` as separate (and use the > > > > > > generated linker script to pull in `libc++abi.so` when you pass > > > > > > `-lc++` to linker). I'd be fine merging `libc++abi.so` into > > > > > > `libc++.so` as well, but we'll need to figure out a transition plan > > > > > > since there are several places in our build right now that expect > > > > > > `libc++abi.so` to exist. We cannot land this change as is because > > > > > > that would break the `-static-libstdc++` use case, since Clang > > > > > > driver only passes `-lc++` to the linker and not `-lc++abi` and > > > > > > there's nothing that would pull `libc++abi.a` in. > > > > > I see, so to summarize, basically you want to use `libcxxabi-objects` > > > > > for the static `libc++.a`, but `libcxxabi` for the dynamic > > > > > `libc++.so`. This change as currently laid out does not permit that > > > > > to happen, since `libcxxabi-objects` implies that the objects are > > > > > merged both in the static and in the shared library. I guess we could > > > > > introduce a new `libcxxabi-objects-static` option, however that would > > > > > be kind of strange. We can either do that, or wait for you to stop > > > > > relying on `libc++abi.so` existing and switch to `libcxxabi-objects` > > > > > wholesale for Fuchsia. WDYT? > > > > Gentle ping. It would be nice to land this in one form or another. If > > > > you don't think Fuchsia can stop relying on `libc++abi.so` being there, > > > > I could add yet another `libcxxabi-objects-static` -- I still feel like > > > > that's better than the status quo. > > > We discussed this on our team and we don't have any issues switching > > > towards combined `libc++.so`, we just need to figure out a transition > > > plan. Let me test this change locally to see if landing this as is will > > > break anything. > > Gentle ping -- did you get any time to investigate this? > I did and we are currently hitting a failure related to our libunwind usage. > I have created D127528 which should address it. Thanks! Since this has been landed, does it mean we can move forward with this patch? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125683/new/ https://reviews.llvm.org/D125683 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits