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

PoC: implement core::panic::panic_error #1

Closed
wants to merge 1 commit into from
Closed
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 compiler/rustc_hir/src/lang_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@ language_item_table! {
// a weak lang item, but do not have it defined.
Panic, sym::panic, panic_fn, Target::Fn, GenericRequirement::Exact(0);
PanicFmt, sym::panic_fmt, panic_fmt, Target::Fn, GenericRequirement::None;
PanicError, sym::panic_error, panic_error, Target::Fn, GenericRequirement::None;
PanicDisplay, sym::panic_display, panic_display, Target::Fn, GenericRequirement::None;
PanicStr, sym::panic_str, panic_str, Target::Fn, GenericRequirement::None;
ConstPanicFmt, sym::const_panic_fmt, const_panic_fmt, Target::Fn, GenericRequirement::None;
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -959,6 +959,7 @@ symbols! {
panic_abort,
panic_bounds_check,
panic_display,
panic_error,
panic_fmt,
panic_handler,
panic_impl,
Expand Down
13 changes: 13 additions & 0 deletions library/core/src/panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,19 @@ pub macro panic_2021 {
),
}

/// Panic the current thread with the given error as the panic payload.
///
/// The message can be of any (`Error + 'static`) type, not just strings.
///
/// See the [`panic!`] macro for more information about panicking.
#[unstable(feature = "panic_error", issue = "none")]
#[inline]
#[track_caller]
#[cfg(not(bootstrap))]
pub fn panic_error<E: 'static + crate::error::Error>(error: E) -> ! {
crate::panicking::panic_error(&error);
}

/// An internal trait used by libstd to pass data from libstd to `panic_unwind`
/// and other panic runtimes. Not intended to be stabilized any time soon, do
/// not use.
Expand Down
61 changes: 57 additions & 4 deletions library/core/src/panic/panic_info.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use crate::any::Any;
use crate::fmt;
use crate::panic::Location;
#[cfg(not(bootstrap))]
use crate::error::Error;

/// A struct providing information about a panic.
///
Expand Down Expand Up @@ -28,11 +30,29 @@ use crate::panic::Location;
#[stable(feature = "panic_hooks", since = "1.10.0")]
#[derive(Debug)]
pub struct PanicInfo<'a> {
payload: &'a (dyn Any + Send),
payload: PayloadKind<'a>,
message: Option<&'a fmt::Arguments<'a>>,
location: &'a Location<'a>,
}

#[derive(Debug)]
enum PayloadKind<'a> {
Any(&'a (dyn Any + Send)),
#[cfg(not(bootstrap))]
Error(&'a (dyn Error + 'static)),
}

impl<'a> PayloadKind<'a> {
fn downcast_ref<T: 'static>(&self) -> Option<&T> {
match self {
PayloadKind::Any(payload) => payload.downcast_ref(),
#[cfg(not(bootstrap))]
PayloadKind::Error(_) => None,
// PayloadKind::Error(error) => error.downcast_ref(),
Copy link
Owner Author

Choose a reason for hiding this comment

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

I kinda wanted to let this downcast for either but there isn't any reason to do so. I'd likely have to internally reimplement the downcast_ref logic to avoid needing to add T: Error.

}
}
}

impl<'a> PanicInfo<'a> {
#[unstable(
feature = "panic_internals",
Expand All @@ -46,7 +66,23 @@ impl<'a> PanicInfo<'a> {
location: &'a Location<'a>,
) -> Self {
struct NoPayload;
PanicInfo { location, message, payload: &NoPayload }
PanicInfo { location, message, payload: PayloadKind::Any(&NoPayload) }
}

#[unstable(
feature = "panic_internals",
reason = "internal details of the implementation of the `panic!` and related macros",
issue = "none"
)]
#[doc(hidden)]
#[inline]
#[cfg(not(bootstrap))]
pub fn error_constructor(
// message: Option<&'a fmt::Arguments<'a>>,
Copy link
Owner Author

Choose a reason for hiding this comment

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

This will likely need a message as well, so that unwrap and expect can provide their message when panicking as an error. My plan is to change the error() method on PanicInfo to be called source, and to treat the message like the outermost error in the chain of errors.

error: &'a (dyn crate::error::Error + 'static),
location: &'a Location<'a>,
) -> Self {
PanicInfo { location, message: None, payload: PayloadKind::Error(error) }
}

