Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement panic::update_hook #92598

Merged
merged 3 commits into from
Jan 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions library/alloc/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down
5 changes: 2 additions & 3 deletions library/alloc/tests/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1783,12 +1783,11 @@ thread_local!(static SILENCE_PANIC: Cell<bool> = 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| {
panic::update_hook(move |prev, info| {
if !SILENCE_PANIC.with(|s| s.get()) {
prev(info);
}
}));
});

let mut rng = thread_rng();

Expand Down
5 changes: 2 additions & 3 deletions library/proc_macro/src/bridge/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -310,16 +310,15 @@ 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| {
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)
}
}));
});
});

BRIDGE_STATE.with(|state| state.set(BridgeState::Connected(self), f))
Expand Down
1 change: 1 addition & 0 deletions library/proc_macro/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down
3 changes: 3 additions & 0 deletions library/std/src/panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down
79 changes: 79 additions & 0 deletions library/std/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -118,6 +124,11 @@ pub fn set_hook(hook: Box<dyn Fn(&PanicInfo<'_>) + '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;
Expand Down Expand Up @@ -167,6 +178,11 @@ pub fn take_hook() -> Box<dyn Fn(&PanicInfo<'_>) + '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;
Expand All @@ -180,6 +196,69 @@ pub fn take_hook() -> Box<dyn Fn(&PanicInfo<'_>) + 'static + Sync + Send> {
}
}

/// 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
///
/// # Panics
///
/// Panics if called from a panicking thread.
///
/// # Examples
///
/// The following will print the custom message, and then the normal output of panic.
///
/// ```should_panic
/// #![feature(panic_update_hook)]
/// use std::panic;
///
/// // 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");
/// ```
#[unstable(feature = "panic_update_hook", issue = "92649")]
pub fn update_hook<F>(hook_fn: F)
where
F: Fn(&(dyn Fn(&PanicInfo<'_>) + Send + Sync + 'static), &PanicInfo<'_>)
+ Sync
+ Send
+ 'static,
Comment on lines +233 to +236
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should have brought this up earlier, but one downside I see with this approach is that you cannot discard the old hook with this interface, and it will always be stored in the closure created internally for the new hook. I'm not sure if this is an issue or not but it's certainly a tradeoff to consider when weighing this version against the old one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edit: actually, after thinking about it for half a minute i realized that this is probably a good thing. If they wanted to just completely discard the old one they should be using set_hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, the use case of update_hook is to be able to safely add an additional panic handler without losing the previous handler. If the user does not care about the previous handler, then set_hook is fine.

{
if thread::panicking() {
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;
HOOK = Hook::Default;

let prev = match old_hook {
Hook::Default => Box::new(default_hook),
Hook::Custom(ptr) => Box::from_raw(ptr),
};

HOOK = Hook::custom(move |info| hook_fn(&prev, info));
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.
Expand Down
36 changes: 36 additions & 0 deletions src/test/ui/panics/panic-handler-chain-update-hook.rs
Original file line number Diff line number Diff line change
@@ -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));
}