Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abort by default v2 #1765

Closed
wants to merge 5 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
99 changes: 99 additions & 0 deletions text/0000-abort-by-default.md
@@ -0,0 +1,99 @@
- Feature Name: abort_by_default
- Start Date: 2016-10-01
- RFC PR: (leave this empty)
- Rust Issue: (leave this empty)

# Summary
[summary]: #summary

Specify abort-by-default in `Cargo.toml` when the user does `cargo new --bin`, as well as various other refinements to the panick strategy system.

# Motivation
[motivation]: #motivation

## Performance

Generally, the performance of bigger programs [improves by 10%](https://www.youtube.com/watch?v=mRGb4hoGuPs), which is no small amount. When overflow checking is enabled, it is as high as 2x, making overflowing checking plausible in production.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might not be true. I watched the video, and I believe that Alex Crichton may have misspoke. At 10:34, he says:

If you don't emit [landing pads] ... you can actually get 10% faster compiles as well as 10% faster binaries.

[emphasis added]

However, the slide behind him says:

  • 10% faster compiles
  • 10% smaller binaries

[emphasis added]

I'm not sure how not emiting landing pads would have that significant of an impact on performance. This performance claim may be the result of a misunderstanding.

Copy link
Member

Choose a reason for hiding this comment

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

Oops, I believe I did indeed misspeak! I've never personally at least seen a runtime improvement from panic=abort

Copy link
Member

@eddyb eddyb Oct 7, 2016

Choose a reason for hiding this comment

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

The folk lore around this (going back to @thestinger aka strcat) would suggest that the only runtime performance you would see is from optimizations that wouldn't fire with the landing pads there, mostly due to the landing pad accessing memory, i.e. unwind drop code preventing simplification of the non-unwind variable usage.
Another example would be inlining, where a function might be too large to inline due to landing pads.

It would be cool to have a tool that could automate comparing the two, but we don't atm.
We used to have -Zno-landing-pads while building stage0 libraries, but IIRC that was mostly to speed up compile times, as LLVM takes longer to optimize code with unwinding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not a folklore. I can get a 10-15% improvement in [redox-os/ralloc].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what @alexcrichton mistakenly said is consistent with my experience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait and I'll make some benchmarks.

Copy link
Member

Choose a reason for hiding this comment

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

https://www.reddit.com/r/rust/comments/55ns2m/safe_and_efficient_bidirectional_trees/ had a 20% cpu time improvement by disabling landing pads. There were two ways to reach that goal. One, to use the -Z option, second to eliminate all panics in the actual code.

Copy link
Member

Choose a reason for hiding this comment

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


The performance gains mainly originates in the fact that it is a lot easier for the compiler to optimize, due to the unwind path disappearing, and hence reasoning about the code becomes easier.

## Binary size

Binary size improves by 10% as well. This is due to the lack of landing pads which are injected in the stack when unwinding is enabled.

## Compile time

Compile time in debug mode improves by 20% on average. In release mode, the number is around 10%.

## Runtime size

Unwinding requires a fair amount of runtime code, which can be seen as conflicting with the goals of Rust.

## Correctness

You often see people abusing `std::panic::catch_unwind` for exception handling. Forcing the user to explicitly opt in to unwinding will make it harder to unintentionally misuse it.

# Detailed design
[design]: #detailed-design

First of all, we add a new possible value to the `panic` field. We call this `any`. It specifies that the library or binary is compatible with any panicking strategy. `any` will default to the `abort` strategy, if compatible with all of its dependencies. If not, it will fall back to `unwind`, and leave a warning.

When `cargo new` is invoked, it will generate `panic=any` in the `Cargo.toml`, both for new libraries and new binaries.

Whenever the user inteacts with `cargo` (by using a command) in an existing (binary) crate and the `Cargo.toml` has no specified panicking strategy, it will add `panic=unwind` and leave a note to the user:
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: s/inteacts/interacts/


Warning: No panic strategy was specified, added unwinding to the
`Cargo.toml` file, please consider change it to your needs. If
your crate's behavior does not depend on unwinding, please add
`panic=any` instead.

This will not happen to libraries, since they must only rely on unwind, if the specify it. Instead, it will add `panic=any` to libraries and give warning:

Warning: No panic strategy was specified, so we default to aborting. If
Copy link
Member

Choose a reason for hiding this comment

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

isn't this message misleading? If I understand correctly, its not that we default to aborting (at least from the POV of the library) -- its rather that we delay the decision to the client binary crate (and then there we would default to aborting).

Perhaps you just mean "No panic strategy was specified, which means you may require support for aborting"?

  • But then it seems maybe we should already be saying that anyway

your crate depends on unwinding, please put `panic=unwind` in
`Cargo.toml`.

## Libraries

For libraries, the `Cargo.toml` is not modified, as they inherit the strategy of the binary.

Choose a reason for hiding this comment

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

This conflicts with the above, which says a default of any will be added.


### Relying on unwinding

If a library specifies `panic=unwind`, it will stored in a rlib metadata field, `unwind_needed`. If this field does not match with the crate which is linking against the library (`abort`), `rustc` will produce an error.

This is done in order to make sure that applications can rely on unwinding without leading to unsafety when being linked against by an aborting runtime.

## Extensions

After several release cycles, an extension could be added, which makes specifying the strategy mandatory.

# Drawbacks
[drawbacks]: #drawbacks

It makes panics global by default (i.e., panicking in some thread will abort the whole program).

It might make it tempting to ignore the existence of an unwind option and consequently shaping the code after panicking being aborting.

## Unwinding is not bad per se

Unwinding has numerous advantages. Especially for certain classes of applications. These includes better error handling and better cleanup.

Unwinding is especially important to long-running applications.

# Alternatives
[alternatives]: #alternatives

Keep unwinding as default.

Make Cargo set `panic=abort` in all new binaries.

Choose a reason for hiding this comment

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

This is no longer an alternative but the main proposal.


Use unwind in `debug` and abort in `release`.

Make use of more exotic instructions like `bound`, allowing e.g. overflow or bound checking without branches. This comes at the expense of error output.

Copy link
Member

Choose a reason for hiding this comment

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

I remember seeing stack maps discussed as an alternative, can someone speak to that? I don't know much about whether those are beneficial here/related to this....

Use crate attributes instead.

# Unresolved questions
[unresolved]: #unresolved-questions

Is there any way we can detect crates which do not rely on unwinding? Search for `catch_unwind`?