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 mips(el)-unknown-linux-musl #31298

Merged
merged 4 commits into from Jan 31, 2016

Conversation

Projects
None yet
3 participants
@japaric
Copy link
Member

japaric commented Jan 30, 2016

This target covers MIPS devices that run the trunk version of OpenWRT.

The x86_64-unknown-linux-musl target always links statically to C libraries. For
the mips(el)-unknown-linux-musl target, we opt for dynamic linking (like most of
other targets do) to keep binary size down.

As for the C compiler flags used in the build system, we use the same flags used
for the mips(el)-unknown-linux-gnu target.

r? @alexcrichton

add support for mips(el)-unknown-linux-musl
This target covers MIPS devices that run the trunk version of OpenWRT.

The x86_64-unknown-linux-musl target always links statically to C libraries. For
the mips(el)-unknown-linux-musl target, we opt for dynamic linking (like most of
other targets do) to keep binary size down.

As for the C compiler flags used in the build system, we use the same flags used
for the mips(el)-unknown-linux-gnu target.
.gitmodules Outdated
@@ -16,4 +16,5 @@
url = https://github.com/rust-lang/rust-installer.git
[submodule "src/liblibc"]
path = src/liblibc
url = https://github.com/rust-lang-nursery/libc.git
url = https://github.com/japaric/libc.git

This comment has been minimized.

@japaric

japaric Jan 30, 2016

Author Member

TODO revert this change

@@ -0,0 +1,24 @@
# mips-unknown-linux-musl configuration
CC_mips-unknown-linux-musl=mips-openwrt-linux-gcc

This comment has been minimized.

@japaric

japaric Jan 30, 2016

Author Member

TODO change the prefixes to something vendor neutral. Probably mips-linux-musl-gcc.

src/liblibc Outdated
@@ -1 +1 @@
Subproject commit 91ff43c736de664f8d3cd351e148c09cdea6731e
Subproject commit 81c05bcddb05d0e5facf66f3945405bbf50b04d3

This comment has been minimized.

@japaric

japaric Jan 30, 2016

Author Member

TODO this will need to be updated

