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

Add feature to disable the panic_impl provided by std #100156

Closed
wants to merge 1 commit into from

Conversation

morr0ne
Copy link
Contributor

@morr0ne morr0ne commented Aug 4, 2022

Currently is only possible to mark a function with #[panic_handler] in no_std environments. This pr adds a feature to disable the panic_impl provided by std so that it can be defined in normal programs. Since the feature is not enabled by default it does not affect existing code.

Should fix #59222 and hopefully a small but useful step towards custom panic implementation: #32837

ACP: rust-lang/libs-team#80

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 4, 2022
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @thomcc (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 4, 2022
@thomcc
Copy link
Member

thomcc commented Aug 7, 2022

I'm not sure what to do about this. If you're using std, why not just use set_hook?

@morr0ne
Copy link
Contributor Author

morr0ne commented Aug 7, 2022

The std panic handler also does a bunch of other stuff beside calling the hook. I want to override all that.
Also allowing more freedom to implement lang-items without being forced to use no_std is always good. I can't think of any drawbacks to this feature. It will only add for those who need it.

@thomcc
Copy link
Member

thomcc commented Aug 8, 2022

Hmm, I think this counts as an unstable API then. I suppose you should write a ACP (see the bot post for links) for it?

I guess if I'm being honest, I am pretty sympathetic to this desire in principal (I agree the default panic handler does things someone might not want, although the programs where I've wanted this changed were all no_std ones, where it's easy). I suspect it needs some consideration, though.

We had a bunch of discussion about stdlib features at rustconf postconf too (CC @cuviper who I know was there, and @joshtriplett who... might have been at that session too?)

(I'm not sure if this is T-libs-api or T-libs, TBH, but since ACP is "API change proposal", I think I'll toggle it?)

@rustbot label +T-libs-api -T-libs

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 8, 2022
@thomcc thomcc added S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 8, 2022
@morr0ne
Copy link
Contributor Author

morr0ne commented Aug 8, 2022

Just to be safe I opened an ACP here rust-lang/libs-team#80, I'm confident there shouldn't be any drawbacks to such a simple change but I'd love more input from the lib's team. This is also my first time contributing to rust so please forgive me if I didn't properly understand the rustc-dev-guide.

@cuviper
Copy link
Member

cuviper commented Aug 8, 2022

Do we have any precedent for a cargo feature that removes functionality? The usual rule is that features should be purely additive, and I think it would be very strange for the standard library to break that rule.

We had a bunch of discussion about stdlib features at rustconf postconf too

Part of that discussion was about removing content from the minimal set, default-features = false. Even this is not usually advised for crates, because a user might already be depending on that content without default features, but we can possibly get away with this for the standard library since there's no stable way to control its features yet.

The idea was that core/alloc/std could go to the extreme that default-features = false has nothing, and then use features add stuff back in to build the default set. There might be an explicit "base" feature with very little, which could be split even further in the future. Perhaps a positive "panic_impl" feature would add the stuff you're trying to mask out here, so you would be able to leave that off in your builds.


For your case, maybe #[panic_handler] should behave more like #[global_allocator], where users can more-directly override the default. This would be more of a compiler change, rather than involving cargo features.

@emmatebibyte
Copy link

I would like to use this feature, I would love to see it implemented in any way seen fit.

@thomcc thomcc added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. labels May 29, 2023
@thomcc
Copy link
Member

thomcc commented May 29, 2023

Are we sure we want to do this as a negative feature? Usually that's a mistake. This is mentioned in the ACP (which seems to have been accepted) rust-lang/libs-team#80, but not discussed further.

@thomcc
Copy link
Member

thomcc commented May 29, 2023

I guess I'll nominate it but it should be a quick discussion.

@thomcc thomcc added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label May 29, 2023
@Amanieu
Copy link
Member

Amanieu commented Jun 1, 2023

We have a general rule against adding new features to the standard library that have no path to stabilization, and it isn't clear that this is worth the additional complexity in configuration options.

The ACP mentions several motivating use cases for this, but I don't think that this feature will actually help achieve these goals:

  • Bypass the default implementation as it might be desirable to avoid all the other stuff that std sets up

It's not clear what "other stuff" refers to here: the panic machinery in std is relatively straightforward and only does 3 things:

  1. Check for recursive panics and abort immediately if that is the case.
  2. Call the panic hook.
  3. Either unwind or abort depending on -C panic.

Everything else, including backtraces, is handled by the default panic hook which can be overridden.

  • Allow the use of rust programs in more constrained environments without having to resort to no_std

Again, the use case isn't clear. What are these constraints? Is it just binary size? We already have ways to disable unwinding (-C panic=abort) and backtrace support (backtrace feature on std) to get the same benefit.

  • Allow to assert at compile time that a program cannot panic with tricks like this, which currently only work in no_std environments.

This is unlikely to ever work with std since the standard library itself has many potential panic points, even in the initialization code before main.

@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jun 1, 2023
@morr0ne
Copy link
Contributor Author

morr0ne commented Jun 2, 2023

Do we have any precedent for a cargo feature that removes functionality? The usual rule is that features should be purely additive, and I think it would be very strange for the standard library to break that rule.

Looking back I do not think having a no_panic_impl option is the best approach. I still feel like this is a useful feature but having a feature that disables stuff does not seem right. A better way would be to either take the #[global_allocator] approach like mentioned above but that would require a compiler change. Another possible implementation idea I recently had was to make use of the already existing panic toggle in the profile:
You can currently change between the normal unwind impl and the abort one like this

[profile.release]
panic = "abort # or unwind

It might be a good idea to had a third toggle label "none" or something similar to remove impl.

[profile.release]
panic = "none"

This could also accept true or false as aliases for "unwind" and "none" similar to what strip does.
This feels much more in line with other rust features and could be useful even without having to resort to build-std. I am not sure If I should open another acp for this or if its fine to keep discussing this here.

It's not clear what "other stuff" refers to here: the panic machinery in std is relatively straightforward and only does 3 things:

Perhaps I could have used better wording. What I meant is that it might be desirable to have completely custom code in the panic_handler. This I feel is less of a case of why and more of why should std restrict this? It can be useful even if rarely and it has no impact on user not making use of the feature. I opened this issue back when I did because I need to override the whole panic_handler while still using std, unfortunately I cannot recall what I needed it for but I feel like having more choice is always better than not.

Again, the use case isn't clear. What are these constraints? Is it just binary size? We already have ways to disable unwinding (-C panic=abort) and backtrace support (backtrace feature on std) to get the same benefit.

The original use case was for an extremely tiny embedded Linux board. Using std would have been immensely useful since I was still running on top of Linux but squeezing every last byte out of the system was essential.

This is unlikely to ever work with std since the standard library itself has many potential panic points, even in the initialization code before main.

I am still trying to compile std myself to make sure it works but there is no reason it shouldn't. The way this kind of linking "hacks" work is by making sure there are no panic being linked. Even if std panics if its never called and removed by compiler optimizations it will compile just fine. Making sure that panics are not present is very important for certain applications, especially industry critical targets that can't afford to have panicking paths. This is far from elegant and I always felt like the language its really lacking in that department but this is still a solution nonetheless.

I feel like there is value in being able to make std more flexible and this feature would allow panicking to meet that goal.

@thomcc
Copy link
Member

thomcc commented Aug 28, 2023

This seems like it needs further design work so I am moving it to the author. The latest proposal feels like it probably needs an RFC.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2023
@bors
Copy link
Contributor

bors commented Dec 16, 2023

☔ The latest upstream changes (presumably #119002) made this pull request unmergeable. Please resolve the merge conflicts.

@thomcc
Copy link
Member

thomcc commented Feb 1, 2024

I'm going to be away for a few months, so I'm rerolling my PRs so that folks don't have to wait for me. Sorry/thanks.

r? libs

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Feb 1, 2024
@rustbot rustbot assigned Mark-Simulacrum and unassigned thomcc Feb 1, 2024
@Dylan-DPC
Copy link
Member

@morr0ne any updates on this?

@morr0ne
Copy link
Contributor Author

morr0ne commented Feb 3, 2024

@morr0ne any updates on this?

I think there is definitely value in adding a mechanism to disable the panic handler provided by std. How to do so in a way that feels “rusty” and has a proper stabilisation path is tricky. I am going to write and RFC that address the issues I mentioned in my previous comments.

@Dylan-DPC Dylan-DPC added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 4, 2024
@Dylan-DPC
Copy link
Member

This is blocked on writing an rfc, which i think hasn't been written and it will take a while to be accepted which will cause this pr to bitrot till then. So closing this and a new one can be created later once the rfc is accepted

@Dylan-DPC Dylan-DPC closed this Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#[panic_handler] does not work with std.
10 participants