Steps towards support for musl-unknown-linux-uclibc #578

cactorium · 2017-04-20T23:02:24Z

Hello! I've been working towards resolving #361 , this PR compiles successfully with a newish compiler (with some minor fixes in gcc, ctest), and all the tests pass for libc-ctest. Basically most of the undefined functions, constants, and structs were just removed from the ctest, and then any constants that weren't correct were fixed. Would it make more sense to conditionally remove them from libc? I wasn't sure when it was appropriate to skip the test for it instead of removing the function/constants, so I just removed all the tests for now because that was a little easier than hunting them down. I'm also guessing the way some of the constants were conditionally set wasn't the correct style, would you guys have any advice on how to do it more correctly? Lemme know how it looks!

alexcrichton

Awesome thanks for the work here! I know you're probably not the only one waiting for better uclibc support :)

I think that uclibc is different enough from glibc/musl that we may want to start a whole separate hierarchy for uclibc? Something like src/unix/notbsd/linux/uclibc/*.rs which is dispatched to from src/unix/notbsd/linux/mod.rs. That should cut down on the #[cfg] traffic I think and alleviate a lot of the changes to libc-test/build.rs.

alexcrichton · 2017-04-20T23:09:36Z

libc-test/build.rs

+            "LC_CTYPE_MASK" | "LC_NUMERIC_MASK" | "LC_TIME_MASK" | "LC_COLLATE_MASK" | "LC_MONETARY_MASK" | "LC_MESSAGES_MASK" |
+            "MADV_MERGEABLE" | "MADV_UNMERGEABLE" | "MADV_HWPOISON" | "IPV6_ADD_MEMBERSHIP" | "IPV6_DROP_MEMBERSHIP" | "IPV6_MULTICAST_LOOP" | "IPV6_V6ONLY" |
+            "MAP_STACK" | "MAP_HUGETLB" | "RTLD_DEEPBIND" |
+            "SOL_IPV6" | "SOL_ICMPV6" | "SOL_TIPC"  if uclibc => true,


Shouldn't these just all be omitted on uclibc?

Fixed (most of them)! The couple that are left appear to exist in the uClibc source code, but they're either optional or I'm missing some compiler flags to trigger their inclusion. I'm pretty sure at least the ones related to locale are optional

alexcrichton · 2017-04-20T23:10:11Z

libc-test/build.rs

@@ -309,6 +316,11 @@ fn main() {
            // This is actually a union, not a struct
            "sigval" => true,

+            // These structs have changed since uClibc was developed
+            "stat" | "stat64" | "dqblk" if uclibc => true,


Shouldn't these be corrected to match what uclibc has internally?

alexcrichton · 2017-04-20T23:10:25Z

libc-test/build.rs

+            // These structs have changed since uClibc was developed
+            "stat" | "stat64" | "dqblk" if uclibc => true,
+            // These structs didn't exist when uClibc was developed
+            "aiocb" if uclibc => true,


Should this be removed from the libc crate on uclibc if it doesn't exist?

Would you have any advice on how to remove aiocb? Just sticking a #cfg[] above it breaks because of s! macro wrapping it

alexcrichton · 2017-04-20T23:10:43Z

libc-test/build.rs

@@ -290,6 +295,8 @@ fn main() {
            // sighandler_t is crazy across platforms
            "sighandler_t" => true,

+            "locale_t" if uclibc => true,


Could you add a short comment to why this is ignored?

My bad, it looks like it builds fine with this removed!

alexcrichton · 2017-04-20T23:11:44Z

libc-test/build.rs

+            "mkostemp" | "mkostemps" | "mkstemps" | "futimes" | "setns" | "sysinfo" | "fallocate" | "posix_fallocate" | "preadv" | "pwritev" | "dup3" | "openpty" | "forkpty" |
+            "getloadavg" | "process_vm_readv" | "process_vm_writev" | "backtrace" | "posix_madvise" |
+            "aio_read" | "aio_write" | "aio_fsync" | "aio_error" | "aio_return" | "aio_suspend" | "aio_cancel" | "lio_listio" |
+            "newlocale" | "duplocale" | "freelocale" | "uselocale" | "nl_langinfo_l" | "wcslen" | "getnameinfo" | "wcstombs" if uclibc && linux => true,


I wonder, should most of these just not be present in the libc crate? Especially the unimplemented ones seem like they shouldn't exist, but including the optional ones is likely ok. It'd be great though if we could verify against a build of uclibc with all optional extensions compiled in.

Fixed (except for the locale ones and backtrace (which I think is also an optional add on?))

alexcrichton · 2017-04-20T23:12:05Z

libc-test/build.rs

+        // uClibc differs in signedness
+        (uclibc && struct_ == "sigaction" && field == "sa_flags") ||
+        // same here
+        (uclibc && struct_ == "msghdr" && field == "msg_iovlen")


Could these be corrected in the uclibc definitions to have the corresponding signededness according to the uclibc header files?

My bad, fixed!

cactorium · 2017-04-21T22:48:11Z

Thank you for the fast reply :)! Would you have any advice on how to separate it out more cleanly? uclibc causes issues at both the notbsd, linux, and mips modules, so I'm not sure where it'd make sense to add in the new module files? So far I've moved a lot of the conflicting definitions in linux (which is where most of the conflicts are) into a notuclibc module, and used #cfgs at the other levels

cactorium · 2017-04-21T22:57:00Z

Also it looks like AF_MAX is set incorrectly on x86_64-unknown-linux, at least on my system libc-test says that it should be 43 instead of 42. Would it be better to open a new pull request to fix that, or just include that with this one?

alexcrichton · 2017-04-24T14:18:07Z

Oh I'd just start a brand new uclibc module instead of a notuclibc hierarchy. This'll likely result in a lot of duplication, but that's ok.

I'd probably just add a clause to ignore AF_MAX, that's something that's likely to change a lot!

alexcrichton · 2017-04-24T14:19:24Z

In general libc avoids #[cfg] directives except inside of the "dispatch to the next layer of the hierarchy" cfg_if! statement. This is mostly to assist with readability as otherwise it gets really hard to know what platform defines what.

To reiterate, though, duplication is totally ok. The only risk is that when moving things around something accidentally gets lost, but it's at least all verified to be correct!

cactorium · 2017-04-25T02:59:39Z

Okay, I think I understand now :)! Is adding a uclibc module in the unix folder an okay place for the split? It looks kinda weird to be me there (because it's the only subdirectory there that isn't an OS), but any lower and there'd be issues because of API differences already. Also would it make sense to bump up AF_MAX to 43 anyways? At least to me it seems like it'd make sense to try to keep track of AF_MAX, even if it changes pretty often!

alexcrichton · 2017-04-26T04:44:24Z

Yeah adding a uclibc folder is fine by me, it's sort of just whatever's the most natural organization today. The idea is to maximize the amount of sharing where possible but that doesn't tend to happen a lot, especially on the Linux side of things.

alexcrichton · 2017-04-26T04:44:56Z

libc-test/build.rs

+            "LC_CTYPE_MASK" | "LC_NUMERIC_MASK" | "LC_TIME_MASK" | "LC_COLLATE_MASK" | "LC_MONETARY_MASK" | "LC_MESSAGES_MASK" |
+            "MADV_MERGEABLE" | "MADV_UNMERGEABLE" | "MADV_HWPOISON" | "IPV6_ADD_MEMBERSHIP" | "IPV6_DROP_MEMBERSHIP" | "IPV6_MULTICAST_LOOP" | "IPV6_V6ONLY" |
+            "MAP_STACK" | "RTLD_DEEPBIND" | "MAP_HUGETLB" |
+            "SOL_IPV6" | "SOL_ICMPV6" if uclibc => true,


Since uclibc has its own module now, most of these can be removed, right?

Good catch! I'm still trying to work out how to enable the second set of ignored constants, but I unimplemented the first set at least

Did you want to work on removing this clause as well as the one below that were added?

I'm pretty sure these need to be ignored, because you can grep them in the codebase, but compiling fails, so I'm assuming these are optional features that my test case (OpenWRT) doesn't have built in. At least to me it seems like ignoring them is the best solution for now

Shouldn't these constants just be deleted from uclibc files in this crate? If they don't actually exist in uclibc it seems like we shouldn't define them?

Agh, sorry, I'm guessing I wasn't being as clear as I should've been. These do exist in uclibc, but they appear to be optionally enabled, because my test case with the OpenWRT toolchain doesn't have them, and they're in #ifdef guards in the headers. The ones that were a few lines above these that were removed definitely didn't exist

Oh ok, that's fine yeah. Should we delete the ones that definitely don't exist, update the comment to match the one below, and then merge?

alexcrichton · 2017-04-26T04:45:17Z

src/unix/mod.rs

+    if #[cfg(target_env = "uclibc")] {
+        mod uclibc;
+        pub use self::uclibc::*;
+    } else if #[cfg(any(target_os = "linux",
                 target_os = "android",


Could you touch up the indentation on this line while you're at it?

Done I think

alexcrichton · 2017-04-26T04:46:13Z

This is looking great to me, thanks for doing the legwork here! How confident are you in all the various definitions for each architecture? Should this only add i686/x86_64 for now?

cactorium · 2017-04-27T03:24:56Z

So I've actually been cross compiling this for mips to use with an OpenWRT router, so I'm fairly confident that it works for that one at least! I just spent some time trying to test against x86 and x86_64, but Rust doesn't seem to have targets for using x86{,_64}-linux-uclibc

alexcrichton · 2017-04-27T14:50:01Z

Ok maybe we should start out with just the mips targets in that case then? I figure we can add the others in over time, running tests as they come in.

cactorium · 2017-04-27T15:23:25Z

Sounds good to me! Is that something that I should try to add in this pull request?

alexcrichton · 2017-04-28T03:30:57Z

Yeah let's update this PR as I think it'd be deleting the non-musl modules, right?

cactorium · 2017-04-29T01:22:15Z

Sorry, I don't understand what you mean

alexcrichton · 2017-04-29T18:37:10Z

Oh sure I'll clarify. So this PR adds "support" for a ton of uclibc architectures, but only tested on one. Could all untested architectres be removed from this PR?

cactorium · 2017-04-30T13:23:31Z

Okay, I think I got it. So basically just delete the unsupported modules?

bors · 2017-05-03T22:51:49Z

☔ The latest upstream changes (presumably #585) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2017-05-05T15:33:38Z

@cactorium wanna update the comment and otherwise get ready to merge?

cactorium · 2017-05-06T06:52:53Z

Hey! Sorry for the delay, I was busy with my sister's graduation. Stuff should be good now I think!

alexcrichton · 2017-05-08T14:49:09Z

@bors: r+

Thanks!

bors · 2017-05-08T14:49:10Z

📌 Commit c0bec43 has been approved by alexcrichton

bors · 2017-05-08T14:49:16Z

⌛ Testing commit c0bec43 with merge 19f3be3...

Steps towards support for musl-unknown-linux-uclibc Hello! I've been working towards resolving #361 , this PR compiles successfully with a newish compiler (with some minor fixes in `gcc`, `ctest`), and all the tests pass for `libc-ctest`. Basically most of the undefined functions, constants, and structs were just removed from the ctest, and then any constants that weren't correct were fixed. Would it make more sense to conditionally remove them from libc? I wasn't sure when it was appropriate to skip the test for it instead of removing the function/constants, so I just removed all the tests for now because that was a little easier than hunting them down. I'm also guessing the way some of the constants were conditionally set wasn't the correct style, would you guys have any advice on how to do it more correctly? Lemme know how it looks!

bors · 2017-05-08T20:10:03Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 19f3be3 to master...

Bump to 0.2.23 I've added a bunch of constants and xattr support for macos. We've also got preliminary support for uclibc with #578.

cactorium added 3 commits April 20, 2017 02:37

Add changes to allow uclibc to build

9a83558

Fix constants for mips-unknown-linux-uclibc

3e70b66

Fix more stuff after updating libc

2d6c7da

alexcrichton reviewed Apr 20, 2017

View reviewed changes

cactorium added 4 commits April 21, 2017 17:02

Omit lots of definitions from mips-unknown-linux-uclibc

56d2d47

Fix build errors for mips-unknown-linux-uclibc

26670c7

Fix more libc-test fails for mips-unknown-linux-uclibc

15c9447

Remove extra comment

2a9b647

cactorium added 5 commits April 24, 2017 21:08

Restructure with separate uclibc branch

6e4beb6

Remove extraneous error file

b956976

Remove uclibc references from notbsd module

7f015b0

Style fixes

ac7bff7

Fix more style

f9ddaf0

Trigger another CI build

8c592f1

alexcrichton reviewed Apr 26, 2017

View reviewed changes

Fix indentation, remove more constants that don't work for uclibc

c14178c

Remove unsupported targets

052d91d

cactorium force-pushed the master branch from 3bdd28b to 052d91d Compare April 30, 2017 13:24

Remove mips64-linux-uclibc as a target

779984b

Merge branch 'master' into master

96a8306

Remove MAP_HUGETLB from mips-unknown-linux-uclibc

c0bec43

bors merged commit c0bec43 into rust-lang:master May 8, 2017

bors mentioned this pull request May 8, 2017

Add experimental L4re support #591

Merged

PlasmaPower mentioned this pull request May 19, 2017

Bump to 0.2.23 #597

Merged

bors added a commit that referenced this pull request May 19, 2017

Auto merge of #597 - PlasmaPower:bump-version, r=alexcrichton

7db3fd5

Bump to 0.2.23 I've added a bunch of constants and xattr support for macos. We've also got preliminary support for uclibc with #578.

Steps towards support for musl-unknown-linux-uclibc #578

Steps towards support for musl-unknown-linux-uclibc #578

Conversation

cactorium commented Apr 20, 2017

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cactorium commented Apr 21, 2017 • edited Loading

cactorium commented Apr 21, 2017

alexcrichton commented Apr 24, 2017

alexcrichton commented Apr 24, 2017 • edited Loading

cactorium commented Apr 25, 2017

alexcrichton commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Apr 26, 2017

cactorium commented Apr 27, 2017

alexcrichton commented Apr 27, 2017

cactorium commented Apr 27, 2017

alexcrichton commented Apr 28, 2017

cactorium commented Apr 29, 2017

alexcrichton commented Apr 29, 2017

cactorium commented Apr 30, 2017

bors commented May 3, 2017

alexcrichton commented May 5, 2017

cactorium commented May 6, 2017

alexcrichton commented May 8, 2017

bors commented May 8, 2017

bors commented May 8, 2017

bors commented May 8, 2017

cactorium commented Apr 21, 2017 •

edited

Loading

alexcrichton commented Apr 24, 2017 •

edited

Loading