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

[MIR] Implement overflow checking #33905

Merged
merged 18 commits into from
Jun 5, 2016
Merged

[MIR] Implement overflow checking #33905

merged 18 commits into from
Jun 5, 2016

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented May 27, 2016

The initial set of changes is from @Aatch's #33255 PR, rebased on master, plus:

Added an Assert terminator to MIR, to simplify working with overflow and bounds checks.
With this terminator, error cases can be accounted for directly, instead of looking for lang item calls.
It also keeps the MIR slimmer, with no extra explicit blocks for the actual panic calls.

Warnings can be produced when the Assert is known to always panic at runtime, e.g.:

warning: index out of bounds: the len is 1 but the index is 3
 --> <anon>:1:14
1 |> fn main() { &[std::io::stdout()][3]; }
  |>              ^^^^^^^^^^^^^^^^^^^^^^

Generalized the OperandValue::FatPtr optimization to any aggregate pair of immediates.
This allows us to generate the same IR for overflow checks as old trans, not something worse.
For example, addition on i16 calls llvm.sadd.with.overflow.i16, which returns {i16, i1}.
However, the Rust type (i16, bool), has to be {i16, i8}, only an immediate bool is i1.
But if we split the pair into an i16 and an i1, we can pass them around as such for free.

The latest addition is a rebase of #34054, updated to work for pairs too. Closes #34054, fixes #33873.

Last but not least, the #[rustc_inherit_overflow_checks] attribute was introduced to control the
overflow checking behavior of generic or #[inline] functions, when translated in another crate.

It is not intended to be used by crates other than libcore, which is in the unusual position of
being distributed as only an optimized build with no checks, even when used from debug mode.
Before MIR-based translation, this worked out fine, as the decision for overflow was made at
translation time, in the crate being compiled, but MIR stored in rlib has to contain the checks.

To avoid always generating the checks and slowing everything down, a decision was made to
use an attribute in the few spots of libcore that need it (see #33255 for previous discussion):

  • core::ops::{Add, Sub, Mul, Neg, Shl, Shr} implementations for integers, which have #[inline] methods and can be used in generic abstractions from other crates
  • core::ops::{Add, Sub, Mul, Neg, Shl, Shr}Assign same as above, for augmented assignment
  • pow and abs methods on integers, which intentionally piggy-back on built-in multiplication and negation, respectively, to get overflow checks
  • core::iter::{Iterator, Chain, Peek}::count and core::iter::Enumerate::{next, nth}, also documented as panicking on overflow, from addition, counting elements of an iterator in an usize

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@bluss
Copy link
Member

bluss commented May 27, 2016

#[rustc_inherit_overflow_checks] is needed on RangeFrom::next in libcore/iter/range.rs too

@eddyb
Copy link
Member Author

eddyb commented May 27, 2016

@bluss Those didn't even make it into my mega warning list, because they are generic over Add and Sub so they just get the same behavior everyone else does, via the integer implementations of those traits (which do have #[rustc_inherit_overflow_checks]).

@alexcrichton
Copy link
Member

@eddyb nice! An unstable attribute seems like a reasonable compromise for libcore for now, although we should discuss in libs how to remove the dependency on it pronto :)

@nikomatsakis
Copy link
Contributor

@alexcrichton I think there's a case to be made that we should extend the language with the ability to enable overflow checks for a given module, as well as to make overflow checks "inherited" from the caller.

@nikomatsakis
Copy link
Contributor

@bors retry

@nikomatsakis
Copy link
Contributor

oops, wrong PR :)

@nikomatsakis
Copy link
Contributor

@bors r-

@nikomatsakis
Copy link
Contributor

(just in case)

@@ -853,9 +853,6 @@ pub fn eval_const_expr_partial<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
Integral(U8(data[idx as usize]))
},

Str(ref s) if idx as usize >= s.len() => signal!(e, IndexOutOfBounds),
// FIXME: return a const char
Str(_) => signal!(e, UnimplementedConstVal("indexing into str")),
Copy link
Contributor

Choose a reason for hiding this comment

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

heh :)

@bors
Copy link
Contributor

bors commented Jun 4, 2016

