[runtimes] Replace LIBCXX_ENABLE_STATIC_ABI_LIBRARY & friends by a new LIBCXX_CXX_ABI choice

[ldionne]

Summary

Instead of having complicated options like LIBCXX_ENABLE_STATIC_ABI_LIBRARY
and LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY, introduce the new
libcxxabi-objects choice of ABI library to allow merging the ABI library
objects into libc++.

Note that the same refactoring can be applied to how libc++abi merges
libunwind's objects, however that can be done as a separate step.

[mstorsjo]

It looks like this patch still lacks compatibility handling for all external users that are setting this option, which need to be able to set the option in this way at least until after the 15.0.0 release.

[ldionne]

In D125683#3515675, @mstorsjo wrote:

It looks like this patch still lacks compatibility handling for all external users that are setting this option, which need to be able to set the option in this way at least until after the 15.0.0 release.

What would you think about erroring out if someone is still passing LIBCXX_STATICALLY_LINK_ABI_IN_{SHARED|STATIC}_LIBRARY? Sure, we can try to support both for a while, but I wonder whether there's any actual benefit to balance the added complexity. Concretely, I suspect this impacts very few people, and the fix is really simple. And if we error out during the CMake step, we don't risk breaking anyone silently.

[mstorsjo]

In D125683#3516883, @ldionne wrote:

In D125683#3515675, @mstorsjo wrote:

It looks like this patch still lacks compatibility handling for all external users that are setting this option, which need to be able to set the option in this way at least until after the 15.0.0 release.

What would you think about erroring out if someone is still passing LIBCXX_STATICALLY_LINK_ABI_IN_{SHARED|STATIC}_LIBRARY? Sure, we can try to support both for a while, but I wonder whether there's any actual benefit to balance the added complexity. Concretely, I suspect this impacts very few people, and the fix is really simple. And if we error out during the CMake step, we don't risk breaking anyone silently.

I guess that's better than silently (well, cmake warns, but it's easy to miss) no longer doing what it used to, but it's not ideal IMO. I do my continuous build testing with a build invocation that also works for the latest release - but with the suggested transition, there's no single build invocation that works for both the main branch and the 14.x release. I guess I could manage though, but I'd prefer a smoother upgrade path.

[ldionne]

In D125683#3516911, @mstorsjo wrote:

I guess that's better than silently (well, cmake warns, but it's easy to miss) no longer doing what it used to, but it's not ideal IMO. I do my continuous build testing with a build invocation that also works for the latest release - but with the suggested transition, there's no single build invocation that works for both the main branch and the 14.x release. I guess I could manage though, but I'd prefer a smoother upgrade path.

Understood. I'll make sure it is backwards compatible.

[ldionne]

Don't remove the old options yet for backwards compatibility.

[ldionne]

clang/cmake/caches/Fuchsia-stage2.cmake

125

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.

[phosek]

clang/cmake/caches/Fuchsia-stage2.cmake

125

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?

[mstorsjo]

libcxx/cmake/caches/MinGW.cmake

3

Unfortunately, setting LIBCXX_CXX_ABI=libcxxabi-objects here in the cache file doesn't have any effect, because the run-buildbot script invokes cmake with a different parameter:

function generate-cmake() {
    generate-cmake-base \
          -DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;libunwind" \
          -DLIBCXX_CXX_ABI=libcxxabi \
          "${@}"
}

A cmake parameter explicitly set on the command line always overrides what's set in a cache file.

libcxxabi/CMakeLists.txt

254 ↗

(On Diff #430051)

The fallback handling (checking LIBCXX_ENABLE_STATIC_ABI_LIBRARY, updating LIBCXX_CXX_ABI based on that) in libcxx doesn't have any effect here, since libcxxabi is processed strictly before libcxx, so here we only see the original values of the options.

However, luckily enough, I've posted D125715 a couple days ago, which lets us get rid of this whole problematic piece of code altogether (together with fixing a couple other issues), now that we have object libraries in palce!

llvm/docs/HowToBuildWindowsItaniumPrograms.rst

159

If the example instructions used to say LIBCXX_ENABLE_STATIC_ABI_LIBRARY=ON before, you should update the LIBCXX_CXX_ABI setting here to libcxxabi-objects too.

[ldionne]

Address some comments (but not @phosek's yet).

[ldionne]

clang/cmake/caches/Fuchsia-stage2.cmake

125

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.

libcxx/cmake/caches/MinGW.cmake

3

I actually don't see why we need to specify the ABI library to use by libc++ manually there, so I'll remove it. It should be just the same for all the CI bots we have anyways, but if we did introduce a CI configuration where we don't want to use libc++abi as our ABI library from libc++, it would make sense not to overwrite it. We are testing the libc++abi code base separately regardless of that setting anyway.

libcxxabi/CMakeLists.txt

254 ↗

(On Diff #430051)

Awesome, thanks! This should go away when I rebase, then.

llvm/docs/HowToBuildWindowsItaniumPrograms.rst

159

Ugh, good catch, thanks. Not sure why I didn't do it that way in the first place.

[phosek]

clang/cmake/caches/Fuchsia-stage2.cmake

125

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.

[ldionne]

clang/cmake/caches/Fuchsia-stage2.cmake

125

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?

[ldionne]

clang/cmake/caches/Fuchsia-stage2.cmake

125

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.

[ldionne]

Rebase onto main.

[phosek]

clang/cmake/caches/Fuchsia-stage2.cmake

125

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.

[ldionne]

clang/cmake/caches/Fuchsia-stage2.cmake

125

Gentle ping -- did you get any time to investigate this?

[phosek]

clang/cmake/caches/Fuchsia-stage2.cmake

125

I did and we are currently hitting a failure related to our libunwind usage. I have created D127528 which should address it.

[ldionne]

clang/cmake/caches/Fuchsia-stage2.cmake

125

Thanks! Since this has been landed, does it mean we can move forward with this patch?