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

add support for i586-unknown-linux-gnu #157

Merged
merged 1 commit into from
Nov 19, 2017
Merged

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Nov 14, 2017

Closes #156 .

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 14, 2017

I don't have a linux machine, is there a way to get the tests to run? Or do I have to test it manually (e.g. by forking cargo and trying to compile it with cross for i586 ?)

Copy link
Contributor

@malbarbo malbarbo left a comment

Choose a reason for hiding this comment

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

It's LGTM.

gcc \
libc6-dev \
make \
file \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is file dependency required?

Copy link
Contributor Author

@gnzlbg gnzlbg Nov 14, 2017

Choose a reason for hiding this comment

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

No, this is a c&p-error from the i586 file in the stdsimd crate. EDIT: fixed.

@@ -0,0 +1,27 @@
FROM ubuntu:17.04
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is based on i686-unknown-linux-gnu, right? If the ubuntu version is kept in 12.04, the docker layers can be shared between i686-unknown-linux-gnu and i586-unknown-linux-gnu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really know my way around docker. Should I downgrade the version in this file, upgrade the version in the i686 docker file, or do I need to do something else to reuse the docker layers?

(FWIW in the stdsimd crate we use 17.04 for everything and I am trying to replace our testing infrastructure there with cross now that you are adding support here for qemu-system).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per @japaric 's comment below I've downgraded this to 12.04.

README.md Outdated
@@ -198,6 +198,7 @@ worst, "hang" (never terminate).
| `armv7-unknown-linux-gnueabihf` | 2.15 | 4.6.2 | 1.0.2m | ✓ | 2.8.0 | ✓ |
| `armv7-unknown-linux-musleabihf` | 1.1.15 | 5.3.1 | N/A | | 2.8.0 | ✓ |
| `asmjs-unknown-emscripten` [4] | 1.1.15 | 1.37.13 | N/A | ✓ | N/A | ✓ |
| `i586-unknown-linux-gnu` | 2.15 | 4.6.2 | 1.0.2m | ✓ | N/A | ✓ |
Copy link
Contributor

Choose a reason for hiding this comment

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

the glibc version is wrong. Ubuntu 17.04 ships with glibc 2.24.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've downgraded the ubuntu version to 12.04, so that this should now be correct.

@@ -0,0 +1,27 @@
FROM ubuntu:17.04
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be as low as possible, i.e. 12.04 as the i686 target, to make the binaries produced with Cross compatible with legacy distributions like centos.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. This should make the version in the readme file correct right?

@malbarbo
Copy link
Contributor

I don't known how to run the test in a non Linux machine... Maybe you can install Linux in a virtual machine?

Instructions to run the tests in a Linux machine:

First you need to build the docker image:

./build-docker-image.sh i586-unknown-linux-gnu

Then you can run the tests:

TRAVIS_OS_NAME=linux TARGET=i586-unknown-linux-gnu CPP=1 DYLIB=1 STD=1 OPENSSL=0.5.5 RUN=1 bash ci/script.sh

@japaric
Copy link
Contributor

japaric commented Nov 14, 2017

I don't have a linux machine, is there a way to get the tests to run?

I can throw this into homu as a try build but test times will be in the order of hours due to the large number of travis builders. If you temporarily comment all the other travis builds I can start a try build.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 14, 2017

@japaric I have another PR with an new ARM target. Maybe after that one passes review I can merge both PRs and then we can do a homu build of both together?

@japaric
Copy link
Contributor

japaric commented Nov 14, 2017

@gnzlbg Fine by me.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 14, 2017

@japaric I've merged both PR here and disabled the other builds so that homu can be run.

Copy link
Contributor

@japaric japaric left a comment

Choose a reason for hiding this comment

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

I just remembered:

apt-get install -y --no-install-recommends g++-arm-linux-gnueabihf

This is wrong for arm-unknown-linux-gnueabihf because that targets supports both ARMv6 and ARMv7 but g++-arm-linux-gnueabihf targets ARMv7 thus with this combination the binary will contain ARMv7 instructions and will crash when executed on a ARMv6 device. You won't detect this problem when testing under QEMU because qemu-arm emulates an ARMv7 processor by default. But quite a few people, including me, have run into this runtime crash (SIGILL) when using the Ubuntu toolchain for cross compilation.

TL;DR The C toolchain (gcc + libc) must be compiled from scratch to target ARMv6

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 14, 2017

@japaric is there a way to force qemu-arm to emulate an armv6 processor ?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 14, 2017

@japaric I've removed the arm target since I don't really know how to do that without a machine where I can test it, the i586 parts should be good to go if you want to try with homu.

@japaric
Copy link
Contributor

japaric commented Nov 14, 2017

is there a way to force qemu-arm to emulate an armv6 processor ?

I think I tried setting a specific CPU before using QEMU_CPU but I think I didn't manage to get a SIGILL when supossedly emulating ARMv6 and running a binary that contained ARMv7-only instructions.

I don't really know how to do that without a machine where I can test it

You can probably copy paste some of the Docker build stuff from rust-lang/rust. There they use crosstool-ng to build an ARMv6 toolchain.

@homunkulus try

@homunkulus
Copy link
Contributor

⌛ Trying commit 81f03cd with merge 7e225bd...

japaric pushed a commit that referenced this pull request Nov 14, 2017
add support for i586-unknown-linux-gnu

Closes #156 .
@homunkulus
Copy link
Contributor

💔 Test failed - status-travis

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 14, 2017

It fails to link cargo: https://travis-ci.org/japaric/cross/builds/302095949#L8670

@japaric
Copy link
Contributor

japaric commented Nov 14, 2017

