From cfc386583291c8868d093d07431a57a20e69c944 Mon Sep 17 00:00:00 2001 From: Brian Campbell Date: Sat, 6 Feb 2016 17:16:56 -0500 Subject: [PATCH] Use libc::abort, not intrinsics::abort, in rtabort! intrinsics::abort compiles down to an illegal instruction, which on Unix-like platforms causes the process to be killed with SIGILL. A more appropriate way to kill the process would be SIGABRT; this indicates better that the runtime has explicitly aborted, rather than some kind of compiler bug or architecture mismatch that SIGILL might indicate. For rtassert!, replace this with libc::abort. libc::abort raises SIGABRT, but is defined to do so in such a way that it will terminate the process even if SIGABRT is currently masked or caught by a signal handler that returns. On non-Unix platforms, retain the existing behavior. On Windows we prefer to avoid depending on the C runtime, and we need a fallback for any other platforms that may be defined. An alternative on Windows would be to call TerminateProcess, but this seems less essential than switching to using SIGABRT on Unix-like platforms, where it is common for the process-killing signal to be printed out or logged. This is a [breaking-change] for any code that depends on the exact signal raised to abort a process via rtabort! cc #31273 cc #31333 --- src/libstd/sys/common/util.rs | 26 ++++++++++++++++++++++++-- src/test/run-pass/out-of-stack.rs | 3 +-- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/libstd/sys/common/util.rs b/src/libstd/sys/common/util.rs index b7a6b7650d540..1df511a8818c4 100644 --- a/src/libstd/sys/common/util.rs +++ b/src/libstd/sys/common/util.rs @@ -10,7 +10,6 @@ use env; use fmt; -use intrinsics; use io::prelude::*; use sync::atomic::{self, Ordering}; use sys::stdio::Stderr; @@ -34,9 +33,32 @@ pub fn dumb_print(args: fmt::Arguments) { let _ = Stderr::new().map(|mut stderr| stderr.write_fmt(args)); } +// On Unix-like platforms, libc::abort will unregister signal handlers +// including the SIGABRT handler, preventing the abort from being blocked, and +// fclose streams, with the side effect of flushing them so libc bufferred +// output will be printed. Additionally the shell will generally print a more +// understandable error message like "Abort trap" rather than "Illegal +// instruction" that intrinsics::abort would cause, as intrinsics::abort is +// implemented as an illegal instruction. +#[cfg(unix)] +unsafe fn abort_internal() -> ! { + use libc; + libc::abort() +} + +// On Windows, we want to avoid using libc, and there isn't a direct +// equivalent of libc::abort. The __failfast intrinsic may be a reasonable +// substitute, but desireability of using it over the abort instrinsic is +// debateable; see https://github.com/rust-lang/rust/pull/31519 for details. +#[cfg(not(unix))] +unsafe fn abort_internal() -> ! { + use intrinsics; + intrinsics::abort() +} + pub fn abort(args: fmt::Arguments) -> ! { dumb_print(format_args!("fatal runtime error: {}\n", args)); - unsafe { intrinsics::abort(); } + unsafe { abort_internal(); } } #[allow(dead_code)] // stack overflow detection not enabled on all platforms diff --git a/src/test/run-pass/out-of-stack.rs b/src/test/run-pass/out-of-stack.rs index 4401861a1631c..58119f965cf82 100644 --- a/src/test/run-pass/out-of-stack.rs +++ b/src/test/run-pass/out-of-stack.rs @@ -46,8 +46,7 @@ fn check_status(status: std::process::ExitStatus) use std::os::unix::process::ExitStatusExt; assert!(!status.success()); - assert!(status.signal() != Some(libc::SIGSEGV) - && status.signal() != Some(libc::SIGBUS)); + assert_eq!(status.signal(), Some(libc::SIGABRT)); } #[cfg(not(unix))]