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

stable mechanism to specify the behavior of panic! in no-std applications #2070

Merged
merged 2 commits into from
Sep 11, 2017

Conversation

japaric
Copy link
Member

@japaric japaric commented Jul 20, 2017

The #![no_std] attribute was stabilized some time ago and it made possible to
build no-std libraries on stable. However, to this day no-std applications
still require a nightly compiler to be built. The main cause of this is that
the behavior of panic! is left undefined in no-std context, and the only way
to specify a panicking behavior is through the unstable panic_fmt language
item
.

This document proposes a stable mechanism to specify the behavior of panic! in
no-std context. This would be a step towards enabling development of no-std
applications like device firmware, kernels and operating systems on the stable
channel.

Rendered

cc @alexcrichton @brson @Ericson2314 @jackpot51 @whitequark

@whitequark
Copy link
Member

👍

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

I would say the former. I imagine that most people don't want custom panic formatting (I struggle to think of a single use case), and it can be easily constructed via the other getters anyway.

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

Sure is. You only need to add an attribute to define the personality lang item, and then make sure the libpanic_unwind crate has the proper #[cfg]s. Unwinding in no-std environments has very few differences from unwinding in std environments, namely:

  • The unwinder normally caches certain DWARF structures in a format that's more easily accessible, but that can be turned off if you don't have a heap or don't want to risk poorly bounded allocation.
  • The unwinder needs to be provided with the start and end of the .eh_frame_hdr.

I've successfully used the LLVM libunwind on a bare-metal target. The only issue with it is that libunwind expects you to provide ELF program headers, which you may not have in your firmware anymore, but that's a trivial patch to libunwind.

@pftbest
Copy link

pftbest commented Jul 20, 2017

The current issue I have with panic_fmt, is that if the panic handler does not print the message (or use it in any other way) and just goes into infinite loop, the message text is still present in the firmware and llvm can't optimize it away.
Can we somehow address this issue, or is it out of the scope for this proposal?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 20, 2017

the message text is still present in the firmware and llvm can't optimize it away.

I'd say we can add that backwards compatibly via a future RFC which allows using #[panic_implementation] on a function with fn() -> ! signature.

@therealprof
Copy link

therealprof commented Jul 20, 2017

@whitequark

I would say the former. I imagine that most people don't want custom panic formatting (I struggle to think of a single use case), and it can be easily constructed via the other getters anyway.

I'd say the latter. Adding location information wastes a lot of space, especially for embedded applications, without bringing any benefit whatsoever for explicit panic!()s. For implicit panics the situation is slightly different but I still don't think it's worth keeping the additional baggage.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 20, 2017

Maybe we could allow any type as the argument to the panic_implementation function. That argument would then have to implement a trait PanicArgument, which has a function taking all the info we want. Together with rustc's MIR inliner, we could probably build something that guarantees that all unused arguments get completely eliminated. I'd say the optimization guarantees are out of scope for this RFC, but we can still ensure that it will be possible to implement them in the future, by making the API as forward compatible as possible.


When using `core::panic!` with formatting, e.g. `panic!("{}", 42)`, the payload
will be uninspectable: it won't be downcastable to any known type. This is where
`core::panic!` diverges from `std::panic!`. The latter returns 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.

Why is that important?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to require allocations for no_std targets

@whitequark
Copy link
Member

I'd say the latter. Adding location information wastes a lot of space, especially for embedded applications, without bringing any benefit whatsoever for explicit panic!()s. For implicit panics the situation is slightly different but I still don't think it's worth keeping the additional baggage.

I disagree. The absolute worst offenders in embedded development are implicit panics (which are manageable) and methods like borrow_mut, which, past a certain point in complexity, are nearly impossible to debug without a debug port and gdb attached, which is somewhat of a luxury.

