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

Panic machinery comments and tweaks #66766

Merged
merged 8 commits into from
Nov 30, 2019
4 changes: 3 additions & 1 deletion src/libcore/panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<dyn Any + Send>`, 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);
}
9 changes: 5 additions & 4 deletions src/libcore/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,13 @@
//! ```
//!
//! This definition allows for panicking with any general message, but it does not
//! allow for failing with a `Box<Any>` value. The reason for this is that libcore
//! is not allowed to allocate.
//! allow for failing with a `Box<Any>` 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

Expand Down Expand Up @@ -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<'_>) -> !;
Expand Down
2 changes: 1 addition & 1 deletion src/libpanic_unwind/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
}
33 changes: 17 additions & 16 deletions src/libstd/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -324,13 +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);
continue_panic_fmt(&info)
begin_panic_handler(&info)
}

fn continue_panic_fmt(info: &PanicInfo<'_>) -> ! {
/// Entry point of panics from the libcore crate (`panic_impl` lang item).
#[cfg_attr(not(test), panic_handler)]
#[unwind(allowed)]
pub fn begin_panic_handler(info: &PanicInfo<'_>) -> ! {
struct PanicPayload<'a> {
inner: &'a fmt::Arguments<'a>,
string: Option<String>,
Expand All @@ -345,6 +341,7 @@ fn continue_panic_fmt(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));
Expand All @@ -354,7 +351,7 @@ fn continue_panic_fmt(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))
}
Expand All @@ -378,7 +375,9 @@ fn continue_panic_fmt(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")]
Expand Down Expand Up @@ -412,10 +411,10 @@ pub fn begin_panic<M: Any + Send>(msg: M, file_line_col: &(&'static str, u32, u3
}

unsafe impl<A: Send + 'static> BoxMeUp for PanicPayload<A> {
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<dyn Any + Send>,
None => Box::new(()),
None => Box::new(()), // this should never happen: we got called twice
Copy link
Member Author

Choose a reason for hiding this comment

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

I was wondering if it is appropriate to call process::abort() here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe. I feel it’s not very important since this is a private API that has exactly one call site.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me re-phrase my question: is there any reason not to do that? Is there any legitimate use of this interface that calls take_box twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually what I am wondering about even more is what the method does not take self by-value?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an impl for a private type PanicPayload, whose value is only ever constructed in one place. A pointer to that value is __rust_start_panic, which we can consider a private implementation detail of the standard library. So both the definition and all uses of this API (all one of them) are under our control. So it doesn’t matter in my opinion what the code does in situations that we know never happen. There is no reason to abort, and there is no reason not to abort. Feel free to change this to abort if you’d like, I don’t mind r+’ing that change.

This method cannot take self by value because it is (only, ever) called by src/libpanic_unwind/lib.rs’s __rust_start_panic which dereferences a raw pointer (cast from usize) to get &mut &mut dyn BoxMeUp. It cannot (easily) be an owned Box<dyn BoxMeUp> because libpanic_unwind doesn’t depend on liballoc which defines Box. Oh and for the trait to be object-safe it might have to be self: Box<Self> or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

It cannot (easily) be an owned Box because libpanic_unwind doesn’t depend on liballoc which defines Box

Ah, thanks, I added a comment along those lines (and made it abort). Please check.

};
Box::into_raw(data)
}
Expand Down Expand Up @@ -459,7 +458,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
Copy link
Contributor

Choose a reason for hiding this comment

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

panic_output().is_none() does not indicate libpanic_abort. It indicates a target such as wasm that doesn’t have anything like stderr, so we can’t print a message at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, with libpanic_unwind we call take_box() which will still trigger the formatting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, I see what you meant now.

// the string at all!
Hook::Default if panic_output().is_none() => {}
Hook::Default => {
info.set_payload(payload.get());
Expand Down Expand Up @@ -493,7 +494,7 @@ pub fn update_count_then_panic(msg: Box<dyn Any + Send>) -> ! {
struct RewrapBox(Box<dyn Any + Send>);

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(())))
}

Expand Down