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

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

Closed
aturon opened this Issue Sep 11, 2017 · 63 comments

Comments

@aturon
Member

aturon commented Sep 11, 2017

Update(2018-08-28)


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:

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 #51341
  • panic_implementation: no_mangle attribute needed? #51342
  • rust_begin_unwind not emitted when crate-type = bin #51671
@whitequark

This comment has been minimized.

Contributor

whitequark commented Sep 12, 2017

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

This comment has been minimized.

Contributor

yodaldevoid commented Sep 14, 2017

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

This comment has been minimized.

Contributor

whitequark commented Sep 14, 2017

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

This comment has been minimized.

Contributor

Zoxc commented Sep 19, 2017

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

This comment has been minimized.

Contributor

Zoxc commented Sep 19, 2017

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

This comment has been minimized.

Contributor

Zoxc commented Sep 19, 2017

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

This comment has been minimized.

Contributor

Zoxc commented Sep 19, 2017

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

This comment has been minimized.

Contributor

Zoxc commented Sep 21, 2017

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

This comment has been minimized.

Contributor

SimonSapin commented Jan 23, 2018

#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

This comment has been minimized.

Contributor

SimonSapin commented Jan 27, 2018

@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

This comment has been minimized.

Contributor

Zoxc commented Jan 27, 2018

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

bors added a commit that referenced this issue Feb 16, 2018

Auto merge of #47687 - SimonSapin:panic-impl, r=sfackler
RFC 2070 part 1: PanicInfo and Location API changes

This implements part of https://rust-lang.github.io/rfcs/2070-panic-implementation.html
Tracking issue: #44489

* Move `std::panic::PanicInfo` and `std::panic::Location` to a new `core::panic` module. The two types and the `std` module were already `#[stable]` and stay that way, the new `core` module is `#[unstable]`.
* Add a new `PanicInfo::message(&self) -> Option<&fmt::Arguments>` method, which is `#[unstable]`.
* Implement `Display` for `PanicInfo` and `Location`

bors added a commit that referenced this issue Feb 18, 2018

Auto merge of #47687 - SimonSapin:panic-impl, r=sfackler
RFC 2070 part 1: PanicInfo and Location API changes

This implements part of https://rust-lang.github.io/rfcs/2070-panic-implementation.html
Tracking issue: #44489

* Move `std::panic::PanicInfo` and `std::panic::Location` to a new `core::panic` module. The two types and the `std` module were already `#[stable]` and stay that way, the new `core` module is `#[unstable]`.
* Add a new `PanicInfo::message(&self) -> Option<&fmt::Arguments>` method, which is `#[unstable]`.
* Implement `Display` for `PanicInfo` and `Location`
@nagisa

This comment has been minimized.

Contributor

nagisa commented Feb 21, 2018

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

This comment has been minimized.

Contributor

nagisa commented Mar 5, 2018

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

This comment has been minimized.

Contributor

glandium commented Mar 15, 2018

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

This comment has been minimized.

Member

japaric commented Mar 15, 2018

@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

This comment has been minimized.

Contributor

nagisa commented Mar 15, 2018

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

This comment has been minimized.

Contributor

glandium commented Mar 15, 2018

@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.

@nagisa

This comment has been minimized.

Contributor

nagisa commented Apr 2, 2018

A summary from the All Hands /wrt panic size problem.

We did discuss various approaches to handling the issue of panic size, and figured out that we’d love to have something that works in all scenarios such as non-lto builds and even debug builds. We didn’t arrive at any particular design, but we managed to reject a number of designs that wouldn’t work and also realised that the only real way to properly implement any design would rely on MIR-only rlibs.

With that in mind, this design seems to be as good as any other and, as long as we stabilise #[panic_implementation] carefully (i.e. the signature and behaviour, but not the exact expansion), we should be in a good place to implement a solution for panic sizes as well.


I’m willing to mentor implementation of the RFC. The instructions written down by @SimonSapin are part-way right; I’ll write up some new and more detailed instructions over the week or two.

@japaric

This comment has been minimized.

Member

japaric commented Aug 28, 2018

As per Niko's request this is a summary of the above thread and covers what's
being stabilized, how it's tested, deviations from the RFC and remaining known
bugs:

RFC text: https://github.com/rust-lang/rfcs/blob/master/text/2070-panic-implementation.md