That looks slightly familiar. IIRC that error is caused because you used a relatively new binutils / gcc when compiling the std facade but used an old binutils (ld) when linking a Rust binary against the facade. This has happened before with 32-bit targets and it was fixed upstream, perhaps this target was not fixed? I believe the solution was to downgrade the binutils version used in the docker image (in rust-lang/rust) where std is built.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 14, 2017

@japaric you mean this here, and the comment above?

https://travis-ci.org/japaric/cross/builds/302095949#L8670

It seems like that is done for i686 but not for i586.

@japaric
Copy link
Contributor

japaric commented Nov 14, 2017

I was referring to this linker error:

= note: /usr/bin/ld: /rust/lib/rustlib/i586-unknown-linux-gnu/lib/liballoc_jemalloc-208c05c6a7233cbb.rlib(jemalloc.pic.o): unrecognized relocation (0x2b) in section `.text.malloc_conf_init'

Specifically the "unrecognized relocation" you can probably find issues with that phrase in rust-lang/rust.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 14, 2017

Did you take a look at the link? It seems to exactly be addressing that error, but it seems to be only doing so for i686 and not i586. I'll go through the issues and see what Ican find.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 14, 2017

@japaric
Copy link
Contributor

japaric commented Nov 14, 2017

Yes, that's the issue. The problem is that std is built in Ubuntu 16.04 and we are using a linker from 12.04.

ENV CFLAGS_i686_unknown_linux_musl=-Wa,-mrelax-relocations=no

This flag should also be applied to i586-gnu otherwise only i686-musl would be "supercompatible". This has to be fixed upstream.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 14, 2017

rust-lang/rust#45991

bors added a commit to rust-lang/rust that referenced this pull request Nov 17, 2017
fix linking error on i586

Try to fix this linking error on i586 in cross:

https://travis-ci.org/japaric/cross/builds/302095949#L8670

The problem is that `std` is built in Ubuntu 16.04 and `cross` uses a linker from 12.04.

Currently this fix solves the problem for `i686-musl`  making it "supercompatible", this PR applies the fix to `i586` as well.

The cross PR is here: cross-rs/cross#157
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 18, 2017

@homunkulus try

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 18, 2017

@japaric might be worth to retry this today or tomorrow, the fix was merged yesterday in rustc :)

@japaric
Copy link
Contributor

japaric commented Nov 18, 2017

@homunkulus retry

@homunkulus
Copy link
Contributor

⌛ Trying commit 81f03cd with merge 6a7bee8...

japaric pushed a commit that referenced this pull request Nov 18, 2017
add support for i586-unknown-linux-gnu

Closes #156 .
@homunkulus
Copy link
Contributor

💔 Test failed - status-travis

@japaric
Copy link
Contributor

japaric commented Nov 18, 2017

linker error:

/checkout/src/liballoc_jemalloc/../jemalloc/src/jemalloc.c:797: undefined reference to `secure_getenv'

that sounds like the std facade is being built against a newer libc than the one we are using. The easiest fix would be to bump our Ubuntu version to match the one that's being used in rust-lang/rust. The fix that would make i586 binaries more compatible would be to lower the libc version used in rust-lang/rust.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 18, 2017

I've bumped the ubuntu version to 16.04 (the one used in rust-lang/rust). If that works we could try lowering it to 14.04 and see if that works.

@japaric
Copy link
Contributor

japaric commented Nov 18, 2017

@homunkulus try

@homunkulus
Copy link
Contributor

⌛ Trying commit 7f3bfb6 with merge 04e7fec...

japaric pushed a commit that referenced this pull request Nov 18, 2017
add support for i586-unknown-linux-gnu

Closes #156 .
@japaric
Copy link
Contributor

japaric commented Nov 18, 2017

@gnzlbg you can try this PR, and any other PR, by the way. It didn't work before because the branch was in a try (failed) stage. In that stage you have to retry. I think you should be able to retry try-ed PRs but I've never tested that before.

@homunkulus
Copy link
Contributor

☀️ Test successful - status-travis
State: approved= try=True

@japaric
Copy link
Contributor

japaric commented Nov 18, 2017

We shouldn't use a libc older than the one used by rust-lang/rust because there's the risk that a new libc symbol will be used in the std facade in the future and that will cause linker error due to us having an older libc without that symbol.

If you uncomment the other build jobs I'll r+ this.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 19, 2017

@japaric I've rebased but I think the versions of libc in the readme are still wrong.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 19, 2017

@japaric so i've updated the version of glibc in the readme to 2.23 and gcc to 5.3.1 (those seem to be the values for ubuntu 16.04). IIUC the openssl version is 1.0.2m for all right?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 19, 2017

@homunkulus try

@homunkulus
Copy link
Contributor

⌛ Trying commit ae1f3d5 with merge 999cc89...

japaric pushed a commit that referenced this pull request Nov 19, 2017
add support for i586-unknown-linux-gnu

Closes #156 .
@homunkulus
Copy link
Contributor

☀️ Test successful - status-travis
State: approved= try=True

@japaric
Copy link
Contributor

japaric commented Nov 19, 2017

IIUC the openssl version is 1.0.2m for all right?

Yes. The OpenSSL is the same on all the images.

Thank you, @gnzlbg.

@japaric japaric merged commit 91637e1 into cross-rs:master Nov 19, 2017
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 21, 2017

@japaric could you do a release of this? I'd like to move stdsimd to cross asap, even if that means loosing arm-unknown-linux-gnueabihf support temporarily for the time being.

@japaric
Copy link
Contributor

japaric commented Nov 21, 2017

@gnzlbg v0.1.14 has been released. You'll have to wait like 1 hour until propely tagged images make it to dockerhub and the release works properly. When this build is done the new release should work.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 21, 2017

Thanks, I'll send a PR to stdsimd to start using cross probably tomorrow. I'll ping you in there.

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 this pull request may close these issues.

4 participants