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

Stabilize the #[alloc_error_handler] attribute (for no_std + liballoc) #66740

Closed
SimonSapin opened this issue Nov 25, 2019 · 33 comments
Closed
Labels
A-allocators Area: Custom and system allocators B-unstable Blocker: Implemented in the nightly compiler and unstable. 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. 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

Summary

This issue formally proposes stabilizing the #[alloc_error_handler] attribute as-is, after adding some documentation.

Tracking issue: #51540

Normally the tracking issue is where we propose FCP to stabilize, but this one already has many comments that go into a number of sub-topics. Since the feature did not originally go through the RFC process, this proposal is loosely structured after the RFC template.

Background

Heap memory in libstd

Many parts of the standard library rely on a global heap memory allocator. For example Box::new takes a single parameter, the value to be boxed, and returns a struct that wraps a pointer to newly-allocated heap memory. The allocator is not part this API (on many platforms it defaults to malloc) and neither is the possibility that allocation fails: the return value always contains a valid pointer.

Allocation can in fact fail (malloc can return a null pointer), but in practice this is uncommon enough and hard enough to recover from that Box::new and many other APIs make the choice of not propagating that error to callers. We call these APIs “infallible” because allocation failure is not a concern of the caller (as opposed to “fallible” APIs like Vec::try_reserve which return a Result). “Infallible” APIs deal with failures by calling the handle_alloc_error(Layout) -> ! function, which never returns. The current behavior in libstd is to print an error message and abort the process. Any low-level code that makes allocations and wants to expose an infallible API is expected to call this function. For example a custom container library could look like:

use std::alloc::{Layout, alloc, handle_alloc_error};
use std::ptr::NonNull;

impl<T> MyBox<T> {
    pub fn new(x: T) -> Self {
        let layout = Layout::new::<T>();
        assert!(layout.size() > 0); // Not dealing with the zero-size case for example brevity
        let maybe_null = unsafe { alloc(layout) };
        let ptr = NonNull::new(maybe_null)
            .unwrap_or_else(|| handle_alloc_error(layout));
        Self(ptr.cast())
    }
}

no_std and liballoc