@@ -93,11 +93,20 @@ mod arch {
use os::raw::{c_long, c_ulong};
use os::unix::raw::{gid_t, uid_t};

#[cfg(target_env = "musl")]
#[stable(feature = "raw_ext", since = "1.1.0")] pub type blkcnt_t = i64;

This comment has been minimized.

@japaric

japaric Jan 30, 2016

Author Member

I'm not sure what stability level this should have. AFAIK, these types are going to be deprecated in the future.

This comment has been minimized.

@alexcrichton

alexcrichton Jan 30, 2016

Member

Ah yeah these are fine to stay as-is

llvm_target: "mipsel-unknown-linux-musl".to_string(),
target_endian: "little".to_string(),
target_pointer_width: "32".to_string(),
arch: "mips".to_string(),

This comment has been minimized.

@alexcrichton

alexcrichton Jan 30, 2016

Member

Should this be mipsel?

This comment has been minimized.

@japaric

japaric Jan 30, 2016

Author Member

Should this be mipsel?

Not sure! The existing mipsel-unknown-linux-gnu target is also using mips as the arch.

AIUI, this arch field is used for #[cfg(target_arch = ...)] and not for codegen (that's what llvm-target is used for). If we want to change both targets (mipsel-gnu and mipsel-musl) to use arch = mipsel, then we'll have to update the cfgs in crates like libc from #[cfg(target_arch = "mips")] to #[cfg(any(target_arch = "mips", any(target_arch = "mipsel"))].

This comment has been minimized.

@alexcrichton

alexcrichton Jan 30, 2016

Member

Oh interesting! That may be a bug in the target triple, however, depending on how you look at it. Either that or we may just want to make sure that we're consistent. Some data points:

  • The newly-added powerpc64le-unknown-linux-gnu triple has powerpc64le as the target_arch.
  • There's some cfg(target_arch = "mipsel") directives in the standard library, which apparently are doing nothing.
  • We don't necessarily require the arch in the triple to equal the arch in target_arch, for example armv7 recently added has the target_arch of arm.

I think that I'd prefer to lean towards leaving this as mips and tweaking the powerpc64le triple to have powerpc64 as the target_arch. That seems the most consistent to me and also the most ergonomic. The two triples can still be differentiated by looking at target_endian

This comment has been minimized.

@japaric

japaric Jan 30, 2016

Author Member

I think that I'd prefer to lean towards leaving this as mips and tweaking the powerpc64le triple to have powerpc64 as the target_arch. That seems the most consistent to me and also the most ergonomic. The two triples can still be differentiated by looking at target_endian

👍 from me

CC_mipsel-unknown-linux-musl=mipsel-openwrt-linux-gcc
CXX_mipsel-unknown-linux-musl=mipsel-openwrt-linux-g++
CPP_mipsel-unknown-linux-musl=mipsel-openwrt-linux-gcc
AR_mipsel-unknown-linux-musl=mipsel-openwrt-linux-ar

This comment has been minimized.

@alexcrichton

alexcrichton Jan 30, 2016

Member

This should also work with a standard mipsel-linux-gnu-gcc toolchain, right?

This comment has been minimized.

@alexcrichton

alexcrichton Jan 30, 2016

Member

Oh, I see your TODO above

@alexcrichton alexcrichton self-assigned this Jan 30, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 30, 2016

Looks good to me, thanks @japaric!

This brings up an interesting facet about the MUSL targets, however, about whether the C standard library is statically or dynamically linked. The x86_64-unknown-linux-musl target was originally created with the intention of only really being used for statically linked binaries. I'd personally prefer that these targets remain consistent, but it seems that the current primary use case (OpenWRT?) is dynamically linking the C standard library.

In that sense all of the MUSL targets should really support either dynamically linking or statically linking, but we don't really support this sort of lazy linkage in the standard library (only on crates.io where you build from source).

I think for now I'd be fine basically choosing "whatever seems best" for the target at hand (e.g. static for x86_64 and perhaps i686 but dynamic for mips), but it's something we may wish to reconsider in the future.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 30, 2016

ah and of course cc @rust-lang/tools about the dynamic-vs-static linkage of musl

@japaric japaric changed the title [blocked] add support for mips(el)-unknown-linux-musl add support for mips(el)-unknown-linux-musl Jan 30, 2016

@japaric japaric force-pushed the japaric:mips-musl branch from 84c1574 to 64ac041 Jan 30, 2016

@japaric

This comment has been minimized.

Copy link
Member Author

japaric commented Jan 30, 2016

This is now merge ready.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 30, 2016

bors added a commit that referenced this pull request Jan 30, 2016

Auto merge of #31298 - japaric:mips-musl, r=alexcrichton
This target covers MIPS devices that run the trunk version of OpenWRT.

The x86_64-unknown-linux-musl target always links statically to C libraries. For
the mips(el)-unknown-linux-musl target, we opt for dynamic linking (like most of
other targets do) to keep binary size down.

As for the C compiler flags used in the build system, we use the same flags used
for the mips(el)-unknown-linux-gnu target.

r? @alexcrichton
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 30, 2016

⌛️ Testing commit 64ac041 with merge 8110338...

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 30, 2016

💔 Test failed - auto-linux-musl-64-opt

@japaric

This comment has been minimized.

Copy link
Member Author

japaric commented Jan 31, 2016

The failed test (os::raw::tests::unix) was caused by rust-lang/libc#122 which changed the definition of nlink_t to usize. I'll send a PR to libc to restore the old definition.

update libc submodule
fixes failed test (std::os::raw::tests::unix) in x86_64-musl target
@japaric

This comment has been minimized.

Copy link
Member Author

japaric commented Jan 31, 2016

@bors: r=alexcrichton 27127db

bors added a commit that referenced this pull request Jan 31, 2016

Auto merge of #31298 - japaric:mips-musl, r=alexcrichton
This target covers MIPS devices that run the trunk version of OpenWRT.

The x86_64-unknown-linux-musl target always links statically to C libraries. For
the mips(el)-unknown-linux-musl target, we opt for dynamic linking (like most of
other targets do) to keep binary size down.

As for the C compiler flags used in the build system, we use the same flags used
for the mips(el)-unknown-linux-gnu target.

r? @alexcrichton
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 31, 2016

⌛️ Testing commit 27127db with merge 9041b93...

@bors bors merged commit 27127db into rust-lang:master Jan 31, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@bors bors referenced this pull request Jan 31, 2016

Merged

Add Illumos support #31078

@japaric japaric deleted the japaric:mips-musl branch Jan 31, 2016

malbarbo added a commit to malbarbo/rust that referenced this pull request Jan 22, 2018

Do not assume dynamic linking for musl/mips[el] targets
All musl targets except mips[el] assume static linking by default. This
can be confusing
https://users.rust-lang.org/t/static-cross-compiled-binaries-arent-really-static/6084

When the musl/mips[el] targets was
[added](rust-lang#31298), dynamic linking
was chosen because of binary size concerns, and probably also because
libunwind
[didn't](https://users.rust-lang.org/t/static-cross-compiled-binaries-arent-really-static/6084/8)
supported mips.

Now that we have `crt-static` target-feature (the user can choose
dynamic link for musl targets), and libunwind
[6.0](https://github.com/llvm-mirror/libunwind/commits/release_60) add
support to mips, we do not need to assume dynamic linking.

bors added a commit that referenced this pull request Jan 27, 2018

Auto merge of #47663 - malbarbo:mips-crt-static, r=alexcrichton
Do not assume dynamic linking for musl/mips[el] targets

All musl targets except mips[el] assume static linking by default. This can be [confusing](https://users.rust-lang.org/t/static-cross-compiled-binaries-arent-really-static/6084).

When the musl/mips[el] targets was [added](#31298), dynamic linking was chosen because of binary size concerns, and probably also because libunwind [didn't](https://users.rust-lang.org/t/static-cross-compiled-binaries-arent-really-static/6084/8) supported mips.

Now that we have `crt-static` target-feature (the user can choose dynamic link for musl targets), and libunwind [6.0](https://github.com/llvm-mirror/libunwind/commits/release_60) add support to mips, we do not need to assume dynamic linking.

bors added a commit that referenced this pull request Jan 28, 2018

Auto merge of #47663 - malbarbo:mips-crt-static, r=alexcrichton
Do not assume dynamic linking for musl/mips[el] targets

All musl targets except mips[el] assume static linking by default. This can be [confusing](https://users.rust-lang.org/t/static-cross-compiled-binaries-arent-really-static/6084).

When the musl/mips[el] targets was [added](#31298), dynamic linking was chosen because of binary size concerns, and probably also because libunwind [didn't](https://users.rust-lang.org/t/static-cross-compiled-binaries-arent-really-static/6084/8) supported mips.

Now that we have `crt-static` target-feature (the user can choose dynamic link for musl targets), and libunwind [6.0](https://github.com/llvm-mirror/libunwind/commits/release_60) add support to mips, we do not need to assume dynamic linking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.