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.

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 error while cross-compiling Servo for ARM #32049

Closed
ghost opened this issue Mar 4, 2016 · 32 comments · Fixed by #34141
Closed

LLVM error while cross-compiling Servo for ARM #32049

ghost opened this issue Mar 4, 2016 · 32 comments · Fixed by #34141
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ghost
Copy link

ghost commented Mar 4, 2016

The profile_csv3 branch compiles successfully on a Mac OS X 10.10 and on a 64-bit Ubuntu 14.04. However, while cross-compiling the profile_csv3 branch for ARM (following instructions here: https://github.com/servo/servo-nightly/blob/master/.travis.yml) using the arm-linux-gnueabihf toolchain on an Ubuntu (14.04/15.10) host, I get the following error:

rustc: /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp:531: void {anonymous}::PromoteMem2Reg::run(): Assertion isAllocaPromotable(AI) && "Cannot promote non-promotable alloca!"' failed. Aborted (core dumped) Build failed, waiting for other jobs to finish... Could not compileimage`.

Servo branch: https://github.com/larsbergstrom/servo/tree/profile_csv3
Environment variables set:
BUILD_TARGET=arm-unknown-linux-gnueabihf
TRIPLET=arm-linux-gnueabihf
PKG_CONFIG_ALLOW_CROSS=1
PKG_CONFIG_PATH=/usr/lib/arm-linux-gnueabihf/pkgconfig
EXPAT_NO_PKG_CONFIG=1
FREETYPE2_NO_PKG_CONFIG=1
FONTCONFIG_NO_PKG_CONFIG=1
CC=arm-linux-gnueabihf-gcc
CXX=arm-linux-gnueabihf-g++
PATH="$PATH:$HOME/bin"

@rzambre
Copy link

rzambre commented Mar 4, 2016

I posted this issue. I had to delete my old account.

@sanxiyn sanxiyn added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Mar 7, 2016
@larsbergstrom
Copy link
Contributor

You can see this by compiling the image crate version 0.7. Note that it only reproduces targeting arm32 (as shown above), not aarch64 or any of our x86_64 linux/osx/windows targets.

cc @eddyb, whom we chatted with about this a bit in #servo

@eddyb
Copy link
Member

eddyb commented Mar 9, 2016

Could someone who already has this set up run cargo rustc --target=arm-unknown-linux-gnueabihf -- -C llvm-args=-print-before-all on the image crate, and gist the output? It would be very helpful.

@larsbergstrom
Copy link
Contributor

@larsbergstrom
Copy link
Contributor

Full file will be available here shortly:
https://dl.dropboxusercontent.com/u/1620890/output.tgz

@larsbergstrom
Copy link
Contributor

7:10 PM <eddyb> larsberg: ugh, isAllocaPromotable is non-trivial and basically depends on all the uses of allocas
7:10 PM  → @brson (opped) joined  
7:11 PM <eddyb> it has to be SROA::rewritePartition, which has its own promotable checks
7:14 PM <eddyb> larsberg: can you get a LLVM debug build?
7:14 PM <eddyb> larsberg: I severely understimated the size of this code
7:14 PM <eddyb> a debugger would let you "call AI->dump()" and at least get to see which alloca it is
7:15 PM  ↔ japaric (was japaric_) nipped out  
7:16 PM <@larsberg> eddyb: Sure, though I might have to do it in a couple of days. I am frantically trying to hack around this so that I can get a one-off ARM32 build of Servo together for a team to do some energy profiling on a board for a paper they're trying to get out in the next few days :-)
7:16 PM <eddyb> :(
7:16 PM <eddyb> larsberg: this is the worst bug

@sanxiyn
Copy link
Member

sanxiyn commented Mar 10, 2016

Reduced this. (Originally .bc weighed over 5 MB.) Clone https://github.com/sanxiyn/image/tree/reduce and:

$ cargo rustc --target arm-unknown-linux-gnueabihf -- -O --emit asm -C save-temps
PromoteMemoryToRegister.cpp:531: void {anonymous}::PromoteMem2Reg::run(): Assertion `isAllocaPromotable(AI) && "Cannot promote non-promotable alloca!"' failed.
$ ls -lh target/arm-unknown-linux-gnueabihf/debug/*.bc
298K image.0.no-opt.bc

@larsbergstrom
Copy link
Contributor

@sanxiyn Awesome work - thanks for doing this! It would have taken me a lot longer, and I probably wouldn't have gotten it down so far.

@alexcrichton
Copy link
Member

Minimized further: https://gist.github.com/alexcrichton/f7b527d749f1784508b1

Not... sure what to learn from that

@eddyb
Copy link
Member

eddyb commented Mar 11, 2016

@alexcrichton The challenge is to get clang to trip the same assert, to prove it's a LLVM bug and have it reported upstream.

@MagaTailor
Copy link

I understand it's a cross-compilation-only bug as it definitely doesn't reproduce natively.

@eddyb
Copy link
Member

eddyb commented Mar 11, 2016

@petevine Is there a way to check if it's a 64-bit -> 32-bit issue? That's the first thing that jumps to mind.

@MagaTailor
Copy link

Not sure what you meant by that. One more thing, the crates used here seem to be the official armv6 ones, whereas I'm using a natively built armv7 compiler so maybe there's some mixing of armv6/v7 code or tools which could be a no-no for C/C++ parts.

@eddyb
Copy link
Member

eddyb commented Mar 11, 2016

@petevine I mean, cross-compiling for x86 from x64 or for arm32 from aarch64 - if it's a 32-64-bit issue, they should both trigger it.

@MagaTailor
Copy link

No, if the cross-tools are not mismatched then there shouldn't be an issue. But they could be in the armv6/v7 department.

@eddyb
Copy link
Member

eddyb commented Mar 11, 2016

@petevine That's not what I mean. This is a bug inside LLVM, which means it could be very well be some host/target type-size dissonance.

@MagaTailor
Copy link

No one here mentioned an armv6 toolchain, right? Yet the target you were using should be paired with armv6 gcc and libraries. (it was modified rather recently to be that way)
If someone could try cross-compiling for armv7-unknown-linux-gnueabihf we might eliminate that angle.

EDIT:
arm-linux-gnueabihf Ubuntu toolchain is mentioned in the first post and that's synonymous with armv7-a default mode for gcc.

@sanxiyn
Copy link
Member

sanxiyn commented Mar 12, 2016

This does not reproduce for i686-unknown-linux-gnu. 32049.rs is alexcrichton's reduction.

$ multirust default nightly-2016-03-08
$ multirust add-target nightly-2016-03-08 arm-unknown-linux-gnueabihf
$ multirust add-target nightly-2016-03-08 i686-unknown-linux-gnu
$ rustc --target arm-unknown-linux-gnueabihf 32049.rs --crate-type lib -O -g
# bug
$ rustc --target i686-unknown-linux-gnu 32049.rs --crate-type lib -O -g
# no bug

@eddyb
Copy link
Member

eddyb commented Mar 12, 2016

@sanxiyn What's the diff between the completely unoptimized IR of those two targets?

@MagaTailor
Copy link

@sanxiyn
What about multirust add-target nightly-2016-03-08 armv7-unknown-linux-gnueabihf and so on?

@eddyb The arm-unknown-linux-gnueabihf target was modified a few weeks ago so it's entirely possible this whole issue is related to that. The code is not bare metal so other crates (std at least) are going to be used too.

@sanxiyn
Copy link
Member

sanxiyn commented Mar 12, 2016

This reproduces on armv7-unknown-linux-gnueabihf, so it can't be a v6 vs v7 issue. This also reproduces on mips-unknown-linux-gnu, suggesting it's not ARM-specific, but 32-bit-specific. Failure to reproduce on i686 can be explained by different alignment rule: i64 is 64-bit aligned on ARM and MIPS, but 32-bit aligned on x86.

@eddyb
Copy link
Member

eddyb commented Mar 12, 2016

@petevine The only way this was going to be an ARM-specific issue (which is it not) is either a very strange datalayout specific to ARM, or straight-up UB in LLVM.
This is a high-level transformation that only depends on datalayout and the IR given.
We only generate platform-dependent IR for platform intrinsics and C ABIs, neither of which are used here.

@MagaTailor
Copy link

Thanks and sorry for the false lead.

@eddyb
Copy link
Member

eddyb commented Mar 12, 2016

@petevine Np, just trying to speed this up, because it looks like a bad bug that could otherwise remain unfixed for a few more years (especially if it's IR a C compiler would never generate).

@eddyb
Copy link
Member

eddyb commented Mar 12, 2016

Since @sanxiyn didn't post it, here's their diff showing the alignment difference, made after passing both inputs through rustfilt.sh (otherwise the random symbol names introduce a lot of noise).

@metajack
Copy link
Contributor

metajack commented Jun 2, 2016

What's the status here? We just hit this again: http://build.servo.org/builders/arm32/builds/1329/steps/compile/logs/stdio

@aturon aturon added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 6, 2016
@nikomatsakis
Copy link
Contributor

Although it's possible I misunderstood him, @eddyb suggested to me that this was an LLVM bug -- which I guess means we may need to consider trying to find a workaround?

@MagaTailor
Copy link

The workaround in the other issue(s) is to not use opt-level=3. Downgrading llvm to 3.6 helped too in one case, IIRC.

@eddyb
Copy link
Member

eddyb commented Jun 7, 2016

@nikomatsakis Since this is tied to alignment, finding a LlVM target for arm where align_of::<i64>() != 8 could fix it. It's why i686 doesn't exhibit the same problem, at least AFAICT.

@eddyb
Copy link
Member

eddyb commented Jun 7, 2016

@alexcrichton's reduced test case reproduces on 2016-03-18 but not on 2016-03-20, and 2016-03-19 doesn't exist, which gives us this range.

EDIT: Great, that's #32080, which probably changed the resulting IR in subtle ways.
EDIT2: Given that Servo has worked since, until recently, my suspect is #33872, which fixed a bug I introduced in #32080.
The solution involves a cast @dotdash thought could eventually cause issues, so it may be relevant.

@eddyb
Copy link
Member

eddyb commented Jun 7, 2016

I can confirm that #33872 is what broke Servo recently. I wonder if fixing that "the right" way (i.e. with memcpy) makes the LLVM bug go away.
I've looked at clang and the IR it generates varies between a store of the cast argument to an alloca of the real type (what we did before #33872) and a memcpy between two allocas ("the right" way), but it never does what we do since #33872 (storing the cast argument to an alloca of its type and casting the pointer to a pointer to the real type).

EDIT: clang behavior: https://godbolt.org/g/EphFuK vs https://godbolt.org/g/5dikH9

@eddyb
Copy link
Member

eddyb commented Jun 7, 2016

Always generating a memcpy (i.e. "the right" way) for a cast argument instead of doing the shady pointer casts seems to work around this LLVM bug.
I don't think it's even possible to get clang to generate the IR patterns which end up in the assertion.

bors added a commit that referenced this issue Jun 7, 2016
trans: always use a memcpy for ABI argument/return casts.

When storing incoming arguments or values returned by call/invoke, always do a `memcpy` from a temporary of the cast type, if there is an ABI cast.
While Clang has gotten smarter ([store](https://godbolt.org/g/EphFuK) vs [memcpy](https://godbolt.org/g/5dikH9)), a `memcpy` will always work.
This is what @dotdash has wanted to do all along, and it fixes #32049.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants