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

rustc: Remove soft-float from MIPS targets #34910

Merged
merged 1 commit into from
Jul 21, 2016

Conversation

alexcrichton
Copy link
Member

Right now two MIPS targets in the compiler, mips-unknown-linux-{gnu,musl} both
generate object files using the soft-float ABI through LLVM by default. This is
also expressed as the -C soft-float codegen option and otherwise isn't used
for any other target in the compiler. This option was added quite some time ago
(back in #9617), and nowadays it's more appropriate to be done through a codegen
option.

This is motivated by #34743 which necessitated an upgrade in the CMake
installation on our bots which necessitated an upgrade in the Ubuntu version
which invalidated the MIPS compilers we were using. The new MIPS compilers
(coming from Debian I believe) all have hard float enabled by default and soft
float support not built in. This meant that we couldn't upgrade the bots
until #34841 landed because otherwise we would fail to compile C code as the
-msoft-float option wouldn't work.

Unfortunately, though, this means that once we upgrade the bots the C code we're
compiling will be compiled for hard float and the Rust code will be compiled
for soft float, a bad mismatch! This PR remedies the situation such that Rust
will compile with hard float as well.

If this lands it will likely produce broken nightlies for a day or two while we
get around to upgrading the bots because the current C toolchain only produces
soft-float binaries, and now rust will be hard-float. Hopefully, though, the
upgrade can go smoothly!

@rust-highfive
Copy link
Collaborator

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member Author

r? @brson

cc @japaric

@rust-highfive rust-highfive assigned brson and unassigned nrc Jul 18, 2016
@alexcrichton alexcrichton mentioned this pull request Jul 18, 2016
3 tasks
@brson
Copy link
Contributor

brson commented Jul 18, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Jul 18, 2016

📌 Commit 617c65e has been approved by brson

@brson brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 18, 2016
@japaric
Copy link
Member

japaric commented Jul 19, 2016

I can't really speak for the mips-gnu target because I am not familiar with the Linux distributions that exist for that target but I guess it makes sense to follow Debian/Ubuntu and use the hard float ABI.

But for mips-musl, one of the main use cases is OpenWRT and AFAICT most OpenWRT releases (for several, different devices) use the soft float ABI so I don't think it makes sense to change the ABI from soft to hard. Furthermore, the mips-linux-musl-gcc toolchain in the rust-buildbot is using the soft float ABI. (I'm assuming this is, indeed, changing the mips-musl target from the soft float ABI to the hard float ABI, if that's not the case carry on!)

@alexcrichton
Copy link
Member Author

@japaric thanks for the info! This does indeed change mips-musl, but I will revert

Right now two MIPS targets in the compiler, `mips-unknown-linux-{gnu,musl}` both
generate object files using the soft-float ABI through LLVM by default. This is
also expressed as the `-C soft-float` codegen option and otherwise isn't used
for any other target in the compiler. This option was added quite some time ago
(back in rust-lang#9617), and nowadays it's more appropriate to be done through a codegen
option.

This is motivated by rust-lang#34743 which necessitated an upgrade in the CMake
installation on our bots which necessitated an upgrade in the Ubuntu version
which invalidated the MIPS compilers we were using. The new MIPS compilers
(coming from Debian I believe) all have hard float enabled by default and soft
float support not built in. This meant that we couldn't upgrade the bots
until rust-lang#34841 landed because otherwise we would fail to compile C code as the
`-msoft-float` option wouldn't work.

Unfortunately, though, this means that once we upgrade the bots the C code we're
compiling will be compiled for hard float and the Rust code will be compiled
for soft float, a bad mismatch! This PR remedies the situation such that Rust
will compile with hard float as well.

If this lands it will likely produce broken nightlies for a day or two while we
get around to upgrading the bots because the current C toolchain only produces
soft-float binaries, and now rust will be hard-float. Hopefully, though, the
upgrade can go smoothly!
@alexcrichton
Copy link
Member Author

@bors: r=brson

@japaric as a side note, should the mipsel musl target also have soft float?

@bors
Copy link
Contributor

bors commented Jul 19, 2016

📌 Commit f77bcc8 has been approved by brson

@japaric
Copy link
Member

japaric commented Jul 19, 2016

@alexcrichton I did a survey of the OpenWRT musl releases and all the mips (8 devices) and mipsel (12 devices) releases are using the soft float ABI.

So, yes. We should change the mipsel-musl target to soft float as well.

@japaric
Copy link
Member

japaric commented Jul 19, 2016

So, yes. We should change the mipsel-musl target to soft float as well.

I should note that the Rust mipsel-musl backend target is using the hard float ABI and the C cross toolchain is also using the hard float ABI, so we need to update both the target definition (+soft-float) and "tweak" the cross toolchain. "Tweak" as in (a) rebuilt the cross toolchain or (b) pass some extra flag (-msoft-float?) to the cross toolchain to force the use of the soft float ABI. I think you've tested option (b) with the mips-gnu toolchain and it didn't work though.

@alexcrichton
Copy link
Member Author

Ah ok, confirmed that -msoft-float doesn't work on the mipsel-musl toolchain. I'll open a bug about this and otherwise we can keep the status quo for now.

@alexcrichton
Copy link
Member Author

Ok, opened up #34922

@nagisa
Copy link
Member

nagisa commented Jul 19, 2016

I feel like mips{,el}-unknown-linux-{musl,...} targets being tailored for what really should be a mips{,el}-unknown-openwrt-... (or -openwrt-linux-) is a mistake. We should investigate what the more conventional Linux distributions targeting MIPS do and use that, while also creating soft-float targets for openwrt.

Sure, OpenWRT still uses linux kernel, but I wouldn’t call it close enough to Linux-the-OS.

@alexcrichton
Copy link
Member Author

@nagisa perhaps yeah, but with a lack of demand I don't think it matters too much one way or another at the moment, we can always tweak in the future.

This is just indicative of how our target specs are not super well defined, and probably won't get solved here either way.

@nagisa
Copy link
Member

nagisa commented Jul 19, 2016

I’d still insist somewhat on making the soft-float targets be named properly, especially now that we know exactly where they are used and do have demand. mips*-openwrt-linux-* would represent the soft-float targets very well.

@alexcrichton
Copy link
Member Author

PRs are of course always welcome! @japaric should be cc'd but I don't see too much in the way otherwise.

@japaric
Copy link
Member

japaric commented Jul 20, 2016

IMO, we should have (at least) two releases: one for soft float and another for hard float. Also, I don't think we should conflate the vendor (*-openwrt-*-*) with the ABI. Yes, all OpenWRT MIPS(el) releases happen to be soft float today, but there may be hard float releases in the future.

The "problem" I see with one release per ABI is naming. gcc uses the same triple (mips-unknown-linux-gnu) for all the different ABIs but because of how rustup works we can't use the same triple for different binary releases. We could use mips-unknown-linux-musl and mips-unknown-linux-muslhf to differentiate them but I don't recall ever seeing the muslhf component in a triple (there is musleabi and musleabihf, but those are ARM exclusive).

Then, there is the issue that there are a few MIPS ABIs: 32, o64, n32, etc. (cf. gcc options, look for -mabi=) and there are also a few FP ABI variants. As Rust gets more use on MIPS, we'll probably start seeing demand for different ABIs; this means demand for more mips(el)-musl releases...

@bors
Copy link
Contributor

bors commented Jul 20, 2016

⌛ Testing commit f77bcc8 with merge b3707ff...

@bors
Copy link
Contributor

bors commented Jul 20, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@alexcrichton
Copy link
Member Author

@bors: retry

On Tue, Jul 19, 2016 at 9:36 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt-rustbuild/builds/1828


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34910 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95D-4RJBdBgPZm6bl0BpIEtOUTRHWks5qXaW5gaJpZM4JPPYw
.

@bstrie bstrie added this to the Launch MIR into Orbit milestone Jul 20, 2016
@alexcrichton
Copy link
Member Author

@bstrie I'm gonna remove this from the "MIR into Orbit" milestone b/c this isn't directly blocking it, we can always deal with the fallout here later and in parallel.

@bors: rollup

@alexcrichton alexcrichton removed this from the Launch MIR into Orbit milestone Jul 20, 2016
@bors
Copy link
Contributor

bors commented Jul 21, 2016

⌛ Testing commit f77bcc8 with merge 77ba66d...

@bors
Copy link
Contributor

bors commented Jul 21, 2016

💔 Test failed - auto-linux-64-cargotest

@alexcrichton
Copy link
Member Author

@bors: retry

On Wed, Jul 20, 2016 at 6:24 PM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-64-cargotest
https://buildbot.rust-lang.org/builders/auto-linux-64-cargotest/builds/1201


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34910 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95G25Vpq80nGYT0nEouWw9_a72eAhks5qXspAgaJpZM4JPPYw
.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 21, 2016
rustc: Remove soft-float from MIPS targets

Right now two MIPS targets in the compiler, `mips-unknown-linux-{gnu,musl}` both
generate object files using the soft-float ABI through LLVM by default. This is
also expressed as the `-C soft-float` codegen option and otherwise isn't used
for any other target in the compiler. This option was added quite some time ago
(back in rust-lang#9617), and nowadays it's more appropriate to be done through a codegen
option.

This is motivated by rust-lang#34743 which necessitated an upgrade in the CMake
installation on our bots which necessitated an upgrade in the Ubuntu version
which invalidated the MIPS compilers we were using. The new MIPS compilers
(coming from Debian I believe) all have hard float enabled by default and soft
float support not built in. This meant that we couldn't upgrade the bots
until rust-lang#34841 landed because otherwise we would fail to compile C code as the
`-msoft-float` option wouldn't work.

Unfortunately, though, this means that once we upgrade the bots the C code we're
compiling will be compiled for hard float and the Rust code will be compiled
for soft float, a bad mismatch! This PR remedies the situation such that Rust
will compile with hard float as well.

If this lands it will likely produce broken nightlies for a day or two while we
get around to upgrading the bots because the current C toolchain only produces
soft-float binaries, and now rust will be hard-float. Hopefully, though, the
upgrade can go smoothly!
@bors bors merged commit f77bcc8 into rust-lang:master Jul 21, 2016
@alexcrichton alexcrichton deleted the hard-float-mips branch August 26, 2016 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants