From 013d2d212351e0553c0b1b129ad811b6c8bdfddc Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 22 Aug 2023 16:17:02 +0100 Subject: [PATCH 1/6] std: unix process_unsupported: Provide a wait status emulation Fixes #114593 Needs FCP due to behavioural changes. --- .../std/src/sys/unix/process/process_unix.rs | 5 + .../sys/unix/process/process_unsupported.rs | 52 +------- .../process_unsupported/wait_status.rs | 113 ++++++++++++++++++ 3 files changed, 120 insertions(+), 50 deletions(-) create mode 100644 library/std/src/sys/unix/process/process_unsupported/wait_status.rs diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index 3963e7f52d552..7d10a16cfd160 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -1061,3 +1061,8 @@ impl crate::os::linux::process::ChildExt for crate::process::Child { #[cfg(test)] #[path = "process_unix/tests.rs"] mod tests; + +// See [`process_unsupported_wait_status::compare_with_linux`]; +#[cfg(all(test, target_os = "linux"))] +#[path = "process_unsupported/wait_status.rs"] +mod process_unsupported_wait_status; diff --git a/library/std/src/sys/unix/process/process_unsupported.rs b/library/std/src/sys/unix/process/process_unsupported.rs index 8e0b971af7316..325d3f23be7c2 100644 --- a/library/std/src/sys/unix/process/process_unsupported.rs +++ b/library/std/src/sys/unix/process/process_unsupported.rs @@ -55,56 +55,8 @@ impl Process { } } -#[derive(PartialEq, Eq, Clone, Copy, Debug, Default)] -pub struct ExitStatus(c_int); - -impl ExitStatus { - #[cfg_attr(target_os = "horizon", allow(unused))] - pub fn success(&self) -> bool { - self.code() == Some(0) - } - - pub fn exit_ok(&self) -> Result<(), ExitStatusError> { - Err(ExitStatusError(1.try_into().unwrap())) - } - - pub fn code(&self) -> Option { - None - } - - pub fn signal(&self) -> Option { - None - } - - pub fn core_dumped(&self) -> bool { - false - } - - pub fn stopped_signal(&self) -> Option { - None - } - - pub fn continued(&self) -> bool { - false - } - - pub fn into_raw(&self) -> c_int { - 0 - } -} - -/// Converts a raw `c_int` to a type-safe `ExitStatus` by wrapping it without copying. -impl From for ExitStatus { - fn from(a: c_int) -> ExitStatus { - ExitStatus(a as i32) - } -} - -impl fmt::Display for ExitStatus { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "exit code: {}", self.0) - } -} +mod wait_status; +pub use wait_status::ExitStatus; #[derive(PartialEq, Eq, Clone, Copy, Debug)] pub struct ExitStatusError(NonZero_c_int); diff --git a/library/std/src/sys/unix/process/process_unsupported/wait_status.rs b/library/std/src/sys/unix/process/process_unsupported/wait_status.rs new file mode 100644 index 0000000000000..30fc78db9a185 --- /dev/null +++ b/library/std/src/sys/unix/process/process_unsupported/wait_status.rs @@ -0,0 +1,113 @@ +//! Emulated wait status for non-Unix #[cfg(unix) platforms +//! +//! Separate module to facilitate testing against a real Unix implementation. + +use crate::ffi::c_int; +use crate::fmt; + +/// Emulated wait status for use by `process_unsupported.rs` +/// +/// Uses the "traditional unix" encoding. For use on platfors which are `#[cfg(unix)]` +/// but do not actually support subprocesses at all. +/// +/// These platforms aren't Unix, but are simply pretending to be for porting convenience. +/// So, we provide a faithful pretence here. +#[derive(PartialEq, Eq, Clone, Copy, Debug, Default)] +pub struct ExitStatus { + wait_status: c_int, +} + +/// Converts a raw `c_int` to a type-safe `ExitStatus` by wrapping it +impl From for ExitStatus { + fn from(wait_status: c_int) -> ExitStatus { + ExitStatus { wait_status } + } +} + +impl fmt::Display for ExitStatus { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "emulated wait status: {}", self.wait_status) + } +} + +impl ExitStatus { + pub fn code(&self) -> Option { + // Linux and FreeBSD both agree that values linux 0x80 + // count as "WIFEXITED" even though this is quite mad. + // Likewise the macros disregard all the high bits, so are happy to declare + // out-of-range values to be WIFEXITED, WIFSTOPPED, etc. + let w = self.wait_status; + if (w & 0x7f) == 0 { + Some((w & 0xff00) >> 8) + } else { + None + } + } + + pub fn signal(&self) -> Option { + let signal = self.wait_status & 0x007f; + if signal > 0 && signal < 0x7f { + Some(signal) + } else { + None + } + } + + pub fn core_dumped(&self) -> bool { + self.signal().is_some() && (self.wait_status & 0x80) != 0 + } + + pub fn stopped_signal(&self) -> Option { + let w = self.wait_status; + if (w & 0xff) == 0x7f { + Some((w & 0xff00) >> 8) + } else { + None + } + } + + pub fn continued(&self) -> bool { + self.wait_status == 0xffff + } + + pub fn into_raw(&self) -> c_int { + self.wait_status + } +} + +// Test that our emulation exactly matches Linux +// +// This test runs *on Linux* but it tests +// the implementation used on non-Unix `#[cfg(unix)]` platforms. +// +// I.e. we're using Linux as a proxy for "trad unix". +#[cfg(all(test, target_os = "linux"))] +mod compare_with_linux { + use super::ExitStatus as Emulated; + use crate::process::ExitStatus as Real; + use crate::os::unix::process::ExitStatusExt as _; + + #[test] + fn compare() { + // Check that we handle out-of-range values similarly, too. + for wstatus in -0xf_ffff..0xf_ffff { + let emulated = Emulated::from(wstatus); + let real = Real::from_raw(wstatus); + + macro_rules! compare { { $method:ident } => { + assert_eq!( + emulated.$method(), + real.$method(), + "{wstatus:#x}.{}()", + stringify!($method), + ); + } } + compare!(code); + compare!(signal); + compare!(core_dumped); + compare!(stopped_signal); + compare!(continued); + compare!(into_raw); + } + } +} From 06567ad7fafcf0a2b380a8908d004a3ca8c88333 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 22 Aug 2023 17:29:17 +0100 Subject: [PATCH 2/6] std: unix process: Test exit statuses / wait statuses This is a pretty basic test but should spot any other platforms which are `#[cfg(unix)]` but not Unix and where the wait status representation is wrong. (And any actual Unix platforms where it's not as expected, but I don't think they exist.) --- .../sys/unix/process/process_common/tests.rs | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/library/std/src/sys/unix/process/process_common/tests.rs b/library/std/src/sys/unix/process/process_common/tests.rs index 03631e4e33bf5..082c564700f08 100644 --- a/library/std/src/sys/unix/process/process_common/tests.rs +++ b/library/std/src/sys/unix/process/process_common/tests.rs @@ -159,3 +159,36 @@ fn test_program_kind() { ); } } + +// Test that Rust std handles wait status values (`ExitStatus`) the way that Unix does, +// at least for the values which represent a Unix exit status (`ExitCode`). +// Should work on every #[cfg(unix)] platform. However: +#[cfg(not(any( + // Fuchsia is not Unix and has totally broken std::os::unix. + // https://github.com/rust-lang/rust/issues/58590#issuecomment-836535609 + target_os = "fuchsia", +)))] +#[test] +fn unix_exit_statuses() { + use crate::num::NonZeroI32; + use crate::os::unix::process::ExitStatusExt; + use crate::process::*; + + for exit_code in 0..=0xff { + // TODO impl From for ExitStatus and then test that here too; + // the two ExitStatus values should be the same + let raw_wait_status = exit_code << 8; + let exit_status = ExitStatus::from_raw(raw_wait_status); + + assert_eq!(exit_status.code(), Some(exit_code)); + + if let Ok(nz) = NonZeroI32::try_from(exit_code) { + assert!(!exit_status.success()); + let es_error = exit_status.exit_ok().unwrap_err(); + assert_eq!(es_error.code().unwrap(), i32::from(nz)); + } else { + assert!(exit_status.success()); + assert_eq!(exit_status.exit_ok(), Ok(())); + } + } +} From 9738a07f986eb6c59ad309e66fa5f94a95d91309 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 22 Aug 2023 19:01:46 +0100 Subject: [PATCH 3/6] fixup! std: unix process_unsupported: Provide a wait status emulation --- .../std/src/sys/unix/process/process_unsupported/wait_status.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/sys/unix/process/process_unsupported/wait_status.rs b/library/std/src/sys/unix/process/process_unsupported/wait_status.rs index 30fc78db9a185..17004779a62e1 100644 --- a/library/std/src/sys/unix/process/process_unsupported/wait_status.rs +++ b/library/std/src/sys/unix/process/process_unsupported/wait_status.rs @@ -84,8 +84,8 @@ impl ExitStatus { #[cfg(all(test, target_os = "linux"))] mod compare_with_linux { use super::ExitStatus as Emulated; - use crate::process::ExitStatus as Real; use crate::os::unix::process::ExitStatusExt as _; + use crate::process::ExitStatus as Real; #[test] fn compare() { From 2727f82526ba1191ed6faaa5ab118833aa2fffdf Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 22 Aug 2023 19:02:06 +0100 Subject: [PATCH 4/6] std: unix process_unsupported: Provide a wait status emulation (fmt) Worsify formatting as required by rustfmt. --- .../process/process_unsupported/wait_status.rs | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/library/std/src/sys/unix/process/process_unsupported/wait_status.rs b/library/std/src/sys/unix/process/process_unsupported/wait_status.rs index 17004779a62e1..f465946a52887 100644 --- a/library/std/src/sys/unix/process/process_unsupported/wait_status.rs +++ b/library/std/src/sys/unix/process/process_unsupported/wait_status.rs @@ -37,20 +37,12 @@ impl ExitStatus { // Likewise the macros disregard all the high bits, so are happy to declare // out-of-range values to be WIFEXITED, WIFSTOPPED, etc. let w = self.wait_status; - if (w & 0x7f) == 0 { - Some((w & 0xff00) >> 8) - } else { - None - } + if (w & 0x7f) == 0 { Some((w & 0xff00) >> 8) } else { None } } pub fn signal(&self) -> Option { let signal = self.wait_status & 0x007f; - if signal > 0 && signal < 0x7f { - Some(signal) - } else { - None - } + if signal > 0 && signal < 0x7f { Some(signal) } else { None } } pub fn core_dumped(&self) -> bool { @@ -59,11 +51,7 @@ impl ExitStatus { pub fn stopped_signal(&self) -> Option { let w = self.wait_status; - if (w & 0xff) == 0x7f { - Some((w & 0xff00) >> 8) - } else { - None - } + if (w & 0xff) == 0x7f { Some((w & 0xff00) >> 8) } else { None } } pub fn continued(&self) -> bool { From 281b0501f622601251972a012c9405aa80726471 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 22 Aug 2023 19:43:00 +0100 Subject: [PATCH 5/6] fixup! std: unix process: Test exit statuses / wait statuses --- library/std/src/sys/unix/process/process_common/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/sys/unix/process/process_common/tests.rs b/library/std/src/sys/unix/process/process_common/tests.rs index 082c564700f08..4e41efc90962a 100644 --- a/library/std/src/sys/unix/process/process_common/tests.rs +++ b/library/std/src/sys/unix/process/process_common/tests.rs @@ -175,7 +175,7 @@ fn unix_exit_statuses() { use crate::process::*; for exit_code in 0..=0xff { - // TODO impl From for ExitStatus and then test that here too; + // FIXME impl From for ExitStatus and then test that here too; // the two ExitStatus values should be the same let raw_wait_status = exit_code << 8; let exit_status = ExitStatus::from_raw(raw_wait_status); From f6255287f7fccbf6062afe180fd54d5b11b62795 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 22 Aug 2023 20:19:58 +0100 Subject: [PATCH 6/6] std: unix process_unsupported: Provide a wait status emulation (tidy) Move tests into a module as demanded by tidy. --- .../process_unsupported/wait_status.rs | 39 ++----------------- .../process_unsupported/wait_status/tests.rs | 36 +++++++++++++++++ 2 files changed, 39 insertions(+), 36 deletions(-) create mode 100644 library/std/src/sys/unix/process/process_unsupported/wait_status/tests.rs diff --git a/library/std/src/sys/unix/process/process_unsupported/wait_status.rs b/library/std/src/sys/unix/process/process_unsupported/wait_status.rs index f465946a52887..3c649db1a35e5 100644 --- a/library/std/src/sys/unix/process/process_unsupported/wait_status.rs +++ b/library/std/src/sys/unix/process/process_unsupported/wait_status.rs @@ -63,39 +63,6 @@ impl ExitStatus { } } -// Test that our emulation exactly matches Linux -// -// This test runs *on Linux* but it tests -// the implementation used on non-Unix `#[cfg(unix)]` platforms. -// -// I.e. we're using Linux as a proxy for "trad unix". -#[cfg(all(test, target_os = "linux"))] -mod compare_with_linux { - use super::ExitStatus as Emulated; - use crate::os::unix::process::ExitStatusExt as _; - use crate::process::ExitStatus as Real; - - #[test] - fn compare() { - // Check that we handle out-of-range values similarly, too. - for wstatus in -0xf_ffff..0xf_ffff { - let emulated = Emulated::from(wstatus); - let real = Real::from_raw(wstatus); - - macro_rules! compare { { $method:ident } => { - assert_eq!( - emulated.$method(), - real.$method(), - "{wstatus:#x}.{}()", - stringify!($method), - ); - } } - compare!(code); - compare!(signal); - compare!(core_dumped); - compare!(stopped_signal); - compare!(continued); - compare!(into_raw); - } - } -} +#[cfg(test)] +#[path = "wait_status/tests.rs"] // needed because of strange layout of process_unsupported +mod tests; diff --git a/library/std/src/sys/unix/process/process_unsupported/wait_status/tests.rs b/library/std/src/sys/unix/process/process_unsupported/wait_status/tests.rs new file mode 100644 index 0000000000000..5132eab10a112 --- /dev/null +++ b/library/std/src/sys/unix/process/process_unsupported/wait_status/tests.rs @@ -0,0 +1,36 @@ +// Note that tests in this file are run on Linux as well as on platforms using process_unsupported + +// Test that our emulation exactly matches Linux +// +// This test runs *on Linux* but it tests +// the implementation used on non-Unix `#[cfg(unix)]` platforms. +// +// I.e. we're using Linux as a proxy for "trad unix". +#[cfg(target_os = "linux")] +#[test] +fn compare_with_linux() { + use super::ExitStatus as Emulated; + use crate::os::unix::process::ExitStatusExt as _; + use crate::process::ExitStatus as Real; + + // Check that we handle out-of-range values similarly, too. + for wstatus in -0xf_ffff..0xf_ffff { + let emulated = Emulated::from(wstatus); + let real = Real::from_raw(wstatus); + + macro_rules! compare { { $method:ident } => { + assert_eq!( + emulated.$method(), + real.$method(), + "{wstatus:#x}.{}()", + stringify!($method), + ); + } } + compare!(code); + compare!(signal); + compare!(core_dumped); + compare!(stopped_signal); + compare!(continued); + compare!(into_raw); + } +}