Page MenuHomePhabricator

[llvm] Remove libcxx, libcxxabi and libunwind from supported LLVM_ENABLE_PROJECTS
ClosedPublic

Authored by ldionne on Aug 23 2022, 8:36 AM.

Details

Summary

This is a breaking change. If you were passing one of those three runtimes
in LLVM_ENABLE_PROJECTS, you need to start passing them in LLVM_ENABLE_RUNTIMES
instead. The runtimes in LLVM_ENABLE_RUNTIMES will start being built using
the "bootstrapping build" instead, which means that they will be built
using the just-built Clang. This is usually what you wanted anyway.

If you were using LLVM_ENABLE_PROJECTS=all with the explicit goal of
building these three runtimes, you can now use LLVM_ENABLE_RUNTIMES=all
and these runtimes will be built using the bootstrapping build.

Diff Detail

Event Timeline

ldionne created this revision.Aug 23 2022, 8:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2022, 8:36 AM
ldionne requested review of this revision.Aug 23 2022, 8:36 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
phosek accepted this revision.Aug 23 2022, 11:40 PM

LGTM

bcain added a subscriber: bcain.Aug 30 2022, 9:09 PM
bcain added a comment.EditedAug 30 2022, 9:14 PM

@ldionne do you have an idea when you plan to land this change? I'm pretty sure it won't break our usage but I'd like to test it out to be sure.

I don't see any LLVM_ENABLE_PROJECTS (beyond clang+lld) - only LLVM_ENABLE_RUNTIMES so I think we're ok.

https://github.com/quic/toolchain_for_hexagon/blob/29a926ac283a4afb9dc4d5ae4a0f2c18b6649243/build-toolchain.sh

bcain added a comment.Aug 31 2022, 5:48 AM

@ldionne do you have an idea when you plan to land this change? I'm pretty sure it won't break our usage but I'd like to test it out to be sure.

I don't see any LLVM_ENABLE_PROJECTS (beyond clang+lld) - only LLVM_ENABLE_RUNTIMES so I think we're ok.

https://github.com/quic/toolchain_for_hexagon/blob/29a926ac283a4afb9dc4d5ae4a0f2c18b6649243/build-toolchain.sh

Oh - I see now, LLVM_ENABLE_PROJECTS=libcxx;libcxxabi is used by our downstream toolchain and not this upstream one. If you could see fit to give us ~one week before you land this, that would be appreciated. And if not, well -- it's not as if we weren't warned.

ldionne updated this revision to Diff 457151.Aug 31 2022, 6:45 PM

Rebase onto main.

ldionne added a subscriber: Restricted Project.Aug 31 2022, 6:47 PM

I'm in no hurry to ship this, but 1 week would seem reasonable. I've been trying to make changes in this stack very slowly to give time for folks to react before we've landed a bunch of changes that would make reverting more difficult.

Pinging libc++ vendors for awareness. I'll aim to land this BREAKING CHANGE in roughly 1 week.

In D132324 I also removed the hack in clang/lib/Driver/ToolChains/Linux.cpp, I think you want to do that too?

lkail added a subscriber: lkail.Sep 11 2022, 7:03 PM

In D132324 I also removed the hack in clang/lib/Driver/ToolChains/Linux.cpp, I think you want to do that too?

I think it makes sense to do that separately as a (in theory) NFC patch.

Ericson2314 accepted this revision.Sep 13 2022, 7:37 PM

OK. As long as we don't forget about it :).

This revision was not accepted when it landed; it landed in state Needs Review.Sep 20 2022, 8:13 AM
This revision was automatically updated to reflect the committed changes.

It looks like this change took down our Sphinx publication bot: https://lab.llvm.org/buildbot/#/builders/89/builds/33379 -- @tonic are you able to help resolve this?

Adding @andreil99 as he can help with the sphinx docs.

Do you want me to revert? I engineered this so as to make it easy to revert if needed.

I can fix the buildbot if you confirm that both Sphinx and Doxygen documentation gets built with LLVM_ENABLE_RUNTIMES.

Btw, it looks like this hit the lldb doc building bot as well: https://lab.llvm.org/buildbot/#/builders/31/builds/28249

I can fix the buildbot if you confirm that both Sphinx and Doxygen documentation gets built with LLVM_ENABLE_RUNTIMES.

This is what I'm seeing on the lldb bot output, which suggests we don't enable LLVM_ENABLE_RUNTIMES currently:

argv: ['cmake', '../src/llvm', '-DLLVM_ENABLE_SPHINX=ON', '-DSPHINX_OUTPUT_HTML=ON', '-DSPHINX_OUTPUT_MAN=ON', '-DLLDB_INCLUDE_TESTS=OFF', '-DLLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN=ON', '-DLLVM_ENABLE_ASSERTIONS=OFF', '-DLLVM_ENABLE_PROJECTS=llvm;libunwind', '-DCMAKE_BUILD_TYPE=Release', '-DLLVM_LIT_ARGS=-v -vv', '-GNinja']

and the publish bot uses:

cmake ../llvm-project/llvm -DLLVM_ENABLE_SPHINX=ON -DSPHINX_OUTPUT_HTML=ON -DSPHINX_OUTPUT_MAN=OFF -DSPHINX_WARNINGS_AS_ERRORS=OFF -DLLVM_ENABLE_ASSERTIONS=OFF -DCMAKE_BUILD_TYPE=Release '-DLLVM_ENABLE_PROJECTS=libc;clang-tools-extra;libcxx;lld;libunwind;openmp;clang;llvm;polly;lldb;libcxxabi;flang' '-DLLVM_LIT_ARGS=-v -vv' -GNinja
``

Do you want me to revert? I engineered this so as to make it easy to revert if needed.

I don't have strong feelings; I think we can live for a day with one-day-out-of-date docs up on the web. But if @andreil99 would like the extra time to look into it, I'm also totally fine with reverting.

This hits all the doc building bots on the exact same way.

The fix is trivial, assuming documentation builds properly with LLVM_ENABLE_RUNTIMES. It didn't support that before.
I cannot check that quickly, unfortunately. So, if somebody can check and confirm, that would help a lot.

This hits all the doc building bots on the exact same way.

The fix is trivial, assuming documentation builds properly with LLVM_ENABLE_RUNTIMES. It didn't support that before.
I cannot check that quickly, unfortunately. So, if somebody can check and confirm, that would help a lot.

It should, give me a second to confirm that on a Linux docker image.

If we try to build docs with LLVM_ENABLE_RUNTIMES might it try to bootstrap them, wasting effort?

That was my concern when I hit this before. TBH I feel putting them under LLVM_ENABLE_PROJECTS while silly, did express well that everything can be built in parallel (modulo "regular" header dependencies like Clang -> LLVM) without bootstrapping being needed.

If we try to build docs with LLVM_ENABLE_RUNTIMES might it try to bootstrap them, wasting effort?

No, it won't.

I'm still working on this, I'll update this thread tomorrow. If I don't manage to get this working tomorrow morning, I'll revert this.

If we try to build docs with LLVM_ENABLE_RUNTIMES might it try to bootstrap them, wasting effort?

No, it won't.

I'm still working on this, I'll update this thread tomorrow. If I don't manage to get this working tomorrow morning, I'll revert this.

I'm going deeper and deeper into the rabbit hole and as I gain more understanding of how the bootstrapping build works, this is not looking positive. Basically, my understanding is that we *have* to build Clang (and compiler-rt) before we even configure the other runtimes, because that's required for their CMake configuration to work properly. When we configure the runtimes like libcxx, CMake performs various compiler checks (like is -fsomethingsomething available), and that obviously requires having the compiler built already. When I think about it, it makes sense: the documentation could even technically include different content based on the compiler we're building libc++ for (although we don't do that and I think it'd be a bad idea).

This is obviously wasted effort in the context of generating only our documentation. Currently, the only way forward I can think of is to change the documentation jobs to perform two CMake invocations: first build the documentation with an invocation rooted at llvm and using LLVM_ENABLE_PROJECTS=clang;lld;etc, and then build the documentation for the runtimes with an invocation rooted at <root>/runtimes using LLVM_ENABLE_RUNTIMES=libcxx;libcxxabi;etc. I'm going to revert this change temporarily as I look at how to make these changes.

That was my concern when I hit this before. TBH I feel putting them under LLVM_ENABLE_PROJECTS while silly, did express well that everything can be built in parallel (modulo "regular" header dependencies like Clang -> LLVM) without bootstrapping being needed.

Yeah, I get you. That being said, there were a lot of problems with supporting that, for example the fact that llvm/CMakeLists.txt creates a lot of variables and selects a lot of defaults that don't make sense for the runtimes. I had documented that in some of the posts I made when we agreed to move the runtimes out of LLVM_ENABLE_PROJECTS a few years back.

I'm going deeper and deeper into the rabbit hole and as I gain more understanding of how the bootstrapping build works, this is not looking positive.

Thank you for doing the research here, and thank you for the revert while continuing to investigate!

ldionne reopened this revision.Sep 21 2022, 6:50 AM
ldionne updated this revision to Diff 464975.Oct 4 2022, 5:25 AM

Rebase onto main.

@andreil99 Are the sphinx documentation publishing changes live (so I can commit this again)?

ldionne updated this revision to Diff 464977.Oct 4 2022, 5:31 AM

Rebase again.

@andreil99 Are the sphinx documentation publishing changes live (so I can commit this again)?

Correct.

ldionne accepted this revision as: Restricted Project.Oct 4 2022, 6:02 AM
This revision is now accepted and ready to land.Oct 4 2022, 6:02 AM
This revision was landed with ongoing or failed builds.Oct 4 2022, 6:04 AM
This revision was automatically updated to reflect the committed changes.

It looks like the libunwind builder is broken by these changes: https://lab.llvm.org/buildbot/#/builders/31/builds/28817

@ldionne Yeah that sort of thing is what I suspected. I am thinking maybe it isn't so bad that these projects can be built this way. IMO it should be permissible to combine any two CMake projects together if they are just to be built in parallel without any dependencies between them.

It looks like the libunwind builder is broken by these changes: https://lab.llvm.org/buildbot/#/builders/31/builds/28817

This is on my plate. I’ll commit the fix shortly. It will pick up at the next buildbot restart.