[bjorn3]

View all comments

There are major questions remaining about the reentrancy that this allows. It doesn't have any users on github outside of a single project that uses it in a panic=abort project to show backtraces. It can still be emulated through #[alloc_error_handler] or set_alloc_error_hook depending on if you use the standard library or not. And finally it makes it harder to do various improvements to the allocator shim.

With this PR the sole remaining symbol in the allocator shim that is not effectively emulating weak symbols is the symbol that prevents skipping the allocator shim on stable even when it would otherwise be empty because libstd + #[global_allocator] is used.

Closes #43596
Fixes #126683

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Cc @rust-lang/wg-allocators

I've been waiting for oom=panic to stabilize!!!

At Cloudflare we were waiting for ability to use it in servers. It was part of the RFC https://github.com/rust-lang/rfcs/blob/master/text/2116-alloc-me-maybe.md#user-profile-server. We currently aren't only because we use stable Rust, and have a ton of hacks in place already put in due to suffering from lack of this feature. In some places we do rely on panics from allocators not being totally UB any more.

Is removal of oom=panic removing just the convenience option, or is the goal to forbid unwinding on allocation failures completely? Will it be allowed to panic in alloc_error_handler or in global allocator functions? (and have it unwind whenever possible instead of always aborting?)

You can still panic in the alloc error hook you register with std::alloc::set_alloc_error_hook with this PR (or the #[alloc_error_handler] if you don't use libstd). Any stabilization of this is blocked on the same reentrancy soundness questions as -Zoom=panic. The only allocation errors that set_alloc_error_hook doesn't cover that -Zoom=panic does is for allocations in code running before user code runs, but panics there can't be recovered from anyway.

You can still panic in the alloc error hook you register with std::alloc::set_alloc_error_hook with this PR

In fact I changed the existing -Zoom=panic test to test this behavior: tests/ui/panics/alloc_error_hook-unwind.rs

☔ The latest upstream changes (presumably #147745) made this pull request unmergeable. Please resolve the merge conflicts.

r? @Amanieu maybe

Reducing the number of symbols used by the global allocation interface is a valid argument, and I do agree that it should be over extern "C" and not extern "C-unwind".

But the reentrancy concerns aren't a reason to remove oom panic support, because the same exact concerns still exist without oom panic, both panics within the allocator (that don't escape) and unwinds from the alloc (error) handler.

Personally I think handling (non-pre-main) OOM through the same panic machinery as standard code panics is the correct thing to do for global general purpose "infallible" allocations, since it's the same general style of "allegedly impossible" error, but I can see logic in saying it's notable enough to use its own specialized machinery, instead of the heavily generic and indirected panic machinery.

Consider this a neutral vote from me, I suppose.

By the way the awkward thing with -Zoom is that only the -Zoom value in the crate where we happen to codegen the allocator shim matters. This for example means that -Zoom=panic doesn't have any effect when using -Cprefer-dynamic and has unpredictable behavior when you don't compile everything with the same -Zoom value, while set_alloc_error_hook always works.

But the reentrancy concerns aren't a reason to remove oom panic support, because the same exact concerns still exist without oom panic, both panics within the allocator (that don't escape) and unwinds from the alloc (error) handler.

Reentrancy from panics within the allocator logic are indeed a tough problem but seem unrelated to OOM since you can already cause reentrancy today by allocating (or calling other non-reentrant code that’s using the allocator) in the panic hook or panic handler. I found such an issue in dlmalloc and I’d be surprised if any other allocator implemented in Rust didn’t have similar issues.

Panicking from the alloc error handler or the allocator shim is less problematic for the allocator because at least that’s after all the allocator’s own code has returned (a null pointer) normally. But the rest of the world that’s using the allocator has to deal with the potential reentrancy.

Reentrancy from panics within the allocator logic are indeed a tough problem but seem unrelated to OOM since you can already cause reentrancy today by allocating (or calling other non-reentrant code that’s using the allocator) in the panic hook or panic handler.

The global allocator is #[rustc_nounwind], so even if the global allocator itself panics that should abort during unwinding. If the panic hook can unwind however that would provide a path to unwind out of allocating code even when said code expects not to be unwound out of. Now that I think about it, I believe we do run the panic hook when the global allocator panics, so that is still reentrancy even if you can't unwind out of the global allocator.

In any case I don't think the reentrancy issue is all that relevant to this PR. It is an explanation why neither -Zoom=panic nor set_alloc_error_hook are stable, but this PR doesn't remove any functionality. It just requires you to change how exactly you enable panics on allocation failure (using set_alloc_error_hook instead of -Zoom=panic).

set_alloc_error_hook is global mutable state where libraries can clobber each other's hooks... so just as an interface, -Zoom=panic does seem somewhat nicer.