Closed Bug 1268727 Opened 4 years ago Closed 3 years ago

rust: build with panic=abort

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox49 wontfix, firefox50 wontfix, firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed

People

(Reporter: rillian, Assigned: froydnj)

References

Details

Attachments

(2 files)

Rust has a 'panic!' mechanism for reporting fatal errors, which unwind the stack of the current thread, calling dtors as it goes, until it's caught either by a thread::JoinHandle or a build-in handler which prints an appropriate error message and exits.

Since this is a language-specific feature, unwinding across an ffi interface (from rust into C) is undefined behaviour. The current implementation is using llvm's C++ exception support. Gecko C++ code is built without exceptions, but the linker does recognize it, so in practice what usually happens is the application dies with an 'UnknownException'. Either way this is messy and uncontrolled without good error reporting.

We're rather just crash immediately on panic!(). Doing so removes the ability to use thread isolation for fault recovery, but consensus is this is much better than having to take care at each ffi interface. Rust code should use Result to propagate recoverable errors, and panic!() becomes like MOZ_CRASH().
There's a proposal to support this by adding `-C panic=abort` to rustc. Once this is landed we should test building this way (our code and the rust std library if it's not using panic_handler) and see if it meets our needs.

https://github.com/rust-lang/rust/pull/32900
https://github.com/rust-lang/rfcs/blob/master/text/1513-less-unwinding.md

Longer term, there are further proposals for registering custom panic handlers which could call MOZ_CRASH or other unified code directly. e.g. https://doc.rust-lang.org/stable/std/panic/fn.set_handler.html
> We're rather just crash immediately on panic!(). 

To be concrete, our current idea for how to implement this is:

* On Unix, call libc's abort() function.
* On Windows, execute an illegal instruction (ud2)

It was pointed out [1] that the `MOZ_REALLY_CRASH` macro [2] is what Gecko really wants. The behavior is similar on Unix (modulo storing __LINE__ at NULL), but on Windows it's different. That being said we'd be more than OK to adopt another compatible strategy!

[1]: https://github.com/rust-lang/rfcs/pull/1513#issuecomment-200478548 
[2]: https://mxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#184

My personal recommendation here would be:

1. Compile all Gecko Rust code with `-C panic=abort`
2. Compile the final Rust crate with `-C lto -C panic=abort`. This will not only provide more optimization opportunities, but it will also enable the compiler to strip landing pads from the standard library (e.g., retroactively remove `panic=unwind` codegen). This will improve binary size slightly but otherwise shouldn't have much of an effect.
3. Install a panic hook which calls out to an FFI function that invokes `MOZ_CRASH`.

Doing that you'll avoid any unwinding in Rust, and you'll also have a custom strategy of aborting the process. All of that will also ideally be stable soon as well!
(In reply to Alex Crichton [:acrichto] from comment #2)
> To be concrete, our current idea for how to implement this is:
> 
> * On Unix, call libc's abort() function.
> * On Windows, execute an illegal instruction (ud2)

These should work fine for our purposes, Breakpad will handle both of these. (In fact, on Unix we will override the `abort` symbol to be `mozalloc_abort`, which just calls `MOZ_CRASH`). The only downside on Windows is that the crashes will show up as EXCEPTION_ILLEGAL_INSTRUCTION instead of EXCEPTION_BREAKPOINT.
Depends on: 1289583
Depends on: 1290973
Assignee: nobody → nfroyd
(In reply to Alex Crichton [:acrichto] from comment #2)
> 3. Install a panic hook which calls out to an FFI function that invokes
> `MOZ_CRASH`.

Is this strictly necessary?  It sounds like, from comment 3, that things should Just Work without installing a panic hook.  Is there some benefit to installing our own panic hook, beyond the Windows exception issue mentioned in comment 3?
Flags: needinfo?(ted)
Flags: needinfo?(acrichton)
Ah yes, to clarify, that is not strictly necessary, but it sounded like it may be what Gecko wants to do. If you compile with panic=abort, then you're guarantee that unwinding won't happen, but you're not necessarily guaranteed how an abort is implemented. On Unix right now it's a call to the `abort()` function, but on Windows it's an undefined instruction (`ud2` on x86).

I was under the impression that Gecko wanted to redirect crashes to your own MOZ_CRASH macro to guarantee that all aborts happen the same way as in C++. That way you could also ferry along the filename/line number information to get reported to the crash reporter, I think.

So the tl;dr; is that no, it's not necessary, but for metrics and reporting it may be a "very nice to have". Does that make sense?
Flags: needinfo?(acrichton)
Perfect, thank you!
We need Rust 1.10 to support the -C panic=abort flag and the
corresponding bits in cargo.  We already have the necessary versions installed
on automation.
Attachment #8778993 - Flags: review?(cmanchester)
Attachment #8778994 - Flags: review?(cmanchester)
Attachment #8778993 - Flags: review?(cmanchester) → review+
Attachment #8778994 - Flags: review?(cmanchester) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff7143a7f30f
part 1 - update Rust requirement to 1.10; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/03fbe28e5a10
part 2 - compile Rust code with panic=abort; r=chmanchester
https://hg.mozilla.org/mozilla-central/rev/ff7143a7f30f
https://hg.mozilla.org/mozilla-central/rev/03fbe28e5a10
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
See Also: → 1294090
(In reply to Nathan Froyd [:froydnj] from comment #4)
> (In reply to Alex Crichton [:acrichto] from comment #2)
> > 3. Install a panic hook which calls out to an FFI function that invokes
> > `MOZ_CRASH`.
> 
> Is this strictly necessary?  It sounds like, from comment 3, that things
> should Just Work without installing a panic hook.  Is there some benefit to
> installing our own panic hook, beyond the Windows exception issue mentioned
> in comment 3?

It would be nice to see what these crashes look like in real life. I wonder if we shouldn't add a built-in way to trigger (via Chrome API) a Rust panic for testing? I don't think there's any way we can trigger these from external code otherwise.
Flags: needinfo?(ted)
Maybe stating the obvious, but for feature parity with MOZ_CRASH we'd need panic! to call custom code and provide:
* The formatted panic message (see MOZ_CRASH_ANNOTATE)
* The file/line info, if it's a debug build
I guess we want this to ride the train.
(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #12)
> Maybe stating the obvious, but for feature parity with MOZ_CRASH we'd need
> panic! to call custom code and provide:
> * The formatted panic message (see MOZ_CRASH_ANNOTATE)
> * The file/line info, if it's a debug build

Did this ever happen? (Specifically I'm interested in getting the formatted panic message into crash reports).
Flags: needinfo?(nfroyd)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> (In reply to Jed Davis [:jld] (⏰UTC-6) from comment #12)
> > Maybe stating the obvious, but for feature parity with MOZ_CRASH we'd need
> > panic! to call custom code and provide:
> > * The formatted panic message (see MOZ_CRASH_ANNOTATE)
> > * The file/line info, if it's a debug build
> 
> Did this ever happen? (Specifically I'm interested in getting the formatted
> panic message into crash reports).

I think bug 1275780 (and followups) made this happen.  Please file a bug if you find this is not the case!
Flags: needinfo?(nfroyd)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.