This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ldionne on May 16 2022, 5:54 AM.

Details

Reviewers
phosek
mstorsjo
nikic
Group Reviewers
Restricted Project
Restricted Project
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.

Diff Detail

Event Timeline

ldionne created this revision.May 16 2022, 5:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2022, 5:54 AM
ldionne requested review of this revision.May 16 2022, 5:54 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptMay 16 2022, 5:54 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, libcxx-commits, Restricted Project, cfe-commits. · View Herald Transcript

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.

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.

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.

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 updated this revision to Diff 430051.May 17 2022, 7:17 AM

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

ldionne added inline comments.May 17 2022, 7:18 AM
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 added inline comments.May 17 2022, 11:57 PM
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 added inline comments.May 18 2022, 3:19 AM
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 updated this revision to Diff 430779.May 19 2022, 12:52 PM
ldionne marked 3 inline comments as done.

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

ldionne added inline comments.May 19 2022, 12:52 PM
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 added inline comments.May 20 2022, 12:11 AM
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 added inline comments.May 20 2022, 6:43 AM
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 added inline comments.May 27 2022, 2:04 PM
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 updated this revision to Diff 432634.May 27 2022, 2:05 PM

Rebase onto main.

phosek added inline comments.May 27 2022, 2:09 PM
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 added inline comments.Jun 9 2022, 10:12 AM
clang/cmake/caches/Fuchsia-stage2.cmake
125

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

phosek added inline comments.Jun 13 2022, 3:22 PM
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 added inline comments.Jun 17 2022, 2:46 PM
clang/cmake/caches/Fuchsia-stage2.cmake
125

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

phosek accepted this revision.Jul 1 2022, 10:35 AM

LGTM

clang/cmake/caches/Fuchsia-stage2.cmake
122

Would it be possible to add set(RUNTIMES_${target}_LIBCXX_CXX_ABI "libcxxabi-objects" CACHE STRING "") here?

189

Would it be possible to add set(RUNTIMES_${target}_LIBCXX_CXX_ABI "libcxxabi-objects" CACHE STRING "") here?

ldionne updated this revision to Diff 442549.Jul 6 2022, 6:33 AM
ldionne marked 2 inline comments as done.

Rebase and address review comments.

nikic requested changes to this revision.Sep 23 2022, 7:05 AM
nikic added a subscriber: nikic.

I have some concerns about this change:

  • It makes it fundamentally impossible to link against a static system-libcxxabi (or any other LIBCXX_CXX_ABI option). This is currently already broken (https://github.com/llvm/llvm-project/issues/57759), but used to work prior to LLVM 15.
  • It makes it fundamentally impossible to use a linker script for the shared library while linking the abi library into the static library. I think this currently doesn't work either, but would be trivial to support. (I was going to work on this when I found this revision.)

I think both of these points highlight that LIBCXX_CXX_ABI=libcxxabi-objects is not the correct representation for this functionality.

This revision now requires changes to proceed.Sep 23 2022, 7:05 AM

I have some concerns about this change:

  • It makes it fundamentally impossible to link against a static system-libcxxabi (or any other LIBCXX_CXX_ABI option). This is currently already broken (https://github.com/llvm/llvm-project/issues/57759), but used to work prior to LLVM 15.
  • It makes it fundamentally impossible to use a linker script for the shared library while linking the abi library into the static library. I think this currently doesn't work either, but would be trivial to support. (I was going to work on this when I found this revision.)

I think both of these points highlight that LIBCXX_CXX_ABI=libcxxabi-objects is not the correct representation for this functionality.

I don't think these limitations are an inherent property of this approach. What I'm trying to do with this change is eliminate these boolean flags that control how we build the library to instead use a more open-ended selection mechanism that is more flexible, more expressive, and which reduces the need for if(....) within the build.

I think the fundamental problem we have is that the ABI library for the static libc++ and for the shared libc++ are selected in one go. If we could instead specify the choice of the ABI library for each independently (for example -DLIBCXX_CXX_SHARED_ABI=libcxxabi -DLIBCXX_CXX_STATIC_ABI=system-libcxxabi), would that solve your problem? Also, this would actually increase the flexibility of the current build by a significant margin without much complexity on the libc++ CMake side.

I have some concerns about this change:

  • It makes it fundamentally impossible to link against a static system-libcxxabi (or any other LIBCXX_CXX_ABI option). This is currently already broken (https://github.com/llvm/llvm-project/issues/57759), but used to work prior to LLVM 15.
  • It makes it fundamentally impossible to use a linker script for the shared library while linking the abi library into the static library. I think this currently doesn't work either, but would be trivial to support. (I was going to work on this when I found this revision.)

I think both of these points highlight that LIBCXX_CXX_ABI=libcxxabi-objects is not the correct representation for this functionality.

I don't think these limitations are an inherent property of this approach. What I'm trying to do with this change is eliminate these boolean flags that control how we build the library to instead use a more open-ended selection mechanism that is more flexible, more expressive, and which reduces the need for if(....) within the build.

I think the fundamental problem we have is that the ABI library for the static libc++ and for the shared libc++ are selected in one go. If we could instead specify the choice of the ABI library for each independently (for example -DLIBCXX_CXX_SHARED_ABI=libcxxabi -DLIBCXX_CXX_STATIC_ABI=system-libcxxabi), would that solve your problem? Also, this would actually increase the flexibility of the current build by a significant margin without much complexity on the libc++ CMake side.

This would indeed address the second point, but not the first. For that, one would have to add additional variations like system-libcxxabi-objects, which would end up increasing the possible abi choices by a factor of two (unless static linking doesn't make sense for some of them).