What's being stabilized:

  • The panic_handler attribute. This attribute lets you define the behavior of
    core::panic!. This attribute is basically the old panic_fmt lang item but
    with a stable API.

What's not being stabilized at this point:

  • Support for payloads in core::panic! (e.g. panic!(42)). This has not been
    implemented but it's backward compatible to add. Rationale was to speed up
    stabilization by not adding new functionality to core::panic!.

We test:

Deviations from the RFC:

Remaining known bugs:

None left. There were a few bugs related to symbol visibility and codegen
options like LTO, but they have been fixed.

@rfcbot

This comment has been minimized.

rfcbot commented Aug 28, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Sep 7, 2018

Can @rust-lang/lang assign someone or update us on the current state here? Was this intended to be stabilized in RC? If so we need a stabilization PR ASAP.

@japaric

This comment has been minimized.

Member

japaric commented Sep 7, 2018

@Mark-Simulacrum

Was this intended to be stabilized in RC?

Yes, it would be ideal to have this in RC1 (1.30-beta). If it doesn't make the beta cutoff, could we backport it?

If so we need a stabilization PR ASAP.

Stabilization PR

Reference PR

Updating / finishing them up right now.

kennytm added a commit to kennytm/rust that referenced this issue Sep 7, 2018

Rollup merge of rust-lang#51366 - japaric:stable-panic-impl, r=Mark-S…
…imulacrum

stabilize #[panic_handler]

closes rust-lang#44489

### Update(2018-09-07)

This was proposed for stabilization in rust-lang#44489 (comment) and its FCP with disposition to merge / accept is nearly over. The summary of what's being stabilized can be found in rust-lang#44489 (comment)

Documentation PRs:

- Reference. rust-lang-nursery/reference#362
- Nomicon. rust-lang-nursery/nomicon#75

---

`#[panic_implementation]` was implemented recently in rust-lang#50338. `#[panic_implementation]` is basically the old `panic_fmt` language item but in a less error prone (\*) shape. There are still some issues and questions to sort out around this feature (cf. rust-lang#44489) but this PR is meant to start a discussion about those issues / questions with the language team.

(\*) `panic_fmt` was not type checked; changes in its function signature caused serious, silent binary size regressions like the one observed in rust-lang#43054

Some unresolved questions from rust-lang#44489:

> 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".

The current implementation formats `PanicInfo` as the first alternative, which is how panic messages are formatted by the `std` panic handler. The `Display` implementation is more than a convenience: `PanicInfo.message` is unstable so it's not possible to replicate the `Display` implementation on stable.

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

I believe @whitequark made more progress with unwinding in no-std since their last comment in rust-lang#44489. Perhaps they can give us an update?

---

Another unresolved question is where this feature should be documented. The feature currently doesn't have any documentation.

cc @rust-lang/lang
cc @jackpot51 @alevy @phil-opp
@rfcbot

This comment has been minimized.

rfcbot commented Sep 7, 2018

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

bors added a commit that referenced this issue Sep 8, 2018

Auto merge of #51366 - japaric:stable-panic-impl, r=Mark-Simulacrum
stabilize #[panic_handler]

closes #44489

### Update(2018-09-07)

This was proposed for stabilization in #44489 (comment) and its FCP with disposition to merge / accept is nearly over. The summary of what's being stabilized can be found in #44489 (comment)

Documentation PRs:

- Reference. rust-lang-nursery/reference#362
- Nomicon. rust-lang-nursery/nomicon#75

---

`#[panic_implementation]` was implemented recently in #50338. `#[panic_implementation]` is basically the old `panic_fmt` language item but in a less error prone (\*) shape. There are still some issues and questions to sort out around this feature (cf. #44489) but this PR is meant to start a discussion about those issues / questions with the language team.

(\*) `panic_fmt` was not type checked; changes in its function signature caused serious, silent binary size regressions like the one observed in #43054

Some unresolved questions from #44489:

> 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".

The current implementation formats `PanicInfo` as the first alternative, which is how panic messages are formatted by the `std` panic handler. The `Display` implementation is more than a convenience: `PanicInfo.message` is unstable so it's not possible to replicate the `Display` implementation on stable.

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

I believe @whitequark made more progress with unwinding in no-std since their last comment in #44489. Perhaps they can give us an update?

---

Another unresolved question is where this feature should be documented. The feature currently doesn't have any documentation.