The Rust standard library is split into three crates (that are relevant to this issue): core, alloc, and std.

  • std expects much functionality to be provided by the underlying operating system or environment: a filesystem, threads, a network stack, … and relevant here: a memory allocator and a way to abort the current process. Large parts of its code are target-specific. Porting it to a new target can take non-trivial efforts.

  • core contains the subset of std that has almost no such requirement. A crate can use the #![no_std] attribute to opt into having its implicit dependency to std replaced by an implicit dependency to core. When all crates in an application do this, this enables porting to a target that might not have std at all. Notably, @rust-embedded does this with micro-controllers that do not have an operating system.

  • alloc is in-between. It depends on core and std depends on it. It contains the subset of std that relies on heap memory allocation, but makes no other external requirements over those of core. Specifically, using alloc requires:

    • A heap memory allocator, that provides an implementation of the alloc function and related functions.
    • An allocation error handler, that provides an implementation of the handle_alloc_error function.

    The std crate provides both of these, so linking it in an application (having any crate in the dependency graph that doesn’t have #![no_std], or has extern crate std;) is sufficient to use alloc. Of course this doesn’t work for targets/environments where std is not available.

#[panic_handler]

core does have an external requirement: a way to handle panics. std normally provides this by printing a message to stderr, optionally with a stack trace, and unwinding the thread. In a no_std application however there may not be an stderr to print to, and unwinding may not be supported. Such apps can therefore provide a handler:

#[panic_handler]
fn panic(panic_info: &core::panic::PanicInfo) -> ! {
    // …
}

(See also in the Nomicon.)

The attribute is effectively a procedural macro that checks the signature of the function and turns it into an extern "Rust" fn with a known symbol name, so that it can be called without going through Rust’s usual crate/module/path name resolution.

The compiler also checks for “top-level” compilations (executables, cdylibs, etc.) that there is exactly one panic handler in the whole crate dependency graph. std (effectively) provides one, so the attribute is both necessary for no_std applications and can only be used there.

#[global_allocator]

Depending on the workload, an alternative allocator may be more performant than the platform’s default. In earlier versions of Rust, the standard library used jemalloc. In order to leave that choice to users, Rust 1.28 stabilized the GlobalAlloc trait and #[global_allocator] attribute, and changed the standard library’s default to the system’s allocator.

This incidentally enabled (in Nightly) the use of alloc in no_std applications which can now provide an allocator implementation not just to be used instead of std’s default, but where std is not necessarily available at all. However such applications still require Rust Nightly in order to fulfil alloc’s second requirement: the allocation error handler.

#[global_allocator] is similar to #[panic_handler]: it also expands to extern "Rust" fn function definitions that can be called by a crate (this time alloc instead of core) that doesn’t have a Cargo-level dependency on the crate that contains the definition, and in that the compiler checks for “top-level” compilation that it isn’t used twice. (It differs in that it can be used when std is linked, and overrides std’s default.)

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.

Stabilizing #[alloc_error_handler] as the way to fulfil this requirement 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

Many of the APIs in the alloc crate that allocate memory are said to be “infallible”. Allocation appears to always succeed as far as their signatures are concerned. When allocation does fail, they call alloc::alloc::handle_alloc_error which never returns. For example, Vec::reserve is said to be infallible while Vec::try_reserve is fallible (and returns a Result). Other libraries who want to expose this infallible style of API may also call handle_alloc_error.

We call an application no_std if it doesn’t link the std crate. That is, if all crates in its dependency graph have the #![no_std] attribute and (after cfg-expansion) do not contain extern crate std;.

A no_std application may use the standard library’s alloc crate if and only if it specifies both a global allocator with the #[global_allocator] attribute, and an allocation error handler with the #[alloc_error_handler] attribute. Each may only be defined once in the crate dependency graph. They can be defined anywhere, not necessarily in the top-level crate. The handler defines what to do when handle_alloc_error is called. It must be a function with the signature as follows:

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

The handler is given the Layout of the allocation that failed, for diagnostics purpose. As it is called in cases that are considered not recoverable, it may not return. std achieves this by aborting the process. In a no_std environment − which might not have processes in the first place − panicking calls the #[panic_handler] which is also required to not return.

Reference-level explanation

#[alloc_error_handler] is very similar to #[panic_handler]: it locally checks that it used on a function with the appropriate signature and turns it into an extern "Rust" fn with a known symbol name, so that alloc::alloc::handle_alloc_error can call it.

Like with the panic handler, the compiler also checks for “top-level” compilations (executables, cdylibs, etc.) that there is exactly one allocation error handler in the whole crate dependency graph. std literally provides one, so the attribute is both necessary for no_std applications and can only be used there.

The above is already implemented, although not well documented. This issue is about deciding to stabilize the attribute. If we find consensus on this direction, documentation should come before or with a stabilization PR. The alloc crate’s doc-comment could be a good place for this documentation, which could be based on the guide-level explanation above.

#[panic_handler] is already stable, so the Rust project is already committed to maintaining this style of attribute.

Alternatives

@SimonSapin SimonSapin added A-allocators Area: Custom and system allocators B-unstable Blocker: Implemented in the nightly compiler and unstable. 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 to stabilize, is described above:

@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

SimonSapin commented Nov 25, 2019

Instead of stabilizing a way to fulfil the requirement to define a handler, another way to unlock the no_std + liballoc on Stable use case could be to remove that requirement: when no handler is defined, the compiler could inject a default handler that panics

If that default handler is accepted, should we not stabilize this attribute and wait for a more general "global things" mechanism?

@rfcbot fcp what if default

@SimonSapin
Copy link
Contributor Author

@rfcbot concern what if default

@withoutboats
Copy link
Contributor

withoutboats commented Nov 25, 2019

Just throwing this out there (and maybe it's already been proposed and rejected): if we wanted to go with the default handler approach, we could make it possible through the PanicInfo API to distinguish alloc failures from other panics, making it straight forward to handle this specially in your panic handler, unlike other panics. Two ways we could do this:

  • Rather than picking a default message, make the Layout the &dyn Any of the payload, or a newtype wrapping Layout. Downcasting the payload to this type assumes an alloc error. Possibly this interacts poorly with existing users' panic handlers.
  • Just directly provide a new field and method on PanicInfo which contains the layout in the case of alloc failures.

Probably this would mean there's little point in having alloc_error_handler.

I don't have any opinion about which of these is best. In my opinion we should just make core + alloc work on stable as soon as reasonable.

@SimonSapin
Copy link
Contributor Author

This is an interesting idea! (Though perhaps one for #66741. The perils of starting two related threads at the same time…)

One thing is that there’s currently no stable way in no_std to end up with PanicInfo::payload returning Some, so making the default allocation error handler do this would be slightly magic. Single-argument core::panic! requires &str and creates an fmt::Arguments, unlike single-argument std::panic! which takes a generic T: Any + Send and creates a payload.

We could extend single-argument core::panic! to also be generic and make a dyn Any payload. In order to not regress also setting a message for &str we’d need some kind of specialization, although it’s the kind that can be hacked together on Stable with auto-ref. Or maybe we can regress this, since PanicInfo::message is still unstable. (CC #66745)

@SimonSapin
Copy link
Contributor Author

@withoutboats Do you feel this idea is a reason not to stabilize the existing #[alloc_error_handler] as-is, as proposed by this thread?


If that default handler is accepted, should we not stabilize this attribute and wait for a more general "global things" mechanism?

No comment in this direction so far, so I’m gonna

@rfcbot resolve what if default


This pFCP seems close to consensus, ping @Centril @alexcrichton @joshtriplett @pnkfelix @scottmcm for checkboxes in #66740 (comment)

@alexcrichton
Copy link
Member

My feelings on this are the same as on the related issue. I disapprove of this stabilization and do not believe we should do so. I do not have the energy, time, or motivation to pursue a blocking objection though. As a result I believe enough others will need to check their boxes such that my approval is not required to proceed.

@withoutboats
Copy link
Contributor

withoutboats commented Jan 29, 2020

@withoutboats Do you feel this idea is a reason not to stabilize the existing #[alloc_error_handler] as-is, as proposed by this thread?

I find Alex's comment on the related issue pretty convincing - we should have a clear idea of what we're doing here, not just stabilize what exists. Personally the idea of handling all of this through the panic handler is appealing to me. I'd like if at the allhands we developed a more complete roadmap for the alloc crate which incorporates solving this issue (in whatever way) as its first step.

@joshtriplett
Copy link
Member

@withoutboats Those concerns seem reasonable to me, and worth considering. I do, in general, hope that the use cases currently served by alloc and core and no_std are in the future addressed by feature flags and custom compliations of std.

Do you believe that we should defer this pair of stabilizations in favor of a better solution?

@jamesmunns
Copy link
Member

I respect Alex's concerns, however having a stable allocator for no_std/embedded has been blocked for a pretty significant amount of time to date, and while it is not a fundamental blocker for all use cases, it makes certain things very difficult, as well as being a soft-blocker for prototyping/less resource constrained use cases.

I understand the need to get this right, rather than just done, and Alex noted a lack of ownership on this topic. If the stabilization proposal from Simon here and in #66741 is judged as unacceptable, I'd be happy to volunteer to help push this effort forward in the next iteration of discussions. I certainly don't think I have any immediate solutions, but I would be happy to shepherd the push to get a minimal no_std allocator story stabilized.

@joshtriplett @withoutboats @SimonSapin - do you have suggestions on who should be contacted to coordinate this discussion? I have a reasonable "allocator implementer/users perspective" (at least from an embedded user's perspective), though I think Alex's points were rather aimed at the coherence of the core/std library's perspective.

@Amanieu
Copy link
Member

Amanieu commented Apr 19, 2020

@jamesmunns In no_std/embedded, have you ever had a need to differentiate out-of-memory situations from other types of panics? If not then I feel that #66741 is probably the "safest" approach to go for since it would be forward-compatible with any future mechanism we decide to have for specifying behavior on OOM (including the one proposed in this issue).

@joshtriplett
Copy link
Member

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

@rfcbot
Copy link

rfcbot commented Apr 20, 2020

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

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

Stabilizing #[alloc_error_handler] as the way to fulfil this requirement would allow:

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

I am somewhat confused by this framing, which is not actually accurate given the parallel developments in #66741. These are some great motivations, but it looks like we are currently seeing two independent changes motivated that way when really the motivation only requires one of them -- once either solution is accepted, the motivation ceases to apply to the other. It doesn't seem fair to use this motivation to justify both changes, when one of them (the one that lands later) will not actually help the cause.

I see little discussion for whether we shouldn't rather just adopt one of the two instead of both ways to enable no_std + liballoc.

@SimonSapin
Copy link
Contributor Author

the motivation only requires one of them

That is correct. This issue and #66741 list each other in their respective "Alternatives" section of the description/proposal. When initially filing them I did not expect both of them to enter FCP at (roughly) the same time.

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Apr 30, 2020
@rfcbot rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Apr 30, 2020
@rfcbot
Copy link

rfcbot commented Apr 30, 2020

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

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@Aaron1011
Copy link
Member

What's the status of this stabilization?

@SimonSapin
Copy link
Contributor Author

To the letter of the process, the proposal is accepted and the next step is a stabilization PR. However @RalfJung made a good point above and I’d like some input from @rust-lang/lang on how they feel about accepting both this and #66741 when either would achieve the main motivation (which is liballoc in no_std programs).

@Amanieu
Copy link
Member

Amanieu commented May 23, 2020

I think we should only stabilize one of the two proposals. It doesn't make sense to stabilize both IMO.

@withoutboats
Copy link
Contributor

I mentioned in a previous comment hoping that we could solve this at the all hands, which didn't happen of course. I'd be happy to sit on a call to hammer out a final resolution to this problem so that we can have alloc available on stable. I don't think it's that hard, we just need a higher bandwidth hour to make an executive decision together.

@pnkfelix
Copy link
Member

pnkfelix commented Jun 1, 2020

To the letter of the process, the proposal is accepted and the next step is a stabilization PR. However @RalfJung made a good point above and I’d like some input from @rust-lang/lang on how they feel about accepting both this and #66741 when either would achieve the main motivation (which is liballoc in no_std programs).

We discussed this oddity of these two proposals both being approved at today's lang team meeting.

Our current understanding that is that the two proposals are not inherently in conflict with one another; we thought it would not be unreasonable to approve both proposals.

I have tasked myself with the job of trying to digest the content of both proposals and the developments in their comment threads well enough to understand why there is a relatively strong push to only approve one.

In any case, it seems that the proposal in #66741 is smaller and has a much smaller surface area of impact. That leads us to say that the T-lang design team is inclined to push for adoption of #66741 in the short-term, and then take our time deciding about what to do with #66740. However, during my immediate review of #66741, I did see @alexcrichton's dismay over the adoption of #66741, so I guess it will be best to wait a bit long so that I have time to digest the content of both comment threads.

@nikomatsakis
Copy link
Contributor

Removing from nomination since there isn't more to discuss in lang team meeting (yet).

@pnkfelix
Copy link
Member

pnkfelix commented Jun 29, 2020

Okay I completed my review of the threads.

@alexcrichton 's comment relaying dismay over adoption of #66741 was, as far as I can tell, a general statement of disappointment in the state of the liballoc crate overall, and bemoaning a lack of leadership/organization/documentation of that crate that alex feels somewhat personally responsible for.

Alex also took that comment and said that its feedback also applies to #66740

Here are some conclusions I took from this:

  • Alex was not making a statement in favor of one proposal or another. Alex doesn't like either of them.
  • Therefore, we should not interpret alex's disapproval as a reason to favor one of these two proposals over another. (My mistake was in interpreting alex's comment as being specifically about Tracking issue for handle_alloc_error defaulting to panic (for no_std + liballoc) #66741, which is what the lang team said we wanted to adopt in the short term.)

Assuming we adopt #66741 in the short term, there remains an open question as to whether to adopt #66740. If we adopt @withoutboats 's proposal to extend PanicInfo with information about allocation failures when relevant, then we really do not need to adopt #66740 on top of #66741. But without such an extension of PanicInfo in place, then we may still need to adopt #66740, solely to allow application authors to have more expressive control of how they respond to allocation failures.

@nikomatsakis
Copy link
Contributor

We discussed this issue in the @rust-lang/lang meeting today and felt that we could delay stabilizing the #[alloc_error_handler] proposal until there is a specific request for it for some reason other than access to lib-alloc from a no-std context.

@Amanieu
Copy link
Member

Amanieu commented Jun 29, 2020

Ah, I somehow missed @withoutboats's proposal. I think that's a great idea which neatly solves the disconnect between panics and OOM aborts.

I agree that we should start with the approach proposed in #66741 since it resolves the immediate problem and is forward compatible with any solution we might choose in the future.

@eloraiby
Copy link

any timeline on this one ?

@nikomatsakis
Copy link
Contributor

I believe the current status is that we chosen not to do this stabilization, and to proceed instead with #66741. As @pnkfelix noted here, this stabilization is blocked on (a) a motivating example and (b) perhaps exploration of @withoutboats 's proposal to extend PanicInfo with information about allocation failures when relevant.

Should we then close this? I think @rust-lang/lang is in favor, but do I hear a second from @rust-lang/libs ?

@Amanieu
Copy link
Member

Amanieu commented Jul 27, 2020

I'm also happy to close this in favor of #66741.

@eloraiby
Copy link

Thanks for the clarification. If all in favour, when can we expect the PanicInfo to be "augmented" and stable ? :)

@nikomatsakis
Copy link
Contributor

@eloraiby I think probably we should open an issue describing the idea and then somebody could open a PR and ping the @rust-lang/libs (and perhaps @rust-lang/lang) teams to decide on that PR.

@SimonSapin
Copy link
Contributor Author

which neatly solves the disconnect between panics and OOM aborts.

Note that #66741 only affects programs that do not link the std crate: it is about the behavior when no #[alloc_error_handler] is defined, and std defines one. The handler in std calls a runtime-configurable hook #51245, then aborts the process.

So #66741 does not affect OOM aborts.

@SimonSapin
Copy link
Contributor Author

Closing per #66740 (comment)

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

No branches or pull requests