Skip to content
This repository has been archived by the owner on Mar 7, 2021. It is now read-only.

Fixes #18 -- call the kernel's BUG macro when we panic #19

Merged
merged 2 commits into from
May 27, 2018
Merged

Conversation

alex
Copy link
Member

@alex alex commented May 26, 2018

No description provided.

@alex alex changed the title Fixes #18 -- call the kerne's BUG macro when we panic Fixes #18 -- call the kernel's BUG macro when we panic May 26, 2018
@alex alex requested a review from geofft May 27, 2018 03:29
@alex
Copy link
Member Author

alex commented May 27, 2018

You can observe on #17 that we get something approximating a useful error message when this happens.

Copy link
Collaborator

@geofft geofft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If only we could get the kernel unwinder to do C++ demangling! LGTM.

@alex alex merged commit 208c6fe into master May 27, 2018
@alex alex deleted the bug-on-abort branch May 27, 2018 17:25
@geofft
Copy link
Collaborator

geofft commented May 27, 2018

I wasn't convinced why this should work, so I did some digging into what exactly is going on:

libcore and libstd have different panic implementations. libcore's calls the panic_fmt lang item, and only takes a format string and Arguments. libstd's can take either a format string and Arguments or a Box<Any>, and calls __rust_start_panic, which is defined by one of the panic crates (panic_abort or panic_unwind). Also, libstd defines the panic_fmt lang item as entering its panic handling, so that panics from libcore take the expected flow. (The panic_fmt lang item is only used for coordination between libcore and the implementation of panicking; AFAICT it's not used by the compiler and #![no_core] programs won't need it.)

You can see this by looking at the tracebacks for a panic from inside libcore or a panic from panic! from std - the libcore panic has a few more calls on the stack to reach libstd.

The corollary here is that the panic=abort / panic=unwind selector from RFC 1513 doesn't directly do very much for programs without libstd. It is the panic_abort crate that actually aborts, and in particular it calls libc::abort() on UNIX! A program without libstd won't link either panic_abort or panic_unwind and won't generate aborts, unless we tell it to do that in our panic_fmt lang item. However, it also has the purpose of disabling unwinding support in the compiler (not telling LLVM to generate code to handle exceptions, etc.), so you get a smaller binary.

One edge case: per the RFC, if you set panic=abort in any crate in your dependency tree and you use libstd, it will use panic_abort. But you need to set panic=abort in every crate in your dependency tree to avoid setting eh_personality. The intention of the former is to support people like us before we were using cargo xbuild; we could use precompiled libcore with panic_unwind and a dummy eh_personality. You'd need something to resolve libcore's eh_personality relocations against, but it wouldn't get used (cf. rust-lang/rust#32837 (comment)). However, we should now be able to drop eh_personality now that we're recompiling everything (which was the intention of adding panic=abort as a target spec option, rust-lang/rust#36647).

The full spec for panic_fmt (from libcore's declaration, see also libstd's definition) is

#[lang = "panic_fmt"]
extern fn panic_fmt(msg: fmt::Arguments, file: &'static str, line: u32, col: u32) -> !;

We should attempt to print out the message, probably with printk.

Also, at some point we'll also need to add #[unwind] or #[unwind(allowed)] to that prototype to mark it as an extern fn that's allowed to unwind (see rust-lang/rust#46833, and rust-lang/rust#48251 which reverted that temporarily because of a regression with Lua, which uses setjmp/longjmp for its implementation which is also what Windows unwinding uses).

Also I think the right thing is to go back to panic=unwind so that we appropriately drop things, poison mutexes, etc. during unwinding. Having a panic_fmt that doesn't terminate the entire program (i.e., kernel panic for us) under panic=abort seems sort of unsound.

@alex
Copy link
Member Author

alex commented May 27, 2018 via email

geofft added a commit that referenced this pull request May 27, 2018
See #19 (comment)
for why we can do this - basically, since every crate is compiled with
panic=abort (using cargo xbuild), the compiler should not generate any
code that needs a personality function.
geofft added a commit that referenced this pull request May 27, 2018
See #19 (comment)
for why we can do this - basically, since every crate is compiled with
panic=abort (using cargo xbuild), the compiler should not generate any
code that needs a personality function.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants