RFC: Introduce panic_thin, a fmtless alternative to panic_fmt by gamozolabs · Pull Request #2305 · rust-lang/rfcs (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation18 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This RFC is meant to make it easier to create applications that do not use fmt allowing for smaller program sizes while still getting some level of panic information. This RFC proposes a solution that does not require changes to current Rust programming styles, nor require writing of new panic messages.

This was inspired by a lot of my work on a bootloader which is restricted to 32 KiB. Any time core::fmt gets introduced to the codebase the code almost always goes over the size limit. To get around this I have had to remove uses of core::fmt by not using the msg argument to panic_fmt and building my bootloader with fat LTO.

Recently there has been a lot of talk on this such as: rust-lang/rust#47409, rust-lang/rust#47526, rustwasm/team#19

Rendered

-B

Has there been discussion or effort made recently to see if we can reduce the size of core::fmt or perhaps provide a less efficient, but smaller, implementation? You say that "This seems like a lot of work for little reward. It's unlikely much could be reduced anyways" but I'd like to see some backup of such a claim -- to my knowledge, no such work has happened recently, and I feel like it's possible we could make code size improvements that are fairly major, especially if we statically restrict to formatting only strings and integer values (which is enough for probably 90% of potential panic messages in space-restricted environments, I'd imagine -- but I could be wrong, having never worked in such an environment).

I think if we are comfortable constraining format strings to strings and integers we probably could make a quite tiny fmt implementation. It could be further advanced by restricting these formats to have no padding or non-decimal representation. Eg. only "{}" allowed. I know a decimal-to-string can be done in 40-50 bytes of x86. But once you start adding padding options, hex formatting, etc this size starts dramatically increasing.

A good question is what is the variety in all of Rusts (libcore + libstd + friends) panics? It might be possible that internally we could not use anything but primitive formats and thus unless a user explicitly uses an advanced format they get a very tiny fmt overhead. Further if documented this would allow embedded users to drive their format strings to use whatever is most optimized for size.

This route probably is the best for everyone, however it depends on how much existing Rust code would have to be changed and might require a bit more work.


## Getting static strings from existing format strings

It is not reasonable to have multiple panic messages for every panic usa, thus it is important that there is backwards compatibility with old panics.

Choose a reason for hiding this comment

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

s/usa/use/ ?

We already have RFC 2070, which conflicts with this RFC.

I also want support for stripping all panic information, and neither RFC 2070 or this RFC offers that.

In RFC 2070 PanicInfo contains a fmt::Arguments so its constructor will be in the binary. But if you don’t actually use PanicInfo::message or Display for PanicInfo in your panic handler, wouldn’t the linker still be able to strip fmt’s formatting code?

@SimonSapin LLVM should be able to remove the code. If we had MIR-only rlibs, the code wouldn't even be generated in the first place.

@Zoxc what do you mean by stripping all panic information? Like having panic() just immediately crash the program with an invalid instruction or something? Or just remove all parameters to it?

I'm a bit concerned with relying on optimizations to fix this issue as there are always chances for slight regressions when changes are made to compiler internals. It just seems dangerously close to breaking at any moment. In fact, the changes in LTO is what caused this to be an issue for me in my project in the first place.

@Zoxc what do you mean by stripping all panic information? Like having panic() just immediately crash the program with an invalid instruction or something? Or just remove all parameters to it?

I want panics to be able to reduce to a single ud2 instruction on x86.

I'm a bit concerned with relying on optimizations to fix this issue as there are always chances for slight regressions when changes are made to compiler internals. It just seems dangerously close to breaking at any moment. In fact, the changes in LTO is what caused this to be an issue for me in my project in the first place.

I think this could be solved by adding a test to the compiler.

Does the standard library need to be recompiled to switch from panic_fmt to panic_thin?

Thanks for the RFC @gamozolabs!

I'd personally be sort of hesitant to add more panic infastructure before we understand what we already have, though. For example RFC 2070 I think would provide a way where, if unused, panic information would be stripped out during LTO. It doesn't contain the feature where you can extract a &'static str from a fmt::Arguments but I'd hope that it would get us pretty far along the way towards achieving this goal.

Perhaps we should hold off on RFCs like this until 2070 is implemented?

Sadly, the 2070 by itself does not provide means to strip out unnecessary panic information, but a fairly small extension to it would allow that.

Centril added the T-libs-api

Relevant to the library API team, which will review and decide on the RFC.

label

Feb 23, 2018

I commented at rust-lang/rust#44489 (comment) but should have done it here as well.

The stated goal of this RFC is to avoid the code size cost of using core::fmt. However I think this is already possible with the current design:

#[panic_implementation] fn panic(info: &core::panic::PanicInfo) -> ! { if let Some(s) = info.payload().downcast_ref::<&'static str>() { print(s) } abort_or_something() }

fn print(s: &str) {…} fn abort_or_something() -> {…}

Creating a PanicInfo involves in many cases calling core::fmt::Arguments::new_v1, but that’s a trivial constructor with #[inline]. As far as I understand, the fmt machinery that takes a lot of code size would not be included as long as impl Dislay for PanicInfo is not used.

(At the moment I think that this downcast to &str would return something with std::panic!("foo") but not core::panic!("foo"), that should be easy to fix.)

@gamozolabs How does this sound?

(Note that there is a push to stabilize rust-lang/rust#44489 soon~ish.)

This RFC exactly as proposed is somewhat in contradiction with #2070 / rust-lang/rust#44489 which is already implemented and about to be stabilized. However I believe that the goal of not including the core::fmt code can already be achieved per the previous comment. As to the goal of eliminating panic strings or location strings if they are unused, I don’t know exactly what LLVM’s LTO is capable of today but since #[panic_implementation] is an attribute implemented in the compiler, it can be extended in the future to accept multiple different signatures (and possibly generate a wrapper function if needed). So we’re not stuck with &PanicInfo forever. However it’s unlikely that we’ll have bandwidth to design and implement this soon.

So I’d like to propose to close this RFC as postponed:

@rfcbot fcp postpone

When rust-lang/rust#44489 is stabilized we can see how far full LTO gets us, and later consider supporting different signatures (including possibly a panic handler that takes no argument at all).

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

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

I'm fine with this, the current panic implementation has been great for my requirements.

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

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

By the power vested in me by Rust, I hereby postpone this RFC.

rfcbot added postponed