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

RFC: Introduce panic_thin, a fmtless alternative to panic_fmt #2305

Closed
wants to merge 2 commits into from

Conversation

gamozolabs
Copy link

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

@Mark-Simulacrum
Copy link
Member

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

@gamozolabs
Copy link
Author

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.
Copy link
Member

Choose a reason for hiding this comment

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

s/usa/use/ ?

@Zoxc
Copy link

Zoxc commented Jan 25, 2018

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.

@SimonSapin
Copy link
Contributor

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?

@Zoxc
Copy link

Zoxc commented Jan 25, 2018

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

@gamozolabs
Copy link
Author

@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
Copy link

Zoxc commented Jan 29, 2018

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

@aturon
Copy link
Member

aturon commented Feb 7, 2018

cc @alexcrichton @sfackler

@sfackler
Copy link
Member

sfackler commented Feb 7, 2018

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

@alexcrichton
Copy link
Member

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?

@nagisa
Copy link
Member

nagisa commented Feb 21, 2018

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 Centril added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Feb 23, 2018
@SimonSapin
Copy link
Contributor

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

@SimonSapin
Copy link
Contributor

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

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 1, 2018

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.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. labels Aug 1, 2018
@gamozolabs
Copy link
Author

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

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Aug 8, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Aug 8, 2018

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

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 18, 2018

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 rfcbot added finished-final-comment-period The final comment period is finished for this RFC. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Aug 18, 2018
@rfcbot rfcbot added postponed RFCs that have been postponed and may be revisited at a later time. and removed disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. labels Aug 18, 2018
@rfcbot rfcbot closed this Aug 18, 2018
@comex comex mentioned this pull request Jun 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
finished-final-comment-period The final comment period is finished for this RFC. postponed RFCs that have been postponed and may be revisited at a later time. 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.