Fuchsia support for std::process via liblaunchpad. #37936

tedsta · 2016-11-22T20:14:12Z

Now we can launch processes on Fuchsia via the Rust standard library! ... Mostly.

Right now, ~5% of the time, reading the stdout/stderr off the pipes will fail. Some Magenta kernel people think it's probably a bug in Magenta's pipes. I wrote a unit test that demonstrates the issue in C, which I was told will expedite a fix. https://fuchsia-review.googlesource.com/#/c/15628/

Hopefully this can get merged once the issue is fixed :)

@raphlinus

rust-highfive · 2016-11-22T20:14:19Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (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.

aturon · 2016-11-22T20:54:06Z

cc @brson @alexcrichton, have one of you been looking at Fuchsia-related PRs?

alexcrichton · 2016-11-22T20:54:54Z

Yep, I'll take this one unless @brson gets to it first.

tedsta · 2016-11-23T00:27:45Z

The linter complained about my fuchsia-specific stuff in Child::wait_with_output in src/libstd/process.rs, so I took it out. Now it panics on Fuchsia due to the bug in Magenta's pipes implementation (libc::read is returning EIO when it should be returning 0 or EOF). Note that the situation isn't any worse than before, because it used to panic at fork() on Fuchsia. Once the issue is fixed on Magenta's end, this should all work.

alexcrichton · 2016-11-23T05:54:41Z

Awesome, thanks for the PR! I wonder, how close to Unix (like Linux Unix) is Fuchsia? If it's relatively far off then it may be worthwhile just creating a whole new src/libstd/sys/fuchsia instead of bolting onto the existing Unix. The changes to process.rs at least seem like they warrant a process_fuchsia.rs at least perhaps as they're changing structures and whatnot.

alexcrichton · 2016-11-23T05:55:16Z

src/libstd/sys/unix/magenta.rs

+    (hnd_type & 0xFFFF) | ((arg & 0xFFFF) << 16)
+}
+
+#[link(name="mxio")]


It's ok to nix these #[link] annotations as they get added via the build script above

alexcrichton · 2016-11-23T05:56:09Z

src/libstd/sys/unix/process.rs

+                      -> io::Result<(*mut launchpad_t, mx_handle_t)> {
+        use sys::magenta::*;
+
+        macro_rules! tlp {


Perhaps this could be done instead with a small value on the stack that has a destructor?

tedsta · 2016-11-23T17:49:29Z

I agree there should be a sys/fuchsia (or maybe sys/magenta since the kernel is called magenta?). It gets less unix-like under musl. File descriptors are bolted on to support musl. A lot of the musl stuff, like fork(), is left unchanged. The kernel will see the linux syscall and return an error.

They say eventually there will be fork emulation, but that we don't want to use it for Rust's libstd. So I wonder if we want to be using the musl's pipe stuff when we could just directly use mx_pipe_t (and all the other things we're wrapping in musl).

raphlinus · 2016-11-23T18:14:01Z

I'll try to dig in to the code soon. Quick comments: +1 to sys/fuchsia. We've been using "fuchsia" as the identifier (for example in the target triple for llvm) so I think it's best to stay consistent with that.

In general, musl (and mxio as an overlay) provide a whole bunch of posix emulation, and that's a good way to get things working, but as a long term trend I think we'd like to migrate to things running directly in terms of lower-level system functionality.

…d refactored out some now-duplicated code into a 'process_common.rs'

tedsta · 2016-11-23T22:01:23Z

Made some changes to address @alexcrichton's comments. I don't like the getters I had to add to Command to avoid making some fields public, but I can't think of a better way to keep this DRY.

alexcrichton · 2016-11-24T02:30:58Z

Oh yeah we definitely prefer to bind to the "true nature" of a platform where possible. For example we don't use file descriptors on Windows but rather use HANDLE, the actual underlying value, instead. I think it'd be fine to bind all primitives to the underlying Fuchsia primitive if they're not file descriptors as well.

alexcrichton

Looking good, thanks!

I think I'm convinced though that we may want to start out with src/libstd/sys/fuchsia rather than continue to bolt onto the existing #[cfg(unix)] category. With functionality like exec not existing and file descriptors being a shim it makes me think that we may wish to stick to a separate body of code. I'd imagine that over time if we continue to add to sys/unix that it'll tangle up the existing code quite a bit and not end up sharing much in the long run?

cc @brson, do you have thoughts?

alexcrichton · 2016-11-28T15:56:34Z

src/libstd/sys/unix/process/process_fuchsia.rs

+        } else {
+            unsafe {
+                mx_cvt(mx_handle_close(self.handle))?;
+                launchpad_destroy(self.launchpad);


If this is a "proper handle" as opposed to a pid, it may be best to do this in a Drop implementation (like Windows) perhaps?

Right, good idea

alexcrichton · 2016-11-28T15:57:18Z

src/libstd/sys/unix/process/process_fuchsia.rs

+            mx_cvt(mx_object_get_info(self.handle, MX_INFO_PROCESS,
+                                      &mut proc_info as *mut _ as *mut libc::c_void,
+                                      mem::size_of::<mx_info_process_t>(), &mut actual,
+                                      &mut avail))?;


We can likely remove the Option<ExitStatus> in this Process by deferring destruction until Drop? (similar to Windows)

It took me a while to grok this statement. I'll play around with it and see where I get.

raphlinus · 2016-11-28T19:17:04Z

Generally looks good to me (I scanned through the code but didn't go through it with a fine-toothed comb). I agree that it would probably be cleaner to separate out the fuchia stuff from the generic Unix mechanisms.

tedsta · 2016-11-29T18:00:49Z

Alright, I'll make a sys/fuchsia. Would it be alright if I keep the shims for now and replace them in a series of PRs? It's quite a project, and if it were all in one PR it'd be pretty huge!

brson · 2016-11-29T21:16:19Z

This plan sounds good to me!

alexcrichton · 2016-11-29T23:43:55Z

@tedsta yeah that's fine by me, let's go ahead and land the process changes with the organization you've got in this PR, and then we can in follow up PRs move everything to sys/fuchsia

…y launchpads and close handles in Drop impls rather than manually

alexcrichton

Just a few minor comments, but otherwise looks good to me!

alexcrichton · 2016-12-01T01:08:21Z

src/libstd/sys/unix/process/process_common.rs

+    #[cfg(not(target_os="fuchsia"))]
+    pub fn get_gid(&self) -> Option<gid_t> {
+        self.gid
+    }


These are #[cfg]'d out for lints I think, right? In that case could these just be tagged with #[allow(dead_code)]?

Right. Done.

alexcrichton · 2016-12-01T01:08:57Z

src/libstd/sys/unix/mod.rs

@@ -164,6 +171,19 @@ pub fn cvt_r<T, F>(mut f: F) -> io::Result<T>
    }
 }

+#[cfg(target_os = "fuchsia")]
+pub fn mx_cvt<T>(t: T) -> io::Result<T> where T: TryInto<mx_status_t>+Copy {


I think this is only used in the fuchsia process module, right? If so could this be moved into there for now? When we extract to sys/fuchsia we can put in src/fuchsia/mod.rs likely.

Right. Done.

alexcrichton · 2016-12-01T01:10:05Z

src/libstd/sys/unix/process/process_fuchsia.rs

+                                   &mut job_copy as *mut mx_handle_t))?;
+        // Create a launchpad
+        mx_cvt(launchpad_create(job_copy, self.get_argv()[0],
+                                &mut launchpad as *mut *mut launchpad_t))?;


