[MIR] Implement overflow checking #33255

Aatch · 2016-04-28T12:15:27Z

Implements overflow checking in the MIR. Also adds checking for x/0, x%0, MIN/-1 and MIN%-1.

Add, Sub, Mul, Shl, and Shr are checked using a new Rvalue: CheckedBinaryOp, while Div, Rem and Neg are handled with explicit checks in the MIR.

Factors out the common pattern across the several places that do arithmetic checks

rust-highfive · 2016-04-28T12:15:38Z

r? @nikomatsakis

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

Aatch · 2016-04-28T12:19:04Z

This currently breaks documentation generation for libc, as it makes the panic lang item required during MIR construction.

/cc @alexcrichton

oli-obk · 2016-04-28T12:38:03Z

src/librustc_mir/build/expr/as_rvalue.rs

+                    ast::IntTy::I64 => ConstInt::I64(std::i64::MIN),
+                    ast::IntTy::Is => {
+                        let int_ty = self.hir.tcx().sess.target.int_type;
+                        let min = match int_ty {


You can generate the correct ConstIsize variant directly, without casting to i64 and going through ConstIsize::new.

let val = match int_ty { ast::IntTy::I32 => ConstIsize::Is32(std::i32::MIN), ast::IntTy::I64 => ConstIsize::Is64(std::i64::MIN), _ => bug!(), }

alexcrichton · 2016-04-28T16:37:05Z

This currently breaks documentation generation for libc, as it makes the panic lang item required during MIR construction.

Ah that's fine, I can fix that at some point upstream for libc

nagisa · 2016-04-28T18:40:28Z

src/librustc_mir/build/block.rs

+        let after = f(self, then_block);
+
+        // If the returned block isn't terminated, add a branch to the "else"
+        // block


What is the purpose of this hack? Does it always result in correct code in case of bugs (when somebody forgets to set a terminator)? If not, then lack of the terminator should just stay a bug, as it currently is.

nagisa · 2016-04-28T19:05:18Z

Have you considered a

BinaryOp(BinOp, Operand, Operand, bool) -trans-to-> (ValueRef, ValueRef)
                                  ^^^^               ^^^^^^^^  ^^^^^^^^ The overflow flag (plain undef in case bool = false)
                                   |                 └ The result value
                                   Whether the operation should use overflow checks

? Well, I guess there isn’t much difference, the current approach also looks fine.

This is simpler to work with than `with_cond`.

This branch shouldn't be hit so if it is, it's probably a mistake.

nagisa · 2016-04-29T11:19:10Z

src/librustc_mir/build/expr/as_rvalue.rs

+            });
+
+            block.and(Rvalue::Use(Operand::Consume(val)))
+        } else {


Doesn’t this branch ignore the check_overflow? Are we checking for division unconditionally in current trans?

To me it seems like it would be cleaner to have something like

if !self.is_overflow_checked() || !op.is_checkable() || !ty.is_integral() { return block.and(Rvalue::BinaryOp(...) }

at the top, and then only do the branching based on the operator elsewhere in this function.

DemiMarie · 2016-04-30T21:30:36Z

Does this generate the LLVM intrinsics or explicit checks?

The LLVM documentation for frontends warns against using the intrinsics unless necessary.

nagisa · 2016-04-30T22:38:29Z

@drbo it does indeed emit intrinsics.

I do not see how one would check for overflow after the fact without intrinsics, and I’m doubtful implementing overflow detection for, say multiplication would be easy without intrinsics or inline assembly. Any ideas?

Aatch · 2016-05-01T00:55:44Z

@drbo that's almost certainly referring to the fact that you shouldn't use the intrinsics unless you actually need to check the result, which is the case here. If we don't check the result, we don't use the intrinsics, it's not like we emit the intrinsics for all arithmetic, just checked arithmetic.

Also, the impact on optimisation isn't a strong factor here, since most of the time chceked arithmetic is used because optimisations haven't been enabled.

nikomatsakis · 2016-05-03T18:06:06Z

src/librustc_mir/build/expr/as_rvalue.rs

+
+                    let (of_block, ok_block) = this.build_cond_br(block, expr_span,
+                                                                  Operand::Consume(is_min));
+                    this.panic(of_block, "attempted to negate with overflow", expr_span);


I imagine we might want some more structured form of terminators here for these special checks (but that needn't derail this PR).

nikomatsakis · 2016-05-03T18:26:33Z

@Aatch ok I reviewed the PR. The code all makes sense and basically looks good to me. I have one question though, I thought you had mentioned some kind of change to MIR to make something analogous to LLVM's "extract-value"? I didn't see anything like that?

Otherwise, it seems like we have to settle the question of what to do about inlined code. I keep going back and forth.

Finally, I suspect we are going to want mildly higher-level terminators that are clearly "automatically inserted" so that things like constant propagation and so forth can recognize them, but I'm not sure about that.

nikomatsakis · 2016-05-03T20:10:00Z

Ah, just saw this comment saying that the codegen part was deferred.

alexcrichton · 2016-05-04T23:50:03Z

The libs team discussed the ramifications of this on the standard library today, and the conclusion was generally positive. We felt that if the standard library wants overflow checks it should write those overflow checks (e.g. with Duration).

@eddyb, could you clarify where this is relied on for pow? That's likely something that we should fix at thie source.

We did have some hesitation, however, at the consideration of this for the Add trait and relevant implementations. @aturon articulated the thought along the lines of if you're writing code that's adding numbers, that code should have overflow checks depending on how the crate is compiled.

I personally interpret that as expecting code like this to panic:

fn add<T: Add>(a: T, b: T) -> T {
    a + b
}

fn main() {
    add(200_u8, 200_u8);
}

I think with this PR, however, it will not panic? That'll get dispatched to the Add implementation in the standard library whose MIR does not have overflow checks?

eddyb · 2016-05-04T23:53:05Z

@alexcrichton pow is specifically written so it behaves like any of the Add or Mul impls for primitives types, i.e. it can trigger an overflow panic if you're using it from debug mode.

arielb1 · 2016-05-05T09:38:28Z

Also, the impact on optimisation isn't a strong factor here, since most of the time chceked arithmetic is used because optimisations haven't been enabled.

Debugopt builds are a thing, you know. I would not expect anyone to run their big testsuite on a non-opt build.

@alexcrichton

The Add impl of primitive types is "short-cutted" during type-checking, so it will work for arithmetic operations. However, other such methods, such as pow aren't.

We may want a #[rustc_inherit_overflow].

alexcrichton · 2016-05-05T16:07:57Z

@eddyb thanks! We should fix that to either always have overflow checks or always have none.

@arielb1

So would the example code snippet I pasted have overflow checks if compiled against an optimized standard library but the code itself was compiled in debug mode?

eddyb · 2016-05-05T16:12:57Z

@alexcrichton Well, the comments indicate that the behavior of pow is quite intentional, so at least some people are bound to see such a "fix" as a "regression".

@arielb1 The "short-cutting" only works when the types are known, though. If trait selection picks the impls in libcore, their method bodies will be used, not the built-in operations.

nikomatsakis · 2016-05-05T18:10:15Z

Comments notwithstanding, I feel like having the ability to have a
function that overflows or not depending on the including crate is not
an existing language feature. As @arielb1 mentioned, we'd want some
kind of attribute or something for that.

On Thu, May 05, 2016 at 09:13:37AM -0700, Eduard-Mihai Burtescu wrote:

@alexcrichton Well, the comments indicate that the behavior of pow is quite intentional, so at least some people are bound to see such a "fix" as a "regression".

@arielb1 The "short-cutting" only works when the types are known, though. If trait selection picks the impls in libcore, their method bodies will be used, not the built-in operations.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#33255 (comment)

bors · 2016-05-08T09:51:53Z

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

eddyb · 2016-05-11T07:49:55Z

@Aatch Have you tried rebasing? Do you want me to do it?

nikomatsakis · 2016-05-17T17:39:52Z

@eddyb what's the story with this branch? I think we achieved a rough consensus that we should maintain existing semantics -- incoherent as they are -- and revisit later?

nikomatsakis · 2016-05-18T14:05:40Z

I'm going to close this PR pending @eddyb's rebase etc.

@Aatch

[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`

Aatch added 3 commits April 28, 2016 23:22

Check arithmetic in the MIR

a50a1c2

Add, Sub, Mul, Shl, and Shr are checked using a new Rvalue: CheckedBinaryOp, while Div, Rem and Neg are handled with explicit checks in the MIR.

Add a with_cond method

196de1a

Factors out the common pattern across the several places that do arithmetic checks

Enable the overflow-related tests for MIR

6d3f7a9

rust-highfive assigned nikomatsakis Apr 28, 2016

oli-obk reviewed Apr 28, 2016
View reviewed changes

nagisa reviewed Apr 28, 2016
View reviewed changes

Aatch added 2 commits April 29, 2016 14:10

Change with_cond to build_cond_br

60a0813

This is simpler to work with than `with_cond`.

Replace the fallback case with a bug!

3d33d46

This branch shouldn't be hit so if it is, it's probably a mistake.

Aatch force-pushed the mir-overflow branch from 9f0d20d to 3d33d46 Compare April 29, 2016 02:35

nagisa reviewed Apr 29, 2016
View reviewed changes

nikomatsakis reviewed May 3, 2016
View reviewed changes

nikomatsakis closed this May 18, 2016

eddyb mentioned this pull request May 27, 2016

[MIR] Implement overflow checking #33905

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MIR] Implement overflow checking #33255

[MIR] Implement overflow checking #33255

Aatch commented Apr 28, 2016

rust-highfive commented Apr 28, 2016

Aatch commented Apr 28, 2016

oli-obk Apr 28, 2016

alexcrichton commented Apr 28, 2016

nagisa Apr 28, 2016 •

edited

Loading

nagisa commented Apr 28, 2016 •

edited

Loading

nagisa Apr 29, 2016 •

edited

Loading

nagisa Apr 29, 2016

DemiMarie commented Apr 30, 2016

nagisa commented Apr 30, 2016

Aatch commented May 1, 2016

nikomatsakis May 3, 2016

nikomatsakis commented May 3, 2016

nikomatsakis commented May 3, 2016

alexcrichton commented May 4, 2016

eddyb commented May 4, 2016

arielb1 commented May 5, 2016

alexcrichton commented May 5, 2016

eddyb commented May 5, 2016

nikomatsakis commented May 5, 2016

bors commented May 8, 2016

eddyb commented May 11, 2016

nikomatsakis commented May 17, 2016

nikomatsakis commented May 18, 2016

[MIR] Implement overflow checking #33255

[MIR] Implement overflow checking #33255

Conversation

Aatch commented Apr 28, 2016

rust-highfive commented Apr 28, 2016

Aatch commented Apr 28, 2016

oli-obk Apr 28, 2016

Choose a reason for hiding this comment

alexcrichton commented Apr 28, 2016

nagisa Apr 28, 2016 • edited Loading

Choose a reason for hiding this comment

nagisa commented Apr 28, 2016 • edited Loading

nagisa Apr 29, 2016 • edited Loading

Choose a reason for hiding this comment

nagisa Apr 29, 2016

Choose a reason for hiding this comment

DemiMarie commented Apr 30, 2016

nagisa commented Apr 30, 2016

Aatch commented May 1, 2016

nikomatsakis May 3, 2016

Choose a reason for hiding this comment

nikomatsakis commented May 3, 2016

nikomatsakis commented May 3, 2016

alexcrichton commented May 4, 2016

eddyb commented May 4, 2016

arielb1 commented May 5, 2016

alexcrichton commented May 5, 2016

eddyb commented May 5, 2016

nikomatsakis commented May 5, 2016

bors commented May 8, 2016

eddyb commented May 11, 2016

nikomatsakis commented May 17, 2016

nikomatsakis commented May 18, 2016

nagisa Apr 28, 2016 •

edited

Loading

nagisa commented Apr 28, 2016 •

edited

Loading

nagisa Apr 29, 2016 •

edited

Loading