Make Rustc build with LLVM trunk. #55751
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
src/rustllvm/RustWrapper.cpp
Outdated
| llvm::DIGlobalVariableExpression *VarExpr = Builder->createGlobalVariableExpression( | ||
| unwrapDI<DIDescriptor>(Context), Name, LinkageName, | ||
| unwrapDI<DIFile>(File), LineNo, unwrapDI<DIType>(Ty), IsLocalToUnit, | ||
| InitExpr, unwrapDIPtr<MDNode>(Decl), nullptr, AlignInBits); |
There was a problem hiding this comment.
In other cases where only one argument out of many changed between versions, we put that argument on a separate line so we can version-gate it without copying the rest of the call. I'd like to see this happen here too, see LLVMRustDIBuilderCreatePointerType for an example.
| @@ -796,7 +796,11 @@ extern "C" LLVMMetadataRef LLVMRustDIBuilderCreateStaticVariable( | |||
| llvm::DIGlobalVariableExpression *VarExpr = Builder->createGlobalVariableExpression( | |||
| unwrapDI<DIDescriptor>(Context), Name, LinkageName, | |||
| unwrapDI<DIFile>(File), LineNo, unwrapDI<DIType>(Ty), IsLocalToUnit, | |||
| InitExpr, unwrapDIPtr<MDNode>(Decl), AlignInBits); | |||
| InitExpr, unwrapDIPtr<MDNode>(Decl), | |||
| #if LLVM_VERSION_GE(8, 0) | |||
There was a problem hiding this comment.
This is going to break the build against rust's LLVM fork, which is also LLVM 8, just an older LLVM 8. This could be fixed either by updating the fork to current LLVM trunk, or adding a && !LLVM_RUSTLLVM here.
There was a problem hiding this comment.
But then wouldn't you need to revert that extra condition when the rust fork will be resynced with the latest llvm?
There was a problem hiding this comment.
Which do you prefer guys? I'm a little nervous about updating your rust LLVM fork in case I break something.
There was a problem hiding this comment.
@nikic is right in that this will require a condition on LLVM_RUSTLLVM to land, and either doing that or updating LLVM is fine. It's probably much less work to set LLVM_RUSTLLVM than it is to update LLVM, but if you're interested in doing that I can show you how!
There was a problem hiding this comment.
Hi @alexcrichton.
Actually, updating your fork would be pretty convenient for me. I'm currently having to use a manually compiled (newer) LLVM in order to get access to the new DILabel stuff that was recently merged.
If your offer is still open, let's do that?
There was a problem hiding this comment.
Sure! So we've got a number of LLVM-related submodules that will need to be updated in this repository:
src/llvm- This is hosted at https://github.com/rust-lang/llvm currently at https://github.com/rust-lang/llvm/tree/rust-llvm-release-8-0-0-v1
- To update this check out the current branch
rust-llvm-release-8-0-0-v1and then rebase on the current git master (we track https://github.com/llvm-mirror/llvm) - When ready, send a PR to the rust-lang/llvm repo (doesn't matter to which branch) and I'll take the PR tip and move it to a new named branch
src/tools/lld- Hosted at https://github.com/rust-lang/lld (same name branch as LLVM)
- Same procedure to update as LLVM, upstream is https://github.com/llvm-mirror/llvm
src/tools/{lldb,clang}- This one @tromey knows more about (hello!). It's possible to upgrade without updating these, but we should probably only try to handle these towards the end of the update.
src/libcompiler_builtins- This submodule lives at https://github.com/rust-lang-nursery/compiler-builtins
- That repository has a submodule that lives at https://github.com/rust-lang/compiler-rt
- The compiler-rt submodule has the same update procedure as all the others
- I'll create a temporary branch on https://github.com/rust-lang-nursery/compiler-builtins when you're ready, but eventually we'll merge the branch into
master
By far the most difficult part of an LLVM update is making sure it doesn't regress across all the platforms that we're supporting. This is 90%-ish covered though by @bors, though, so there's not too much worry about merging broken code!
The best way to test this out is to test a few configurations locally. First make sure the test suite runs locally for you (don't forget to enable LLVM assertions!). After that some interesting containers to run are:
./src/ci/docker/run.sh wasm32-unknown./src/ci/docker/run.sh arm-android./src/ci/docker/run.sh dist-various-1./src/ci/docker/run.sh dist-various-2./src/ci/docker/run.sh armhf-gnu
And... that should cover a lot of ground! Sometimes Windows has its own bugs to work through but that's more difficult to diagnose unless you have a Windows machine. If you get this far that's more than enough :)
@vext01 does that all make sense? Sorry it's kind of a lot!
There was a problem hiding this comment.
Ok I've actually gone ahead and started to create rust-llvm-release-8-0-0-v2 branches in various repositories! Enough SIMD activity has been happening in upstream WebAssembly backend business I'm also interested in this too now!
There was a problem hiding this comment.
So far the upgrade has been going far smoother than I thought it would, here's a PR -- #55835
There was a problem hiding this comment.
Thanks Alex. I was about to respond to this today. Sorry for the delay.
These are in rust-lang-nursery. I use the same branch names that are used in rust's llvm fork. clang is unmodified, but lldb has a number of rust-related patches that must be preserved. It's fine for now to disable lldb when upgrading, and I can fix it up; but eventually I would like to change this policy. |
|
I've included this with #55835 so I'm going to close this in favor of that, but thanks regardless for the PR! |
Hi,
We were discussing in #55714 the possibility of having a LLVM version gated fix for the recent change to
DiBuilder::createGlobalVariableExpression.In short, an extra optional argument was added to this LLVM function, but for some reason it was not appended to the end of argument list, thus breaking backward compat.
This change uses conditional compilation to fix this.
All tests pass on yesterday's LLVM. Presumably Bors will test using an earlier LLVM before the change.
What do you think? Can we include this?
Thanks