&mut launchpad.0 I think here could avoid the as cast

launchpad is actually a regular *mut launchpad_t. It get's Copyed as LaunchpadDestructor.0. Should "move" it into the LaunchpadDestructor and s/LaunchpadDestructor/Launchpad instead?

alexcrichton · 2016-12-01T01:10:14Z

src/libstd/sys/unix/process/process_fuchsia.rs

+        // Duplicate the job handle
+        let mut job_copy: mx_handle_t = MX_HANDLE_INVALID;
+        mx_cvt(mx_handle_duplicate(job_handle, MX_RIGHT_SAME_RIGHTS,
+                                   &mut job_copy as *mut mx_handle_t))?;


I think the as cast here is fine to elide

alexcrichton · 2016-12-01T21:25:13Z

@bors: r+

bors · 2016-12-01T21:25:17Z

📌 Commit e1b752b has been approved by alexcrichton

@raphlinus

Fuchsia support for std::process via liblaunchpad. Now we can launch processes on Fuchsia via the Rust standard library! ... Mostly. Right now, ~5% of the time, reading the stdout/stderr off the pipes will fail. Some Magenta kernel people think it's probably a bug in Magenta's pipes. I wrote a unit test that demonstrates the issue in C, which I was told will expedite a fix. https://fuchsia-review.googlesource.com/#/c/15628/ Hopefully this can get merged once the issue is fixed :) @raphlinus

bors · 2016-12-02T11:30:55Z

Fuchsia support for std::process via liblaunchpad.

5c23f2e

rust-highfive assigned aturon Nov 22, 2016

alexcrichton assigned alexcrichton and unassigned aturon Nov 22, 2016

tedsta added 2 commits November 22, 2016 16:08

Cleaned up and appeased the linter

fae86b9

Whoops :| s/update/unwrap/

ae09957

alexcrichton reviewed Nov 23, 2016

View reviewed changes

Separated fuchsia-specific process stuff into 'process_fuchsia.rs' an…

5c1c485

…d refactored out some now-duplicated code into a 'process_common.rs'

alexcrichton reviewed Nov 28, 2016

View reviewed changes

Removed Option<ExitStatus> member from fuchsia Process struct. Destro…

8d9d07a

…y launchpads and close handles in Drop impls rather than manually

alexcrichton reviewed Dec 1, 2016

View reviewed changes

std::process fuchsia support cleanup

e1b752b

bors merged commit e1b752b into rust-lang:master Dec 2, 2016

tedsta deleted the fuchsia_std_process branch January 24, 2017 21:00

Fuchsia support for std::process via liblaunchpad. #37936

Fuchsia support for std::process via liblaunchpad. #37936

Uh oh!

Conversation

tedsta commented Nov 22, 2016

Uh oh!

rust-highfive commented Nov 22, 2016

Uh oh!

aturon commented Nov 22, 2016

Uh oh!

alexcrichton commented Nov 22, 2016

Uh oh!

tedsta commented Nov 23, 2016

Uh oh!

alexcrichton commented Nov 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tedsta commented Nov 23, 2016

Uh oh!

raphlinus commented Nov 23, 2016

Uh oh!

tedsta commented Nov 23, 2016

Uh oh!

alexcrichton commented Nov 24, 2016

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raphlinus commented Nov 28, 2016

Uh oh!

tedsta commented Nov 29, 2016

Uh oh!

brson commented Nov 29, 2016

Uh oh!

alexcrichton commented Nov 29, 2016

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Dec 1, 2016

Uh oh!

bors commented Dec 1, 2016

Uh oh!

bors commented Dec 2, 2016

Uh oh!

Uh oh!