Tracking issue for RFC 2070: stable mechanism to specify the behavior of panic! in no-std applications

[aturon]

Update(2018-08-28)

• Stabilization summary.
• FCP checkboxes

---

This is a tracking issue for the RFC "stable mechanism to specify the behavior of panic! in no-std applications " (rust-lang/rfcs#2070).

Steps:

• Implement the RFC
• Adjust documentation (see instructions on forge)
  • The doc-comment of libcore mentions rust_begin_panic
• Stabilization PR (see instructions on forge)

Unresolved questions:

fmt::Display

Should the Display of PanicInfo format the panic information as "panicked at 'reason', src/main.rs:27:4", as "'reason', src/main.rs:27:4", or simply as
"reason".

Unwinding in no-std

Is this design compatible, or can it be extended to work, with unwinding
implementations for no-std environments?

Issues to solve before stabilization

• panic_implementation: Error message still talks about the panic_fmt lang item when missing panic_implementation: Error message talks about the panic_impl lang item when missing #51341
• panic_implementation: no_mangle attribute needed? panic_implementation: no_mangle attribute needed? #51342
• rust_begin_unwind not emitted when crate-type = bin rust_begin_unwind not emitted when crate-type = bin #51671

[whitequark]

Unwinding in no-std

Is this design compatible, or can it be extended to work, with unwinding
implementations for no-std environments?

As someone with a working unwinding implementation in a no_std environment (not with a Rust personality function though), I can answer this if someone summarizes the feature for me. I kind of lost track and I don't have time to get into every minor detail at the moment.

[yodaldevoid]

As someone with a working unwinding implementation in a no_std environment (not with a Rust personality function though)...

Sorry to bring this off topic so soon, but could you explain this? I was under the impression that unless you wanted to implement unwinding in an ad-hoc way (e.g. doing unwinding during panic by explicitly calling some function) you had to implement the eh_personality function.

[whitequark]

I was under the impression that unless you wanted to implement unwinding in an ad-hoc way (e.g. doing unwinding during panic by explicitly calling some function) you had to implement the eh_personality function.

I use unwinding in a language runtime. The language has exceptions (and so unwinding), however Rust frames are never unwound, and so there's no need for the personality function.

[Zoxc]

I would like the Location interface to both support the ability to pass backtraces (effectively a fn caller(&self) -> Option<Location>) and decompression of location information which requires accessing this information in a closure passed to a Location function to avoid heap allocation.

[Zoxc]

We should also support no location information at all, and abstract location information (for example just an identifying number which can be used to find the relevant information in some external source).

[Zoxc]

I did not notice that PanicInfo and Location were already stable. I guess we can just deprecate PanicInfo::location and make it return None if we want some more expressive API in the future.

[Zoxc]

I'm really not a fan of using the same API for payloads and messages. I'd rather panic! be pure message and provide some other API for payloads. So basically I want PanicInfo without location and payload. I have not seen any motivation for payloads for no_std use cases either.

[Zoxc]

I kind of prefer a PanicInfo type with no fields or methods and just an Display implementation.

One problem with mixing the payload and the message is that we can't strip panic messages from the binary without changing the meaning of programs relying on payloads.

[SimonSapin]

#47687 does part of the implementation. I think after that what’s needed is:

• Change the signature of the panic_fmt lang item from (msg: fmt::Arguments, file: &'static str, line: u32, col: u32) -> ! to (&PanicInfo) -> !. This a breaking change to an unstable feature. (Maybe also rename it to begin_panic or something without fmt in the name?)
  • When creating a PanicInfo in src/libcore/panicking.rs to call the modified lang item, use the &'static str message as the payload in panic() and a value of a new private struct NoPayload; type in panic_fmt()
  • Make that core::panicking::panic function generic to accept any P: Any + Send payload instead of just &'static str.
• Add an unstable #[panic_implementation] proc-macro attribute gated on #![feature(panic_implementation)] that checks that it is on a extern fn with the appropriate signature, and expands to #[lang = "panic_fmt"]. This expansion uses stability hygiene such that the containing crate does not need the lang_items feature gate, only the panic_implementation one (until that one is stabilized)
• Change the error[E0152]: duplicate lang item found: `panic_fmt` error to have its own error number and a message that talks about duplicate #[panic_implementation] instead. Or maybe write new code to emit that new error when appropriate. See what the implementation of #[global_allocator] does.

Note however that src/libcore/panicking.rs contains some comments about doing a things a certain way so that so that the linker can eliminate some of the string formatting machinery to limit code size bloat when formatting is not otherwise used. It’s possible that these changes would defeat those optimizations. I don’t know what we can do to try and preserve them while implementing RFC 2070.

CC @japaric

[SimonSapin]

@Zoxc We can still propose changing RFC 2070, but a Box<Any + Send + 'static> payload for panics is already part of some stable APIs:

• In std::thread::Result, the return type of std::thread::JoinHandle and std::panic::catch_unwind
• As a parameter of std::panic::resume_unwind
• PanicInfo::payload and std::panic::set_hook are already stable

[Zoxc]

@SimonSapin We could just pass a NoPayload struct for panics without payloads though. Anyway we don't need to repeat mistakes in libcore.

[nagisa]

From what I can tell, the RFC still does not address removal of unnecessary panic information directly, however it is an extremely easy thing to do that does not require much extra design past what was already accepted.

Consider for example this scenario:

#[panic_implementation]
fn my_panic_impl(pi: &PanicInfo) -> ! {
    abort()
}

This is a case, where it is entirely within the power of compiler to not keep any strings, line numbers, etc. pertaining to panic locations. However as soon as the panic implementation uses any information:

#[panic_implementation]
fn my_panic_impl(pi: &PanicInfo) -> ! {
    abort_with_line(pi.location(),map(|l| l.line()));
}

the compiler is forced to keep all the panic information, despite the fact that only the line numbers are of any interest for that particular implementation of panicking.

---

I want to propose a following change (benefits of which are laid out at the end of this comment):

struct AnyConcreteTypeMayGoHere { line: Option<u64> }

impl ::core::panic::PanicInfoCtr for AnyConcreteTypeMayGoHere {
    const fn new_v1(message: Option<&fmt::Arguments>, location: Option<Location>, ...etc) -> Self {
        AnyConcreteTypeMayGoHere { line: location.map(|l| l.line()) }
    }
}

#[panic_implementation]
fn my_panic_impl(pi: &AnyConcreteTypeMayGoHere) -> ! { abort_with_line(pi.line) }

and at the location of panic rust would generate code similar to this (with _ filled in with the type that my_panic_impl takes as an argument):

static PANIC_INFO: _ = <_ as ::core::panic::PanicInfoCtr>::new_v1(args, loc, ...);
my_panic_impl(&PANIC_INFO);

Benefits of this approach are:

1. It allows users to only keep the important bits about the panic, without relying on the compiler optimisations. AnyConcreteTypeMayGoHere will only retain line info even before optimisations come into play. It would also allow omitting the panic information entirely (important not only for embedded and wasm, but also closed-source code that doesn't want to expose internal information¹)
2. It is fairly extensible in backward-compatible manner without breaking the users. Consider for example addition of column numbers, which bloated various applications where code size is very important. With AnyConcreteTypeMayGoHere not retaining the column number in the first place, no extra bloat would’ve been introduced. Furthermore, if we wanted to add another argument to the PanicInfo constructor, new_v2(...) with default implementation calling new_v1 would be a backward-compatible change, etc etc.

libpanic_unwind/libpanic_abort would then provide their own #[panic_implementation] that takes &std::panic::PanicInfo as an argument, and libcore would provide a few default common-sense implementations of PanicInfoCtr trait -- one that mimics the std::panic::PanicInfo as closely as feasible, one that retains no information whatsoever, etc, so that #[panic_implementation] could be made stable before const fn is. And people whom none of defaults work out can implement their own PanicInfo using the const_fn unstable feature.

¹: there were a few issues about this, but I can’t find them anymore.

[nagisa]

Status update: I realised above cannot work without being very unwieldy. Namely, the proposal above would require the panic_implementation to be available to all the leaf crates, before code for them could be generated. This, unlike e.g. custom allocator providers, wouldn’t be something that would work simply by linking in some stuff from anywhere in the crate graph.

[glandium]

There is an issue that IMHO, a stable mechanism for panic! in no-std applications should take into account: for at least two reasons, you can only set the panic handler in leaf crates.

One of them is issue #48661, where the linker simply removes the panic handler if it's in its own crate.

The second of them is that the cargo test harness pulls std, and the build fails with:

error: duplicate lang item in crate `std`: `panic_fmt`.
  |
  = note: first defined in crate `panic`.

and there is no way for such a crate to be built differently for test.

[japaric]

@glandium

The second of them is that the cargo test harness pulls std, and the build fails with:

This may be a problem short term but custom test framework will become available this year and with those it will be possible to write test runners than don't rely on unwinding (e.g. unit tests return Err to indicate failure) which will make it possible to test targets (e.g. thumbv7m-none-eabi) that don't have a panic implementation that unwinds.

@nagisa

Namely, the proposal above would require the panic_implementation to be available to all the leaf crates, before code for them could be generated.

This is probably a bad idea but would it be possible to store all panic!s in MIR format and defer the actual generation of the static variable (and the llvm-ir) until the user defined PanicInfo struct is known?

[nagisa]

Yeah, without overthinking the idea too much, I think that would be possible, however it has a hard prerequisite on MIR-only (r)libs.

[glandium]

@japaric: there are other reasons than unwinding to want to use std for tests on otherwise no_std crates. If your panic function is in a crate that contains nothing else, maybe a #[cfg(not(test))] on the extern crate can save you, but if your panic function is in a crate along with other things, there's no way out.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[sanmai-NL]

@Centril: Can you please update the OP with the current status (stabilization etc.)?

[Centril]

@sanmai-NL I will defer this to @japaric since they are most up to date as to the status.

[sanmai-NL]

@Centril: I saw at the end of this thread a very clear status indication, so I considered it a management thing requiring no involvement in the effort.

[Centril]

@sanmai-NL ok, I ticked some boxes which seemed fulfilled; but I don't have the time to do a more in-depth review right now. :)

[andreeaflorescu]

Hey, I got to this issue because it is linked to the message method of PanicInfo which is currently available only in rust nightly according to the the docs. As this issue is closed I was wondering if there's any plan to move the message method to stable.
Related to the following issues in Firecracker:

• pretty-printing panics
• user inquiry

Is there another recommended way to get the panic information? I was also thinking about replacing the panic_hook with catch_unwind but we might get into problems if the external crates that we are using are implementing panic with abort.

[curiousleo]

@bors I think this issue should be re-opened until panic_info_message is no longer an unstable feature. Currently it still is:

rust/src/libcore/panic.rs

Line 93 in 0f12bad

#[unstable(feature = "panic_info_message", issue = "44489")]

.

[yodaldevoid]

@Centril @japaric about the the above message as @bors is a bot.

[curiousleo]

@bors is a bot.

Lol thanks!

[eddyb]

It's not even the bot (@bors) closing the issue, this is a GitHub feature.
When a PR with the right keywords in its description (#51366 in this case) gets merged, issues can be closed automatically by GitHub.

@curiousleo I think the problem there is the unstable feature panic_info_message didn't get its own tracking issue, but I'm not sure.

Also a bit surprised that @andreeaflorescu didn't get a reply in almost a year...

[SimonSapin]

#66771 changes the tracking issue of PanicInfo::message to #66745, which also has some discussion of what to do about it.

[curiousleo]

#66771 changes the tracking issue of PanicInfo::message to #66745, which also has some discussion of what to do about it.

Thanks for following up! That's massively appreciated. I'll keep an eye on those two issues.