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

Tracking issue for handle_alloc_error defaulting to panic (for no_std + liballoc) #66741

Closed
3 tasks
SimonSapin opened this issue Nov 25, 2019 · 90 comments · Fixed by #102318
Closed
3 tasks

Tracking issue for handle_alloc_error defaulting to panic (for no_std + liballoc) #66741

SimonSapin opened this issue Nov 25, 2019 · 90 comments · Fixed by #102318
Labels
A-allocators Area: Custom and system allocators B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-tracking-impl-incomplete Status: The implementation is incomplete. T-lang Relevant to the language 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. WG-embedded Working group: Embedded systems

Comments

@SimonSapin
Copy link
Contributor

SimonSapin commented Nov 25, 2019

The proposal below was implemented in #76448, feature-gated under #![feature(default_alloc_error_handler)].

Issues to resolve before stabilization:

Initial proposal:


Summary

This issue is for getting consensus on a change initially proposed in the tracking issue for #[alloc_error_handler]: #51540 (comment)

When no #[alloc_error_handler] is defined (which implies that std is not linked, since it literally has such a handler), alloc::alloc::handle_alloc_error should default to calling core::panic! with a message identical to the one that std prints to stderr before aborting in that case.

Although #51540 (comment) suggested that a full RFC would not be necessary, this is loosely structured after the RFC template.

Background

See the Background section of the sibling issue proposing stabilization of the attribute.

Motivation

As of Rust 1.36, specifying an allocation error handler is the only requirement for using the alloc crate in no_std environments (i.e. without the std crate being also linked in the program) that cannot be fulfilled by users on the Stable release channel.

Removing this requirement by having a default behavior would allow:

  • no_std + liballoc applications to start running on the Stable channel
  • no_std applications that run on Stable to start using liballoc

Guide-level explanation

When std is linked in an application, alloc::alloc::handle_alloc_error defaults to printing an error message to stderr and aborting the process.

When std is not linked and no other #[alloc_error_handler] is defined, handle_alloc_error defaults to panicking as if the following handler were defined:

#[alloc_error_handler]
fn default_handler(layout: core::alloc::Layout) -> ! {
    panic!("memory allocation of {} bytes failed", layout.size())
}

Reference-level explanation

The implementation for this would be very similar to that of #[global_allocator]. (Links in the next two paragraphs go to that implementation.)

alloc::alloc::handle_alloc_error is modified to call an extern "Rust" { fn … } declaration.

The definition of this function does not exist in Rust source code. Instead, it is synthesized by the compiler for “top-level” compilations (executables, cdylibs, etc.) when alloc is in the crate dependency graph. If an #[alloc_error_handler] is defined, the synthesized function calls it. If not, the synthesized function calls alloc::alloc::default_error_handler which is a new lang item. (Or is it?)

In order to allow experimentation for this new default behavior, it should initially be gated behind the #![feature(default_alloc_error_handler)] feature flag. When no handler is defined, a call to the default is (at first) only synthesized if any of the crates in the dependency graph has that feature gate. If none of them do, the current compilation error continues to be emitted.

Alternatives

The status quo is that no_std + alloc requires Nightly.

Stabilizing #[alloc_error_handler] or some other mechanism for specifying this handler is another way to unlock the no_std + liballoc on Stable use case. This removes the initial motivation for coming up with this default behavior. However perhaps this default is still desirable? In a no_std environment where there is no process to abort, the allocation error handler will likely be very similar to the panic handler (which is already mandatory).

@SimonSapin SimonSapin added A-allocators Area: Custom and system allocators C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language 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. labels Nov 25, 2019
@SimonSapin
Copy link
Contributor Author

Proposing FCP for deciding to adopt this approach:

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Nov 25, 2019

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

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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 Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Nov 25, 2019
@SimonSapin
Copy link
Contributor Author

Stabilizing #[alloc_error_handler] or some other mechanism for specifying this handler is another way to unlock the no_std + liballoc on Stable use case. This removes the initial motivation for coming up with this default behavior. However perhaps this default is still desirable? In a no_std environment where there is no process to abort, the allocation error handler will likely be very similar to the panic handler (which is already mandatory).

Should we still adopt this default behavior if the #[alloc_error_handler] attribute is to be stabilized soon?

@rfcbot concern what if stable attribute

@jplatte
Copy link
Contributor

jplatte commented Jan 28, 2020

What's the way forward here? Personally I would answer this simply with "yes":

Should we still adopt this default behavior if the #[alloc_error_handler] attribute is to be stabilized soon?

@SimonSapin
Copy link
Contributor Author

I’m ok with that. I’ll make this not a blocking concern for now, but anyone feel free to discuss some more.

@rfcbot resolve what if stable attribute

What's the way forward here?

This proposal still needs at least 6 out of the 8 members of @rust-lang/lang and @rust-lang/libs who haven’t yet to approve it in #66741 (comment)

@alexcrichton
Copy link
Member

I personally do not have a mode of checking off my box here which aligns with what I feel. I do not think this is a good change to make and I personally lament the current state of the alloc crate where I do not believe it should have been stabilized in the first place. I would like to register a blocking objection for this but I do not have the time or the energy to do so. On one hand I would like to not be part of the critical path here so it can proceed without me, but as a member of the libs team I'm not sure that's possible.

Overall I feel that alloc has next-to-no leadership and is simply a result of "let's just ship what's there without thinking about it". This has led to very little documentation about how to use it effectively and fundamentally no actual way to use it ergonomically and effectively.

I don't feel strongly enough about this though to pursue a blocking objection, nor am I really that interested in trying to debate the finer points here. The alloc crate will haunt me no matter what whether I check off my box here or even if we decide to not go with this. Overall I feel stuck and don't know what to do. The best I can say is that I know rfcbot doesn't require full sign-off for entering FCP, so I'm going to not check my box off and assume that when enough of others have checked their box off it will proceed without me.

@Lokathor
Copy link
Contributor

Cheer up Alex! It's not all that bad. While I would agree that there's some obvious bad parts to the alloc crate, I think that this particular change is extremely unlikely to cause any backwards compatibility concerns later on.

@SimonSapin
Copy link
Contributor Author

@alexcrichton Thanks for writing up your thoughts on this. I hear you on these concerns. What do you think of taking some time at the next Rust All Hands for @rust-lang/libs to discuss the crate organization of the standard library and such high-level design?

@Ericson2314
Copy link
Contributor

Ericson2314 commented Jan 29, 2020

@alexcrichton that was my feeling for many years, but recently I've been pleased and impressed with the work on the alloc-wg (which, to be clear, I've only been a belated and minor contributor to, not trying to complement myself here!)

It looks like this RFC wasn't really done in consultation to that working group? Maybe the libs team could kick this over to them, and whatever their decision they could contextualize it in a more thorough long-term design you are interested in.


My personal opinion is that I don't like this RFC either, but if the allocator parameter stuff the working group has prototyped is merged it will matter a lot less as all no_std code can (and should) return alloc errors explicitly with Result giving the caller maximum flexibility to locally handle the error or punt and let the global handler deal with it. In other words, no_std code shouldn't be using this handler at all so I don't care so much how it works.

@SimonSapin
Copy link
Contributor Author

My understanding of https://github.com/rust-lang/wg-allocators is that it is not about everything allocation-related, but specifically about making it possible to use a non-global allocator with standard library containers. So the behavior of handle_alloc_error is not in scope for that working group.

giving the caller maximum flexibility to locally handle the error or punt and let the global handler deal with it […] no_std code shouldn't be using this handler at all

Box::new and many other existing APIs do call handle_alloc_error on errors and will keep doing so, including on no_std

@Ericson2314
Copy link
Contributor

Ericson2314 commented Jan 29, 2020

My understanding of

That sounds right to me, but if you and/or the rest of the libs team wants to change the scope of the working group, they can. If @alexcrichton feels stretched thin, maybe that's something he'd want to pursue.

Box::new and many other existing APIs do call handle_alloc_error on errors and will keep doing so, including on no_std

That is right and I cannot change it. It is my opinion one ought to use Box::try_new_in instead. But it's just my opinion, and that hasn't landed yet, and so it remains to be seen whether or not core ecosystem crates will make the switch.

@Lokathor
Copy link
Contributor

Even if you always used Box::try_new and Vec::reserve and all that, you'd still need an allocation error handler defined to link alloc into a no_std binary. So even if we encourage people to never call the allocation handler, we would need either this issue or the attribute issue to land for no_std binaries in Stable. Between the two options, I would urge the teams to accept this proposal because it is the minimal amount to stabilize while also allowing Stable no_std + alloc binaries.

@withoutboats
Copy link
Contributor

My understanding of https://github.com/rust-lang/wg-allocators is that it is not about everything allocation-related, but specifically about making it possible to use a non-global allocator with standard library containers. So the behavior of handle_alloc_error is not in scope for that working group.

Agreed. IMO the alloc crate issues have nothing to do with wg-allocators, but instead to do with our story around no-std and support for diverse platforms that don't support all of std. It would be bizarre to link this issue to wg-allocators.

@programmerjake

This comment has been minimized.

@Lokathor
Copy link
Contributor

Lokathor commented Mar 2, 2020

The allocator is never supposed to call handle_alloc_error. it is intended entirely for code that tried an allocation and it failed and the code does not want to deal with the failure so it immediately ends the thread.

@programmerjake

This comment has been minimized.

@joshtriplett
Copy link
Member

Checking Centril's box as he's taking a break from the project.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Apr 20, 2020
@rfcbot
Copy link

rfcbot commented Apr 20, 2020

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

@pellico

This comment was marked as off-topic.

@Amanieu
Copy link
Member

Amanieu commented Sep 26, 2022

I read through the thread and it seems the only concern was related to possible UB while unwinding, but this has already been resolved by #88098.

I opened a stabilization PR: #102318

bors added a commit to rust-lang-ci/rust that referenced this issue Dec 16, 2022
… r=oli-obk

Stabilize default_alloc_error_handler

Tracking issue: rust-lang#66741

This turns `feature(default_alloc_error_handler)` on by default, which causes the compiler to automatically generate a default OOM handler which panics if `#[alloc_error_handler]` is not provided.

The FCP completed over 2 years ago but the stabilization was blocked due to an issue with unwinding. This was fixed by rust-lang#88098 so stabilization can be unblocked.

Closes rust-lang#66741
@bors bors closed this as completed in f5e0b76 Dec 17, 2022
@RalfJung
Copy link
Member

RalfJung commented Dec 21, 2022

Isn't it a bit odd that the default alloc error handler with std aborts, but without std it panics and unwinds? Why does __rdl_oom not trigger an immediate-abort panic (similar to what happens when we unwind out of a nounwind function) and then that could be shared between std and no-std situations?

@SimonSapin
Copy link
Contributor Author

What’s an immediate-abort panic? Is it possible when "normal" panic unwinds? (As to why: I’d guess that distinction wasn’t a thing a decade ago and this wasn’t revisited since.)

@RalfJung
Copy link
Member

RalfJung commented Dec 21, 2022

It's a (fairly) recent thing. The panic info has a can_unwind field now which, if false, causes the panic machinery to always abort, never unwind. The entry point for that is panic_str_nounwind in core/src/panicking.rs.

@Lokathor
Copy link
Contributor

Most no_std environments will set panic=abort anyway in the Cargo.toml, and anyone who doesn't might want the ability to catch the unwind and possibly do something to handle it.

It's probably the std handler that should be moved away from aborting, not the other way around.

@bjorn3
Copy link
Member

bjorn3 commented Dec 21, 2022

There is already a -Zoom=panic flag. It isn't respected by this code however to switch between panic and panic_nounwind. Only libstd has code to determine which one to use.

@RalfJung
Copy link
Member

It's probably the std handler that should be moved away from aborting, not the other way around.

Then that should happen in one fell swoop, not via a strange piecemeal stabilization.

Right now there's an incentive for people to go no_std on std-supporting targets just to get the panic no-abort OOM behavior, which is bad IMO.

@RalfJung
Copy link
Member

I opened a PR to adjust this: #106045.

compiler-errors added a commit to compiler-errors/rust that referenced this issue Jan 4, 2023
…nieu

default OOM handler: use non-unwinding panic, to match std handler

The OOM handler in std will by default abort. This adjusts the default in liballoc to do the same, using the `can_unwind` flag on the panic info to indicate a non-unwinding panic.

In practice this probably makes little difference since the liballoc default will only come into play in no-std situations where people write a custom panic handler, which most likely will not implement unwinding. But still, this seems more consistent.

Cc `@rust-lang/wg-allocators,` rust-lang#66741
Aaron1011 pushed a commit to Aaron1011/rust that referenced this issue Jan 6, 2023
… r=oli-obk

Stabilize default_alloc_error_handler

Tracking issue: rust-lang#66741

This turns `feature(default_alloc_error_handler)` on by default, which causes the compiler to automatically generate a default OOM handler which panics if `#[alloc_error_handler]` is not provided.

The FCP completed over 2 years ago but the stabilization was blocked due to an issue with unwinding. This was fixed by rust-lang#88098 so stabilization can be unblocked.

Closes rust-lang#66741
thomcc pushed a commit to tcdi/postgrestd that referenced this issue May 31, 2023
default OOM handler: use non-unwinding panic, to match std handler

The OOM handler in std will by default abort. This adjusts the default in liballoc to do the same, using the `can_unwind` flag on the panic info to indicate a non-unwinding panic.

In practice this probably makes little difference since the liballoc default will only come into play in no-std situations where people write a custom panic handler, which most likely will not implement unwinding. But still, this seems more consistent.

Cc `@rust-lang/wg-allocators,` rust-lang/rust#66741
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-allocators Area: Custom and system allocators B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-tracking-impl-incomplete Status: The implementation is incomplete. T-lang Relevant to the language 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. WG-embedded Working group: Embedded systems
Projects
None yet
Development

Successfully merging a pull request may close this issue.