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

Make PanicInfo::message infallible #115561

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 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
15 changes: 5 additions & 10 deletions library/core/src/panic/panic_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use crate::panic::Location;
#[derive(Debug)]
pub struct PanicInfo<'a> {
payload: &'a (dyn Any + Send),
message: Option<&'a fmt::Arguments<'a>>,
message: fmt::Arguments<'a>,
location: &'a Location<'a>,
can_unwind: bool,
force_no_backtrace: bool,
Expand All @@ -40,7 +40,7 @@ impl<'a> PanicInfo<'a> {
#[doc(hidden)]
#[inline]
pub fn internal_constructor(
message: Option<&'a fmt::Arguments<'a>>,
message: fmt::Arguments<'a>,
location: &'a Location<'a>,
can_unwind: bool,
force_no_backtrace: bool,
Expand Down Expand Up @@ -92,7 +92,7 @@ impl<'a> PanicInfo<'a> {
/// returns that message ready to be used for example with [`fmt::write`]
#[must_use]
#[unstable(feature = "panic_info_message", issue = "66745")]
pub fn message(&self) -> Option<&fmt::Arguments<'_>> {
pub fn message(&self) -> fmt::Arguments<'_> {
self.message
}

Expand Down Expand Up @@ -161,13 +161,8 @@ impl fmt::Display for PanicInfo<'_> {
fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
formatter.write_str("panicked at ")?;
self.location.fmt(formatter)?;
if let Some(message) = self.message {
formatter.write_str(":\n")?;
formatter.write_fmt(*message)?;
} else if let Some(payload) = self.payload.downcast_ref::<&'static str>() {
formatter.write_str(":\n")?;
formatter.write_str(payload)?;
}
formatter.write_str(":\n")?;
formatter.write_fmt(self.message)?;
// NOTE: we cannot use downcast_ref::<String>() here
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
// since String is not available in core!
// The payload is a String when `std::panic!` is called with multiple arguments,
Expand Down
4 changes: 2 additions & 2 deletions library/core/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ pub const fn panic_fmt(fmt: fmt::Arguments<'_>) -> ! {
}

let pi = PanicInfo::internal_constructor(
Some(&fmt),
fmt,
Location::caller(),
/* can_unwind */ true,
/* force_no_backtrace */ false,
Expand Down Expand Up @@ -96,7 +96,7 @@ pub fn panic_nounwind_fmt(fmt: fmt::Arguments<'_>, force_no_backtrace: bool) ->

// PanicInfo with the `can_unwind` flag set to false forces an abort.
let pi = PanicInfo::internal_constructor(
Some(&fmt),
fmt,
Location::caller(),
/* can_unwind */ false,
force_no_backtrace,
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 @@ -292,6 +292,7 @@
#![feature(float_gamma)]
#![feature(float_minimum_maximum)]
#![feature(float_next_up_down)]
#![feature(fmt_internals)]
#![feature(hasher_prefixfree_extras)]
#![feature(hashmap_internals)]
#![feature(ip)]
Expand Down
10 changes: 6 additions & 4 deletions library/std/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,7 @@ pub fn begin_panic_handler(info: &PanicInfo<'_>) -> ! {
}

let loc = info.location().unwrap(); // The current implementation always returns Some
let msg = info.message().unwrap(); // The current implementation always returns Some
let msg = info.message(); // The current implementation always returns Some
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
crate::sys_common::backtrace::__rust_end_short_backtrace(move || {
// FIXME: can we just pass `info` along rather than taking it apart here, only to have
// `rust_panic_with_hook` construct a new `PanicInfo`?
Expand All @@ -629,7 +629,7 @@ pub fn begin_panic_handler(info: &PanicInfo<'_>) -> ! {
);
} else {
rust_panic_with_hook(
&mut PanicPayload::new(msg),
&mut PanicPayload::new(&msg),
info.message(),
loc,
info.can_unwind(),
Expand Down Expand Up @@ -658,9 +658,11 @@ pub const fn begin_panic<M: Any + Send>(msg: M) -> ! {

let loc = Location::caller();
return crate::sys_common::backtrace::__rust_end_short_backtrace(move || {
let message =
*(&msg as &dyn Any).downcast_ref::<&'static str>().unwrap_or(&"<non-str payload>");
Comment on lines +661 to +662
Copy link
Member

Choose a reason for hiding this comment

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

It might be good if this also attempts to downcast to String, just like the default panic hook does:

None => match info.payload().downcast_ref::<String>() {

Then panic_any(some_string), and panic!(format!(..)) in Rust ≤2018, will also work.

(For a String you'll have to use format_args!("{}", s) though, rather than new_v1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can't be done, as we'd be borrowing from msg while also using it as the payload (and thus moving out of it).

Copy link
Member

Choose a reason for hiding this comment

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

Right. It is possible, since the payload isn't actually consumed until after the PanicInfo is gone (at the end of rust_panic_with_hook), but it'd require refactoring some of the code in this file quite a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is the payload.get() method call, not the move. BoxMeUp::get must be mutable in order for the #[panic_handler] to be able to lazily allocate a String by formatting the Message from the PanicInfo that gets passed to it.

It's all a bit roundabout, but if we're willing to break some use cases that assume there's a String in the payload for panic!("{}", 42), then that does seem fixable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

The problem is the payload.get() method call, not the move. BoxMeUp::get must be mutable in order for the #[panic_handler] to be able to lazily allocate a String by formatting the Message from the PanicInfo that gets passed to it.

Sure, but .get() is always called when constructing the PanicInfo that's passed to the panic hook, so at that point the String will have been allocated and can be borrowed by format_args!().

So, rust_panic_with_hook could just .get() the payload first and then try downcasting it to &'static str and String to create a format_args!("{s}") to make the PanicInfo with. (That also avoids formatting message twice when someone uses .message() in a panic hook. (Since at that point the message has already been formatted into a String.))

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, a lot of the annoyances here come from PanicInfo (the argument to #[panic_handler]) and PanicInfo (the argument to the panic hook) being the same type. I'm hoping we might be able to do something about that: #115974 That'd make this all a lot easier. :)

Comment on lines +661 to +662
Copy link
Member

Choose a reason for hiding this comment

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

"<non-str payload>"

The default panic hook prints "Box<dyn Any>" in that case. I'm not particularly attached to that specific message, but we should probably keep those the same.

None => "Box<dyn Any>",

Copy link
Member

Choose a reason for hiding this comment

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

Or, even better: the default panic hook should not do any downcasting and instead use .message(), now that it works in all cases.

rust_panic_with_hook(
&mut PanicPayload::new(msg),
None,
core::fmt::Arguments::new_v1(&[message], &[]),
loc,
/* can_unwind */ true,
/* force_no_backtrace */ false,
Expand Down Expand Up @@ -707,7 +709,7 @@ pub const fn begin_panic<M: Any + Send>(msg: M) -> ! {
/// abort or unwind.
fn rust_panic_with_hook(
payload: &mut dyn BoxMeUp,
message: Option<&fmt::Arguments<'_>>,
message: fmt::Arguments<'_>,
location: &Location<'_>,
can_unwind: bool,
force_no_backtrace: bool,
Expand Down
7 changes: 2 additions & 5 deletions tests/run-make/wasm-exceptions-nostd/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,8 @@ fn panic_handler(info: &core::panic::PanicInfo<'_>) -> ! {
use alloc::boxed::Box;
use alloc::string::ToString;

let msg = info
.message()
.map(|msg| msg.to_string())
.unwrap_or("(no message)".to_string());
let exception = Box::new(msg.to_string());
let msg = info.message().to_string();
let exception = Box::new(msg);
unsafe {
let exception_raw = Box::into_raw(exception);
wasm_throw(exception_raw as *mut u8);
Expand Down
11 changes: 11 additions & 0 deletions tests/ui/panics/panic_info_message.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// revisions: e2015 e2018 e2021
//[e2018] compile-flags: --edition=2018
//[e2021] compile-flags: --edition=2021
// run-pass

#![feature(panic_info_message)]

fn main() {
std::panic::set_hook(Box::new(|info| assert_eq!(info.message().as_str().unwrap(), "cake")));
let _ = std::panic::catch_unwind(|| panic!("cake"));
}
Loading