Fix building libstd on emscripten targets. #31986

ashleysommer · Mar 1, 2016

The main cause of the problem is that libstd/os/mod.rs treats emscripten targets as an alias of linux targets, whereas liblibc treats emscripten targets as musl-compliant, so it gets a slightly different struct stat64 defined.
This commit adds conditional compilation checks to use the correct timestamp format on fs metadata functions in the case of compiling to emscripten targets.

This commit also depends needs https://github.com/ashleysommer/rust/commit/f1575cff2d631e977038fdba3fa3422ba5f8f2fe applied in order to successfully build libstd with emscripten target.

rust-highfive · Mar 1, 2016

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

ashleysommer · Mar 1, 2016

This is the error I was getting before the changes:

os/linux/fs.rs:19:5: 19:19 error: unresolved import `os::linux::raw`. Could not find `linux` in `os` [E0432]
os/linux/fs.rs:19 use os::linux::raw;
                      ^~~~~~~~~~~~~~
os/linux/fs.rs:19:5: 19:19 help: run `rustc --explain E0432` to see a detailed explanation
os/linux/fs.rs:76:16: 76:65 error: casting `&libc::unix::notbsd::linux::musl::b32::asmjs::stat` as `*const libc::unix::notbsd::linux::musl::b32::asmjs::stat64` is invalid
os/linux/fs.rs:76             &*(self.as_inner().as_inner() as *const libc::stat64
                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
os/linux/fs.rs:105:9: 105:44 error: attempted access of field `st_atime` on type `&libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
os/linux/fs.rs:105         self.as_inner().as_inner().st_atime as i64
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
os/linux/fs.rs:108:9: 108:49 error: attempted access of field `st_atime_nsec` on type `&libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
os/linux/fs.rs:108         self.as_inner().as_inner().st_atime_nsec as i64
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
os/linux/fs.rs:111:9: 111:44 error: attempted access of field `st_mtime` on type `&libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
os/linux/fs.rs:111         self.as_inner().as_inner().st_mtime as i64
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
os/linux/fs.rs:114:9: 114:49 error: attempted access of field `st_mtime_nsec` on type `&libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
os/linux/fs.rs:114         self.as_inner().as_inner().st_mtime_nsec as i64
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
os/linux/fs.rs:117:9: 117:44 error: attempted access of field `st_ctime` on type `&libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
os/linux/fs.rs:117         self.as_inner().as_inner().st_ctime as i64
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
os/linux/fs.rs:120:9: 120:49 error: attempted access of field `st_ctime_nsec` on type `&libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
os/linux/fs.rs:120         self.as_inner().as_inner().st_ctime_nsec as i64
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sys/unix/fs.rs:153:21: 153:39 error: attempted access of field `st_mtime` on type `libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
sys/unix/fs.rs:153             tv_sec: self.stat.st_mtime as libc::time_t,
                                       ^~~~~~~~~~~~~~~~~~
sys/unix/fs.rs:153:31: 153:39 help: did you mean `st_mtim`?
sys/unix/fs.rs:153             tv_sec: self.stat.st_mtime as libc::time_t,
                                                 ^~~~~~~~
sys/unix/fs.rs:154:22: 154:45 error: attempted access of field `st_mtime_nsec` on type `libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
sys/unix/fs.rs:154             tv_nsec: self.stat.st_mtime_nsec as libc::c_long,
                                        ^~~~~~~~~~~~~~~~~~~~~~~
sys/unix/fs.rs:154:32: 154:45 help: did you mean `st_mtim`?
sys/unix/fs.rs:154             tv_nsec: self.stat.st_mtime_nsec as libc::c_long,
                                                  ^~~~~~~~~~~~~
sys/unix/fs.rs:160:21: 160:39 error: attempted access of field `st_atime` on type `libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
sys/unix/fs.rs:160             tv_sec: self.stat.st_atime as libc::time_t,
                                       ^~~~~~~~~~~~~~~~~~
sys/unix/fs.rs:160:31: 160:39 help: did you mean `st_atim`?
sys/unix/fs.rs:160             tv_sec: self.stat.st_atime as libc::time_t,
                                                 ^~~~~~~~
sys/unix/fs.rs:161:22: 161:45 error: attempted access of field `st_atime_nsec` on type `libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
sys/unix/fs.rs:161             tv_nsec: self.stat.st_atime_nsec as libc::c_long,
                                        ^~~~~~~~~~~~~~~~~~~~~~~
sys/unix/fs.rs:161:32: 161:45 help: did you mean `st_atim`?
sys/unix/fs.rs:161             tv_nsec: self.stat.st_atime_nsec as libc::c_long,
                                                  ^~~~~~~~~~~~~
error: aborting due to 11 previous errors
Could not compile `std`.

ashleysommer · Mar 1, 2016

I'm believe libstd used to build fine with emscripten targets, but I think this commit broke it: aa23c98

These new fs metadata functions as defined in libstd/os/linux/fs.rs assume the stat64 struct to be defined as per liblibc/src/unix/notbsd/linux/other/b32/mod.rs but instead liblibc uses liblibc/src/unix/notbsd/linux/musl/b32/asmjs.rs for emscripten targets, which has different fields in its stat64.

tomaka · Mar 1, 2016

tomaka reviewed Mar 1, 2016

View changes

tomaka · Mar 1, 2016

src/libstd/os/linux/fs.rs

+#[allow(deprecated)]
+use os::emscripten::raw;
+
+#[cfg(not(target_os = "emscripten"))]


That could be replaced with just use super::raw;.

alexcrichton · Mar 1, 2016

The os::linux::fs module is actually intended to only be compiled on target_os = "linux", could you add a new os::emscripten module for the emscripten port? (that was the original intention)

alexcrichton · Mar 1, 2016

alexcrichton reviewed Mar 1, 2016

View changes

alexcrichton · Mar 1, 2016

src/libstd/sys/unix/fs.rs

-            tv_sec: self.stat.st_atime as libc::time_t,
-            tv_nsec: self.stat.st_atime_nsec as libc::c_long,
-        }))
+        #[cfg(any(target_env = "musl", target_os = "emscripten"))]


Some of these additions of musl may not be necessary because the standard library is already guaranteed to compile for x86_64-unknown-linux-musl

alexcrichton · Mar 1, 2016

alexcrichton reviewed Mar 1, 2016

View changes

alexcrichton · Mar 1, 2016

src/libstd/sys/unix/fs.rs

-            tv_nsec: self.stat.st_birthtime_nsec as libc::c_long,
-        }))
+        #[cfg(any(target_os = "emscripten"))]
+        { return Ok(SystemTime::from( self.stat.st_ctim )); }


I think that ctim isn't the created time but rather the time of the last status change.

ashleysommer · Mar 1, 2016

Ok, thanks for the tips. I reverted all the changes in libstd/os/linux/fs.rs and created a new os submodule called emscripten. I copied over the three module files from os/linux/*.rs then removed all the unneeded stuff like checking for x86_64 and checking for MIPS.

I also made some changes in the libstd/os/emscripten/linux/raw.rs file so that the stat struct better matches the stat and stat64 definition in liblibc/src/unix/notbsd/linux/musl/b32/asmjs.rs while maintaining compatibility with the expectations that libstd has about the format of raw::stat.

EDIT: Is it reasonable to assume that when compiling for target_os="emscripten" then target_arch will always be asjms?

alexcrichton · Mar 2, 2016

alexcrichton reviewed Mar 2, 2016

View changes

alexcrichton · Mar 2, 2016

src/libstd/sys/unix/fs.rs

    }

    pub fn accessed(&self) -> io::Result<SystemTime> {
-        Ok(SystemTime::from(libc::timespec {
-            tv_sec: self.stat.st_atime as libc::time_t,


Could this call self.st_atime() and self.st_atime_nsec()? I think that would help remove the #[cfg]

I've had a look at doing that, but I don't think it is possible.
st_atime() and st_atime_sec() are functions available only on structs that implement libstd::os::emscripted::fs::MetedataExt or libstd::sys::unix::ext::fs::MetadataExt but I believe the FileAttr struct doesn't implement either of those traits.
In this case the FileAttr struct only has access to its own inner stat: stat64 struct.

Oh, right.

In the interest of consistency with other platforms, I think the best solution here would actually be to just modify the libc bindings themselves. Anyone using them manually will require these #[cfg] directives anyway, so we may as well get them all aligned.

Can you elaborate on what you mean here?
Are you saying to change liblibc so that it uses the definitions in unix/notbsd/linux/other/b32 rather than unix/notbsd/linux/musl/b32/asmjs for emscripten targets?
Im not sure if it will be as easy as that, but I can try it out to see how it works.

If we do that, and it works, then most of the changes in this PR will not be required.

Yeah for all other platforms we store the two fields in st_atim differently, one for the seconds and one for the nanoseconds. Looks like we accidentally used timeval here and we should just switch to two separate fields like in other platofrms.

I dont think it was an accident. It was most likely copied from here:
https://github.com/kripken/emscripten/blob/incoming/system/lib/libc/musl/arch/emscripten/bits/stat.h

Yeah, but to mirror how other platforms work and avoid this #[cfg] we expand the struct to an equivalent set of individual fields.

Oh I get it. Yes, I see it the struct will still be byte-compatible and equivalent with individual fields.

Ah I think it's actually getting taken care of in rust-lang/libc#210, once that's merged this can probably just update libc again

Ok, I saw the change was merged, so I targeted libc to that commit, and undone these lines.

alexcrichton · Mar 2, 2016

alexcrichton reviewed Mar 2, 2016

View changes

alexcrichton · Mar 2, 2016

src/libstd/sys/unix/fs.rs

@@ -174,7 +191,8 @@ impl FileAttr {

    #[cfg(not(any(target_os = "bitrig",
                  target_os = "freebsd",
-                  target_os = "openbsd")))]
+                  target_os = "openbsd"
+                  )))]


This and the above change look like they may no longer be necessary

alexcrichton · Mar 2, 2016

alexcrichton reviewed Mar 2, 2016

View changes

alexcrichton · Mar 2, 2016

src/libstd/sys/unix/fs.rs

@@ -25,15 +25,21 @@ use sys::time::SystemTime;
 use sys::{cvt, cvt_r};
 use sys_common::{AsInner, FromInner};

-#[cfg(target_os = "linux")]
+#[cfg(any(target_os = "linux", target_env = "musl", target_os = "emscripten"))]


Because MUSL implies target_os = "linux" I think that clause can be removed from this and the bits below

ashleysommer · Mar 2, 2016

Should I squash all these commits together?

ashleysommer · Mar 7, 2016

Updated to liblibc e19309c, then took out the extra compile-time cfg checks for emscripten in the FileAttr struct impl. Also rebased to current master, and rebuilt locally and ran all my compilation and usability tests.

alexcrichton · Mar 7, 2016

Thanks! Could you also squash the commits down together as well?

alexcrichton · Mar 7, 2016

alexcrichton reviewed Mar 7, 2016

View changes

alexcrichton · Mar 7, 2016

src/libstd/sys/unix/fs.rs

@@ -156,7 +160,7 @@ impl FileAttr {
    }

    pub fn accessed(&self) -> io::Result<SystemTime> {
-        Ok(SystemTime::from(libc::timespec {
+        return Ok(SystemTime::from(libc::timespec {


This change can probably be left out for now

Yep, that was an accident. Shouldn't have been in the commit.

alexcrichton · Mar 7, 2016

@bors: r+ 660bbf4

bors · Mar 7, 2016

⌛️ Testing commit 660bbf4 with merge d5d838b...

bors · Mar 7, 2016

💔 Test failed - auto-win-gnu-64-nopt-t

alexcrichton · Mar 7, 2016

@bors: retry

On Mon, Mar 7, 2016 at 5:46 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-win-gnu-64-nopt-t
http://buildbot.rust-lang.org/builders/auto-win-gnu-64-nopt-t/builds/3341

—
Reply to this email directly or view it on GitHub
#31986 (comment).

bors · Mar 7, 2016

⌛️ Testing commit 660bbf4 with merge e06d2ad...

alexcrichton · Mar 8, 2016

@bors: retry force

bors · Mar 8, 2016

⌛️ Testing commit 660bbf4 with merge 4352a85...

bors · Mar 8, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

rust-highfive assigned brson Mar 1, 2016

bors merged commit 660bbf4 into rust-lang:master Mar 8, 2016
2 checks passed

2 checks passed

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

homu Test successful
Details

ashleysommer deleted the ashleysommer:emscripten_fixes branch Mar 8, 2016

gifnksm referenced this pull request Mar 8, 2016
Closed
libstd fails to build on emscripten #31951

rust-lang/rust

Join GitHub today

Fix building libstd on emscripten targets. #31986

Conversation

ashleysommer commented Mar 1, 2016

rust-highfive assigned brson Mar 1, 2016

This comment has been minimized.

rust-highfive commented Mar 1, 2016

This comment has been minimized.

ashleysommer commented Mar 1, 2016

This comment has been minimized.

ashleysommer commented Mar 1, 2016

tomaka reviewed Mar 1, 2016 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton commented Mar 1, 2016

alexcrichton reviewed Mar 1, 2016 View changes

This comment has been minimized.

alexcrichton reviewed Mar 1, 2016 View changes

This comment has been minimized.

This comment has been minimized.

ashleysommer commented Mar 1, 2016

alexcrichton reviewed Mar 2, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Mar 2, 2016 View changes

This comment has been minimized.

alexcrichton reviewed Mar 2, 2016 View changes

This comment has been minimized.

This comment has been minimized.

ashleysommer commented Mar 2, 2016

This comment has been minimized.

ashleysommer commented Mar 7, 2016

This comment has been minimized.

alexcrichton commented Mar 7, 2016

alexcrichton reviewed Mar 7, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton commented Mar 7, 2016

bors added a commit that referenced this pull request Mar 7, 2016

This comment has been minimized.

bors commented Mar 7, 2016

This comment has been minimized.

bors commented Mar 7, 2016

This comment has been minimized.

alexcrichton commented Mar 7, 2016

This comment has been minimized.

bors commented Mar 7, 2016

bors added a commit that referenced this pull request Mar 7, 2016

This comment has been minimized.

alexcrichton commented Mar 8, 2016

This comment has been minimized.

bors commented Mar 8, 2016

bors added a commit that referenced this pull request Mar 8, 2016

This comment has been minimized.

bors commented Mar 8, 2016

Hide details View details bors merged commit 660bbf4 into rust-lang:master Mar 8, 2016 2 checks passed

2 checks passed

ashleysommer deleted the ashleysommer:emscripten_fixes branch Mar 8, 2016

gifnksm referenced this pull request Mar 8, 2016

pmatos pushed a commit to LinkiTools/rust that referenced this pull request Sep 27, 2016

tomaka reviewed Mar 1, 2016

View changes

alexcrichton reviewed Mar 1, 2016

View changes

alexcrichton reviewed Mar 1, 2016

View changes

alexcrichton reviewed Mar 2, 2016

View changes

alexcrichton reviewed Mar 2, 2016

View changes

alexcrichton reviewed Mar 2, 2016

View changes

alexcrichton reviewed Mar 7, 2016

View changes

bors merged commit `660bbf4` into rust-lang:master Mar 8, 2016
2 checks passed