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).
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
50 ms | x64 debian > LLVM.Bindings/Go::go.test |
Event Timeline
@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.
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.
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?
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? |
llvm/utils/release/test-release.sh | ||
---|---|---|
241 |
Not exactly, but almost. The plan going forward is to
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.
Hmm, that definitely seems like an oversight in my patch. I'll update it in case we end up going with that. |
@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.
llvm/utils/release/test-release.sh | ||
---|---|---|
241 | Geez, thank you. |
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? |
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. |
👍🏻
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).
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.
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?