Report allocation errors through the panic handler #192

Amanieu · 2023-03-23T00:09:26Z

Proposal

Problem statement

We currently use a completely separate mechanism to handle allocation errors (OOM) than panics, despite both of these being very similar. However both are very similar: they involve handling runtime failures that are generally unrecoverable. It would be better to unify them under a single mechanism which is already stable: panic handlers (no_std) and panic hooks (std).

Motivation, use-cases

This change has already been done for no_std in rust-lang/rust#66741: it was noticed that the vast majority of users of #[alloc_error_handler] were just calling panic anyways. This avoided the need to stabilize the alloc_error_handler attribute by defaulting to a panic if one was not specified.

The OOM hook API has also remained unstable for a very long time with no clear path to stabilization. The best way forward would be to unify it with the panic hook API just like it has been done in no_std.

Some use cases (firefox) still need access to the size of the failed allocation, so this is provided as a custom panic payload.

Solution sketches

// alloc::alloc

pub struct AllocErrorPanicPayload { .. }

impl AllocErrorPanicPayload {
    pub fn layout(&self) -> Layout;
}

This ACP proposes to make the following changes:

Introduce the AllocErrorPanicPayload type which holds the Layout of a failed allocation.
Make handle_alloc_error call the normal panic handler with a special payload of AllocErrorPanicPayload.
- This can be observed by panic hooks with std or by the panic handler with no_std.
- Unless -Zoom=panic is used, this is a non-unwinding panic: if the panic hook returns then the process is aborted. This maintains the current behavior of aborting on OOM by default.
Remove the unstable alloc_error_hook std API (Tracking issue for the OOM hook (alloc_error_hook) rust#51245). Its job is now handled by panic hooks.
Remove the unstable #[alloc_error_handler] attribute (Tracking issue for the #[alloc_error_handler] attribute (for no_std liballoc) rust#51540) for no_std programs. Its job is now handled by #[panic_handler].

In the standard library, no additional allocations are made as part of the OOM handling process, except in 1 specific situation:

With -Zoom=panic, if the panic hook returns then an allocation is necessary to box the payload and allocate an exception object. However it's not a big deal if this, fails, the process will simply abort immediately in that case.

Links and related work

Tracking issues that will be closed by this change:

Tracking issue for the OOM hook (alloc_error_hook) rust#51245 alloc_error_hook
Tracking issue for the #[alloc_error_handler] attribute (for no_std liballoc) rust#51540 alloc_error_handler
- Discussion about removing alloc_error_handler

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

The text was updated successfully, but these errors were encountered:

…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

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

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 #51540 Closes #51245

m-ou-se · 2023-10-02T13:54:23Z

I don't think this is the right thing to do.

Currently, a (no-std) #[panic_handler] never gets a payload(), not even a &'static str or String, even for something as simple as panic!("hello"). Payloads are currently a std-only feature. It is true that one can call info.payload() in a #[panic_handler], but that's likely a mistake (see rust-lang/rust#115974). That method has never done anything useful in a panic handler. Currently, any time anyone implements a #[panic_handler] by downcasting a payload() they're just adding dead code: downcasting there always returns None.

So, I disagree this proposal is a way "to unify them under a single mechanism". &dyn Any panic payloads are currently a std-only feature, that just happens to also compile (but not work) in no-std.

Amanieu · 2023-10-02T17:25:18Z

I don't see this as a strong argument to avoid using payloads, it's just that payloads are underused today. Treating OOM as a panic is almost always what people want, and allows the resulting error message to be properly integrated into whatever error reporting mechanism an application is using (e.g. uploading crash reports).

m-ou-se · 2023-10-03T14:16:32Z

Using payloads in a std panic hook is fine. I'm saying that using payloads in a #[panic_handler] would be a entirely new concept. It's true that we already have a .payload() method available in #[panic_handler]s, but it doesn't do anything.

A (std) panic hook is expected to do .payload().downcast_ref<String>() (and &str) to get the panic message, but that will always return None in a #[panic_handler]. So, we can never get the concept of a 'payload' to mean the same thing in panic handlers and panic hooks. So I disagree this is a 'unified single mechanism'.

It's a valid discussion to have whether we should start exposing the concept of a panic payload in #[panic_handler], but that would be an entirely new thing, not an existing (unified) mechanism.

(And on that discussion: I don't think we should start using payloads in #[panic_handler], because it will never mean the same thing as a payload in a panic hook: it will not have a payload for formatted panic messages. And std will need to take ownership of a panic payload to be able to unwind/catch, which isn't possible when just borrowing the payload like a #[panic_handler] does.)

m-ou-se · 2023-10-03T14:18:13Z

Treating OOM as a panic is almost always what people want, and allows the resulting error message to be properly integrated into whatever error reporting mechanism an application is using (e.g. uploading crash reports).

That will happen regardless of whether we have a AllocErrorPanicPayload payload mechanism or a default alloc_error_handler that panics, right?

RalfJung · 2024-04-17T07:59:38Z

Using payloads in a std panic hook is fine. I'm saying that using payloads in a #[panic_handler] would be a entirely new concept. It's true that we already have a .payload() method available in #[panic_handler]s, but it doesn't do anything.

I've been wondering before why panic hooks and panic handlers take the same type as argument. In a sense there's really two different types here that are just merged into one -- "PanicInfo with a message" and "PanicInfo with a payload".

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 #51540 Closes #51245

Amanieu added T-libs-api api-change-proposal A proposal to add or alter unstable APIs in the standard libraries labels Mar 23, 2023

This was referenced Mar 23, 2023

Report allocation errors as panics rust-lang/rust#109507

Merged

Tracking issue for the #[alloc_error_handler] attribute (for no_std liballoc) rust-lang/rust#51540

Open

Amanieu mentioned this issue Jun 5, 2023

Report allocation errors as panics, second attempt rust-lang/rust#112331

Open

m-ou-se mentioned this issue Oct 2, 2023

Split core's PanicInfo and std's PanicInfo rust-lang/rust#115974

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report allocation errors through the panic handler #192

Report allocation errors through the panic handler #192

Amanieu commented Mar 23, 2023

m-ou-se commented Oct 2, 2023

Amanieu commented Oct 2, 2023

m-ou-se commented Oct 3, 2023 •

edited

Loading

m-ou-se commented Oct 3, 2023

RalfJung commented Apr 17, 2024

Report allocation errors through the panic handler #192

Report allocation errors through the panic handler #192

Comments

Amanieu commented Mar 23, 2023

Proposal

Problem statement

Motivation, use-cases

Solution sketches

Links and related work

What happens now?

m-ou-se commented Oct 2, 2023

Amanieu commented Oct 2, 2023

m-ou-se commented Oct 3, 2023 • edited Loading

m-ou-se commented Oct 3, 2023

RalfJung commented Apr 17, 2024

m-ou-se commented Oct 3, 2023 •

edited

Loading