From 8bdf5c3de6c6e4e01f7f6241cd0f2a606c7486df Mon Sep 17 00:00:00 2001 From: Badel2 <2badel2@gmail.com> Date: Wed, 5 Jan 2022 22:42:21 +0100 Subject: [PATCH 1/3] Implement panic::update_hook --- library/alloc/tests/lib.rs | 1 + library/alloc/tests/slice.rs | 13 ++-- library/proc_macro/src/bridge/client.rs | 21 ++++--- library/proc_macro/src/lib.rs | 1 + library/std/src/panic.rs | 3 + library/std/src/panicking.rs | 63 +++++++++++++++++++ .../ui/panics/panic-while-updating-hook.rs | 16 +++++ 7 files changed, 102 insertions(+), 16 deletions(-) create mode 100644 src/test/ui/panics/panic-while-updating-hook.rs diff --git a/library/alloc/tests/lib.rs b/library/alloc/tests/lib.rs index eec24a5c3f7e6..7b8eeb90b5a80 100644 --- a/library/alloc/tests/lib.rs +++ b/library/alloc/tests/lib.rs @@ -38,6 +38,7 @@ #![feature(const_trait_impl)] #![feature(const_str_from_utf8)] #![feature(nonnull_slice_from_raw_parts)] +#![feature(panic_update_hook)] use std::collections::hash_map::DefaultHasher; use std::hash::{Hash, Hasher}; diff --git a/library/alloc/tests/slice.rs b/library/alloc/tests/slice.rs index 18ea6a2141377..a02f7b1f2774b 100644 --- a/library/alloc/tests/slice.rs +++ b/library/alloc/tests/slice.rs @@ -1783,12 +1783,13 @@ thread_local!(static SILENCE_PANIC: Cell = Cell::new(false)); #[test] #[cfg_attr(target_os = "emscripten", ignore)] // no threads fn panic_safe() { - let prev = panic::take_hook(); - panic::set_hook(Box::new(move |info| { - if !SILENCE_PANIC.with(|s| s.get()) { - prev(info); - } - })); + panic::update_hook(|prev| { + Box::new(move |info| { + if !SILENCE_PANIC.with(|s| s.get()) { + prev(info); + } + }) + }); let mut rng = thread_rng(); diff --git a/library/proc_macro/src/bridge/client.rs b/library/proc_macro/src/bridge/client.rs index 83a2ac6f0d4f1..5ff7bbf6f96f1 100644 --- a/library/proc_macro/src/bridge/client.rs +++ b/library/proc_macro/src/bridge/client.rs @@ -310,16 +310,17 @@ impl Bridge<'_> { // NB. the server can't do this because it may use a different libstd. static HIDE_PANICS_DURING_EXPANSION: Once = Once::new(); HIDE_PANICS_DURING_EXPANSION.call_once(|| { - let prev = panic::take_hook(); - panic::set_hook(Box::new(move |info| { - let show = BridgeState::with(|state| match state { - BridgeState::NotConnected => true, - BridgeState::Connected(_) | BridgeState::InUse => force_show_panics, - }); - if show { - prev(info) - } - })); + panic::update_hook(|prev| { + Box::new(move |info| { + let show = BridgeState::with(|state| match state { + BridgeState::NotConnected => true, + BridgeState::Connected(_) | BridgeState::InUse => force_show_panics, + }); + if show { + prev(info) + } + }) + }); }); BRIDGE_STATE.with(|state| state.set(BridgeState::Connected(self), f)) diff --git a/library/proc_macro/src/lib.rs b/library/proc_macro/src/lib.rs index 69af598f91e1c..c5afca6d56a2d 100644 --- a/library/proc_macro/src/lib.rs +++ b/library/proc_macro/src/lib.rs @@ -30,6 +30,7 @@ #![feature(restricted_std)] #![feature(rustc_attrs)] #![feature(min_specialization)] +#![feature(panic_update_hook)] #![recursion_limit = "256"] #[unstable(feature = "proc_macro_internals", issue = "27812")] diff --git a/library/std/src/panic.rs b/library/std/src/panic.rs index c0605b2f4121c..02ecf2e3e822e 100644 --- a/library/std/src/panic.rs +++ b/library/std/src/panic.rs @@ -36,6 +36,9 @@ pub use core::panic::panic_2021; #[stable(feature = "panic_hooks", since = "1.10.0")] pub use crate::panicking::{set_hook, take_hook}; +#[unstable(feature = "panic_update_hook", issue = "92649")] +pub use crate::panicking::update_hook; + #[stable(feature = "panic_hooks", since = "1.10.0")] pub use core::panic::{Location, PanicInfo}; diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs index 87854fe4f2970..cf970dccfc940 100644 --- a/library/std/src/panicking.rs +++ b/library/std/src/panicking.rs @@ -180,6 +180,69 @@ pub fn take_hook() -> Box) + 'static + Sync + Send> { } } +/// Atomic combination of [`take_hook`] + [`set_hook`]. +/// +/// [`take_hook`]: ./fn.take_hook.html +/// [`set_hook`]: ./fn.set_hook.html +/// +/// # Panics +/// +/// Panics if called from a panicking thread. +/// +/// Panics if the provided closure calls any of the functions [`panic::take_hook`], +/// [`panic::set_hook`], or [`panic::update_hook`]. +/// +/// Note: if the provided closure panics, the panic will not be able to be handled, resulting in a +/// double panic that aborts the process with a generic error message. +/// +/// [`panic::take_hook`]: ./fn.take_hook.html +/// [`panic::set_hook`]: ./fn.set_hook.html +/// [`panic::update_hook`]: ./fn.update_hook.html +/// +/// # Examples +/// +/// The following will print the custom message, and then the normal output of panic. +/// +/// ```should_panic +/// #![feature(panic_update_hook)] +/// use std::panic; +/// +/// panic::update_hook(|prev| { +/// Box::new(move |panic_info| { +/// println!("Print custom message and execute panic handler as usual"); +/// prev(panic_info); +/// }) +/// }); +/// +/// panic!("Custom and then normal"); +/// ``` +#[unstable(feature = "panic_update_hook", issue = "92649")] +pub fn update_hook(hook_fn: F) +where + F: FnOnce( + Box) + 'static + Sync + Send>, + ) -> Box) + 'static + Sync + Send>, +{ + if thread::panicking() { + panic!("cannot modify the panic hook from a panicking thread"); + } + + unsafe { + let guard = HOOK_LOCK.write(); + let old_hook = HOOK; + HOOK = Hook::Default; + + let hook_for_fn = match old_hook { + Hook::Default => Box::new(default_hook), + Hook::Custom(ptr) => Box::from_raw(ptr), + }; + + let hook = hook_fn(hook_for_fn); + HOOK = Hook::Custom(Box::into_raw(hook)); + drop(guard); + } +} + fn default_hook(info: &PanicInfo<'_>) { // If this is a double panic, make sure that we print a backtrace // for this panic. Otherwise only print it if logging is enabled. diff --git a/src/test/ui/panics/panic-while-updating-hook.rs b/src/test/ui/panics/panic-while-updating-hook.rs new file mode 100644 index 0000000000000..8c95f1b8b7840 --- /dev/null +++ b/src/test/ui/panics/panic-while-updating-hook.rs @@ -0,0 +1,16 @@ +// run-fail +// error-pattern: panicked while processing panic +#![allow(stable_features)] + +// ignore-emscripten no threads support + +#![feature(std_panic)] +#![feature(panic_update_hook)] + +use std::panic; + +fn main() { + panic::update_hook(|_prev| { + panic!("inside update_hook"); + }) +} From 8ef3ce866e2f20bdcc567e2f7012f81ce2e60298 Mon Sep 17 00:00:00 2001 From: Badel2 <2badel2@gmail.com> Date: Fri, 7 Jan 2022 17:04:33 +0100 Subject: [PATCH 2/3] Change panic::update_hook to simplify usage And to remove possibility of panics while changing the panic handler, because that resulted in a double panic. --- library/alloc/tests/slice.rs | 10 ++--- library/proc_macro/src/bridge/client.rs | 18 ++++---- library/std/src/panicking.rs | 45 ++++++++++--------- .../panics/panic-handler-chain-update-hook.rs | 36 +++++++++++++++ .../ui/panics/panic-while-updating-hook.rs | 16 ------- 5 files changed, 71 insertions(+), 54 deletions(-) create mode 100644 src/test/ui/panics/panic-handler-chain-update-hook.rs delete mode 100644 src/test/ui/panics/panic-while-updating-hook.rs diff --git a/library/alloc/tests/slice.rs b/library/alloc/tests/slice.rs index a02f7b1f2774b..b93d7938bc9a5 100644 --- a/library/alloc/tests/slice.rs +++ b/library/alloc/tests/slice.rs @@ -1783,12 +1783,10 @@ thread_local!(static SILENCE_PANIC: Cell = Cell::new(false)); #[test] #[cfg_attr(target_os = "emscripten", ignore)] // no threads fn panic_safe() { - panic::update_hook(|prev| { - Box::new(move |info| { - if !SILENCE_PANIC.with(|s| s.get()) { - prev(info); - } - }) + panic::update_hook(move |prev, info| { + if !SILENCE_PANIC.with(|s| s.get()) { + prev(info); + } }); let mut rng = thread_rng(); diff --git a/library/proc_macro/src/bridge/client.rs b/library/proc_macro/src/bridge/client.rs index 5ff7bbf6f96f1..9e9750eb8de40 100644 --- a/library/proc_macro/src/bridge/client.rs +++ b/library/proc_macro/src/bridge/client.rs @@ -310,16 +310,14 @@ impl Bridge<'_> { // NB. the server can't do this because it may use a different libstd. static HIDE_PANICS_DURING_EXPANSION: Once = Once::new(); HIDE_PANICS_DURING_EXPANSION.call_once(|| { - panic::update_hook(|prev| { - Box::new(move |info| { - let show = BridgeState::with(|state| match state { - BridgeState::NotConnected => true, - BridgeState::Connected(_) | BridgeState::InUse => force_show_panics, - }); - if show { - prev(info) - } - }) + panic::update_hook(move |prev, info| { + let show = BridgeState::with(|state| match state { + BridgeState::NotConnected => true, + BridgeState::Connected(_) | BridgeState::InUse => force_show_panics, + }); + if show { + prev(info) + } }); }); diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs index cf970dccfc940..19040cb12e02a 100644 --- a/library/std/src/panicking.rs +++ b/library/std/src/panicking.rs @@ -76,6 +76,12 @@ enum Hook { Custom(*mut (dyn Fn(&PanicInfo<'_>) + 'static + Sync + Send)), } +impl Hook { + fn custom(f: impl Fn(&PanicInfo<'_>) + 'static + Sync + Send) -> Self { + Self::Custom(Box::into_raw(Box::new(f))) + } +} + static HOOK_LOCK: StaticRWLock = StaticRWLock::new(); static mut HOOK: Hook = Hook::Default; @@ -180,7 +186,8 @@ pub fn take_hook() -> Box) + 'static + Sync + Send> { } } -/// Atomic combination of [`take_hook`] + [`set_hook`]. +/// Atomic combination of [`take_hook`] and [`set_hook`]. Use this to replace the panic handler with +/// a new panic handler that does something and then executes the old handler. /// /// [`take_hook`]: ./fn.take_hook.html /// [`set_hook`]: ./fn.set_hook.html @@ -189,16 +196,6 @@ pub fn take_hook() -> Box) + 'static + Sync + Send> { /// /// Panics if called from a panicking thread. /// -/// Panics if the provided closure calls any of the functions [`panic::take_hook`], -/// [`panic::set_hook`], or [`panic::update_hook`]. -/// -/// Note: if the provided closure panics, the panic will not be able to be handled, resulting in a -/// double panic that aborts the process with a generic error message. -/// -/// [`panic::take_hook`]: ./fn.take_hook.html -/// [`panic::set_hook`]: ./fn.set_hook.html -/// [`panic::update_hook`]: ./fn.update_hook.html -/// /// # Examples /// /// The following will print the custom message, and then the normal output of panic. @@ -207,11 +204,15 @@ pub fn take_hook() -> Box) + 'static + Sync + Send> { /// #![feature(panic_update_hook)] /// use std::panic; /// -/// panic::update_hook(|prev| { -/// Box::new(move |panic_info| { -/// println!("Print custom message and execute panic handler as usual"); -/// prev(panic_info); -/// }) +/// // Equivalent to +/// // let prev = panic::take_hook(); +/// // panic::set_hook(move |info| { +/// // println!("..."); +/// // prev(info); +/// // ); +/// panic::update_hook(move |prev, info| { +/// println!("Print custom message and execute panic handler as usual"); +/// prev(info); /// }); /// /// panic!("Custom and then normal"); @@ -219,9 +220,10 @@ pub fn take_hook() -> Box) + 'static + Sync + Send> { #[unstable(feature = "panic_update_hook", issue = "92649")] pub fn update_hook(hook_fn: F) where - F: FnOnce( - Box) + 'static + Sync + Send>, - ) -> Box) + 'static + Sync + Send>, + F: Fn(&(dyn Fn(&PanicInfo<'_>) + Send + Sync + 'static), &PanicInfo<'_>) + + Sync + + Send + + 'static, { if thread::panicking() { panic!("cannot modify the panic hook from a panicking thread"); @@ -232,13 +234,12 @@ where let old_hook = HOOK; HOOK = Hook::Default; - let hook_for_fn = match old_hook { + let prev = match old_hook { Hook::Default => Box::new(default_hook), Hook::Custom(ptr) => Box::from_raw(ptr), }; - let hook = hook_fn(hook_for_fn); - HOOK = Hook::Custom(Box::into_raw(hook)); + HOOK = Hook::custom(move |info| hook_fn(&prev, info)); drop(guard); } } diff --git a/src/test/ui/panics/panic-handler-chain-update-hook.rs b/src/test/ui/panics/panic-handler-chain-update-hook.rs new file mode 100644 index 0000000000000..4dd08ba4ad4e2 --- /dev/null +++ b/src/test/ui/panics/panic-handler-chain-update-hook.rs @@ -0,0 +1,36 @@ +// run-pass +// needs-unwind +#![allow(stable_features)] + +// ignore-emscripten no threads support + +#![feature(std_panic)] +#![feature(panic_update_hook)] + +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::panic; +use std::thread; + +static A: AtomicUsize = AtomicUsize::new(0); +static B: AtomicUsize = AtomicUsize::new(0); +static C: AtomicUsize = AtomicUsize::new(0); + +fn main() { + panic::set_hook(Box::new(|_| { A.fetch_add(1, Ordering::SeqCst); })); + panic::update_hook(|prev, info| { + B.fetch_add(1, Ordering::SeqCst); + prev(info); + }); + panic::update_hook(|prev, info| { + C.fetch_add(1, Ordering::SeqCst); + prev(info); + }); + + let _ = thread::spawn(|| { + panic!(); + }).join(); + + assert_eq!(1, A.load(Ordering::SeqCst)); + assert_eq!(1, B.load(Ordering::SeqCst)); + assert_eq!(1, C.load(Ordering::SeqCst)); +} diff --git a/src/test/ui/panics/panic-while-updating-hook.rs b/src/test/ui/panics/panic-while-updating-hook.rs deleted file mode 100644 index 8c95f1b8b7840..0000000000000 --- a/src/test/ui/panics/panic-while-updating-hook.rs +++ /dev/null @@ -1,16 +0,0 @@ -// run-fail -// error-pattern: panicked while processing panic -#![allow(stable_features)] - -// ignore-emscripten no threads support - -#![feature(std_panic)] -#![feature(panic_update_hook)] - -use std::panic; - -fn main() { - panic::update_hook(|_prev| { - panic!("inside update_hook"); - }) -} From 0c58586c9cbdd4db62c9c127ba89fd4d1d53acd7 Mon Sep 17 00:00:00 2001 From: Badel2 <2badel2@gmail.com> Date: Fri, 7 Jan 2022 23:34:45 +0100 Subject: [PATCH 3/3] Add safety comments to panic::(set/take/update)_hook --- library/std/src/panicking.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs index 19040cb12e02a..44f573297eed1 100644 --- a/library/std/src/panicking.rs +++ b/library/std/src/panicking.rs @@ -124,6 +124,11 @@ pub fn set_hook(hook: Box) + 'static + Sync + Send>) { panic!("cannot modify the panic hook from a panicking thread"); } + // SAFETY: + // + // - `HOOK` can only be modified while holding write access to `HOOK_LOCK`. + // - The argument of `Box::from_raw` is always a valid pointer that was created using + // `Box::into_raw`. unsafe { let guard = HOOK_LOCK.write(); let old_hook = HOOK; @@ -173,6 +178,11 @@ pub fn take_hook() -> Box) + 'static + Sync + Send> { panic!("cannot modify the panic hook from a panicking thread"); } + // SAFETY: + // + // - `HOOK` can only be modified while holding write access to `HOOK_LOCK`. + // - The argument of `Box::from_raw` is always a valid pointer that was created using + // `Box::into_raw`. unsafe { let guard = HOOK_LOCK.write(); let hook = HOOK; @@ -229,6 +239,11 @@ where panic!("cannot modify the panic hook from a panicking thread"); } + // SAFETY: + // + // - `HOOK` can only be modified while holding write access to `HOOK_LOCK`. + // - The argument of `Box::from_raw` is always a valid pointer that was created using + // `Box::into_raw`. unsafe { let guard = HOOK_LOCK.write(); let old_hook = HOOK;