Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make Rustc build with LLVM trunk. #55751

Closed
wants to merge 2 commits into from
Closed

Make Rustc build with LLVM trunk. #55751

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8bce2b
Make Rustc build with LLVM trunk.
vext01 Nov 7, 2018
0383dfa
Conditionally compile in only the extra argument.
vext01 Nov 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/rustllvm/RustWrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then wouldn't you need to revert that extra condition when the rust fork will be resynced with the latest llvm?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which do you prefer guys? I'm a little nervous about updating your rust LLVM fork in case I break something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Contributor Author

@vext01 vext01 Nov 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! So we've got a number of LLVM-related submodules that will need to be updated in this repository:

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far the upgrade has been going far smoother than I thought it would, here's a PR -- #55835

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Alex. I was about to respond to this today. Sorry for the delay.

/* templateParams */ nullptr,
#endif
AlignInBits);

InitVal->setMetadata("dbg", VarExpr);

Expand Down