cc @rust-lang/lang
cc @jackpot51 @alevy @phil-opp

kennytm added a commit to kennytm/rust that referenced this issue Sep 8, 2018

Rollup merge of rust-lang#51366 - japaric:stable-panic-impl, r=Mark-S…
…imulacrum

stabilize #[panic_handler]

closes rust-lang#44489

### Update(2018-09-07)

This was proposed for stabilization in rust-lang#44489 (comment) and its FCP with disposition to merge / accept is nearly over. The summary of what's being stabilized can be found in rust-lang#44489 (comment)

Documentation PRs:

- Reference. rust-lang-nursery/reference#362
- Nomicon. rust-lang-nursery/nomicon#75

---

`#[panic_implementation]` was implemented recently in rust-lang#50338. `#[panic_implementation]` is basically the old `panic_fmt` language item but in a less error prone (\*) shape. There are still some issues and questions to sort out around this feature (cf. rust-lang#44489) but this PR is meant to start a discussion about those issues / questions with the language team.

(\*) `panic_fmt` was not type checked; changes in its function signature caused serious, silent binary size regressions like the one observed in rust-lang#43054

Some unresolved questions from rust-lang#44489:

> 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".

The current implementation formats `PanicInfo` as the first alternative, which is how panic messages are formatted by the `std` panic handler. The `Display` implementation is more than a convenience: `PanicInfo.message` is unstable so it's not possible to replicate the `Display` implementation on stable.

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

I believe @whitequark made more progress with unwinding in no-std since their last comment in rust-lang#44489. Perhaps they can give us an update?

---

Another unresolved question is where this feature should be documented. The feature currently doesn't have any documentation.

cc @rust-lang/lang
cc @jackpot51 @alevy @phil-opp

bors added a commit that referenced this issue Sep 8, 2018

Auto merge of #51366 - japaric:stable-panic-impl, r=Mark-Simulacrum
stabilize #[panic_handler]

closes #44489

### Update(2018-09-07)

This was proposed for stabilization in #44489 (comment) and its FCP with disposition to merge / accept is nearly over. The summary of what's being stabilized can be found in #44489 (comment)

Documentation PRs:

- Reference. rust-lang-nursery/reference#362
- Nomicon. rust-lang-nursery/nomicon#75

---

`#[panic_implementation]` was implemented recently in #50338. `#[panic_implementation]` is basically the old `panic_fmt` language item but in a less error prone (\*) shape. There are still some issues and questions to sort out around this feature (cf. #44489) but this PR is meant to start a discussion about those issues / questions with the language team.

(\*) `panic_fmt` was not type checked; changes in its function signature caused serious, silent binary size regressions like the one observed in #43054

Some unresolved questions from #44489:

> 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".

The current implementation formats `PanicInfo` as the first alternative, which is how panic messages are formatted by the `std` panic handler. The `Display` implementation is more than a convenience: `PanicInfo.message` is unstable so it's not possible to replicate the `Display` implementation on stable.

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

I believe @whitequark made more progress with unwinding in no-std since their last comment in #44489. Perhaps they can give us an update?

---

Another unresolved question is where this feature should be documented. The feature currently doesn't have any documentation.

cc @rust-lang/lang
cc @jackpot51 @alevy @phil-opp

@bors bors closed this in #51366 Sep 8, 2018

josephlr added a commit to josephlr/bootloader that referenced this issue Sep 11, 2018

Remove stabilized features
panic_implemenation was renamed to panic_handler:
   rust-lang/rust#44489 (comment)
panic_handler was stablized:
   rust-lang/rust#51366

`cargo check` now succeeds without warnings

phil-opp added a commit to rust-osdev/bootloader that referenced this issue Sep 30, 2018

Remove stabilized features (#25)
panic_implemenation was renamed to panic_handler:
   rust-lang/rust#44489 (comment)
panic_handler was stablized:
   rust-lang/rust#51366

`cargo check` now succeeds without warnings
@sanmai-NL

This comment has been minimized.

sanmai-NL commented Oct 19, 2018

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

@Centril

This comment has been minimized.

Contributor

Centril commented Oct 19, 2018

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

@sanmai-NL

This comment has been minimized.

sanmai-NL commented Oct 19, 2018

@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

This comment has been minimized.

Contributor

Centril commented Oct 19, 2018

@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. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment