Auto merge of #29462 - alexcrichton:refactor-process-ret, r=aturon

* Store the native representation directly in the `ExitStatus` structure instead
  of a "parsed version" (mostly for Unix).
* On Windows, be more robust against processes exiting with the status of 259.
  Unfortunately this exit code corresponds to `STILL_ACTIVE`, causing libstd to
  think the process was still alive, causing an infinite loop. Instead the loop
  is removed altogether and `WaitForSingleObject` is used to wait for the
  process to exit.

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

@@ -75,10 +75,7 @@ pub trait ExitStatusExt {
 #[stable(feature = "rust1", since = "1.0.0")]
 impl ExitStatusExt for process::ExitStatus {
     fn signal(&self) -> Option<i32> {
-        match *self.as_inner() {
-            sys::process::ExitStatus::Signal(s) => Some(s),
-            _ => None
-        }
+        self.as_inner().signal()
     }
 }
 

src/libstd/sys/unix/process.rs

@@ -8,6 +8,8 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
+#![allow(non_snake_case)]
+
 use prelude::v1::*;
 use os::unix::prelude::*;
 
@@ -84,33 +86,62 @@ impl Command {
 
 /// Unix exit statuses
 #[derive(PartialEq, Eq, Clone, Copy, Debug)]
-pub enum ExitStatus {
-    /// Normal termination with an exit code.
-    Code(i32),
+pub struct ExitStatus(c_int);
 
-    /// Termination by signal, with the signal number.
-    ///
-    /// Never generated on Windows.
-    Signal(i32),
+#[cfg(any(target_os = "linux", target_os = "android",
+          target_os = "nacl"))]
+mod status_imp {
+    pub fn WIFEXITED(status: i32) -> bool { (status & 0xff) == 0 }
+    pub fn WEXITSTATUS(status: i32) -> i32 { (status >> 8) & 0xff }
+    pub fn WTERMSIG(status: i32) -> i32 { status & 0x7f }
+}
+
+#[cfg(any(target_os = "macos",
+          target_os = "ios",
+          target_os = "freebsd",
+          target_os = "dragonfly",
+          target_os = "bitrig",
+          target_os = "netbsd",
+          target_os = "openbsd"))]
+mod status_imp {
+    pub fn WIFEXITED(status: i32) -> bool { (status & 0x7f) == 0 }
+    pub fn WEXITSTATUS(status: i32) -> i32 { status >> 8 }
+    pub fn WTERMSIG(status: i32) -> i32 { status & 0o177 }
 }
 
 impl ExitStatus {
-    pub fn success(&self) -> bool {
-        *self == ExitStatus::Code(0)
+    fn exited(&self) -> bool {
+        status_imp::WIFEXITED(self.0)
     }
+
+    pub fn success(&self) -> bool {
+        self.code() == Some(0)
+    }
+
     pub fn code(&self) -> Option<i32> {
-        match *self {
-            ExitStatus::Code(c) => Some(c),
-            _ => None
+        if self.exited() {
+            Some(status_imp::WEXITSTATUS(self.0))
+        } else {
+            None
+        }
+    }
+
+    pub fn signal(&self) -> Option<i32> {
+        if !self.exited() {
+            Some(status_imp::WTERMSIG(self.0))
+        } else {
+            None
         }
     }
 }
 
 impl fmt::Display for ExitStatus {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match *self {
-            ExitStatus::Code(code) =>  write!(f, "exit code: {}", code),
-            ExitStatus::Signal(code) =>  write!(f, "signal: {}", code),
+        if let Some(code) = self.code() {
+            write!(f, "exit code: {}", code)
+        } else {
+            let signal = self.signal().unwrap();
+            write!(f, "signal: {}", signal)
         }
     }
 }
@@ -351,7 +382,7 @@ impl Process {
     pub fn wait(&self) -> io::Result<ExitStatus> {
         let mut status = 0 as c_int;
         try!(cvt_r(|| unsafe { c::waitpid(self.pid, &mut status, 0) }));
-        Ok(translate_status(status))
+        Ok(ExitStatus(status))
     }
 
     pub fn try_wait(&self) -> Option<ExitStatus> {
@@ -360,7 +391,7 @@ impl Process {
             c::waitpid(self.pid, &mut status, c::WNOHANG)
         }) {
             Ok(0) => None,
-            Ok(n) if n == self.pid => Some(translate_status(status)),
+            Ok(n) if n == self.pid => Some(ExitStatus(status)),
             Ok(n) => panic!("unknown pid: {}", n),
             Err(e) => panic!("unknown waitpid error: {}", e),
         }
@@ -418,36 +449,6 @@ fn make_envp(env: Option<&HashMap<OsString, OsString>>)
     }
 }
 
-fn translate_status(status: c_int) -> ExitStatus {
-    #![allow(non_snake_case)]
-    #[cfg(any(target_os = "linux", target_os = "android",
-              target_os = "nacl"))]
-    mod imp {
-        pub fn WIFEXITED(status: i32) -> bool { (status & 0xff) == 0 }
-        pub fn WEXITSTATUS(status: i32) -> i32 { (status >> 8) & 0xff }
-        pub fn WTERMSIG(status: i32) -> i32 { status & 0x7f }
-    }
-
-    #[cfg(any(target_os = "macos",
-              target_os = "ios",
-              target_os = "freebsd",
-              target_os = "dragonfly",
-              target_os = "bitrig",
-              target_os = "netbsd",
-              target_os = "openbsd"))]
-    mod imp {
-        pub fn WIFEXITED(status: i32) -> bool { (status & 0x7f) == 0 }
-        pub fn WEXITSTATUS(status: i32) -> i32 { status >> 8 }
-        pub fn WTERMSIG(status: i32) -> i32 { status & 0o177 }
-    }
-
-    if imp::WIFEXITED(status) {
-        ExitStatus::Code(imp::WEXITSTATUS(status))
-    } else {
-        ExitStatus::Signal(imp::WTERMSIG(status))
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;

src/libstd/sys/windows/process.rs

@@ -201,21 +201,16 @@ impl Process {
     }
 
     pub fn wait(&self) -> io::Result<ExitStatus> {
-        use libc::{STILL_ACTIVE, INFINITE, WAIT_OBJECT_0};
+        use libc::{INFINITE, WAIT_OBJECT_0};
         use libc::{GetExitCodeProcess, WaitForSingleObject};
 
         unsafe {
-            loop {
+            if WaitForSingleObject(self.handle.raw(), INFINITE) != WAIT_OBJECT_0 {
+                return Err(Error::last_os_error())
+            }
             let mut status = 0;
             try!(cvt(GetExitCodeProcess(self.handle.raw(), &mut status)));
-                if status != STILL_ACTIVE {
-                    return Ok(ExitStatus(status as i32));
-                }
-                match WaitForSingleObject(self.handle.raw(), INFINITE) {
-                    WAIT_OBJECT_0 => {}
-                    _ => return Err(Error::last_os_error()),
-                }
-            }
+            Ok(ExitStatus(status))
         }
     }
 
@@ -225,14 +220,14 @@ impl Process {
 }
 
 #[derive(PartialEq, Eq, Clone, Copy, Debug)]
-pub struct ExitStatus(i32);
+pub struct ExitStatus(libc::DWORD);
 
 impl ExitStatus {
     pub fn success(&self) -> bool {
         self.0 == 0
     }
     pub fn code(&self) -> Option<i32> {
-        Some(self.0)
+        Some(self.0 as i32)
     }
 }
 

src/test/run-pass/weird-exit-code.rs

@@ -0,0 +1,37 @@
+// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// On Windows the GetExitCodeProcess API is used to get the exit code of a
+// process, but it's easy to mistake a process exiting with the code 259 as
+// "still running" because this is the value of the STILL_ACTIVE constant. Make
+// sure we handle this case in the standard library and correctly report the
+// status.
+//
+// Note that this is disabled on unix as processes exiting with 259 will have
+// their exit status truncated to 3 (only the lower 8 bits are used).
+
+use std::process::{self, Command};
+use std::env;
+
+fn main() {
+    if !cfg!(windows) {
+        return
+    }
+
+    if env::args().len() == 1 {
+        let status = Command::new(env::current_exe().unwrap())
+                             .arg("foo")
+                             .status()
+                             .unwrap();
+        assert_eq!(status.code(), Some(259));
+    } else {
+        process::exit(259);
+    }
+}