From 44d1b149d2831192503a7797b98f108a3b5b1032 Mon Sep 17 00:00:00 2001 From: Andrea Canciani Date: Wed, 23 Sep 2015 19:47:15 +0200 Subject: [PATCH 1/3] Separate panic logging code Move the panic logging code to a function separate from `on_panic` and simplify the code to decide whether the backtrace should be logged. --- src/libstd/panicking.rs | 41 +++++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/src/libstd/panicking.rs b/src/libstd/panicking.rs index 9715939d644aa..01a3900f24fc2 100644 --- a/src/libstd/panicking.rs +++ b/src/libstd/panicking.rs @@ -24,7 +24,8 @@ thread_local! { } } -pub fn on_panic(obj: &(Any+Send), file: &'static str, line: u32) { +fn log_panic(obj: &(Any+Send), file: &'static str, line: u32, + log_backtrace: bool) { let msg = match obj.downcast_ref::<&'static str>() { Some(s) => *s, None => match obj.downcast_ref::() { @@ -35,37 +36,33 @@ pub fn on_panic(obj: &(Any+Send), file: &'static str, line: u32) { let mut err = Stderr::new().ok(); let thread = thread_info::current_thread(); let name = thread.as_ref().and_then(|t| t.name()).unwrap_or(""); + + let write = |err: &mut ::io::Write| { + let _ = writeln!(err, "thread '{}' panicked at '{}', {}:{}", + name, msg, file, line); + if log_backtrace { + let _ = backtrace::write(err); + } + }; + let prev = LOCAL_STDERR.with(|s| s.borrow_mut().take()); match (prev, err.as_mut()) { (Some(mut stderr), _) => { // FIXME: what to do when the thread printing panics? - let _ = writeln!(stderr, - "thread '{}' panicked at '{}', {}:{}\n", - name, msg, file, line); - if backtrace::log_enabled() { - let _ = backtrace::write(&mut *stderr); - } + write(&mut *stderr); let mut s = Some(stderr); LOCAL_STDERR.with(|slot| { *slot.borrow_mut() = s.take(); }); } - (None, Some(ref mut err)) => { - let _ = writeln!(err, "thread '{}' panicked at '{}', {}:{}", - name, msg, file, line); - if backtrace::log_enabled() { - let _ = backtrace::write(err); - } - } + (None, Some(ref mut err)) => { write(err) } _ => {} } +} - // If this is a double panic, make sure that we printed a backtrace - // for this panic. - match err { - Some(ref mut err) if unwind::panicking() && !backtrace::log_enabled() => { - let _ = backtrace::write(err); - } - _ => {} - } +pub fn on_panic(obj: &(Any+Send), file: &'static str, line: u32) { + // If this is a double panic, make sure that we print a backtrace + // for this panic. Otherwise only print it if logging is enabled. + let log_backtrace = unwind::panicking() || backtrace::log_enabled(); + log_panic(obj, file, line, log_backtrace); } From c7b84909b00dcf5f762778b4aa9783770c69416d Mon Sep 17 00:00:00 2001 From: Andrea Canciani Date: Thu, 24 Sep 2015 23:49:38 +0200 Subject: [PATCH 2/3] Explicitly count the number of panics Move the panic handling logic from the `unwind` module to `panicking` and use a panic counter to distinguish between normal state, panics and double panics. --- src/libstd/panicking.rs | 24 ++++++++++++++++++++++-- src/libstd/sys/common/unwind/mod.rs | 22 ++++------------------ 2 files changed, 26 insertions(+), 20 deletions(-) diff --git a/src/libstd/panicking.rs b/src/libstd/panicking.rs index 01a3900f24fc2..fc242dee99fee 100644 --- a/src/libstd/panicking.rs +++ b/src/libstd/panicking.rs @@ -12,11 +12,15 @@ use prelude::v1::*; use io::prelude::*; use any::Any; +use cell::Cell; use cell::RefCell; +use intrinsics; use sys::stdio::Stderr; use sys_common::backtrace; use sys_common::thread_info; -use sys_common::unwind; +use sys_common::util; + +thread_local! { pub static PANIC_COUNT: Cell = Cell::new(0) } thread_local! { pub static LOCAL_STDERR: RefCell>> = { @@ -61,8 +65,24 @@ fn log_panic(obj: &(Any+Send), file: &'static str, line: u32, } pub fn on_panic(obj: &(Any+Send), file: &'static str, line: u32) { + let panics = PANIC_COUNT.with(|s| { + let count = s.get() + 1; + s.set(count); + count + }); + // If this is a double panic, make sure that we print a backtrace // for this panic. Otherwise only print it if logging is enabled. - let log_backtrace = unwind::panicking() || backtrace::log_enabled(); + let log_backtrace = panics >= 2 || backtrace::log_enabled(); log_panic(obj, file, line, log_backtrace); + + if panics >= 2 { + // If a thread panics while it's already unwinding then we + // have limited options. Currently our preference is to + // just abort. In the future we may consider resuming + // unwinding or otherwise exiting the thread cleanly. + util::dumb_print(format_args!("thread panicked while panicking. \ + aborting.")); + unsafe { intrinsics::abort() } + } } diff --git a/src/libstd/sys/common/unwind/mod.rs b/src/libstd/sys/common/unwind/mod.rs index 8148bb6b7b82e..c06d7886a757b 100644 --- a/src/libstd/sys/common/unwind/mod.rs +++ b/src/libstd/sys/common/unwind/mod.rs @@ -64,9 +64,8 @@ use prelude::v1::*; use any::Any; use boxed; -use cell::Cell; use cmp; -use panicking; +use panicking::{self,PANIC_COUNT}; use fmt; use intrinsics; use mem; @@ -92,8 +91,6 @@ pub mod imp; #[path = "gcc.rs"] #[doc(hidden)] pub mod imp; -thread_local! { static PANICKING: Cell = Cell::new(false) } - /// Invoke a closure, capturing the cause of panic if one occurs. /// /// This function will return `Ok(())` if the closure did not panic, and will @@ -131,9 +128,9 @@ pub unsafe fn try(f: F) -> Result<(), Box> { // care of exposing correctly. unsafe fn inner_try(f: fn(*mut u8), data: *mut u8) -> Result<(), Box> { - PANICKING.with(|s| { + PANIC_COUNT.with(|s| { let prev = s.get(); - s.set(false); + s.set(0); let ep = intrinsics::try(f, data); s.set(prev); if ep.is_null() { @@ -161,7 +158,7 @@ pub unsafe fn try(f: F) -> Result<(), Box> { /// Determines whether the current thread is unwinding because of panic. pub fn panicking() -> bool { - PANICKING.with(|s| s.get()) + PANIC_COUNT.with(|s| s.get() != 0) } // An uninlined, unmangled function upon which to slap yer breakpoints @@ -234,17 +231,6 @@ fn begin_unwind_inner(msg: Box, // First, invoke the default panic handler. panicking::on_panic(&*msg, file, line); - if panicking() { - // If a thread panics while it's already unwinding then we - // have limited options. Currently our preference is to - // just abort. In the future we may consider resuming - // unwinding or otherwise exiting the thread cleanly. - super::util::dumb_print(format_args!("thread panicked while panicking. \ - aborting.")); - unsafe { intrinsics::abort() } - } - PANICKING.with(|s| s.set(true)); - // Finally, perform the unwinding. rust_panic(msg); } From 54c0231b14a71a46e6607255d509457e0df0b8be Mon Sep 17 00:00:00 2001 From: Andrea Canciani Date: Thu, 24 Sep 2015 17:21:29 +0200 Subject: [PATCH 3/3] Abort earlier upon multi-panics The double-panic `abort` is run after the logging code, to provide feedback in case of a double-panic. This means that if the panic logging fails with a panic, the `abort` might never be reached. This should not normally occur, but if the `on_panic` function detects more than 2 panics, aborting *before* logging makes panic handling somewhat more robust, as it avoids an infinite recursion, which would eventually crash the process, but also make the problem harder to debug. This handles the FIXME about what to do if the thread printing panics. --- src/libstd/panicking.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/libstd/panicking.rs b/src/libstd/panicking.rs index fc242dee99fee..2b2af350c992c 100644 --- a/src/libstd/panicking.rs +++ b/src/libstd/panicking.rs @@ -52,7 +52,6 @@ fn log_panic(obj: &(Any+Send), file: &'static str, line: u32, let prev = LOCAL_STDERR.with(|s| s.borrow_mut().take()); match (prev, err.as_mut()) { (Some(mut stderr), _) => { - // FIXME: what to do when the thread printing panics? write(&mut *stderr); let mut s = Some(stderr); LOCAL_STDERR.with(|slot| { @@ -71,6 +70,17 @@ pub fn on_panic(obj: &(Any+Send), file: &'static str, line: u32) { count }); + // If this is the third nested call, on_panic triggered the last panic, + // otherwise the double-panic check would have aborted the process. + // Even if it is likely that on_panic was unable to log the backtrace, + // abort immediately to avoid infinite recursion, so that attaching a + // debugger provides a useable stacktrace. + if panics >= 3 { + util::dumb_print(format_args!("thread panicked while processing \ + panic. aborting.")); + unsafe { intrinsics::abort() } + } + // If this is a double panic, make sure that we print a backtrace // for this panic. Otherwise only print it if logging is enabled. let log_backtrace = panics >= 2 || backtrace::log_enabled();