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

[RalfJung]

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, #66741

[rustbot]

r? @nagisa

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

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

[nbdd0121]

The default OOM handler should not abort when -Z oom=panic is used.

[CAD97]

Technically, a no-unwind panic is still a panic (and it's an unstable flag anyway), so you can argue that it still meets the letter of the request.

Probably, though, the more reasonable behavior would be for -Zoom=abort to panic_nounwind¹ and -Zoom=panic to panic! (thus getting the -Cpanic=abort or -Cpanic=unwind behavior).

You can argue it should be a three-way switch where -Zoom=abort does rtabort! and bypasses the panic handler, with a different -Zoom=panic_nounwind to include calling the panic handler; I don't really have a strong preference. The reason to have the version that doesn't use the panic handler is if handle_alloc_error is expected to indicate that the panic handler would also raise OOM and bypassing it might still get a message printed.

Footnotes

1. For various reasons, I have a weak preference for the function/operation to be panic_abort (calling the panic handler with can_unwind = false); to me this is explicitly asking for the -Cpanic=abort behavior for this panic even when -Cpanic=unwind is used, and is clearer than panic_nounwind w.r.t. behavior. (_nounwind is more claiming the property nounwind than describing behavior.) ↩

[CAD97]

Copying over a bit of rationale from #66741 for clarity:

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.

The intent of this PR IIUC isn't to change -Zoom=panic; it's to change the no_std behavior, which does not reflect -Zoom (-Zoom only happens when the std oom handler gets brought in), to match std's default behavior of aborting — as of #102218, no_std provides a default oom handler which panic!s.

This PR merely switches the default no_std oom handler from causing a may-unwind panic to causing a mayn't-unwind panic. The std oom handler (and thus -Zoom) is uneffected.

It could make sense to make -Zoom control the no_std default oom handler, but it currently doesn't. This PR changes the behavior of that default handler to match the default -Zoom behavior. A later PR can extend the -Zoom switch to control the no_std oom handler, but it doesn't need to happen instead of this fix to the stable behavior without the flag.

[RalfJung]

Indeed this PR has little to do with -Zoom. That flag affects the std alloc error handler; this PR changes the default no_std alloc error handler.

[Lokathor]

I want to clarify: With this change, no_std + alloc that encounters OOM will abort without calling the panic handler?

[bjorn3]

With this change it will call the panic handler, but the panic handler will be told that it must abort and may not unwind. If it attempts to unwind anyway, this will result in another aborting panic, just like when trying to unwind out of an extern "C" function with #![feature(c_unwind)].

[nbdd0121]

-Zoom=panic is a bad name, it's probably better to call it -Zoom=unwind. I understand that -Z oom=panic is for std's OOM handler, but I think the default OOM handler should be consistent with std's.

The question is: after -Zoom=panic stabilisation, should the default OOM handler do an unwindable panic or an aborting one? I think it should be the former.

[Lokathor]

An OOM panic should probably default to doing the same that any other panic would do, as set by the target profile or the panic setting in Cargo.toml. And then if the user wants a difference between the two types of panic for some reason they can set that separately.

[RalfJung]

This PR is about no_std situations, where -Zoom has no effect at all. I don't quite see why that flag is even relevant here.

[nbdd0121]

Your entire argument to make this change is to make default OOM handler consistent with std OOM handler. And -Zoom affects std OOM handler. How is that irrelevant?

[RalfJung]

I am mostly concerned with consistency for what happens on stable.

[nbdd0121]

The question is: after -Zoom=panic stabilisation, should the default OOM handler do an unwindable panic or an aborting one? I think it should be the former.

And my concern is about what will happen after -Zoom=panic is stabilised.

[RalfJung]

That's the part I don't get. We have a discrepancy now between the behavior of std since 1.0 and the soon-to-be-stable behavior of no_std. Hypothetical future stabilizations IMO make no difference at all for this discussion. Even if -Zoom gets stabilized, surely we should strive for consistency between std and no_std in their default configuration.

We should probably not stabilize -Zoom until we can make it affect both std and no_std. But that is an entirely off-topic discussion for this PR.

[nbdd0121]

Would it be okay if we switch the default OOM handler later to do a normal panic, after we stabilise -Zoom=panic? My only concern was that the panic-no-unwind behaviour become something that we guarantee.

BTW, a panic-no-unwind is pretty useless for no_std applications, because can_unwind on PanicInfo is unstable and therefore a stable no_std panic handler couldn't handle it any differently from a normal panic.

[bjorn3]

BTW, a panic-no-unwind is pretty useless for no_std applications, because can_unwind on PanicInfo is unstable and therefore a stable no_std panic handler couldn't handle it any differently from a normal panic.

Until extern "C-unwind" stabilizes, a no_std panic handler can't unwind at all. Unwinding out of an asm!() block is UB and unwinding out of extern "C" is too. The only way to initiate unwinding is to call into a C or C++ function that unwinds, which needs extern "C-unwind".

[RalfJung]

Would it be okay if we switch the default OOM handler later to do a normal panic, after we stabilise -Zoom=panic? My only concern was that the panic-no-unwind behaviour become something that we guarantee.

I would hope that we only stabilize this flag after figuring out how to make it affect both the std and no_std default OOM handler. (And even that is still confusing since custom OOM handlers could ignore the flag. But that's a discussion for the -Zoom tracking issue, if such a thing exists.)

Or maybe by "stabilise -Zoom=panic" you mean changing the std handler to panic (with unwind) rather than abort by default? If we do that then yes we should definitely also change the no_std default handler to panic (with unwind).

[Amanieu]

It is currently not possible for no_std code to unwind on stable for multiple reasons, but the main one is that the unwinding machinery is currently only implemented in std and relies on unstable features. As such, no_std isn't really inconsistent with how std currently works since the only thing the custom panic handler can do is abort anyways.

I would like to see OOM move towards being treated like a normal panic that goes through the panic handler. This unifies the std and no_std behavior and allows OOMs to be handled by the panic hook and reported in application-specific ways. Allocations in the panic handler aren't really a big deal in practice: if these allocations fail then we simply hard-abort due to a recursive panic, which is what is currently happening anyway. The -Zoom flag would then simply control whether the OOM panic is allowed to unwind via PanicInfo::can_unwind.

[bjorn3]

Allocations in the panic handler aren't really a big deal in practice: if these allocations fail then we simply hard-abort due to a recursive panic, which is what is currently happening anyway.

That would mean that no message will be printed at all about the memory exhaustion as the initial panic that should print this message allocates before the panic message is printed. A dedicated allocation failure handler can avoid allocating entirely while printing.

#0  __GI___libc_malloc (bytes=1) at malloc.c:3031
#1  0x000055555556d5f9 in alloc::alloc::alloc () at library/alloc/src/alloc.rs:99
#2  alloc::alloc::Global::alloc_impl () at library/alloc/src/alloc.rs:181
#3  alloc::alloc::{impl#1}::allocate () at library/alloc/src/alloc.rs:241
#4  alloc::raw_vec::RawVec::allocate_in<u8, alloc::alloc::Global> () at library/alloc/src/raw_vec.rs:185
#5  alloc::raw_vec::RawVec::with_capacity_in<u8, alloc::alloc::Global> () at library/alloc/src/raw_vec.rs:131
#6  alloc::vec::Vec::with_capacity_in<u8, alloc::alloc::Global> () at library/alloc/src/vec/mod.rs:673
#7  alloc::slice::hack::{impl#1}::to_vec<u8, alloc::alloc::Global> () at library/alloc/src/slice.rs:157
#8  alloc::slice::hack::to_vec<u8, alloc::alloc::Global> () at library/alloc/src/slice.rs:106
#9  alloc::slice::{impl#0}::to_vec_in<u8, alloc::alloc::Global> () at library/alloc/src/slice.rs:436
#10 alloc::slice::{impl#0}::to_vec<u8> () at library/alloc/src/slice.rs:411
#11 std::sys::unix::os::getenv () at library/std/src/sys/unix/os.rs:557
#12 std::env::_var_os () at library/std/src/env.rs:273
#13 0x000055555556e50f in std::env::var_os<&str> () at library/std/src/env.rs:269
#14 std::panic::get_backtrace_style () at library/std/src/panic.rs:298
#15 0x0000555555570d63 in std::panicking::default_hook () at library/std/src/panicking.rs:241
#16 0x0000555555571948 in std::panicking::rust_panic_with_hook () at library/std/src/panicking.rs:688
#17 0x000055555555aa34 in std::panicking::begin_panic::{{closure}} ()
#18 0x000055555555b2a5 in std::sys_common::backtrace::__rust_end_short_backtrace ()
#19 0x000055555555a97f in std::panicking::begin_panic ()
#20 0x000055555555a929 in main ()

[RalfJung]

As such, no_std isn't really inconsistent with how std currently works since the only thing the custom panic handler can do is abort anyways.

Fair. But in that case this PR makes no difference, and I would argue also helps clarify that this will not actually ever unwind. (It also adds some nounwind attributes which could help the optimizer.)

[Amanieu]

I think this PR is mostly fine, but I would like one change to be made. Could you check the value of __rust_alloc_error_handler_should_panic to decide whether the panic should be able to unwind? This variable is defined by the compiler based on the -Zoom flag. This would be similar to this check in the std oom handler.

[RalfJung]

Sure, can do. I was not sure if we could use that symbol in no_std situations.

That will negate the nounwind bound though, but I guess that's fine for now.

[Amanieu]

The nounwind bound on __rust_alloc_error_handler was removed when -Zoom=panic was added. There was no measurable impact on performance.

[RalfJung]

All right, adjusted things as requested.
r? @Amanieu

[Amanieu]

In the future, I think it would be nice to eliminate the custom OOM handler in std, eliminate #[alloc_error_handler] and route everything through the default handler here.

@bors r+

[bors]

📌 Commit 5974f6f has been approved by Amanieu

It is now in the queue for this repository.

[RalfJung]

In the future, I think it would be nice to eliminate the custom OOM handler in std

Agreed, though it will be tricky to maintain support for alloc_error_hook that way. Though that is unstable, so I guess we can remove it and tell people to define an #[alloc_error_handler] instead -- which however is static, whereas the OOM hook is dynamic.

EDIT: Actually alloc_error_hook doesn't seem to use anything from std so maybe this would just work?

eliminate #[alloc_error_handler]

As in entirely remove that feature? I am confused.

[bjorn3]

#[alloc_error_handler] can't be defined twice AFAIK and libstd defines it once. As such alloc_error_hook is still necessary if you want to allow a custom alloc error hook when using libstd.

[RalfJung]

The entire point was to remove the one that is defined in std, and move its implementation inside __rdl_oom.

That sounds like it could cause a bunch of trouble (or at least strange error messages) if panics allocate (e.g. to display the backtrace), but other than that probably should work? Not sure what is even hard about it -- why hasn't it been done? Is it only because prior to non-unwinding panics we couldn't preserve the current default behavior of having an abort (rather than unwind), or is it because printing the backtrace will probably allocate and then things go south?

[Amanieu]

I would like to remove the custom handling for OOM entirely and route everything through the panic handler. This already the current behavior with #![no_std] since #[alloc_error_handler] is unstable.

That would mean that no message will be printed at all about the memory exhaustion as the initial panic that should print this message allocates before the panic message is printed. A dedicated allocation failure handler can avoid allocating entirely while printing.

Sure, but in practice most OOMs are caused by large allocations. Any small allocations in the panic handler are still likely to succeed. In the rare case where it still panics, then it will still safely fall back to an abort (albeit with a confusing error message).

[RalfJung]

I would like to remove the custom handling for OOM entirely and route everything through the panic handler.

Oh I see, makes sense.

Well, the first step (moving std's rust_oom, together with the hook handling, into alloc as __rdl_oom) should be fairly easy as a follow-up to this PR -- which would change behavior to do full backtrace printing by default.

[gngshn]

Hello @RalfJung
It seems that this patch breaks no_std + alloc use case in embedded linux apps, In some embedded linux, code size is important, we can use no_std + alloc to reduce code size. but this patch make this method not work again in stable rust.
here is my code example

#![no_std]
#![no_main]

extern crate alloc;

use alloc::{boxed::Box, vec};
use libc_alloc::LibcAlloc;
use libc_print::std_name::println;

#[global_allocator]
static GLOBAL_ALLOC: LibcAlloc = LibcAlloc;

#[panic_handler]
fn panic(_: &core::panic::PanicInfo) -> ! {
    unsafe {
        libc::abort();
    }
}

#[no_mangle]
pub extern "C" fn main() -> i32 {
    println!("hello world!");
    let test1 = vec![1, 2, 3];
    for v in test1 {
        println!("{v}")
    }
    let test2 = Box::new(10);
    println!("{test2}");
    0
}

in nightly-2022-12-29, it works perfectly. but now, it will be link error

"_rust_eh_personality", referenced from:
                core::panicking::panic_nounwind_fmt::hdc7b70c1a43a8935 in libcore-d0a8c087921eb265.rlib(core-d0a8c087921eb265.core.5d748034-cgu.0.rcgu.o)

can you help resolve this issue in rust 1.68 for our embedded linux developers? thanks very much.

cc @Amanieu

[nbdd0121]

Does it work with -Zbuild-std?

[gngshn]

Does it work with -Zbuild-std?

Hello @nbdd0121
Thank you for you reply
-Zbuild-std=core,alloc can work, but that need nightly rust again. We need use no_std + alloc in stable rust, so we can use rust in production.

[nbdd0121]

So here's why it doesn't work: panic_nounwind_fmt is a nounwind function, calling into panic handler, which Rust considers to be potentially unwindable, because libcore that we distribute is compiled with -Cpanic=unwind. Therefore a guard is added to prevent unwinding from happening. This guard references eh personality, which is not present in absence of libstd.

[nbdd0121]

@gngshn Please file a new issue for this

[gngshn]

@gngshn Please file a new issue for this

OK, I will file a new issue, thanks