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

compiler-rt's ARM implementation of udivsi3 seems... wrong? #173

Closed
alexcrichton opened this issue Jul 3, 2017 · 15 comments · Fixed by #285
Closed

compiler-rt's ARM implementation of udivsi3 seems... wrong? #173

alexcrichton opened this issue Jul 3, 2017 · 15 comments · Fixed by #285

Comments

@alexcrichton
Copy link
Member

alexcrichton commented Jul 3, 2017

I updated the compiler-rt submodule in #172 which notably pulled in llvm-mirror/compiler-rt@2fb759f I believe. I merged #172 despite failing tests believing it was a weird nightly thing, but the integration PR also failed with weird errors.

I did some more investigation locally and found I could reproduce the error on #172 locally relatively easily. When forced the compiler-builtins crate to use the Rust implementation of the __udivsi3 intrinsic then the error went away. The only other change to happen to this file since we last updated compiler-rt is llvm-mirror/compiler-rt@2624197 llvm-mirror/compiler-rt@6b34053, which when cherry-picked didn't seem to fix the problem.

I'm going to send a PR to avoid usage of compiler-rt's implementation, but it's presumably faster than what we have in Rust, so this is a tracking issue for closing that perf gap.

alexcrichton added a commit to alexcrichton/compiler-builtins that referenced this issue Jul 3, 2017
Although compiler-rt presumably has a more optimized implementation written in
assembly, it appears buggy for whatever reason, causing rust-lang#173.

For now let's see if integration into rust-lang/rust will work with the
Rust-defined implementation!
bors added a commit that referenced this issue Jul 3, 2017
Use the Rust implementation of udivsi3 on ARM

Although compiler-rt presumably has a more optimized implementation written in
assembly, it appears buggy for whatever reason, causing #173.

For now let's see if integration into rust-lang/rust will work with the
Rust-defined implementation!
@alexcrichton
Copy link
Member Author

@japaric I think you're more familiar with ARM assembly than I am, mind double-checking me on this to make sure I'm not off the rails?

@alexcrichton
Copy link
Member Author

I'll also note that I don't actually know what the error, if any, is in the udivsi3.S file. It may not actually have any errors! What I'm doing could may be hiding the error somewhere else...

alexcrichton added a commit to alexcrichton/compiler-builtins that referenced this issue Jul 3, 2017
Although compiler-rt presumably has a more optimized implementation written in
assembly, it appears buggy for whatever reason, causing rust-lang#173.

For now let's see if integration into rust-lang/rust will work with the
Rust-defined implementation!
bors added a commit that referenced this issue Jul 3, 2017
Use the Rust implementation of udivsi3 on ARM

Although compiler-rt presumably has a more optimized implementation written in
assembly, it appears buggy for whatever reason, causing #173.

For now let's see if integration into rust-lang/rust will work with the
Rust-defined implementation!
alexcrichton added a commit to alexcrichton/compiler-builtins that referenced this issue Jul 3, 2017
Although compiler-rt presumably has a more optimized implementation written in
assembly, it appears buggy for whatever reason, causing rust-lang#173.

For now let's see if integration into rust-lang/rust will work with the
Rust-defined implementation!
bors added a commit that referenced this issue Jul 3, 2017
Use the Rust implementation of udivsi3 on ARM

Although compiler-rt presumably has a more optimized implementation written in
assembly, it appears buggy for whatever reason, causing #173.

For now let's see if integration into rust-lang/rust will work with the
Rust-defined implementation!
@kennytm
Copy link
Member

kennytm commented Jul 3, 2017

Would pulling in llvm-mirror/compiler-rt@6b34053 fix the issue?

@alexcrichton
Copy link
Member Author

Oh that's actually the commit I meant to mention above, unfortunately pulling that in didn't fix it

@parched
Copy link
Contributor

parched commented Jul 5, 2017

@alexcrichton I think I can fix this, how did you reproduce it locally?

@alexcrichton
Copy link
Member Author

@parched I think it should suffice to revert alexcrichton@681aaa9 and then run ci/run-docker.sh arm-unknown-linux-gnueabi

@parched
Copy link
Contributor

parched commented Jul 6, 2017

@alexcrichton Thanks, trying that I get to

...
+ cargo test --no-default-features --target arm-unknown-linux-gnueabi --features gen-tests mangled-names
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
     Running /target/arm-unknown-linux-gnueabi/debug/deps/compiler_builtins-eb4701e69158fd89
/target/arm-unknown-linux-gnueabi/debug/deps/compiler_builtins-eb4701e69158fd89: 1: /target/arm-unknown-linux-gnueabi/debug/deps/compiler_builtins-eb4701e69158fd89: Syntax error: word unexpected (expecting ")")

Have you seen that before?

@alexcrichton
Copy link
Member Author

@parched I believe that's related to Linux's support to run non-native binaries by hook them with something like a QEMU interpreter. You may need to do something like https://github.com/rust-lang-nursery/compiler-builtins/blob/238647af806470dc73e585c03682083931d29cd5/.travis.yml#L48-L49 on your host system.

@japaric
Copy link
Member

japaric commented Jul 6, 2017

@parched Alternatively you pass --no-run to Cargo to compile the test but not run it, and then manually run the binary using QEMU (qemu-arm target/..).

@alexcrichton That reminds me, now that Cargo has "runner" support we might be able to drop the qemu-user-static stuff and replace it with runner = "qemu-arm" in .cargo/config.

@alexcrichton
Copy link
Member Author

Oh right yeah, that migh work great!

@parched
Copy link
Contributor

parched commented Jul 7, 2017

Ok thanks guys, I originally tried with just qemu-arm but it complained about missing dynamic linker library or something. I've found though I just needed to pass it the directory of the libs in the cross toolchain with -L. I made a small fix for the problem but I've found there is a more general problem with the builtins on a Thumb-1 but not Thumb-2 capable CPU. This looks like it is addressing it though https://reviews.llvm.org/rL298713.

@parched
Copy link
Contributor

parched commented Jul 8, 2017

@alexcrichton IMO for now we should just revert llvm-mirror/compiler-rt@2fb759f

@alexcrichton
Copy link
Member Author

We could, yes, but ideally we'd get that upstream

@parched
Copy link
Contributor

parched commented Aug 13, 2017

Fyi https://reviews.llvm.org/D31220 has been accepted upstream now

@alexcrichton
Copy link
Member Author

Great! As soon as we upgrade we can switch and close out this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants