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

Open
3 tasks
SimonSapin opened this issue Nov 25, 2019 · 81 comments
Open
3 tasks
Labels
A-allocators B-unstable C-tracking-issue disposition-merge finished-final-comment-period T-lang T-libs-api WG-embedded

Comments

@SimonSapin
Copy link
Contributor

@SimonSapin 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:

  • Document the new behavior
  • Get some usage experience beyond a synthetic test case #66741 (comment)
  • The current implementation might have UB #76448 (comment) #66741 (comment)

    Removing that unwind attribute needs to be benchmarked as it could lead to many extra unwind edges

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 C-feature-request T-lang T-libs-api labels Nov 25, 2019
@SimonSapin
Copy link
Contributor Author

@SimonSapin SimonSapin commented Nov 25, 2019

Proposing FCP for deciding to adopt this approach:

@rfcbot fcp merge

@rfcbot
Copy link

@rfcbot 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 disposition-merge labels Nov 25, 2019
@SimonSapin
Copy link
Contributor Author

@SimonSapin SimonSapin commented Nov 25, 2019

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

@SimonSapin SimonSapin commented Jan 28, 2020

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

@alexcrichton alexcrichton commented Jan 29, 2020

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

@Lokathor Lokathor commented Jan 29, 2020

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

@SimonSapin SimonSapin commented Jan 29, 2020

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

@SimonSapin SimonSapin commented Jan 29, 2020

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

@Lokathor Lokathor commented Jan 29, 2020

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

@withoutboats withoutboats commented Jan 31, 2020

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

@joshtriplett joshtriplett commented Apr 20, 2020

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

@rfcbot rfcbot added final-comment-period and removed proposed-final-comment-period labels Apr 20, 2020
@rfcbot
Copy link

@rfcbot rfcbot commented Apr 20, 2020

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

@Lokathor
Copy link
Contributor

@Lokathor Lokathor commented Jan 6, 2021

you can make a no_std bin on stable, though it's usually tricky to get right, and also I think it was never put in the release notes that you can do it.

@SimonSapin
Copy link
Contributor Author

@SimonSapin SimonSapin commented Jan 6, 2021

I believe it can’t, that’s what I meant by the unwind crate is #[unstable].

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Jan 6, 2021

I believe it can’t, that’s what I meant by the unwind crate is #[unstable].

-C panic=unwind works on stable though, so I am not sure if the stability of the crate is that relevant?

@SimonSapin
Copy link
Contributor Author

@SimonSapin SimonSapin commented Jan 6, 2021

As far as I understand -C panic=unwind v.s. -C panic=abort is represented in compiler code by the PanicStrategy enum and controls:

  • Whether code generation includes landing pads for unwinding
  • Which of the unwind or abort crates gets linked, but only if std is also linked. (Technically, if a crate with the perma-unstable needs_panic_runtime attribute is linked.) This is implemented in compiler/rustc_metadata/src/creader.rs. These crates define symbols like __rust_start_panic but don’t do anything unless called.

Compiling a crate with crate-type of bin, dylib, cdylib, or staticlib requires #[panic_handler] to be defined, either in that crate or in a recursive dependency. std defines one which eventually calls __rust_start_panic. If std is not linked it’s the program’s responsibility to define a #[panic_handler] that somehow never returns.

@Lokathor
Copy link
Contributor

@Lokathor Lokathor commented Jan 6, 2021

so can't the custom panic handler potentially unwind the stack?

@SimonSapin
Copy link
Contributor Author

@SimonSapin SimonSapin commented Jan 6, 2021

On Stable it would get no help from the standard library to do it.

I don’t know what it takes re-implement unwinding. Probably a fair amount of platform-specific code, but that aside I don’t know if it’s possible without relying and unstable language features.

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Jan 6, 2021

If std is not linked it’s the program’s responsibility to define a #[panic_handler] that somehow never returns.

Oh right, I forgot about that.

So... the safe thing to do would be to document that if #[panic_handler] unwinds, then #[handle_alloc_error] must be changed to not panic. The latter is impossible on stable; for the former we are not sure. But even if the former is not possible on stable this feels like something worth documenting.

@Amanieu
Copy link
Member

@Amanieu Amanieu commented Jan 7, 2021

It is currently not possible to support unwinding in no_std on stable because you need to define the eh_personality lang item and use intrinsics::try, both of which are unstable.

However I would like this to be supported eventually, so we should design handle_alloc_error with the goal of eventually supporting unwinding from alloc error handlers.

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Jan 7, 2021

However I would like this to be supported eventually

I agree.

so we should design handle_alloc_error with the goal of eventually supporting unwinding from alloc error handlers.

I don't think that is a direct consequence; there is also the alternative of requiring the alloc error handler to not unwind or wrapping it in an abort-on-unwind shim.

If allocation truly can unwind, we'll have to get rid of the Box nullary operator and also carefully review all the nounwind LLVM annotations for allocation functions.

@programmerjake
Copy link
Member

@programmerjake programmerjake commented Jan 8, 2021

I think wrapping the panic in abort-on-unwind code is probably the best option, since I'm pretty sure I've written code that assumes allocation doesn't unwind for safety, and I'd guess I'm far from the only one.

@haraldh
Copy link
Contributor

@haraldh haraldh commented Jan 18, 2021

May I unassign myself? I can't help much with the remaining issues, because I lack the time and knowledge.