#[unstable(
Expand All @@ -57,7 +93,7 @@ impl<'a> PanicInfo<'a> {
#[doc(hidden)]
#[inline]
pub fn set_payload(&mut self, info: &'a (dyn Any + Send)) {
self.payload = info;
self.payload = PayloadKind::Any(info);
}

/// Returns the payload associated with the panic.
Expand All @@ -84,7 +120,13 @@ impl<'a> PanicInfo<'a> {
#[must_use]
#[stable(feature = "panic_hooks", since = "1.10.0")]
pub fn payload(&self) -> &(dyn Any + Send) {
self.payload
#[cfg(not(bootstrap))]
struct HackerVoice;
match self.payload {
PayloadKind::Any(payload) => payload,
#[cfg(not(bootstrap))]
PayloadKind::Error(_) => &HackerVoice, // I'm in
Copy link
Owner Author

Choose a reason for hiding this comment

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

I will not apologize

}
}

/// If the `panic!` macro from the `core` crate (not from `std`)
Expand All @@ -96,6 +138,17 @@ impl<'a> PanicInfo<'a> {
self.message
}

/// If the panic was created with `panic_error` returns the source error of the panic.
#[must_use]
#[unstable(feature = "panic_error", issue = "none")]
#[cfg(not(bootstrap))]
pub fn error(&self) -> Option<&(dyn core::error::Error + 'static)> {
match self.payload {
PayloadKind::Any(_) => None,
PayloadKind::Error(source) => Some(source),
}
}

/// Returns information about the location from which the panic originated,
/// if available.
///
Expand Down
33 changes: 33 additions & 0 deletions library/core/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,39 @@ pub const fn panic_fmt(fmt: fmt::Arguments<'_>) -> ! {
unsafe { panic_impl(&pi) }
}

/// The entry point for panicking with a formatted message.
///
/// This is designed to reduce the amount of code required at the call
/// site as much as possible (so that `panic!()` has as low an impact
/// on (e.g.) the inlining of other functions as possible), by moving
/// the actual formatting into this shared place.
#[cold]
// If panic_immediate_abort, inline the abort call,
// otherwise avoid inlining because of it is cold path.
#[cfg(not(bootstrap))]
#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never))]
#[cfg_attr(feature = "panic_immediate_abort", inline)]
#[track_caller]
#[lang = "panic_error"] // needed for const-evaluated panics
#[rustc_do_not_const_check] // hooked by const-eval
pub const fn panic_error(error: &(dyn core::error::Error + 'static)) -> ! {
if cfg!(feature = "panic_immediate_abort") {
super::intrinsics::abort()
}

// 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<'_>) -> !;
}

let pi = PanicInfo::error_constructor(error, Location::caller());

// SAFETY: `panic_impl` is defined in safe Rust code and thus is safe to call.
unsafe { panic_impl(&pi) }
}

/// This function is used instead of panic_fmt in const eval.
#[lang = "const_panic_fmt"]
pub const fn const_panic_fmt(fmt: fmt::Arguments<'_>) -> ! {
Expand Down
1 change: 1 addition & 0 deletions library/std/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,7 @@
#![feature(nll)]
#![feature(nonnull_slice_from_raw_parts)]
#![feature(once_cell)]
#![cfg_attr(not(bootstrap), feature(panic_error))]
#![feature(panic_info_message)]
#![feature(panic_internals)]
#![feature(panic_unwind)]
Expand Down
151 changes: 151 additions & 0 deletions library/std/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,8 @@ fn default_hook(info: &PanicInfo<'_>) {
Some(s) => *s,
None => match info.payload().downcast_ref::<String>() {
Some(s) => &s[..],
#[cfg(not(bootstrap))]
None if info.error().is_some() => "non-recoverable runtime Error",
Copy link
Owner Author

Choose a reason for hiding this comment

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

Once std::error::Report is merged this will be updated to actually report the source error when one exists, rather than requiring the user installs a custom hook that actually prints the error.

None => "Box<dyn Any>",
},
};
Expand Down Expand Up @@ -442,6 +444,7 @@ pub fn panicking() -> bool {

/// Entry point of panics from the libcore crate (`panic_impl` lang item).
#[cfg(not(test))]
#[cfg(bootstrap)]
#[panic_handler]
pub fn begin_panic_handler(info: &PanicInfo<'_>) -> ! {
struct PanicPayload<'a> {
Expand Down Expand Up @@ -504,6 +507,77 @@ pub fn begin_panic_handler(info: &PanicInfo<'_>) -> ! {
})
}

/// Entry point of panics from the libcore crate (`panic_impl` lang item).
#[cfg(not(test))]
#[cfg(not(bootstrap))]
#[panic_handler]
pub fn begin_panic_handler(info: &PanicInfo<'_>) -> ! {
struct PanicPayload<'a> {
inner: &'a fmt::Arguments<'a>,
string: Option<String>,
}

impl<'a> PanicPayload<'a> {
fn new(inner: &'a fmt::Arguments<'a>) -> PanicPayload<'a> {
PanicPayload { inner, string: None }
}

fn fill(&mut self) -> &mut String {
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));
s
})
}
}