A proper solution here would be rustc saving location information elsewhere and only storing an index into that table in the executable itself. But that would be a completely different RFC (that I'm very interested in).

@therealprof
Copy link

@whitequark Maybe I'm not too deep into Rust yet to fully understand this but I do find the information carried along by implicit panics quite useless, even with unwinding support, because it only tells you where the panic happened but not the why.

Hence I find it more important to focus on the efficiency of explicit panic!() to not make them prohibitive expensive. If we can get the location information for close to free, I'm all game.

@pftbest
Copy link

pftbest commented Jul 20, 2017

I think we need to print the file and line information by default. The only case against them is firmware size, but in that case you can manually print the parts you need.

@whitequark Do you propose to print the error code to UART and then have a txt file that is generated during build with full error messages?

@eddyb
Copy link
Member

eddyb commented Jul 20, 2017

A proper solution here would be rustc saving location information elsewhere and only storing an index into that table in the executable itself.

All you need is to give a section name to the globals generated by rustc_trans for out-of-bounds and overflow checks and, syntactically, to those created by format_args!, isn't it?
If you have a section name you can just drop it, right? I guess you might want to keep some formatting but specifically not the ones from panics.

@japaric
Copy link
Member Author

japaric commented Jul 20, 2017

@pftbest

The current issue I have with panic_fmt, is that if the panic handler does not
print the message (or use it in any other way) and just goes into infinite
loop, the message text is still present in the firmware and llvm can't
optimize it away.

I can't repro. Are you using the right panic_fmt signature? The ABI changed
recently and the signature is not compile time checked. If you don't use the
right one then LLVM can't optimize away the strings or the formatting machinery.
With this RFC that ABI mismatch should never occur again. cf.
rust-lang/rust#43054

This is what I tested:

fn main() {
    #[cfg(A)]
    panic!("The quick brown fox jumps over the lazy dog.");

    #[cfg(B)]
    Err::<(), _>("The quick brown fox jumps over the lazy dog.").unwrap();

    #[cfg(C)]
    panic!("The answer is {}", 42);
}

#[lang = "panic_fmt"]
#[no_mangle]
pub unsafe extern "C" fn rust_begin_unwind(
    _msg: ::core::fmt::Arguments,
    _file: &'static str,
    _line: u32,
    _col: u32, // <- new-ish
) -> ! {
    // test::black_box
    #[cfg(X)]
    unsafe { asm!("" : : "r"(&_msg)) }

    #[cfg(any(X, Y))]
    loop {}

    #[cfg(Z)]
    ::core::intrinsics::abort()
}
# A, B or C with either Y or Z
$ arm-none-eabi-size -Ax app
target/thumbv7m-none-eabi/release/app  :
section           size        addr
.vector_table      304   134217728
.text              516   134218032
.ARM.attributes     46           0
Total              866

$ arm-none-eabi-objcopy -O binary app app.bin

$ arm-none-eabi-strings app.bin && echo nothing
nothing
# A, B or C with X
$ arm-none-eabi-size -Ax app
section           size        addr
.vector_table      304   134217728
.text              610   134218032
.rodata             44   134218656
.ARM.attributes     46           0
Total             1004

$ arm-none-eabi-objcopy -O binary app app.bin

$ arm-none-eabi-strings app.bin
The quick brown fox jumps over the lazy dog.

Only option X keeps the strings in because the panic_fmt implementation is
"using" them. In options Y and Z both the string literal and the location
information are optimized away.

@japaric
Copy link
Member Author

japaric commented Jul 20, 2017

@whitequark

A proper solution here would be rustc saving location information elsewhere
and only storing an index into that table in the executable itself. But that
would be a completely different RFC (that I'm very interested in).

That sounds a bit like what stlog, a Symbol Table logger, does. The executable
keeps only keeps indices to "strings" (they are just symbol names), and the host
can map those indices back to strings. This effectively lets you log a string of
any size by just logging one byte, which is the size of the index in the current
implementation.

It seems that such functionality could be added in a backward compatible way,
assuming this RFC is accepted as it is. I'm thinking of something like this:

panic!(42);

// expands into
{
    // same as today
    static _FILE_LINE_COL: (&str, u32, u32) = (file!(), line!(), col!());
    // new
    #[link_section = ".panic.location"] // not allocated in the executable
    static _PANIC_LOCATION: (&str, u32, u32) = (file!(), line!(), col!());

    core::panicking::panic(&42, &_FILE_LINE_COL, &_PANIC_LOCATION);
}

// and PanicInfo gains a new method
impl PanicInfo {
    fn panic_loc(&self) -> usize {
        // the address of the _PANIC_LOCATION variable from before
        &self.panic_loc as *const _ as usize
    }
}

// usage would look like this
#[panic_implementation]
fn panic(pi: &PanicInfo) -> ! {
    let panic_loc: usize = pi.panic_loc();

    // (.. report the `panic_loc` address somehow ..)

    // then abort or diverge in some other way
}

Then a tool can map the address back into the location by looking into the
.panic.location section of the executable. This doesn't require changing
rustc, just changing PanicInfo and core::panic!. However, it does require
messing with linker scripts to make sure .panic.location is not ALLOC / LOAD
and it's not gc-ed by the linker.

EDIT: panic_loc can return addr / mem::size_of::<(&str, u32, u32)>() instead of just addr to make it a proper index (0, 1, 2, etc.)

EDIT 2: Actually, it make require some rustc help to make sure the strings that file!() returns are not allocated in the executable but in some .panic.strings section.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Jul 20, 2017

I need to read this more thoroughly later, but right now, I hate to say I want to wait longer for stabilization.

The issue is panicking, allocating, shims around main, logging, and other things I am forgetting are special cases of the cyclic modules/crates. The MLs have had a proper solution for this for decades, and anything less than that in Rust will be a severe disappointment to me.

I don't want to hold the no-std ecosystem hostage to this, but I think that the fact that a lot of these have a somewhat low-level feel is no coincidence. The need for module-level cycles vs tying the knot with traits correlates with the need for statics/singletons/ambient authority. These things are all major warts warts that can and should be avoided in higher-level code, but are often unavoidable freestanding.

CC @nikomatsakis


OTOH, when #1974 was merged, it was acknowledged that the Alloc trait can probably be stabilized long before the mechanism to define the global allocators. I am fine with this RFC as an incremental improvement over the status quo if we separate the improvements from a declaration that stable is imminent.

@whitequark
Copy link
Member

Do you propose to print the error code to UART and then have a txt file that is generated during build with full error messages?

I propose no particular mechanism. For example, it would work if one of two PanicInfo structs would be used: the first would, when Displayed, print file, line, etc, and the second, when Displayed, one u32 containing the index into extermal location information.

All you need is to give a section name to the globals generated by rustc_trans for out-of-bounds and overflow checks and, syntactically, to those created by format_args!, isn't it?
If you have a section name you can just drop it, right? I guess you might want to keep some formatting but specifically not the ones from panics.

Yes, absolutely.

@pftbest
Copy link

pftbest commented Jul 21, 2017

@japaric strange, now I can't reproduce it either. Maybe i did have a wrong signature at some point. Either way it works now, so I have no objections.

@tarcieri
Copy link

Some copypasta from this thread: https://internals.rust-lang.org/t/stabilization-of-items-needed-for-no-std-applications/5236/8

What would be really nice here are some default strategies for #[panic_fmt] which could be easily stabilized.

You could imagine something like:

#[lang_default(panic_strategy, loop)]

...which goes into an infinite loop. Or if you got the abort intrinsic working:

#[lang_default(panic_strategy, abort)]

One of the no_std environments I work in has a minimalist pseudo-libc that supports a few things like printf(), abort(), etc (although little else). Perhaps you could even have:

#[lang_default(panic_strategy, print)]

@whitequark
Copy link
Member

One of the no_std environments I work in has a minimalist pseudo-libc that supports a few things like printf(), abort(), etc (although little else). Perhaps you could even have:

#[lang_default(panic_strategy, print)]

I really dislike the idea of adding intrinsic support of libc into core Rust.

@japaric
Copy link
Member Author

japaric commented Jul 21, 2017

@tarcieri With this proposal those "default strategies" can be just crates on crates.io. See below

Application:

// src/main.rs
extern crate panic_loop; // instead of #[lang_default(panic_strategy, loop)]

// ..

panic-loop v1.0.0:

// src/lib.rs
// ..

#[panic_implementation]
fn panic(_: &PanicInfo) -> ! {
    loop {}
}

No need to bake them in the language.

@mark-i-m
Copy link
Member

Perhaps I'm missing something critical, but what exactly is the purpose of payloads? Can you give an example use case or a motivation for why they were included in this rfc?

@therealprof
Copy link

@mark-i-m There're plenty of cases (especially in the embedded world) where you might want to panic but don't have the ability to render the cause for the panic in a human readable text, e.g. because the device does not a display and/or serial output. What you might have though is the ability to blink a LED or write a byte to flash and the custom payload looks like a reasonable and lightweight way to convey this information to a panic handler.

@mark-i-m
Copy link
Member

@therealprof Hmm... I see... That does sound like a useful feature, but I kind of would like to see that get its own RFC, rather than being tacked onto this one...

@mark-i-m
Copy link
Member

I say that because, I can understand why it would be convenient, but I also think it can be done for with just the plain panicking mechanism proposed; you can put whatever you want into the panic implementation...

@sfackler
Copy link
Member

@mark-i-m I'm not sure I understand - the payload is what allows the libstd implementation of the panic handler to use panic hooks (the default one of which prints the standard thread 'foo' panicked message).

@mark-i-m
Copy link
Member

Actually, the confusion was stemming from my ignorance that payloads already exist in libstd; it seemed like a random extra feature being lumped.

So now my question is why does the RFC say this:

When using core::panic! with formatting, e.g. panic!("{}", 42), the payload will be uninspectable: it won't be downcastable to any known type

Is it a side effect of how Any works or something?

@durka
Copy link
Contributor

durka commented Jul 25, 2017

@mark-i-m from the beginning of the RFC:

panic! in no-std environments must continue to be free of memory allocations and its API can only be changed in a backward compatible way.

Therefore core::panic!("{}", 42) can't create a String payload because it can't allocate.

@eddyb
Copy link
Member

eddyb commented Jul 25, 2017

Right now I think that goes through a formatting-specific API that libstd implements and does become a String, e.g. if you catch the payload Any, even if the panicking code is in libcore.
Wouldn't that be a backwards-incompatible change?

@joshtriplett
Copy link
Member

This looks great, and I'm excited to see no_std becoming more usable in stable.

@jethrogb
Copy link
Contributor

jethrogb commented Aug 2, 2017

@eddyb I think you're pointing out two separate issues.

Isn't that still a breaking change? The payload from panic_fmt is a String.

I'd think that by explicitly returning Any we have not committed to anything in particular here? PanicInfo docs for reference

fmt::Arguments has references to the values to print, you can pass it down the stack but not return/unwind with it! Payloads travel up all the way to a thread boundary or some other catch_panic.

This sounds like it might be a problem with the proposed PanicInfo::message as well then? Especially with regards to the unresolved question "Unwinding in no-std"

@eddyb
Copy link
Member

eddyb commented Aug 2, 2017

We're returning Any so you can access that String or your own custom type. i.e. panic!("...", ...) is equivalent to panic!(format!("...", ...)) and instead of the string you can put in any value.
Even if the static types won't change, uses in the wild will get broken.

@jethrogb
Copy link
Contributor

jethrogb commented Aug 2, 2017

I suppose we can't make it such that when you try to downcast a payload of fmt::Arguments into a String, it just works?

edit: yeah I'm not sure what amount of magic would be able to turn a &fmt::Arguments into a &String

@eddyb
Copy link
Member

eddyb commented Aug 2, 2017

Well, the payload won't be fmt::Arguments so I'm not even sure that's a problem. Formatting is the only case to handle in libcore, only libstd allows panicking with a payload AFAIK, so only pass fmt::Arguments ddown the stack and leave the payload to code which can allocate a Box<Any> and unwind.
The existing panic_fmt "weak lang item" is already pretty well thought out. cc @alexcrichton

@alexcrichton
Copy link
Member

The Any trait requires 'static and fmt::Arguments is not 'static. We can't ever return fmt::Arguments through the payload for this reason.

@joshtriplett
Copy link
Member

@japaric I'm very interested in seeing this. Could you please address the concern @eddyb raised, regarding fmt::Arguments? I think it makes sense to separate "printing" from "aborting", here, with the former happening before any unwind (if any).

@alexcrichton alexcrichton added T-libs-api Relevant to the library API team, which will review and decide on the RFC. and removed T-lang Relevant to the language team, which will review and decide on the RFC. labels Aug 22, 2017
@alexcrichton
Copy link
Member

I've switched this over to T-libs and this has been pretty quiet for awhile so I'd like to propose that we merge this. I think this'll be a great step towards helping out the #![no_std] ecosystem! Note that we'd of course always appreciate input from @rust-lang/lang as well!

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 22, 2017

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Aug 22, 2017
@Ericson2314
Copy link
Contributor

Ericson2314 commented Aug 22, 2017

Back in #2070 (comment) I was weary about the immanence of stabilization implied throughout, but I can tackle that in the tracking issue---this plan is certainly better than the status quo, good work @japaric.


The std vs core inconsistency is somewhat worrisome to me, in that std::panic! is a depending on tons of library code, but that was wart in the previous dynamic panic handler RFC. I'd be interested in deprecating the current payload method and making a new one which with and without std downcasts to FormatArgs for panic!("...{}...", ...). The old one can then wrap the new one, and attempt the FormatArgs downcast, converting to String on the fly. Then std::panic can be the same as core::panic! (The old one would be in a one-off perma-unstable trait in std.)

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Sep 1, 2017
@rfcbot
Copy link
Collaborator

rfcbot commented Sep 1, 2017

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

@rfcbot rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Sep 1, 2017
@rfcbot
Copy link
Collaborator

rfcbot commented Sep 11, 2017

The final comment period is now complete.

@aturon
Copy link
Member

aturon commented Sep 11, 2017

This RFC has been merged! Tracking issue.

@aturon aturon merged commit ad6cea5 into rust-lang:master Sep 11, 2017
@Centril Centril added A-panic Panics related proposals & ideas A-attributes Proposals relating to attributes A-no_std Proposals relating to #[no_std]. labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Proposals relating to attributes A-no_std Proposals relating to #[no_std]. A-panic Panics related proposals & ideas final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.