Split core's PanicInfo and std's PanicInfo #115974

m-ou-se · 2023-09-19T16:17:11Z

PanicInfo is used in two ways:

As argument to the #[panic_handler] in no_std context.
As argument to the panic hook in std context.

In situation 1, the PanicInfo always has a message (of type fmt::Arguments), but never a payload (of type &dyn Any).

In situation 2, the PanicInfo always has a payload (which is often a String), but not always a message.

Having these as the same type is annoying. It means we can't add .message() to the first one without also finding a way to properly support it on the second one. (Which is what #66745 is blocked on.)

It also means that, because the implementation is in core, the implementation cannot make use of the String type (which doesn't exist in core):

rust/library/core/src/panic/panic_info.rs

Lines 171 to 172 in 0692db1

 // NOTE: we cannot use downcast_ref::<String>() here 

 // since String is not available in core!

This also means that we cannot easily add a useful method like PanicInfo::payload_as_str() -> Option<&str> that works for both &'static str and String payloads.

I don't see any good reasons for these to be the same type, other than historical reasons.

This PR is makes 1 and 2 separate types. To try to avoid breaking existing code and reduce churn, the first one is still named core::panic::PanicInfo, and std::panic::PanicInfo is a new (deprecated) alias to PanicHookInfo. The crater run showed this as a viable option, since people write core:: when defining a #[panic_handler] (because they're in no_std) and std:: when writing a panic hook (since then they're definitely using std). On top of that, many definitions of a panic hook don't specify a type at all: they are written as a closure with an inferred argument type.

(Based on some thoughts I was having here: #115561 (comment))

For the release notes:

We have renamed std::panic::PanicInfo to std::panic::PanicHookInfo. The old name will continue to work as an alias, but will result in a deprecation warning starting in Rust 1.82.0.

core::panic::PanicInfo will remain unchanged, however, as this is now a different type.

The reason is that these types have different roles: std::panic::PanicHookInfo is the argument to the panic hook in std context (where panics can have an arbitrary payload), while core::panic::PanicInfo is the argument to the #[panic_handler] in no_std context (where panics always carry a formatted message). Separating these types allows us to add more useful methods to these types, such as std::panic::PanicHookInfo::payload_as_str() and core::panic::PanicInfo::message().

…try> Split core's PanicInfo and std's PanicInfo This experimental PR is based on some thoughts I was having here: rust-lang#115561 (comment) --- `PanicInfo` is used in two ways: 1. As argument to the `#[panic_handler]` in `no_std` context. 2. As argument to the [panic hook](https://doc.rust-lang.org/stable/std/panic/fn.set_hook.html) in `std` context. In situation 1, the `PanicInfo` always has a *message* (of type `fmt::Arguments`), but never a *payload* (of type `&dyn Any`). In situation 2, the `PanicInfo` always has a *payload* (which is often a `String`), but not always a *message*. Having these as the same type is annoying. It means we can't add `.message()` to the first one without also finding a way to properly support it on the second one. (Which is what rust-lang#66745 is blocked on.) It also means that, because the implementation is in `core`, the implementation cannot make use of the `String` type (which doesn't exist in `core`): https://github.com/rust-lang/rust/blob/0692db1a9082380e027f354912229dfd6af37e78/library/core/src/panic/panic_info.rs#L171-L172 This also means that we cannot easily add a useful method like `PanicInfo::payload_as_str() -> Option<&str>` that works for both `&'static str` and `String` payloads. I don't see any good reasons for these to be the same type, other than historical reasons. This PR is an experiment to see what happens if we make 1 and 2 separate types. To try to avoid breaking existing code, they are still named `core::panic::PanicInfo` and `std::panic::PanicInfo`, in the hope that people write the former when used in `#[panic_handler]` (since then they'd be using `no_std`) and the latter when used in a panic hook (since then they're definitely using `std`). Let's see how much explodes. :)

RalfJung · 2023-09-21T12:21:00Z

I like this idea. :) I think it is somewhat similar to my musings from a few years ago, but goes a lot further: instead of just making these two cases the two variants of an enum, they become two entirely separate types. This means the parts they share have to be duplicated, but being able to have one of them in std does seem extremely useful.

library/std/src/panicking.rs

craterbot · 2023-09-22T15:33:40Z

🎉 Experiment pr-115974 is completed!
📊 44 regressed and 3 fixed (370213 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

m-ou-se · 2023-09-23T10:51:24Z

Crater results:

15 failures are because of usage of the unstable .message() method (which changed signature). All of these crates explicitly opted out of stability using a nightly feature flag to use this unstable method.

Of those 15, two additionally also use .payload() on core::panic::PanicInfo, which no longer exists (in core) after this PR. I will add a #[deprecated] fn payload to core's PanicInfo that returns a dummy value (identical in behavior to before this PR) to address that. (But these two crates would need changes anyway because of their unstable usage of .message().)

20 crates that failed to build because a dependency cannot be built.

7 unrelated failures (disk full, etc.).

And 2 regressions of stable code:

yacurses 0.2.4 imports PanicInfo from core rather than std, even though it is only used with a panic handler for std::panic::set_hook: https://github.com/Lokathor/yacurses/blob/8d6230a428f521f6de33a508e922c6da7aa8be6f/src/lib.rs#L52 (cc @Lokathor)
damip/rswasm (only on GitHub, not on crates.io) uses core::panic::PanicInfo in the panic hook signature, which is also only used with std::panic::set_hook: https://github.com/damip/rswasm/blob/5c58c9e079e5bdc3fef611072410ca4f4e6d99eb/src/lib.rs#L55 (cc @damip) (edit: fixed!)

Neither of these two are cases where a panic handler/hook is a function that is used both as a no_std #[panic_handler] and as a std panic hook. In both cases, it is only used in combination with std::panic::set_hook, so they can be fixed by just changing core to std.

m-ou-se · 2023-09-23T10:52:06Z

Looks like we might actually be able to do this.

Rollup of 6 pull requests Successful merges: - rust-lang#115974 (Split core's PanicInfo and std's PanicInfo) - rust-lang#125659 (Remove usage of `isize` in example) - rust-lang#125669 (CI: Update riscv64gc-linux job to Ubuntu 22.04, rename to riscv64gc-gnu) - rust-lang#125684 (Account for existing bindings when suggesting `pin!()`) - rust-lang#126055 (Expand list of trait implementers in E0277 when calling rustc with --verbose) - rust-lang#126174 (Migrate `tests/run-make/prefer-dylib` to `rmake.rs`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#115974 - m-ou-se:panicinfo-and-panicinfo, r=Amanieu Split core's PanicInfo and std's PanicInfo `PanicInfo` is used in two ways: 1. As argument to the `#[panic_handler]` in `no_std` context. 2. As argument to the [panic hook](https://doc.rust-lang.org/stable/std/panic/fn.set_hook.html) in `std` context. In situation 1, the `PanicInfo` always has a *message* (of type `fmt::Arguments`), but never a *payload* (of type `&dyn Any`). In situation 2, the `PanicInfo` always has a *payload* (which is often a `String`), but not always a *message*. Having these as the same type is annoying. It means we can't add `.message()` to the first one without also finding a way to properly support it on the second one. (Which is what rust-lang#66745 is blocked on.) It also means that, because the implementation is in `core`, the implementation cannot make use of the `String` type (which doesn't exist in `core`): https://github.com/rust-lang/rust/blob/0692db1a9082380e027f354912229dfd6af37e78/library/core/src/panic/panic_info.rs#L171-L172 This also means that we cannot easily add a useful method like `PanicInfo::payload_as_str() -> Option<&str>` that works for both `&'static str` and `String` payloads. I don't see any good reasons for these to be the same type, other than historical reasons. --- This PR is makes 1 and 2 separate types. To try to avoid breaking existing code and reduce churn, the first one is still named `core::panic::PanicInfo`, and `std::panic::PanicInfo` is a new (deprecated) alias to `PanicHookInfo`. The crater run showed this as a viable option, since people write `core::` when defining a `#[panic_handler]` (because they're in `no_std`) and `std::` when writing a panic hook (since then they're definitely using `std`). On top of that, many definitions of a panic hook don't specify a type at all: they are written as a closure with an inferred argument type. (Based on some thoughts I was having here: rust-lang#115561 (comment)) --- For the release notes: > We have renamed `std::panic::PanicInfo` to `std::panic::PanicHookInfo`. The old name will continue to work as an alias, but will result in a deprecation warning starting in Rust 1.82.0. > > `core::panic::PanicInfo` will remain unchanged, however, as this is now a *different type*. > > The reason is that these types have different roles: `std::panic::PanicHookInfo` is the argument to the [panic hook](https://doc.rust-lang.org/stable/std/panic/fn.set_hook.html) in std context (where panics can have an arbitrary payload), while `core::panic::PanicInfo` is the argument to the [`#[panic_handler]`](https://doc.rust-lang.org/nomicon/panic-handler.html) in no_std context (where panics always carry a formatted *message*). Separating these types allows us to add more useful methods to these types, such as `std::panic::PanicHookInfo::payload_as_str()` and `core::panic::PanicInfo::message()`.

library/core/src/panic/panic_info.rs

library/std/src/panic.rs

RalfJung · 2024-06-12T06:14:18Z

library/std/src/panicking.rs

- None => "Box<dyn Any>",
- },
- };
+ let msg = payload_as_str(info.payload());


Can't this use info.payload_as_str()?

I want to use the same fallback string ("Box<dyn Any>") as in the other cases here, so I re-use the same helper function here. And that helper function doesn't use PanicHookInfo::payload_as_str(), because it is also used in places where there is no PanicHookInfo.

Makes sense... looking at my own code doing these downcasts, it's mostly working on the return value from catch_unwind, where there's also no PanicHookInfo. Maybe we should have some more widely used helper method that works on all Box<dyn Any ...> or &dyn Any or so.

m-ou-se · 2024-06-12T11:21:41Z

@RalfJung Thanks for catching those things! I've addressed your comments in #126322

Manishearth · 2024-06-20T22:41:18Z

This caused a minor regression in nightly: #126766

Pass `fmt::Arguments` by reference to `PanicInfo` and `PanicMessage` Resolves rust-lang#129330 For some reason after rust-lang#115974 and rust-lang#126732 optimizations applied to panic handler became worse and compiler stopped removing panic locations if they are not used in the panic message. This PR fixes that and maybe we can merge it into beta before rust 1.81 is released. Note: optimization only works with `lto = "fat"`. r? libs-api

`panic_info_message` has been stable since 1.81.0 see https://stackoverflow.com/questions/78900800/trying-to-access-the-panic-info-message-causes-a-mismatched-types-error and rust-lang/rust#115974

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [rust](https://github.com/rust-lang/rust) | minor | `1.80.1` -> `1.81.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>rust-lang/rust (rust)</summary> ### [`v1.81.0`](https://github.com/rust-lang/rust/blob/HEAD/RELEASES.md#Version-1810-2024-09-05) [Compare Source](rust-lang/rust@1.80.1...1.81.0) \========================== <a id="1.81.0-Language"></a> ## Language - [Abort on uncaught panics in `extern "C"` functions.](rust-lang/rust#116088) - [Fix ambiguous cases of multiple `&` in elided self lifetimes.](rust-lang/rust#117967) - [Stabilize `#[expect]` for lints (RFC 2383),](rust-lang/rust#120924) like `#[allow]` with a warning if the lint is *not* fulfilled. - [Change method resolution to constrain hidden types instead of rejecting method candidates.](rust-lang/rust#123962) - [Bump `elided_lifetimes_in_associated_constant` to deny.](rust-lang/rust#124211) - [`offset_from`: always allow pointers to point to the same address.](rust-lang/rust#124921) - [Allow constraining opaque types during subtyping in the trait system.](rust-lang/rust#125447) - [Allow constraining opaque types during various unsizing casts.](rust-lang/rust#125610) - [Deny keyword lifetimes pre-expansion.](rust-lang/rust#126762) <a id="1.81.0-Compiler"></a> ## Compiler - [Make casts of pointers to trait objects stricter.](rust-lang/rust#120248) - [Check alias args for well-formedness even if they have escaping bound vars.](rust-lang/rust#123737) - [Deprecate no-op codegen option `-Cinline-threshold=...`.](rust-lang/rust#124712) - [Re-implement a type-size based limit.](rust-lang/rust#125507) - [Properly account for alignment in `transmute` size checks.](rust-lang/rust#125740) - [Remove the `box_pointers` lint.](rust-lang/rust#126018) - [Ensure the interpreter checks bool/char for validity when they are used in a cast.](rust-lang/rust#126265) - [Improve coverage instrumentation for functions containing nested items.](rust-lang/rust#127199) - Target changes: - [Add Tier 3 `no_std` Xtensa targets:](rust-lang/rust#125141) `xtensa-esp32-none-elf`, `xtensa-esp32s2-none-elf`, `xtensa-esp32s3-none-elf` - [Add Tier 3 `std` Xtensa targets:](rust-lang/rust#126380) `xtensa-esp32-espidf`, `xtensa-esp32s2-espidf`, `xtensa-esp32s3-espidf` - [Add Tier 3 i686 Redox OS target:](rust-lang/rust#126192) `i686-unknown-redox` - [Promote `arm64ec-pc-windows-msvc` to Tier 2.](rust-lang/rust#126039) - [Promote `loongarch64-unknown-linux-musl` to Tier 2 with host tools.](rust-lang/rust#126298) - [Enable full tools and profiler for LoongArch Linux targets.](rust-lang/rust#127078) - [Unconditionally warn on usage of `wasm32-wasi`.](rust-lang/rust#126662) (see compatibility note below) - Refer to Rust's \[platform support page]\[platform-support-doc] for more information on Rust's tiered platform support. <a id="1.81.0-Libraries"></a> ## Libraries - [Split core's `PanicInfo` and std's `PanicInfo`.](rust-lang/rust#115974) (see compatibility note below) - [Generalize `{Rc,Arc}::make_mut()` to unsized types.](rust-lang/rust#116113) - [Replace sort implementations with stable `driftsort` and unstable `ipnsort`.](rust-lang/rust#124032) All `slice::sort*` and `slice::select_nth*` methods are expected to see significant performance improvements. See the [research project](https://github.com/Voultapher/sort-research-rs) for more details. - [Document behavior of `create_dir_all` with respect to empty paths.](rust-lang/rust#125112) - [Fix interleaved output in the default panic hook when multiple threads panic simultaneously.](rust-lang/rust#127397) <a id="1.81.0-Stabilized-APIs"></a> ## Stabilized APIs - [`core::error`](https://doc.rust-lang.org/stable/core/error/index.html) - [`hint::assert_unchecked`](https://doc.rust-lang.org/stable/core/hint/fn.assert_unchecked.html) - [`fs::exists`](https://doc.rust-lang.org/stable/std/fs/fn.exists.html) - [`AtomicBool::fetch_not`](https://doc.rust-lang.org/stable/core/sync/atomic/struct.AtomicBool.html#method.fetch_not) - [`Duration::abs_diff`](https://doc.rust-lang.org/stable/core/time/struct.Duration.html#method.abs_diff) - [`IoSlice::advance`](https://doc.rust-lang.org/stable/std/io/struct.IoSlice.html#method.advance) - [`IoSlice::advance_slices`](https://doc.rust-lang.org/stable/std/io/struct.IoSlice.html#method.advance_slices) - [`IoSliceMut::advance`](https://doc.rust-lang.org/stable/std/io/struct.IoSliceMut.html#method.advance) - [`IoSliceMut::advance_slices`](https://doc.rust-lang.org/stable/std/io/struct.IoSliceMut.html#method.advance_slices) - [`PanicHookInfo`](https://doc.rust-lang.org/stable/std/panic/struct.PanicHookInfo.html) - [`PanicInfo::message`](https://doc.rust-lang.org/stable/core/panic/struct.PanicInfo.html#method.message) - [`PanicMessage`](https://doc.rust-lang.org/stable/core/panic/struct.PanicMessage.html) These APIs are now stable in const contexts: - [`char::from_u32_unchecked`](https://doc.rust-lang.org/stable/core/char/fn.from_u32\_unchecked.html) (function) - [`char::from_u32_unchecked`](https://doc.rust-lang.org/stable/core/primitive.char.html#method.from_u32\_unchecked) (method) - [`CStr::count_bytes`](https://doc.rust-lang.org/stable/core/ffi/c_str/struct.CStr.html#method.count_bytes) - [`CStr::from_ptr`](https://doc.rust-lang.org/stable/core/ffi/c_str/struct.CStr.html#method.from_ptr) <a id="1.81.0-Cargo"></a> ## Cargo - [Generated `.cargo_vcs_info.json` is always included, even when `--allow-dirty` is passed.](rust-lang/cargo#13960) - [Disallow `package.license-file` and `package.readme` pointing to non-existent files during packaging.](rust-lang/cargo#13921) - [Disallow passing `--release`/`--debug` flag along with the `--profile` flag.](rust-lang/cargo#13971) - [Remove `lib.plugin` key support in `Cargo.toml`. Rust plugin support has been deprecated for four years and was removed in 1.75.0.](rust-lang/cargo#13902) <a id="1.81.0-Compatibility-Notes"></a> ## Compatibility Notes - Usage of the `wasm32-wasi` target will now issue a compiler warning and request users switch to the `wasm32-wasip1` target instead. Both targets are the same, `wasm32-wasi` is only being renamed, and this [change to the WASI target](https://blog.rust-lang.org/2024/04/09/updates-to-rusts-wasi-targets.html) is being done to enable removing `wasm32-wasi` in January 2025. - We have renamed `std::panic::PanicInfo` to `std::panic::PanicHookInfo`. The old name will continue to work as an alias, but will result in a deprecation warning starting in Rust 1.82.0. `core::panic::PanicInfo` will remain unchanged, however, as this is now a *different type*. The reason is that these types have different roles: `std::panic::PanicHookInfo` is the argument to the [panic hook](https://doc.rust-lang.org/stable/std/panic/fn.set_hook.html) in std context (where panics can have an arbitrary payload), while `core::panic::PanicInfo` is the argument to the [`#[panic_handler]`](https://doc.rust-lang.org/nomicon/panic-handler.html) in no_std context (where panics always carry a formatted *message*). Separating these types allows us to add more useful methods to these types, such as `std::panic::PanicHookInfo::payload_as_str()` and `core::panic::PanicInfo::message()`. - The new sort implementations may panic if a type's implementation of [`Ord`](https://doc.rust-lang.org/std/cmp/trait.Ord.html) (or the given comparison function) does not implement a [total order](https://en.wikipedia.org/wiki/Total_order) as the trait requires. `Ord`'s supertraits (`PartialOrd`, `Eq`, and `PartialEq`) must also be consistent. The previous implementations would not "notice" any problem, but the new implementations have a good chance of detecting inconsistencies, throwing a panic rather than returning knowingly unsorted data. - [In very rare cases, a change in the internal evaluation order of the trait solver may result in new fatal overflow errors.](rust-lang/rust#126128) <a id="1.81.0-Internal-Changes"></a> ## Internal Changes These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. - [Add a Rust-for Linux `auto` CI job to check kernel builds.](rust-lang/rust#125209) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

m-ou-se added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Sep 19, 2023

m-ou-se self-assigned this Sep 19, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2023

m-ou-se mentioned this pull request Sep 19, 2023

Make PanicInfo::message infallible #115561

Closed

m-ou-se removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2023

This comment was marked as outdated.

Sign in to view

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 19, 2023

This comment was marked as outdated.

Sign in to view

m-ou-se mentioned this pull request Sep 20, 2023

Tracking issue for cleaning up core's and std's panic machinery #116005

Open

15 tasks

This comment was marked as outdated.

Sign in to view

RalfJung reviewed Sep 21, 2023

View reviewed changes

library/std/src/panicking.rs Outdated Show resolved Hide resolved

library/std/src/panicking.rs Outdated Show resolved Hide resolved

library/std/src/panicking.rs Outdated Show resolved Hide resolved

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Sep 22, 2023

jieyouxu mentioned this pull request Jun 11, 2024

Rollup of 6 pull requests #126284

Merged

bors merged commit d9deb38 into rust-lang:master Jun 12, 2024
6 checks passed

rustbot added this to the 1.81.0 milestone Jun 12, 2024

rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 12, 2024

m-ou-se deleted the panicinfo-and-panicinfo branch June 12, 2024 02:29

StackOverflowExcept1on mentioned this pull request Jun 12, 2024

Tracking issue for PanicInfo::message #66745

Closed

9 tasks