Stabilize default_alloc_error_handler by Amanieu · Pull Request #102318 · rust-l...

r? @lcnr

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

considering that the FCP was 2 years ago, i would expect us to go through another FCP which explains the reason why stabilization was blocked. Won't block this without an FCP, but i definitely want someone to look at this who's a bit more knowledgeable about this than me

r? compiler

When using this with cg_clif I get:

           /home/gh-bjorn3/cg_clif/build_sysroot/sysroot_src/library/alloc/src/alloc.rs:419: undefined reference to `rust_oom'

It seems that the current implementation depends on -Zfunction-sections=yes, while cg_clif sets it to no for linker performance reasons. To elaborate on why it depends on this. The code in question is

pub mod __alloc_error_handler {
    use crate::alloc::Layout;

    // called via generated `__rust_alloc_error_handler`

    // if there is no `#[alloc_error_handler]`
    #[rustc_std_internal_symbol]
    pub unsafe fn __rdl_oom(size: usize, _align: usize) -> ! {
        panic!("memory allocation of {size} bytes failed")
    }

    // if there is an `#[alloc_error_handler]`
    #[rustc_std_internal_symbol]
    pub unsafe fn __rg_oom(size: usize, align: usize) -> ! {
        let layout = unsafe { Layout::from_size_align_unchecked(size, align) };
        extern "Rust" {
            #[lang = "oom"]
            fn oom_impl(layout: Layout) -> !;
        }
        unsafe { oom_impl(layout) }
    }
}

__rust_alloc_error_handler will be generated as a call to either of these functions. __rdl_oom when #![feature(default_alloc_error_handler)] is used, no problem here and __rg_oom when it isn't used. The problem is that in this case rust_oom (which is the actual symbol name of oom_impl) is not defined. In other words it only just so happens to work because __rg_oom is garbage collected by the linker by default. Both compiling liballoc with -Zfunction-sections=no and using -Clink-dead-code when compiling the executable or dylib will cause __rg_oom to not be garbage collected and thus give a linker error.

I guess this is fine, but for people using alloc without global error handling at all (the current unstable cfg stuff, a feature hopefully someday) I hope one doesn't get needlessly synthesized.

We should probably have a test that uses this in a way we expect it to be used without requiring other feature gates. The tests touched by this PR all use other feature gates.

@rustbot ready

Same thing for WASM where you don't use libc / eh_personality usually.

I've removed most of the unstable features from the tests. The only remaining features are:

• rustc_private which is needed to use rustc's private copy of libc. The test harness doesn't have access to cargo to use the normal libc crate.
• lang_items is needed because the standard library is built with -C panic=unwind so a personality function must be provided. This isn't needed on targets that build the standard library with -C panic=abort.

Stabilization report for @rust-lang/lang

More details (Guide/Reference level explanations) can be found in the main post of the tracking issue. There are no outstanding issues with this feature. The only alternative was to stabilize the #[alloc_error_handler] attribute to allow users to define their own error handler. This alternative was rejected by T-lang in #66740 (comment)

Motivation (verbatim from tracking issue)

As of Rust 1.36, specifying an allocation error handler is the only requirement for using the alloc crate in no_std environments (i.e. without the std crate being also linked in the program) that cannot be fulfilled by users on the Stable release channel.

Removing this requirement by having a default behavior would allow:

• no_std + liballoc applications to start running on the Stable channel
• no_std applications that run on Stable to start using liballoc

We talked about this in today's @rust-lang/lang meeting. We're going to confirm consensus via a quick FCP, since it's been 2 years.

@rfcbot merge

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot fcp reviewed

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

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

This will be merged soon.

Commit e66220f has been approved by oli-obk

It is now in the queue for this repository.

Testing commit e66220f with merge bbb9cfb...

Test successful - checks-actions
Approved by: oli-obk
Pushing bbb9cfb to master...

Finished benchmarking commit (bbb9cfb): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

Cycles

Results