unsafe impl<'a> BoxMeUp for PanicPayload<'a> {
fn take_box(&mut self) -> *mut (dyn Any + Send) {
// We do two allocations here, unfortunately. But (a) they're required with the current
// scheme, and (b) we don't handle panic + OOM properly anyway (see comment in
// begin_panic below).
let contents = mem::take(self.fill());
Box::into_raw(Box::new(contents))
}

fn get(&mut self) -> &(dyn Any + Send) {
self.fill()
}
}

struct StrPanicPayload(&'static str);

unsafe impl BoxMeUp for StrPanicPayload {
fn take_box(&mut self) -> *mut (dyn Any + Send) {
Box::into_raw(Box::new(self.0))
}

fn get(&mut self) -> &(dyn Any + Send) {
&self.0
}
}

let loc = info.location().unwrap(); // The current implementation always returns Some
if let Some(error) = info.error() {
crate::sys_common::backtrace::__rust_end_short_backtrace(move || {
rust_panic_error_with_hook(error, loc);
})
} else {
let msg = info.message().unwrap(); // The current implementation always returns Some
crate::sys_common::backtrace::__rust_end_short_backtrace(move || {
if let Some(msg) = msg.as_str() {
rust_panic_with_hook(&mut StrPanicPayload(msg), info.message(), loc);
} else {
rust_panic_with_hook(&mut PanicPayload::new(msg), info.message(), loc);
}
})
}
Comment on lines +564 to +578
Copy link
Owner Author

Choose a reason for hiding this comment

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

I assume this is to help make the types for the msg more uniform so we can downcast to them more consistently, but I really do not like that this is creating a new PanicInfo when we already have one...

}

/// 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.
Expand Down Expand Up @@ -624,6 +698,83 @@ fn rust_panic_with_hook(
rust_panic(payload)
}

/// Central point for dispatching error panics.
///
/// Executes the primary logic for a panic, including checking for recursive
/// panics, panic hooks, and finally dispatching to the panic runtime to either
/// abort or unwind.
#[cfg(not(bootstrap))]
fn rust_panic_error_with_hook(
error: &(dyn crate::error::Error + 'static),
location: &Location<'_>,
) -> ! {
let (must_abort, panics) = panic_count::increase();

// If this is the third nested call (e.g., panics == 2, this is 0-indexed),
// the panic hook probably triggered the last panic, otherwise the
// double-panic check would have aborted the process. In this case abort the
// process real quickly as we don't want to try calling it again as it'll
// probably just panic again.
if must_abort || panics > 2 {
if panics > 2 {
// Don't try to print the message in this case
// - perhaps that is causing the recursive panics.
rtprintpanic!("thread panicked while processing panic. aborting.\n");
} else {
// Unfortunately, this does not print a backtrace, because creating
// a `Backtrace` will allocate, which we must to avoid here.
let panicinfo = PanicInfo::error_constructor(error, location);
rtprintpanic!("{}\npanicked after panic::always_abort(), aborting.\n", panicinfo);
}
intrinsics::abort()
}

unsafe {
let info = PanicInfo::error_constructor(error, location);
let _guard = HOOK_LOCK.read();
match HOOK {
// 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 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 => {
default_hook(&info);
}
Hook::Custom(ptr) => {
(*ptr)(&info);
}
};
}

if panics > 1 {
// 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.
rtprintpanic!("thread panicked while panicking. aborting.\n");
intrinsics::abort()
}

struct ErrorPanicPayload;

unsafe impl BoxMeUp for ErrorPanicPayload {
fn take_box(&mut self) -> *mut (dyn Any + Send) {
// ErrorPanicPayload is a zst so this box should not allocate
let data = Box::new(ErrorPanicPayload);
Box::into_raw(data)
}

fn get(&mut self) -> &(dyn Any + Send) {
&ErrorPanicPayload
}
}

rust_panic(&mut ErrorPanicPayload)
Copy link
Owner Author

Choose a reason for hiding this comment

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

Ideally it should be possible to panic with no payload at all. My intuition is that propagating the error with an unwinding panic to be caught and potentially reacted to is an anti-pattern, since converting a dyn Error to a panic semantically represents promoting a recoverable error to a non-recoverable error. I'm interested in hearing real world usecases of propagating and handling payloads via panics and am open to the possibility that I'm making the wrong call here, but my expectation is that if you need to react to an error in a panic it should never have been a panic in the first place.

For now we make a dummy payload that shouldn't need to allocate and which only indicates that the current panic started as a dyn Error.

}

/// 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<dyn Any + Send>) -> ! {
Expand Down
Loading