-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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.
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
LLVM 4.0 Upgrade #40123
LLVM 4.0 Upgrade #40123
Conversation
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
@TimNN you can probably work faster than going through @bors by selectively adding I'd recommend doing that for a couple of the cross targets at least and then probably some of the other builders as well (such as emscripten) My guess is that AppVeyor will be one of the most difficult pieces to update as part of this upgrade. Unfortunately we don't have any extra capacity there for running tests so it may be difficult to do so on the PR before bors :( |
The debuginfo failures all occur because I'm unsure what the best fix would be here ( |
mean that gdb outputs In any case, requiring |
I vaguely remember about statics being optimized away the last summer already, when I wrote debuginfo tests for unions (one of the failing tests in this PR). |
It means that gdb prints Apparently I made some assumptions that weren't quite correct. The symbols exist (output of
Since this is apparently indeed debuginfo related I guess cc @michaelwoerister, @dylanmckay |
The generated IR, in case that helps anyone: https://gist.github.com/TimNN/92152a2f8062909805657d1bb4131998 |
The failed build of the
I saw the following panics:
Do they ring a bell for anyone? |
The |
The android image fails with an LLVM assertion:
There are also some warnings (see below for an example), of which I am unsure how relevant / important they are.
|
@TimNN the former may be a bug in LLVM? (or just something we've never exposed ourselves before). The latter is normal, I believe it's happening on builds today. |
Oh we've also got ~10 extra capacity on Travis so feel free to test more than one row at a time if you'd like :) |
@alexcrichton: Have you ever seen something like the qemu failures in #40123 (comment) before?
Ah, ok. I've been doing 3 at a time (when not debugging emscripten), but I guess I can run a few more :) |
The
I guess the fix here is to backport (part of?) rust-lang/llvm@58731be as well. |
I've been investigating the emscripten failures, see below for the findings. The one thing that both test have in common is that they use fixed sized arrays (
The
(with this code:) use std::mem;
#[repr(packed)]
#[derive(Copy, Clone, PartialEq, Debug)]
struct Foo {
bar: u8,
baz: u64
}
pub fn main() {
let foos = [Foo { bar: 1, baz: 2 }; 10];
assert_eq!(mem::size_of::<[Foo; 10]>(), 90);
for i in 0..10 {
println!("{:?}, {:?}", foos[i], Foo { bar: 1, baz: 2});
}
for &foo in &foos {
println!("{:?}, {:?}", foo, Foo { bar: 1, baz: 2 });
}
assert!(false);
}
The let mut x = E([0; 32]);
write_volatile(&mut x, E([1; 32]));
assert_eq!(read_volatile(&x), E([1; 32])); // line 61
assert_eq!(x, E([1; 32])); // line 62 The output:
If line 61 is commented:
Note that other |
The QEMU failures don't look familiar but are perhaps indicative of the program segfaulting or otherwise exiting un-cleanly. Do you have the full logs I could help take a look at? The missing |
Ah, sorry, I linked the logs only from the original post, here they are: https://travis-ci.org/rust-lang/rust/jobs/205860539 |
Fascinating! Unfortunately I may not be of much help. Also thanks for the links, I should have looked around for them! Of the failures so far:
Some other tips I'd have is:
|
Yeah, I'm doing that right now :)
Alright, so the problem is, I think, that the following |
a387dec
to
03412b9
Compare
@TimNN heh yeah that'd do it! I wonder if some more headers need to be copied from the NetBSD base system or something like that? Unfortunately I forget now at this point where I got those instructions from to build a NetBSD cross-compiler... |
Those warnings aren't new. I forget the cause. |
Some notes on the
|
Odd! I wouldn't entirely rule out a bug in qemu-test-{client,server} FWIW |
a2f620f
to
934aa51
Compare
Yay, looks like all the builds timed out again, so things seem to be good to go. (I didn't verify all the logs this time). |
@TimNN looks great to me! I hope to branch beta later today, so want to pull out the fast-fail? I'll r+ this after the beta is branched. I'd also like to reiterate that you're at least my own personal "Rust Hero of the last N Months" where N is two and counting. If we delay this for 3 more days then it'll be a 2+ month PR! |
@alexcrichton: I removed the always fail commit.
Thanks a lot! All the positive encouragement and feedback has helped a lot in keeping me motivated to work on the upgrade :). |
Mine too! :) |
@bors: r+ Beta's branched, let's do this! |
📌 Commit 8994277 has been approved by |
LLVM 4.0 Upgrade Since nobody has done this yet, I decided to get things started: **Todo:** * [x] push the relevant commits to `rust-lang/llvm` and `rust-lang/compiler-rt` * [x] cleanup `.gitmodules` * [x] Verify if there are any other commits from `rust-lang/llvm` which need backporting * [x] Investigate / fix debuginfo ("`<optimized out>`") failures * [x] Use correct emscripten version in docker image --- Closes #37609. --- **Test results:** Everything is green 🎉
☀️ Test successful - status-appveyor, status-travis |
Thanks for slogging through this @TimNN. |
Congratulations @TimNN! |
Thank you @TimNN! |
🎉 |
According to rust-lang/rust#40123 rust now supports LLVM 4
Wow! Thanks a lot @TimNN! That is some effort! |
I've just noticed that README mentions clang 3.x. Shouldn't this be updated? |
@Kixunil That part of the readme is about the C and C++ compiler used for compiling C and C++ dependencies during the build, not about the LLVM version. |
@rkruppe I guess I'm too hungry and tired. Thank you for pointing that out! :) |
Since nobody has done this yet, I decided to get things started:
Todo:
rust-lang/llvm
andrust-lang/compiler-rt
.gitmodules
rust-lang/llvm
which need backporting<optimized out>
") failuresCloses #37609.
Test results:
Everything is green 🎉