This is an archive of the discontinued LLVM Phabricator instance.

[release] Use the Bootstrapping build for building LLVM releases
ClosedPublic

Authored by ldionne on Oct 28 2021, 12:39 PM.

Details

Summary

Instead of using the (now deprecated) Projects build for libcxx, libcxxabi
and libunwind, this patch uses the Bootstrapping build. This implies that
Clang will be built from scratch, and then the runtimes will be built using
that just-built Clang instead of the system compiler. This is the correct
way of assembling a toolchain, since we don't want to ship runtimes that
were built with a non-Clang compiler (or a potentially older Clang).

Diff Detail

Unit TestsFailed

Event Timeline

ldionne requested review of this revision.Oct 28 2021, 12:39 PM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2021, 12:39 PM

@phosek With this patch as-is, we end up not building the runtimes at all because install-runtimes isn't executed when we run ninja -C build. I *think* that's because install-runtimes is not added to the ALL target in CMake. Is that voluntary or is it an oversight?

Also, @tsellar, do you want the LLVM releases to be built using the just-built Clang, or the system compiler? IMO it makes sense to use the bootstrapping build, but if we want to build the Phase 1 runtimes using the system compiler, we could use two CMake invocations, one rooted at <monorepo>/llvm and one rooted at <monorepo>/runtimes. This patch assumes that we want to bootstrap Clang and build the runtimes with it at each phase.

Also, @tsellar, do you want the LLVM releases to be built using the just-built Clang, or the system compiler? IMO it makes sense to use the bootstrapping build, but if we want to build the Phase 1 runtimes using the system compiler, we could use two CMake invocations, one rooted at <monorepo>/llvm and one rooted at <monorepo>/runtimes. This patch assumes that we want to bootstrap Clang and build the runtimes with it at each phase.

Using the bootstrap build seems fine with me. Is the phase1 runtime build used in phase2 or phase3 or is it just discarded?

ychen added a subscriber: ychen.Oct 28 2021, 1:42 PM

Also, @tsellar, do you want the LLVM releases to be built using the just-built Clang, or the system compiler? IMO it makes sense to use the bootstrapping build, but if we want to build the Phase 1 runtimes using the system compiler, we could use two CMake invocations, one rooted at <monorepo>/llvm and one rooted at <monorepo>/runtimes. This patch assumes that we want to bootstrap Clang and build the runtimes with it at each phase.

Using the bootstrap build seems fine with me. Is the phase1 runtime build used in phase2 or phase3 or is it just discarded?

In the current setup, it would be discarded. I *think* both alternatives are viable -- perhaps it would be better to build the Phase2/3 Clang against the previous phase's runtime libraries, but I think it would be fine to do this as a follow-up since right now we never use the previous phase's runtimes IIUC. Is that correct?

Note that for the above to be correct, we have to fix the fact that the runtimes aren't built at all with this patch as-is, see my question to @phosek above. We won't move forward with this change until it has been addressed, of course.

Also, @tsellar, do you want the LLVM releases to be built using the just-built Clang, or the system compiler? IMO it makes sense to use the bootstrapping build, but if we want to build the Phase 1 runtimes using the system compiler, we could use two CMake invocations, one rooted at <monorepo>/llvm and one rooted at <monorepo>/runtimes. This patch assumes that we want to bootstrap Clang and build the runtimes with it at each phase.

Using the bootstrap build seems fine with me. Is the phase1 runtime build used in phase2 or phase3 or is it just discarded?

In the current setup, it would be discarded. I *think* both alternatives are viable -- perhaps it would be better to build the Phase2/3 Clang against the previous phase's runtime libraries, but I think it would be fine to do this as a follow-up since right now we never use the previous phase's runtimes IIUC. Is that correct?

Yes, we can make other changes here in a a follow up change if we want.

@phosek With this patch as-is, we end up not building the runtimes at all because install-runtimes isn't executed when we run ninja -C build. I *think* that's because install-runtimes is not added to the ALL target in CMake. Is that voluntary or is it an oversight?

That's because nobody ever asked for it. From what I know, all users of the bootstrapping build use the distribution support.

I'm fine including install-runtimes in the ALL target, but I also think that releases should ideally be using distribution support.

@phosek With this patch as-is, we end up not building the runtimes at all because install-runtimes isn't executed when we run ninja -C build. I *think* that's because install-runtimes is not added to the ALL target in CMake. Is that voluntary or is it an oversight?

That's because nobody ever asked for it. From what I know, all users of the bootstrapping build use the distribution support.

I'm fine including install-runtimes in the ALL target, but I also think that releases should ideally be using distribution support.

Oh, well, in that case, I think we should definitely be using the distribution support. @phosek any appetite for commandeering this to migrate test-release.sh to a proper distribution build?

amyk added a subscriber: amyk.Dec 7 2021, 2:16 PM

