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 the #[alloc_error_handler] attribute (for no_std + liballoc) #51540

Open
2 tasks
SimonSapin opened this issue Jun 13, 2018 · 139 comments · Fixed by #109507 · May be fixed by #112331
Open
2 tasks

Tracking issue for the #[alloc_error_handler] attribute (for no_std + liballoc) #51540

SimonSapin opened this issue Jun 13, 2018 · 139 comments · Fixed by #109507 · May be fixed by #112331
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-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. S-tracking-design-concerns Status: There are blocking ❌ design concerns. S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. 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 Jun 13, 2018

This attribute is mandatory when using the alloc crate without the std crate. It is used like this:

#[alloc_error_handler]
fn foo(_: core::alloc::Layout) -> ! {
    // …
}

Implementation PR: #52191

Blocking issues

Original issue:


In a no_std program or staticlib, linking to the alloc crate may cause this error:

error: language item required, but not found: `oom`

This is fixed by providing the oom lang item, which is is normally provided by the std crate (where it calls a dynamically-settable hook #51245, then aborts). This is called by alloc::alloc::handle_alloc_error (which is called by Vec and others on memory allocation failure).

#![feature(lang_items)]

#[lang = "oom"]
extern fn foo(_: core::alloc::Layout) -> ! {
    // example implementation based on libc
    extern "C" { fn abort() -> !; }
    unsafe { abort() }
}

However, defining a lang item is an unstable feature.

Possible solutions include:

  1. Add and stabilize a dedicated attribute similar to the #[panic_implementation] attribute:
#[alloc_error_handler]
fn foo(_: core::alloc::Layout) -> ! {
    // …
}

The downside is that this is one more mandatory hoop to jump through for no_std program that would have been fine with a default hook that aborts.

  1. Move std’s dynamically-settable hook into alloc: Move OOM handling to liballoc and remove the oom lang item #51607. The downside is some mandatory space overhead.
@SimonSapin SimonSapin added A-allocators Area: Custom and system allocators T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Jun 13, 2018
@SimonSapin
Copy link
Contributor Author

Better idea: remove the lang item and move all of OOM handling to liballoc, except for the default hook that prints to stderr. Have compiler magic similar to that of #[global_allocator] that uses the printing default hook when std is linked, or a no-op default hook otherwise.

@glandium
Copy link
Contributor

Bikeshedding on oom renaming: alloc_error would be shorter.

@SimonSapin
Copy link
Contributor Author

I forgot to link #51543, which does indeed use alloc_error.

SimonSapin added a commit to SimonSapin/rust that referenced this issue Jun 27, 2018
This removes the need for `no_std` programs to define that lang item themselves:

Fixes rust-lang#51540
@SimonSapin
Copy link
Contributor Author

@japaric, @glandium, @Amanieu I’ve updated the the issue description here with arguments from #51607 and rust-lang/rfcs#2480 and to list two alternatives.

@japaric
Copy link
Member

japaric commented Jun 30, 2018

Replying to @glandium's rust-lang/rfcs#2480 (comment) here to not derail the alloc RFC.

Every #[no_std] user of the alloc crate would have to implement their own

Not everyone has to implement their own. An implementation can be packed into a crate and it just takes adding a extern crate oom_abort; to register an OOM handler -- this is no different than what we have for global allocators (e.g. extern crate alloc_jemalloc).

and they can't use instrinsics::abort, so it becomes harder than necessary.

Then stabilize intrinsics::abort or provide a stable oom_abort crate with the rust-std component. Though it's better to stabilize intrinsics::abort because then we can use it for #[panic_implementation] too.

The problem is that with a #[oom] attribute, you don't get a default handler at all.

No default OOM handler is good. There's no sensible default for every single combination of no_std program and target anyways. For example, intrinsics::abort produces a linker error on MSP430 because there's no abort instruction in the MSP430 instruction set.

Also, not having a default is consistent with both #[panic_implementation] and #[global_allocator] in #[no_std] context. Why special case the OOM handler?


Another reason why I like static registration of global properties is that it's less error prone. Say you want the OOM (or panic) handler to behave differently between development and final release. With #[oom] you can write this:

#[cfg(debug_assertions)]
extern crate oom_report; // for development, verbose

#[cfg(debug_assertions)]
extern crate oom_panic; // for release, minimal size

This is clearly wrong, you get a compiler error. With the set_alloc_error_hook you don't get a compiler error; you won't notice the problem until you hit an OOM in dev and lost your opportunity to track down the root of the OOM.

The other reason I like #[oom] / #[panic_implementation] is that you can be sure of the behavior of the OOM / panic if you register the handler in the top crate.

extern crate oom_abort; // I'm 100% sure OOM = abort

With the hook API you have no guarantee

fn main() {
    set_alloc_error_hook(|| abort()); // OOM = abort, maybe

    dependency::function(); // this is totally free to change the OOM handler

    // ..
}

Finally, if you need the ability to override the OOM handler at runtime using hooks you can implement that on top of #[oom].

@SimonSapin
Copy link
Contributor Author

@japaric I find this convincing regarding static v.s. dynamic, thanks.

For example, intrinsics::abort produces a linker error on MSP430 because there's no abort instruction in the MSP430 instruction set.

Oh, I was wondering if something like that ever happened.

How does one typically deal with unrecoverable situations on MSP430? An infinite loop?

No default OOM handler is good.

I think we can have a default, with a Sufficiently Advanced Compiler. (Grep for has_global_allocator for code that does something similar.)

But you’re saying we should not, and force that question on ever no_std user. This would be one more barrier before being able to get to Hello World.

@SimonSapin
Copy link
Contributor Author

#51607 (comment)

FWIW an attribute like #[oom] I also think would be a great idea (and I think I'm also convinced that it may be worth it over this dynamic strategy), and it could be implemented pretty similar to #[panic_implementation]

Right. I think the remaining question is: should there be a default? If not, what should be a "typical" implementation that we’d recommend in docs? Should we add a stable wrapper for core::intrinsics::abort? (In what module?)

The docs for core::intrinsics::abort claim:

The stabilized version of this intrinsic is std::process::abort

But besides availability, the two functions do not have equivalent behavior: servo/servo#16899.

@japaric
Copy link
Member

japaric commented Jul 5, 2018

How does one typically deal with unrecoverable situations on MSP430? An infinite loop?

@pftbest and @cr1901 would be more qualified to answer that question. I'm not a MSP430 developer myself.

On Cortex-M the abort instruction triggers the HardFault handler. While developing I configure panic and HardFault to log info about the program, or just to trigger a breakpoint if binary size is a problem. In release mode, I usually set panic to abort and make the HardFault handler disable interrupts, shut down the system (e.g. turn off motors), signal the failure (e.g. turn on a red LED) and go into an infinite loop but this is very application specific. Also, in some applications reaching an unrecoverable situation means that something is (very) wrong with your software and that it should not be deployed until it's fixed.

But you’re saying we should not, and force that question on ever no_std user. This would be one more barrier before being able to get to Hello World.

#[global_allocator] is not mandatory for #[no_std] binaries. And the oom lang item is only required when you are using #[global_allocator]. So, no, not all no_std program developers have to deal with the oom handler.


should there be a default?

I think there should not be a default.

If not, what should be a "typical" implementation that we’d recommend in docs?

The #![no_std] application - target space is too broad to recommend anything that will behave the same everywhere. Consider intrinsics::abort:

  • on wasm means terminate executino and notify the wasm interpreter or host system
  • on msp430 it causes a linker error
  • on Cortex-M it defers the error handling to the HardFault exception handler
  • on Linux systems it terminates the process with a SIGILL exit code

Should we add a stable wrapper for core::intrinsics::abort? (In what module?)

Yes. As core::arch::$ARCH::abort maybe? But only on architectures whose instruction sets define an abort / trap instruction.

The docs for core::intrinsics::abort claim:

Those docs should be fixed. iirc process::abort tries to does some clean up (of the Rust runtime?) before aborting the process. intrinsics::abort is an LLVM intrinsic that maps to the abort instruction of the target instruction set so the two are not equivalent.

@SimonSapin SimonSapin changed the title Tracking issue for no_std OOM handling (the oom lang item) Tracking issue for the #[alloc_error_handler] attribute (for no_std + liballoc) Jul 6, 2018
@cr1901
Copy link
Contributor

cr1901 commented Jul 7, 2018

How does one typically deal with unrecoverable situations on MSP430? An infinite loop?

Infinite loop is how I typically deal w/ it. @pftbest can override me if he knows something better though, as it's been a while since I delved into msp430 internals :).

@SimonSapin
Copy link
Contributor Author

SimonSapin commented Jul 10, 2018

#52191 has landed with an attribute for a statically-dispatched function, and no default.

@mark-i-m
Copy link
Member

mm/krust/libkrust.a(alloc-062fb091d60c735a.alloc.67kypoku-cgu.11.rcgu.o): In function `alloc::alloc::handle_alloc_error':
alloc.67kypoku-cgu.11:(.text._ZN5alloc5alloc18handle_alloc_error17h59bd4dd5f11cdd3fE+0x2): undefined reference to `rust_oom'

I'm getting the above in one my projects. I have defined the handle_alloc_error function as in the OP. The code compiles just fine, but the linker cannot find the function.

@tmandry
Copy link
Member

tmandry commented Mar 23, 2023

Sounds like concerns raised by @glandium have mostly been resolved.

@rfcbot reviewed

That kind of makes it confusing that the non-panic version of the flag does call the panic hook... even if the final result is an abort rather than an unwinding panic... Maybe the -Zoom=panic flag should be changed to e.g. -Zoom=unwind?

Taking off my lang hat: +1, although it's unclear what it would do with -Cpanic=abort. Perhaps something like -Zoom=nounwind as the default?

@RalfJung
Copy link
Member

RalfJung commented Mar 24, 2023 via email

@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 4, 2023
@rfcbot
Copy link

rfcbot commented Apr 4, 2023

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

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

rfcbot commented Apr 14, 2023

The final comment period, with a disposition to close, 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.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Apr 14, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 17, 2023
…twco

Report allocation errors as panics

OOM is now reported as a panic but with a custom payload type (`AllocErrorPanicPayload`) which holds the layout that was passed to `handle_alloc_error`.

This should be review one commit at a time:
- The first commit adds `AllocErrorPanicPayload` and changes allocation errors to always be reported as panics.
- The second commit removes `#[alloc_error_handler]` and the `alloc_error_hook` API.

ACP: rust-lang/libs-team#192

Closes rust-lang#51540
Closes rust-lang#51245
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 20, 2023
@bors bors closed this as completed in 39cf520 Apr 22, 2023
tantaman added a commit to vlcn-io/cr-sqlite that referenced this issue Apr 23, 2023
tantaman added a commit to vlcn-io/cr-sqlite that referenced this issue Apr 24, 2023
tcharding added a commit to tcharding/rust-bitcoin that referenced this issue Apr 26, 2023
Seems we no longer need an explicit error handler, remove it.

I did not grok the reason (long thread link below) but just removed it
and checked that the embedded crates still ran correctly.

rust-lang/rust#51540)
apoelstra added a commit to rust-bitcoin/rust-bitcoin that referenced this issue Apr 26, 2023
d378451 embedded: Remove error handler (Tobin C. Harding)

Pull request description:

  Seems we no longer need an explicit error handler, remove it.

  I did not grok the reason (long thread link below) but just removed it and checked that the embedded crates still ran correctly.

  rust-lang/rust#51540

  Fix: #1813

ACKs for top commit:
  Kixunil:
    ACK d378451
  apoelstra:
    ACK d378451

Tree-SHA512: 76ce3f346e7e4e62595c6a6c415476b0cabcf27ded8e79a3e0b692a98b12ff9e90bec2841d18790bb62a16c553ad60492fc09e1aa4bf550c7070cd29b5ac1702
bjorn3 pushed a commit to bjorn3/rust that referenced this issue Apr 29, 2023
Report allocation errors as panics

OOM is now reported as a panic but with a custom payload type (`AllocErrorPanicPayload`) which holds the layout that was passed to `handle_alloc_error`.

This should be review one commit at a time:
- The first commit adds `AllocErrorPanicPayload` and changes allocation errors to always be reported as panics.
- The second commit removes `#[alloc_error_handler]` and the `alloc_error_hook` API.

ACP: rust-lang/libs-team#192

Closes rust-lang#51540
Closes rust-lang#51245
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 5, 2023
Report allocation errors as panics

OOM is now reported as a panic but with a custom payload type (`AllocErrorPanicPayload`) which holds the layout that was passed to `handle_alloc_error`.

This should be review one commit at a time:
- The first commit adds `AllocErrorPanicPayload` and changes allocation errors to always be reported as panics.
- The second commit removes `#[alloc_error_handler]` and the `alloc_error_hook` API.

ACP: rust-lang/libs-team#192

Closes rust-lang#51540
Closes rust-lang#51245
@Amanieu Amanieu linked a pull request Jun 5, 2023 that will close this issue
antoyo pushed a commit to antoyo/rust that referenced this issue Jun 19, 2023
Report allocation errors as panics

OOM is now reported as a panic but with a custom payload type (`AllocErrorPanicPayload`) which holds the layout that was passed to `handle_alloc_error`.

This should be review one commit at a time:
- The first commit adds `AllocErrorPanicPayload` and changes allocation errors to always be reported as panics.
- The second commit removes `#[alloc_error_handler]` and the `alloc_error_hook` API.

ACP: rust-lang/libs-team#192

Closes rust-lang#51540
Closes rust-lang#51245
Subhra264 pushed a commit to Subhra264/rust-bitcoin that referenced this issue Jun 24, 2023
Seems we no longer need an explicit error handler, remove it.

I did not grok the reason (long thread link below) but just removed it
and checked that the embedded crates still ran correctly.

rust-lang/rust#51540)
tantaman added a commit to vlcn-io/cr-sqlite that referenced this issue Sep 5, 2023
@RalfJung
Copy link
Member

RalfJung commented Dec 9, 2023

Given that this issue is closed, why is the unstable book still listing a feature that points here?
https://doc.rust-lang.org/nightly/unstable-book/language-features/alloc-error-handler.html

@SimonSapin
Copy link
Contributor Author

This tracking issue was closed when PR #109507 merged to remove the feature, but that was reverted in PR #110782. Reopening to reflect that.

PR #112331 is open to remove the feature again, and is already marked as closing this.

@SimonSapin SimonSapin reopened this Dec 9, 2023
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-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. S-tracking-design-concerns Status: There are blocking ❌ design concerns. S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. 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