@KodrAus KodrAus changed the title Tracking isuse for handle_alloc_error defaulting to panic (for no_std + liballoc) Tracking issue for handle_alloc_error defaulting to panic (for no_std + liballoc) Jan 21, 2021
@KodrAus
Copy link
Contributor

@KodrAus KodrAus commented Jan 22, 2021

@haraldh! No problem ❤️ Thanks for helping take it all this far.

So is our current decision point:

  • Block stabilization on fixing the possible UB from unwinding in our allocation error handler so that when it is possible to unwind on stable in no-std we're all good by:
    • Requiring that #[alloc_error_handler] not panic, unless #[panic_handler] doesn't unwind
    • Rethink our assumptions made in the language and allocator infra so that unwinding becomes expected
  • Don't block stabilization on the unwinding issue since it's only possible on nightly anyways and sounds like it's not a common thing to do in no-std. Punt on whether unwinding should be supported.

The documented no unwinding requirement seems like a reasonable path to me, but this is out of my domain.

@Lokathor
Copy link
Contributor

@Lokathor Lokathor commented Jan 22, 2021

Right now, we don't have a solution, but what we'd be stabilizing wouldn't need a solution. Any solution we come up with would affect other cases which aren't being stabilized by this.

So probably the second option.

@KodrAus
Copy link
Contributor

@KodrAus KodrAus commented Jan 22, 2021

@Lokathor do you think there’s a single clear point where we’ll know that it’s becoming possible on stable to unwind while also being able to specify your own alloc error handler? If not I guess we run the risk of accidentally stabilizing with no decision having been made.

@Lokathor
Copy link
Contributor

@Lokathor Lokathor commented Jan 22, 2021

If you want to have unwinding you need to use the eh_personality language item, so as long as that's not stable then you can't unwind on stable. I'm not sure where to note that down though, i don't know of a tracker issue for that or anything.

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Jan 22, 2021

At the very least, the code that marks the allocator functions as nounwind should have its comments extended to say that this is correct even for the default alloc_error_handler, because [all the things we discussed above].

@nbdd0121
Copy link
Contributor

@nbdd0121 nbdd0121 commented Feb 1, 2021

Forgive me if I am missing something obvious, but couldn't we just abort if alloc error handler unwinds?

pub unsafe extern "C" fn __rdl_oom(size: usize, _align: usize) -> ! {
    struct AbortOnUnwind;
    impl Drop for AbortOnUnwind {
        fn drop(&mut self) {
            panic!();
        }
    }
    let guard = AbortOnUnwind;
    panic!("memory allocation of {} bytes failed", size)
}

@HeroicKatora
Copy link
Contributor

@HeroicKatora HeroicKatora commented Mar 2, 2021

Under caveat of potentially being a 'clever' solution and not a perfectly ergonomic one. What if the standard distribution were to provide a crate, similar to alloc, which contains the default implementation of the oom error handler? As a std-internal one it can be annotated with any internal item including nounwind and being marked as a lang item for oom handling. The mechanism for using it would be a familiar one:

extern crate alloc;
// not w.r.t. bikeshedding
extern crate alloc_handler_std;

And later, once the discussion on the stabilized form for writing those handlers has settled, users can customize it by replacing it with special oom-handling through another dependency. This method seems to have been accepted for panic handlers.

@SimonSapin
Copy link
Contributor Author

@SimonSapin SimonSapin commented Mar 2, 2021

It’s no the handler definition that is annotated with nounwind but handle_alloc_error which calls it. So having a separate crate for an opt-in panicking handler does not help with the unwinding question.

@Diggsey
Copy link
Contributor

@Diggsey Diggsey commented Mar 3, 2021

Why don't we remove the explicit nounwind attribute from handle_alloc_error, and just make the compiler smart enough not to generate unwind edges if it knows they're not necessary?

As a first pass, this could just be special-cased in the compiler. Special casing is not ideal, but the extra unwind edges are purely a performance consideration, so this special-casing does not fundamentally change behaviour.

@HeroicKatora
Copy link
Contributor

@HeroicKatora HeroicKatora commented Mar 3, 2021

It’s no the handler definition that is annotated with nounwind but handle_alloc_error which calls it. [..]

I didn't meant to suggest that it must, but rather that one could add additional attributes—i.e. including some affecting llvm codegen, or for mir—if that turns out to be necessary for optimization/inlining purposes. Doing it via an external crate does not stabilize any symbol and this allows customizing either part of the interface in separate issues. It's essentially wraping this 'feature(_)` in an established syntax, such that it hopefully addresses some concerns of accidental stabilization? But it was just a random thought and I might be wrong.


Also note that this does not only affect embedded but also blocks some no_std + alloc x86 work of mine.

@nbdd0121
Copy link
Contributor

@nbdd0121 nbdd0121 commented Sep 25, 2021

The current implementation might have UB

It wouldn't UB since abort_unwinding_calls MIR transform will ensure that unwinding through non-unwindable function will cause an abort.

@jarys
Copy link

@jarys jarys commented Nov 12, 2021

Hello guys,

I am a Trezor hardware wallet firmware developer and I would be glad, if you stabilized no_std + alloc.

We rewrite our code base into Rust and using alloc simplifies things. Currently, I use nightly, but we would prefer to switch back to the stable Rust in production version.

🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-allocators B-unstable C-tracking-issue disposition-merge finished-final-comment-period T-lang T-libs-api WG-embedded
Projects
None yet
Development

No branches or pull requests