Implement -Z oom=panic

[Amanieu]

This PR removes the #[rustc_allocator_nounwind] attribute on alloc_error_handler which allows it to unwind with a panic instead of always aborting. This is then used to implement -Z oom=panic as per RFC 2116 (tracking issue #43596).

Perf and binary size tests show negligible impact.

[rust-highfive]

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

[Amanieu]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 97c23fdb1834d1669314cc73b3b31a9cb451e28a with merge 14b72f38ec9756851f7af637cd45f181890e3bd7...

[bors]

☀️ Try build successful - checks-actions
Build commit: 14b72f38ec9756851f7af637cd45f181890e3bd7 (14b72f38ec9756851f7af637cd45f181890e3bd7)

[rust-timer]

Queued 14b72f38ec9756851f7af637cd45f181890e3bd7 with parent 0035d9d, future comparison URL.

[rust-timer]

Finished benchmarking try commit (14b72f38ec9756851f7af637cd45f181890e3bd7): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

• Small improvement in instruction counts (up to -0.3% on full builds of helloworld)
• Moderate regression in instruction counts (up to 1.1% on incr-full builds of inflate)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

[Amanieu]

There seems to be no impact on performance and binary size increases by only 0.1%, so I think we can simply remove #[rustc_allocator_nounwind] from alloc_error_handler and allow it to safely unwind.

The only remaining issue is deciding how we should expose the ability to have OOM unwind to users:

• The original RFC suggested a compiler option -C oom=unwind similar to -C panic=unwind.
• We have an (unstable) OOM hook (Tracking issue for the OOM hook (alloc_error_hook) #51245) similar to the panic hook which aborts by default but could be overridden to panic instead.
• We also have an unstable #[alloc_error_handler] attribute, but this is already used within std to implement OOM hooks and provide the default hook that aborts.

[mati865]

I think benchmarking compiler is not very relevant here. Std's microbenchmarks should give more meaningful results.

[Amanieu]

I disagree, the compiler is a great benchmark because it makes a lot of memory allocations as part of the compilation process (usually through Vec, Box and HashMap). The aim of this PR is to evaluate what impact removing #[rustc_allocator_nounwind] has on codegen for allocation-heavy code.

[yaahc]

This issue documented the binary size blowup that happened the first time they started allowing unwinding on oom: #42808. We should do all the same checks they did to make sure we're not reintroducing the same issues.

[Amanieu]

I ran the same tests: on the serde one the size increase was only 0.8%, on the main-only one is 4KB larger (1.4%) but that could be due to rounding to the page size.

In conclusion I think this is an entirely reasonable tradeoff for allowing OOM to be handled in user code.

[nbdd0121]

That's a surprisingly small overhead.

I am uncertain if there are any code currently assuming that handle_alloc_error won't unwind though, since Box is kind of special in MIR.

[nbdd0121]

One particular worry is that Box is a NullaryOp in MIR instead of a terminator.

[Amanieu]

You're right, the drop isn't getting called in this example:

#![feature(box_syntax)]
use std::panic::catch_unwind;

struct Foo;

impl Drop for Foo {
    fn drop(&mut self) {
        println!("Foo dropped");
    }
}

fn main() {
    catch_unwind(|| {
        let foo = Foo;
        let a = box [0u8; 1024*1024*1024];
        let b = box [0u8; 1024*1024*1024];
        let c = box [0u8; 1024*1024*1024];
        let d = box [0u8; 1024*1024*1024];
        println!("No oom");
    });
    println!("Caught unwind!");
}

I'm not sure how difficult it would be to make box support unwinding...

[bjorn3]

I think

rust/library/alloc/src/alloc.rs

Line 392 in a49e38e

pub unsafe extern "C" fn __rg_oom(size: usize, align: usize) -> ! {

needs to be changed to use C-unwind as abi.

Edit: __rdl_oom needs to be changed too.

[bors]

📌 Commit aa36237 has been approved by nagisa

[bors]

⌛ Testing commit aa36237 with merge d8dbcea50d1273641d256cb9a884d1a930ea4c6e...

[bors]

💔 Test failed - checks-actions

[bjorn3]

Looks like you accidentally committed a change to the src/tools/rust-analyzer submodule.

[Amanieu]

My bad, this sometimes happens when switching between branches that have different submodule commits checked out.

[Amanieu]

@bors r=nagisa

[bors]

📌 Commit 0254e31 has been approved by nagisa

[bors]

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

[bors]

⌛ Testing commit 0254e31 with merge 987e5b0c45dddba1b7ea8aff8a82d491400fd364...

[bors]

💔 Test failed - checks-actions

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[ehuss]

@bors retry

Accidentally cancelled this.

[bors]

⌛ Testing commit 0254e31 with merge a8f752f012f12dbeff295ee772d568ebc1d9b08a...

[bors]

💔 Test failed - checks-actions

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[ehuss]

@bors retry

Waiting on getting CI working.

[Dylan-DPC]

@bors treeclosed-

[bors]

⌛ Testing commit 0254e31 with merge d6f3a4e...

[bors]

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing d6f3a4e to master...

[rust-timer]

Finished benchmarking commit (d6f3a4e): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression