Implement Make `handle_alloc_error` default to panic (for no_std + liballoc) #76448

haraldh · 2020-09-07T14:34:39Z

Related: #66741

Guarded with #![feature(default_alloc_error_handler)] a default
alloc_error_handler is called, if a custom allocator is used and no
other custom #[alloc_error_handler] is defined.

rust-highfive · 2020-09-07T14:34:42Z

r? @nikomatsakis

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

haraldh · 2020-09-11T13:45:14Z

Hmm, seems like library/alloc stage1 gets tested with cargo and rustc from stage0 without --cfg bootstrap set.

2020-09-11T09:57:49.7283157Z Testing alloc stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

2020-09-11T09:58:15.7745629Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "-p" "alloc" "--" "--quiet"

haraldh · 2020-09-11T13:45:47Z

@nikomatsakis @Amanieu need some help here.

haraldh · 2020-09-11T13:52:48Z

or maybe @SimonSapin can help

Amanieu · 2020-09-11T14:06:13Z

Under #[cfg(test)] I think you need to import handle_alloc_error from libstd instead of defining it yourself.

haraldh · 2020-09-11T14:07:52Z

So, this one passes.

$ python x.py test  --exclude library/alloc --exclude src/tools/linkchecker

@Amanieu will try, thanks

haraldh · 2020-09-11T15:07:53Z

ready for review 😀

jonas-schievink · 2020-09-13T16:02:21Z

r? @Amanieu

Amanieu · 2020-09-13T19:18:19Z

library/alloc/src/alloc.rs

+    unsafe {
+        __rust_alloc_error_handler(layout.size(), layout.align());
+    }
+}


For tests you can just do:

pub use std::alloc::handle_alloc_error;

This works since std is imported by tests: https://github.com/haraldh/rust-1/blob/5ac1d7867411e1f28d3cee45df8c219fb39f414e/library/alloc/src/lib.rs#L146

ok, changed

Amanieu · 2020-09-13T19:21:47Z

compiler/rustc_passes/src/weak_lang_items.rs

-                tcx.sess.err("`#[alloc_error_handler]` function required, but not found");
+                if !tcx.features().default_alloc_error_handler {
+                    tcx.sess.err("`#[alloc_error_handler]` function required, but not found.");
+                    tcx.sess.err("Use `#![feature(default_alloc_error_handler)]` for a default error handler.");


Could this be a note or suggestion instead?

changed to tcx.sess.note_without_error

Amanieu · 2020-09-14T09:22:30Z

@bors r+

bors · 2020-09-14T09:22:31Z

📌 Commit 8e46ce0d22b01bb32adc20b4e911f129da65ed44 has been approved by Amanieu

bors · 2020-10-01T13:21:28Z

💔 Test failed - checks-actions

haraldh · 2020-10-01T13:36:47Z

target arm-unknown-linux-gnueabihf:

undefined reference to `__aeabi_unwind_cpp_pr0'

Hmpf... seems like I need help with my test cases... @Amanieu @SimonSapin ... they work on x86_64-linux ... no idea what is wrong here.

Amanieu · 2020-10-01T20:09:16Z

It seems that this symbol is required on ARM. You can just define it as an empty function like here: https://github.com/rust-lang/compiler-builtins/blob/7b996ca0fa969199332d703b81fb411d85e5f7c4/examples/intrinsics.rs#L383

…balloc) Related: rust-lang#66741 Guarded with `#![feature(default_alloc_error_handler)]` a default `alloc_error_handler` is called, if a custom allocator is used and no other custom `#[alloc_error_handler]` is defined. The panic message does not contain the size anymore, because it would pull in the fmt machinery, which would blow up the code size significantly.

haraldh · 2020-10-02T07:47:12Z

@Amanieu thanks a lot for the link. I thought I messed up some unwind internals, which only show up on some architectures.

Added those stubs. Let's see what is missing with the next bors run.

Amanieu · 2020-10-02T08:33:46Z

@bors r+

bors · 2020-10-02T08:33:47Z

📌 Commit cadd12b has been approved by Amanieu

…_reduced, r=Amanieu Implement Make `handle_alloc_error` default to panic (for no_std + liballoc) Related: rust-lang#66741 Guarded with `#![feature(default_alloc_error_handler)]` a default `alloc_error_handler` is called, if a custom allocator is used and no other custom `#[alloc_error_handler]` is defined.

bors · 2020-10-04T08:56:10Z

⌛ Testing commit cadd12b with merge 0d37dca...

bors · 2020-10-04T11:02:50Z

☀️ Test successful - checks-actions, checks-azure
Approved by: Amanieu
Pushing 0d37dca to master...

bjorn3 · 2020-10-05T07:52:20Z

compiler/rustc_codegen_llvm/src/allocator.rs

@@ -82,4 +88,41 @@ pub(crate) unsafe fn codegen(tcx: TyCtxt<'_>, mods: &mut ModuleLlvm, kind: Alloc
        }
        llvm::LLVMDisposeBuilder(llbuilder);
    }