Thanks for putting up this patch (and taking a look at the revision I've put up). I've left a question on this patch to keep the conversation more contained.

llvm/utils/release/test-release.sh
241

I was initially thinking that perhaps maybe we can opt for a hybrid of this idea and my idea in https://reviews.llvm.org/D115105 because I can see adding a new option for users to specifically control which projects people want to build as a part of runtimes, but then I realize, we're doing it this way where we just add to runtimes because the plan going forward is to only support building libcxx/libcxxabi/libunwind as a part of runtimes, right?

Since compiler-rt can also be built a part of runtimes, would it still make sense to also add this a part of runtimes?
Unless if we introduce a new option to add the ability to build compiler-rt as apart of runtimes?

ldionne added inline comments.Dec 7 2021, 2:46 PM
llvm/utils/release/test-release.sh
241

we're doing it this way where we just add to runtimes because the plan going forward is to only support building libcxx/libcxxabi/libunwind as a part of runtimes, right?

Not exactly, but almost. The plan going forward is to

  1. support building libcxx/libcxxabi/libunwind as part of the Bootstrapping build (i.e. using LLVM_ENABLE_RUNTIMES and using <monorepo>/llvm as the CMake root). This is what's used by test-release.sh in my patch and your patch.
  2. support building libcxx/libcxxabi/libunwind using LLVM_ENABLE_RUNTIMES but with <monorepo>/runtimes as the CMake root. This isn't used by test-release.sh and I don't think we want to do this.

The way I understand it, people use test-release.sh basically to build and release a toolchain, and the bootstrapping build is the most suited for that job. So I don't think it makes sense for test-release.sh to support using build (2) for libcxx/libcxxabi/libunwind. For the same reason, it made sense to me to automatically route projects to LLVM_ENABLE_RUNTIMES instead of LLVM_ENABLE_PROJECTS so as to avoid breaking anything that uses the script.

But as I said, I don't really mind whatever we do. If @tstellar and others are happy to update their scripts that call test-release.sh to pass -runtimes instead of everything inside -projects, I'm happy.

Since compiler-rt can also be built a part of runtimes, would it still make sense to also add this a part of runtimes?

Hmm, that definitely seems like an oversight in my patch. I'll update it in case we end up going with that.

ldionne updated this revision to Diff 392547.Dec 7 2021, 2:49 PM

Add compiler-rt to LLVM_ENABLE_RUNTIMES too

phosek added a comment.Dec 8 2021, 9:29 AM

@phosek With this patch as-is, we end up not building the runtimes at all because install-runtimes isn't executed when we run ninja -C build. I *think* that's because install-runtimes is not added to the ALL target in CMake. Is that voluntary or is it an oversight?

That's because nobody ever asked for it. From what I know, all users of the bootstrapping build use the distribution support.

I'm fine including install-runtimes in the ALL target, but I also think that releases should ideally be using distribution support.

Oh, well, in that case, I think we should definitely be using the distribution support. @phosek any appetite for commandeering this to migrate test-release.sh to a proper distribution build?

@ldionne That's going to be a larger change so I'd prefer starting with this one and switching to distribution build as a follow up change.

@ldionne That's going to be a larger change so I'd prefer starting with this one and switching to distribution build as a follow up change.

Ok, sounds good to me. I do have a question for you though -- is there a way to enable CMAKE_POSITION_INDEPENDENT_CODE=ON for all targets of the bootstrapping build at once? This is going to be needed once I rebase onto main.

amyk added inline comments.Dec 10 2021, 5:29 PM
llvm/utils/release/test-release.sh
241

Thanks @ldionne for explaining! I just noticed, but I think you also probably meant this in your latest update of this patch?

ldionne added inline comments.Dec 13 2021, 6:47 AM
llvm/utils/release/test-release.sh
241

Geez, thank you.

ldionne updated this revision to Diff 393877.Dec 13 2021, 6:49 AM

Rebase onto main and fix bug with compiler-rt.

amyk added inline comments.Jan 26 2022, 10:26 AM
llvm/utils/release/test-release.sh
241

I initially missed this, but thank you for updating/addressing the review comment! :-)

A quick question I had was, with LLVM 14 approaching soon, will this method be the way we build for the upcoming release?

ldionne updated this revision to Diff 403397.Jan 26 2022, 1:42 PM
ldionne marked an inline comment as done.

Rebase onto main. Gentle ping!

llvm/utils/release/test-release.sh
241

Yes, I hope so. I am hoping to get some attention on this patch. @tstellar , I'm sure you're very busy with the release, but could you help me figure out how to properly test this? It would be great to build the runtimes in LLVM 14 the way we document it.

amyk added a comment.Feb 1 2022, 2:58 PM

Rebase onto main. Gentle ping!

👍🏻
I'll also be starting the release testing process on the AIX platform when LLVM 14 rolls around. I've tested this patch previously on both AIX and Linux for PowerPC previously on LLVM 13 and can confirm that this patch works for Power.
Actually, on AIX in particular, this patch is actually needed and is the only way I'm able to build using test-release.sh on AIX, so I agree it would be great if we could build in this way for LLVM 14. @tstellar

Okay, I ran this on my mac and it seems to work. I also looked at the resulting LLVM release, and it looked normal (everything seemed to be in the right place in /usr/local/bin and /usr/local/include).

tstellar accepted this revision.Feb 2 2022, 2:12 PM

LGTM.

This revision is now accepted and ready to land.Feb 2 2022, 2:12 PM

Thanks! I'm going to ship this and cherry-pick onto the 14 release branch, as discussed on Discord. Hopefully there shouldn't be any issues. If there are, I guess we'll find out when the testers start testing the release.

In essence, this is a simple change but it's actually pretty wide reaching, since we're fundamentally changing how libc++ is being built (with the just-built Clang instead of the system compiler). This is supposed to be more correct, however it's possible that there's other unintended consequences. I don't think it should, but if it does and if that jeopardizes the release, we can always revert to give us more time to investigate.

This revision was landed with ongoing or failed builds.Feb 2 2022, 2:19 PM
This revision was automatically updated to reflect the committed changes.