From cd5d0c7b102d2573165efdbd2ffc31c4a5be3bb5 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 25 Nov 2019 12:14:23 +0100 Subject: [PATCH 1/8] Rename continue_panic_fmt to panic_handler, and make it the #[panic_handler] directly The "continue" in the name was really confusing; it sounds way too much like "resume" which is a totally different concept around panics. --- src/libstd/panicking.rs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/libstd/panicking.rs b/src/libstd/panicking.rs index 6819a4a04d737..a0f6183d514ea 100644 --- a/src/libstd/panicking.rs +++ b/src/libstd/panicking.rs @@ -296,14 +296,6 @@ pub fn panicking() -> bool { update_panic_count(0) != 0 } -/// Entry point of panic from the libcore crate (`panic_impl` lang item). -#[cfg(not(test))] -#[panic_handler] -#[unwind(allowed)] -pub fn rust_begin_panic(info: &PanicInfo<'_>) -> ! { - continue_panic_fmt(&info) -} - /// The entry point for panicking with a formatted message. /// /// This is designed to reduce the amount of code required at the call @@ -327,10 +319,13 @@ pub fn begin_panic_fmt(msg: &fmt::Arguments<'_>, let (file, line, col) = *file_line_col; let location = Location::internal_constructor(file, line, col); let info = PanicInfo::internal_constructor(Some(msg), &location); - continue_panic_fmt(&info) + panic_handler(&info) } -fn continue_panic_fmt(info: &PanicInfo<'_>) -> ! { +/// Entry point of panic from the libcore crate (`panic_impl` lang item). +#[cfg_attr(not(test), panic_handler)] +#[unwind(allowed)] +fn panic_handler(info: &PanicInfo<'_>) -> ! { struct PanicPayload<'a> { inner: &'a fmt::Arguments<'a>, string: Option, From 08f779cb4b481be58eeb5ecc421f69503780e8b1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 25 Nov 2019 12:16:08 +0100 Subject: [PATCH 2/8] better comment and rename BoxMeUp::box_me_up to take_box --- src/libcore/panic.rs | 4 +++- src/libpanic_unwind/lib.rs | 2 +- src/libstd/panicking.rs | 9 +++++---- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/libcore/panic.rs b/src/libcore/panic.rs index cdd38449a1be2..0abc481f6e53b 100644 --- a/src/libcore/panic.rs +++ b/src/libcore/panic.rs @@ -266,6 +266,8 @@ impl fmt::Display for Location<'_> { #[unstable(feature = "std_internals", issue = "0")] #[doc(hidden)] pub unsafe trait BoxMeUp { - fn box_me_up(&mut self) -> *mut (dyn Any + Send); + /// The return type is actually `Box`, but we cannot use `Box` in libcore. + /// After this method got called, only some dummy default value is left in `self`. + fn take_box(&mut self) -> *mut (dyn Any + Send); fn get(&mut self) -> &(dyn Any + Send); } diff --git a/src/libpanic_unwind/lib.rs b/src/libpanic_unwind/lib.rs index d97a7a8a87d8d..0c834e5c2a05c 100644 --- a/src/libpanic_unwind/lib.rs +++ b/src/libpanic_unwind/lib.rs @@ -94,5 +94,5 @@ pub unsafe extern "C" fn __rust_maybe_catch_panic(f: fn(*mut u8), #[unwind(allowed)] pub unsafe extern "C" fn __rust_start_panic(payload: usize) -> u32 { let payload = payload as *mut &mut dyn BoxMeUp; - imp::panic(Box::from_raw((*payload).box_me_up())) + imp::panic(Box::from_raw((*payload).take_box())) } diff --git a/src/libstd/panicking.rs b/src/libstd/panicking.rs index a0f6183d514ea..a16eec45b9aa1 100644 --- a/src/libstd/panicking.rs +++ b/src/libstd/panicking.rs @@ -340,6 +340,7 @@ fn panic_handler(info: &PanicInfo<'_>) -> ! { use crate::fmt::Write; let inner = self.inner; + // Lazily, the first time this gets called, run the actual string formatting. self.string.get_or_insert_with(|| { let mut s = String::new(); drop(s.write_fmt(*inner)); @@ -349,7 +350,7 @@ fn panic_handler(info: &PanicInfo<'_>) -> ! { } unsafe impl<'a> BoxMeUp for PanicPayload<'a> { - fn box_me_up(&mut self) -> *mut (dyn Any + Send) { + fn take_box(&mut self) -> *mut (dyn Any + Send) { let contents = mem::take(self.fill()); Box::into_raw(Box::new(contents)) } @@ -407,10 +408,10 @@ pub fn begin_panic(msg: M, file_line_col: &(&'static str, u32, u3 } unsafe impl BoxMeUp for PanicPayload { - fn box_me_up(&mut self) -> *mut (dyn Any + Send) { + fn take_box(&mut self) -> *mut (dyn Any + Send) { let data = match self.inner.take() { Some(a) => Box::new(a) as Box, - None => Box::new(()), + None => Box::new(()), // this should never happen: we got called twice }; Box::into_raw(data) } @@ -488,7 +489,7 @@ pub fn update_count_then_panic(msg: Box) -> ! { struct RewrapBox(Box); unsafe impl BoxMeUp for RewrapBox { - fn box_me_up(&mut self) -> *mut (dyn Any + Send) { + fn take_box(&mut self) -> *mut (dyn Any + Send) { Box::into_raw(mem::replace(&mut self.0, Box::new(()))) } From 3c485795517d1f5a6ebfff6368dfae7a7cd85b85 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 25 Nov 2019 12:24:39 +0100 Subject: [PATCH 3/8] more panicking comments --- src/libcore/panicking.rs | 9 +++++---- src/libstd/panicking.rs | 4 +++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/libcore/panicking.rs b/src/libcore/panicking.rs index 4833194be37ac..5a8d647396dda 100644 --- a/src/libcore/panicking.rs +++ b/src/libcore/panicking.rs @@ -11,13 +11,13 @@ //! ``` //! //! This definition allows for panicking with any general message, but it does not -//! allow for failing with a `Box` value. The reason for this is that libcore -//! is not allowed to allocate. +//! allow for failing with a `Box` value. (`PanicInfo` just contains a `&(dyn Any + Send)`, +//! for which we fill in a dummy value in `PanicInfo::internal_constructor`.) +//! The reason for this is that libcore is not allowed to allocate. //! //! This module contains a few other panicking functions, but these are just the //! necessary lang items for the compiler. All panics are funneled through this -//! one function. Currently, the actual symbol is declared in the standard -//! library, but the location of this may change over time. +//! one function. The actual symbol is declared through the `#[panic_handler]` attribute. // ignore-tidy-undocumented-unsafe @@ -72,6 +72,7 @@ pub fn panic_fmt(fmt: fmt::Arguments<'_>, location: &Location<'_>) -> ! { } // NOTE This function never crosses the FFI boundary; it's a Rust-to-Rust call + // that gets resolved to the `#[panic_handler]` function. extern "Rust" { #[lang = "panic_impl"] fn panic_impl(pi: &PanicInfo<'_>) -> !; diff --git a/src/libstd/panicking.rs b/src/libstd/panicking.rs index a16eec45b9aa1..cb035f48d90f4 100644 --- a/src/libstd/panicking.rs +++ b/src/libstd/panicking.rs @@ -455,7 +455,9 @@ fn rust_panic_with_hook(payload: &mut dyn BoxMeUp, match HOOK { // Some platforms know that printing to stderr won't ever actually // print anything, and if that's the case we can skip the default - // hook. + // hook. Since string formatting happens lazily when calling `payload` + // methods, this means that with libpanic_abort, we don't format + // the string at all! Hook::Default if panic_output().is_none() => {} Hook::Default => { info.set_payload(payload.get()); From 3a8e1b63cfc472a3c4884f6a31ab2236d7dd2fb7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 26 Nov 2019 08:18:57 +0100 Subject: [PATCH 4/8] panic_handler -> begin_panic_handler (and more comments) --- src/libstd/panicking.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/libstd/panicking.rs b/src/libstd/panicking.rs index cb035f48d90f4..31dcbc6a7cbda 100644 --- a/src/libstd/panicking.rs +++ b/src/libstd/panicking.rs @@ -316,16 +316,17 @@ pub fn begin_panic_fmt(msg: &fmt::Arguments<'_>, unsafe { intrinsics::abort() } } + // Just package everything into a `PanicInfo` and continue like libcore panics. let (file, line, col) = *file_line_col; let location = Location::internal_constructor(file, line, col); let info = PanicInfo::internal_constructor(Some(msg), &location); - panic_handler(&info) + begin_panic_handler(&info) } -/// Entry point of panic from the libcore crate (`panic_impl` lang item). +/// Entry point of panics from the libcore crate (`panic_impl` lang item). #[cfg_attr(not(test), panic_handler)] #[unwind(allowed)] -fn panic_handler(info: &PanicInfo<'_>) -> ! { +pub fn begin_panic_handler(info: &PanicInfo<'_>) -> ! { struct PanicPayload<'a> { inner: &'a fmt::Arguments<'a>, string: Option, @@ -374,7 +375,9 @@ fn panic_handler(info: &PanicInfo<'_>) -> ! { &file_line_col); } -/// This is the entry point of panicking for panic!() and assert!(). +/// This is the entry point of panicking for the non-format-string variants of +/// panic!() and assert!(). In particular, this is the only entry point that supports +/// arbitrary payloads, not just format strings. #[unstable(feature = "libstd_sys_internals", reason = "used by the panic! macro", issue = "0")] From 3e96ca2bf7f7c558623e372e7a9800ac752faa9c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 26 Nov 2019 09:24:39 +0100 Subject: [PATCH 5/8] abort on BoxMeUp misuse --- src/libcore/panic.rs | 8 ++++++++ src/libstd/panicking.rs | 5 +++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/libcore/panic.rs b/src/libcore/panic.rs index 0abc481f6e53b..99b372d92c8b6 100644 --- a/src/libcore/panic.rs +++ b/src/libcore/panic.rs @@ -266,8 +266,16 @@ impl fmt::Display for Location<'_> { #[unstable(feature = "std_internals", issue = "0")] #[doc(hidden)] pub unsafe trait BoxMeUp { + /// Take full ownership of the contents. /// The return type is actually `Box`, but we cannot use `Box` in libcore. + /// /// After this method got called, only some dummy default value is left in `self`. + /// Calling this method twice, or calling `get` after calling this method, is an error. + /// + /// The argument is borrowed because the panic runtime (`__rust_start_panic`) only + /// gets a borrowed `dyn BoxMeUp`. fn take_box(&mut self) -> *mut (dyn Any + Send); + + /// Just borrow the contents. fn get(&mut self) -> &(dyn Any + Send); } diff --git a/src/libstd/panicking.rs b/src/libstd/panicking.rs index 31dcbc6a7cbda..5ba5d89bb63b9 100644 --- a/src/libstd/panicking.rs +++ b/src/libstd/panicking.rs @@ -20,6 +20,7 @@ use crate::sys_common::rwlock::RWLock; use crate::sys_common::{thread_info, util}; use crate::sys_common::backtrace::{self, RustBacktrace}; use crate::thread; +use crate::process; #[cfg(not(test))] use crate::io::set_panic; @@ -414,7 +415,7 @@ pub fn begin_panic(msg: M, file_line_col: &(&'static str, u32, u3 fn take_box(&mut self) -> *mut (dyn Any + Send) { let data = match self.inner.take() { Some(a) => Box::new(a) as Box, - None => Box::new(()), // this should never happen: we got called twice + None => process::abort(), }; Box::into_raw(data) } @@ -422,7 +423,7 @@ pub fn begin_panic(msg: M, file_line_col: &(&'static str, u32, u3 fn get(&mut self) -> &(dyn Any + Send) { match self.inner { Some(ref a) => a, - None => &(), + None => process::abort(), } } } From 61486f4de3b5e04f401a4da2edae2fc4c9f02bc7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 26 Nov 2019 09:27:11 +0100 Subject: [PATCH 6/8] expand comment --- src/libstd/panicking.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libstd/panicking.rs b/src/libstd/panicking.rs index 5ba5d89bb63b9..8f9a4c05b5702 100644 --- a/src/libstd/panicking.rs +++ b/src/libstd/panicking.rs @@ -457,11 +457,12 @@ fn rust_panic_with_hook(payload: &mut dyn BoxMeUp, let mut info = PanicInfo::internal_constructor(message, &location); HOOK_LOCK.read(); match HOOK { - // Some platforms know that printing to stderr won't ever actually + // Some platforms (like wasm) know that printing to stderr won't ever actually // print anything, and if that's the case we can skip the default // hook. Since string formatting happens lazily when calling `payload` - // methods, this means that with libpanic_abort, we don't format - // the string at all! + // methods, this means we avoid formatting the string at all! + // (The panic runtime might still call `payload.take_box()` though and trigger + // formatting.) Hook::Default if panic_output().is_none() => {} Hook::Default => { info.set_payload(payload.get()); From 4a19ef938c3670afb8cc278d3d31a803b19addab Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 26 Nov 2019 09:29:39 +0100 Subject: [PATCH 7/8] explain why __rust_start_panic does not take a Box --- src/libstd/panicking.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libstd/panicking.rs b/src/libstd/panicking.rs index 8f9a4c05b5702..e0d980fc306b5 100644 --- a/src/libstd/panicking.rs +++ b/src/libstd/panicking.rs @@ -47,6 +47,8 @@ extern { vtable_ptr: *mut usize) -> u32; /// `payload` is actually a `*mut &mut dyn BoxMeUp` but that would cause FFI warnings. + /// It cannot be `Box` because the other end of this call does not depend + /// on liballoc, and thus cannot use `Box`. #[unwind(allowed)] fn __rust_start_panic(payload: usize) -> u32; } From babe9fcbc1d5aa5a6a53b7d2e34777cfe28e2c41 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 26 Nov 2019 10:23:15 +0100 Subject: [PATCH 8/8] rename update_count_then_panic -> rust_panic_without_hook --- src/libstd/panic.rs | 2 +- src/libstd/panicking.rs | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/libstd/panic.rs b/src/libstd/panic.rs index e36496d4c1c0d..cd024068d2d47 100644 --- a/src/libstd/panic.rs +++ b/src/libstd/panic.rs @@ -425,5 +425,5 @@ pub fn catch_unwind R + UnwindSafe, R>(f: F) -> Result { /// ``` #[stable(feature = "resume_unwind", since = "1.9.0")] pub fn resume_unwind(payload: Box) -> ! { - panicking::update_count_then_panic(payload) + panicking::rust_panic_without_hook(payload) } diff --git a/src/libstd/panicking.rs b/src/libstd/panicking.rs index e0d980fc306b5..c028ddcd676fc 100644 --- a/src/libstd/panicking.rs +++ b/src/libstd/panicking.rs @@ -491,8 +491,9 @@ fn rust_panic_with_hook(payload: &mut dyn BoxMeUp, rust_panic(payload) } -/// Shim around rust_panic. Called by resume_unwind. -pub fn update_count_then_panic(msg: Box) -> ! { +/// This is the entry point for `resume_unwind`. +/// It just forwards the payload to the panic runtime. +pub fn rust_panic_without_hook(payload: Box) -> ! { update_panic_count(1); struct RewrapBox(Box); @@ -507,7 +508,7 @@ pub fn update_count_then_panic(msg: Box) -> ! { } } - rust_panic(&mut RewrapBox(msg)) + rust_panic(&mut RewrapBox(payload)) } /// An unmangled function (through `rustc_std_internal_symbol`) on which to slap