+
+    // rust alloc error handler


Is there any reason you duplicated this code rather than adding a new entry to ALLOCATOR_METHODS?

@bjorn3

ALLOCATOR_METHODS is also used elsewhere to build other stuff, which I didn't want to clobber

the alloc error handler has a different function signature

the alloc error handler has a different name

@bjorn3 feel free to refactor the stuff without side effects

RalfJung · 2020-10-09T09:04:46Z

library/alloc/src/alloc.rs

+            #[lang = "oom"]
+            fn oom_impl(layout: Layout) -> !;
+        }
+        unsafe { oom_impl(layout) }


Does this mean that, unlike before, the #[alloc_error_handler] is now allowed to unwind? There's no #[rustc_allocator_nounwind] any more.

Hm, but __rust_alloc_error_handler above still has #[rustc_allocator_nounwind]. How is that not UB when it ends up calling __rdl_oom?

Yeah, I blindly copied #[rustc_allocator_nounwind]. Sorry. Remove it, if it is safe.

Oops, or the other way round, I might have missed it.

RalfJung · 2020-10-09T09:08:51Z

library/alloc/src/alloc.rs

+
+    // if there is a `#[alloc_error_handler]`
+    #[rustc_std_internal_symbol]
+    pub unsafe extern "C" fn __rg_oom(size: usize, align: usize) -> ! {


Nothing like this exists for the allocator, right? There the generated __rust_alloc etc symbols directly call the lang item? Looks like this is needed because the argument types differ; is there any plan to get rid of that mismatch (and the resulting two indirections through unknown extern functions)?

Ah no, the __rd functions are just generated by the macro expansion code. That has the advantage that they do not themselves call an unknown extern function.

This can be removed, if compiler/rustc_codegen_llvm/src/allocator.rs calls directly into the lang item, but with the extern "Rust" API and a Layout object reference.

I just didn't know, how to generate a extern "Rust" fn in compiler/rustc_codegen_llvm/src/allocator.rs.

I think the symmetric thing to do (when compared with #[global_allocator]) would be for the #[alloc_error_handler] attribute to generate __rg_oom, but I guess it doesn't really matter.

It is just somewhat dissatisfying that the very same problem is solved three times in slightly different ways with various amounts of code sharing (global allocator, alloc error handler, panic handler).

I think the symmetric thing to do (when compared with #[global_allocator]) would be for the #[alloc_error_handler] attribute to generate __rg_oom, but I guess it doesn't really matter.

It is just somewhat dissatisfying that the very same problem is solved three times in slightly different ways with various amounts of code sharing (global allocator, alloc error handler, panic handler).

I agree, but to implement it efficiently, I lack the expertise to redirect it via the rust ABI.

RalfJung · 2020-10-09T09:13:15Z

library/alloc/src/alloc.rs

+#[doc(hidden)]
+#[allow(unused_attributes)]
+#[unstable(feature = "alloc_internals", issue = "none")]
+pub mod __default_lib_allocator {


I don't understand this module name, given that this is not an allocator, but an alloc-error handler.

rust-highfive assigned nikomatsakis Sep 7, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2020

haraldh mentioned this pull request Sep 7, 2020

Tracking issue for handle_alloc_error defaulting to panic (for no_std + liballoc) #66741

Closed

3 tasks

haraldh force-pushed the default_alloc_error_handler_reduced branch from 962e10b to 6379296 Compare September 7, 2020 16:17

haraldh marked this pull request as draft September 11, 2020 05:53

haraldh force-pushed the default_alloc_error_handler_reduced branch 6 times, most recently from 15e2e25 to 4bca7fa Compare September 11, 2020 09:37

haraldh force-pushed the default_alloc_error_handler_reduced branch 2 times, most recently from d77f320 to 5ac1d78 Compare September 11, 2020 14:33

haraldh marked this pull request as ready for review September 11, 2020 15:04

rust-highfive assigned Amanieu and unassigned nikomatsakis Sep 13, 2020

Amanieu reviewed Sep 13, 2020

View reviewed changes

haraldh force-pushed the default_alloc_error_handler_reduced branch 2 times, most recently from a9efe49 to 8e46ce0 Compare September 14, 2020 08:50

haraldh requested a review from Amanieu September 14, 2020 09:14

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 1, 2020

haraldh force-pushed the default_alloc_error_handler_reduced branch from 7de481e to cadd12b Compare October 2, 2020 07:00

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 2, 2020

Dylan-DPC-zz mentioned this pull request Oct 3, 2020

Rollup of 9 pull requests #77483

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 4, 2020

bors merged commit 0d37dca into rust-lang:master Oct 4, 2020

rustbot added this to the 1.49.0 milestone Oct 4, 2020

This was referenced Oct 4, 2020

Add #[cfg(panic = '...')] #74754

Merged

Give impl Trait in a const fn its own feature gate #77464

Merged

haraldh deleted the default_alloc_error_handler_reduced branch October 4, 2020 12:07

bjorn3 reviewed Oct 5, 2020

View reviewed changes

RalfJung mentioned this pull request Oct 9, 2020

Access to the API from C? hyperium/hyper#2265

Closed

RalfJung reviewed Oct 9, 2020

View reviewed changes

bjorn3 mentioned this pull request Jun 27, 2021

Implement "default_alloc_error_handler" feature rust-lang/rustc_codegen_cranelift#1182

Closed

Implement Make handle_alloc_error default to panic (for no_std + liballoc) #76448

Implement Make handle_alloc_error default to panic (for no_std + liballoc) #76448

Conversation

haraldh commented Sep 7, 2020 • edited

rust-highfive commented Sep 7, 2020

haraldh commented Sep 11, 2020

haraldh commented Sep 11, 2020

haraldh commented Sep 11, 2020

Amanieu commented Sep 11, 2020

haraldh commented Sep 11, 2020

haraldh commented Sep 11, 2020 • edited

jonas-schievink commented Sep 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amanieu commented Sep 14, 2020

bors commented Sep 14, 2020

bors commented Oct 1, 2020

haraldh commented Oct 1, 2020

Amanieu commented Oct 1, 2020

haraldh commented Oct 2, 2020

Amanieu commented Oct 2, 2020

bors commented Oct 2, 2020

bors commented Oct 4, 2020

bors commented Oct 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Oct 9, 2020 • edited

Choose a reason for hiding this comment

RalfJung Oct 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Implement Make `handle_alloc_error` default to panic (for no_std + liballoc) #76448

Implement Make `handle_alloc_error` default to panic (for no_std + liballoc) #76448

haraldh commented Sep 7, 2020 •

edited

haraldh commented Sep 11, 2020 •

edited

RalfJung Oct 9, 2020 •

edited

RalfJung Oct 9, 2020 •

edited