☔ The latest upstream changes (presumably #33803) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

r=me

@bors
Copy link
Contributor

bors commented Jun 5, 2016

☔ The latest upstream changes (presumably #33622) made this pull request unmergeable. Please resolve the merge conflicts.

@eddyb
Copy link
Member Author

eddyb commented Jun 5, 2016

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 5, 2016

📌 Commit c497e87 has been approved by nikomatsakis

@arielb1
Copy link
Contributor

arielb1 commented Jun 5, 2016

The r= is on the wrong commit.

@eddyb
Copy link
Member Author

eddyb commented Jun 5, 2016

@arielb1 No, I just pushed a potential fix, the commit I r='d didn't actually pass the latest test, I'll r= again once I know it works.

@eddyb
Copy link
Member Author

eddyb commented Jun 5, 2016

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 5, 2016

📌 Commit cee244d has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 5, 2016

⌛ Testing commit cee244d with merge 8cbffc5...

bors added a commit that referenced this pull request Jun 5, 2016
[MIR] Implement overflow checking

The initial set of changes is from @Aatch's #33255 PR, rebased on master, plus:

Added an `Assert` terminator to MIR, to simplify working with overflow and bounds checks.
With this terminator, error cases can be accounted for directly, instead of looking for lang item calls.
It also keeps the MIR slimmer, with no extra explicit blocks for the actual panic calls.

Warnings can be produced when the `Assert` is known to always panic at runtime, e.g.:
```rust
warning: index out of bounds: the len is 1 but the index is 3
 --> <anon>:1:14
1 |> fn main() { &[std::io::stdout()][3]; }
  |>              ^^^^^^^^^^^^^^^^^^^^^^
```

Generalized the `OperandValue::FatPtr` optimization to any aggregate pair of immediates.
This allows us to generate the same IR for overflow checks as old trans, not something worse.
For example, addition on `i16` calls `llvm.sadd.with.overflow.i16`, which returns `{i16, i1}`.
However, the Rust type `(i16, bool)`, has to be `{i16, i8}`, only an immediate `bool` is `i1`.
But if we split the pair into an `i16` and an `i1`, we can pass them around as such for free.

The latest addition is a rebase of #34054, updated to work for pairs too. Closes #34054, fixes #33873.

Last but not least, the `#[rustc_inherit_overflow_checks]` attribute was introduced to control the
overflow checking behavior of generic or `#[inline]` functions, when translated in another crate.

It is **not** intended to be used by crates other than `libcore`, which is in the unusual position of
being distributed as only an optimized build with no checks, even when used from debug mode.
Before MIR-based translation, this worked out fine, as the decision for overflow was made at
translation time, in the crate being compiled, but MIR stored in `rlib` has to contain the checks.

To avoid always generating the checks and slowing everything down, a decision was made to
use an attribute in the few spots of `libcore` that need it (see #33255 for previous discussion):
* `core::ops::{Add, Sub, Mul, Neg, Shl, Shr}` implementations for integers, which have `#[inline]` methods and can be used in generic abstractions from other crates
* `core::ops::{Add, Sub, Mul, Neg, Shl, Shr}Assign` same as above, for augmented assignment
* `pow` and `abs` methods on integers, which intentionally piggy-back on built-in multiplication and negation, respectively, to get overflow checks
* `core::iter::{Iterator, Chain, Peek}::count` and `core::iter::Enumerate::{next, nth}`, also documented as panicking on overflow, from addition, counting elements of an iterator in an `usize`
@bors bors merged commit cee244d into rust-lang:master Jun 5, 2016
@eddyb eddyb deleted the mir-overflow branch June 5, 2016 16:15
bors added a commit that referenced this pull request Aug 1, 2016
Switch to MIR-based translation by default.

This patch makes `-Z orbit` default to "on", which means that by default, functions will be translated from Rust to LLVM IR through the upcoming MIR backend, instead of the antiquated AST backend.

This switch is made possible by the recently merged #33622, #33905 and smaller fixes.

If you experience any issues, please file a report for each of them. You can switch to the old backend to work around problems by either setting `RUSTFLAGS="-Zorbit=off"` or by annotating specific functions with `#[rustc_no_mir]` (which requires `#![feature(rustc_attrs)]` at the crate-level).

I would like this PR to get into nightly soon so that we can get early feedback in this release cycle and focus on correctness fixes and performance improvements, with the potential for removing the old backend implementation before beta branches off.

cc @rust-lang/compiler
bors added a commit that referenced this pull request Aug 2, 2016
Switch to MIR-based translation by default.

This patch makes `-Z orbit` default to "on", which means that by default, functions will be translated from Rust to LLVM IR through the upcoming MIR backend, instead of the antiquated AST backend.

This switch is made possible by the recently merged #33622, #33905 and smaller fixes.

If you experience any issues, please file a report for each of them. You can switch to the old backend to work around problems by either setting `RUSTFLAGS="-Zorbit=off"` or by annotating specific functions with `#[rustc_no_mir]` (which requires `#![feature(rustc_attrs)]` at the crate-level).

I would like this PR to get into nightly soon so that we can get early feedback in this release cycle and focus on correctness fixes and performance improvements, with the potential for removing the old backend implementation before beta branches off.

cc @rust-lang/compiler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MIR trans cannot handle casting for call return values.
9 participants