Implement arithmetic overflow changes #22532

Merged
merged 26 commits into from Mar 3, 2015

Conversation

Projects
None yet
@pnkfelix
Member

pnkfelix commented Feb 19, 2015

Rebase and follow-through on work done by @cmr and @Aatch.

Implements most of rust-lang/rfcs#560. Errors encountered from the checks during building were fixed.

The checks for division, remainder and bit-shifting have not been implemented yet.

See also PR #20795, as well as the tracking issue #22020

cc @Aatch ; cc @nikomatsakis

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Feb 19, 2015

Collaborator

r? @aturon

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

Collaborator

rust-highfive commented Feb 19, 2015

r? @aturon

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

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Feb 19, 2015

Member

(I'm actually still running make check-stage1 && make check-stage2 right now; I'll be reporting back when they finish. Also, not necessarily all of the review notes from #20795 were addressed; I will go back over that PR and see which notes are still relevant.)

Member

pnkfelix commented Feb 19, 2015

(I'm actually still running make check-stage1 && make check-stage2 right now; I'll be reporting back when they finish. Also, not necessarily all of the review notes from #20795 were addressed; I will go back over that PR and see which notes are still relevant.)

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Feb 19, 2015

Member

current PR gets to run: x86_64-apple-darwin/stage1/test/stdtest-x86_64-apple-darwin and then hits overflow panics in collections::hash::map::test_map, num::strconv::tests, num::tests, rand::test, u16::tests, u32::tests, u64::tests, u8::tests, uint::tests, usize::tests. On skimming the names of tests that fail, most overflows seem likely intentional and easily addressed.

Member

pnkfelix commented Feb 19, 2015

current PR gets to run: x86_64-apple-darwin/stage1/test/stdtest-x86_64-apple-darwin and then hits overflow panics in collections::hash::map::test_map, num::strconv::tests, num::tests, rand::test, u16::tests, u32::tests, u64::tests, u8::tests, uint::tests, usize::tests. On skimming the names of tests that fail, most overflows seem likely intentional and easily addressed.

@pnkfelix pnkfelix referenced this pull request Feb 19, 2015

Closed

Tracking issue for Integer Overflow (RFC 560) #22020

11 of 15 tasks complete
@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Feb 19, 2015

Member

okay, now stdtest finshes, and the next problem we hit is in libserialize tests: json::tests::test_read_number

Member

pnkfelix commented Feb 19, 2015

okay, now stdtest finshes, and the next problem we hit is in libserialize tests: json::tests::test_read_number

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Feb 19, 2015

Member

Now stdtest and serializetest both finish, and we hit the big one: collectionstest!:

failures:
    btree::map::test::test_basic_large
    btree::map::test::test_iter_mixed
    btree::map::test::test_range
    btree::map::test::test_range_1000
    btree::map::test::test_range_small
    ring_buf::tests::test_clone
    ring_buf::tests::test_insert
    ring_buf::tests::test_mut_rev_iter
    ring_buf::tests::test_mut_rev_iter_wrap
    ring_buf::tests::test_param_int
    ring_buf::tests::test_param_reccy
    ring_buf::tests::test_param_taggy
    ring_buf::tests::test_param_taggypar
    ring_buf::tests::test_remove
    ring_buf::tests::test_rev_iter
    ring_buf::tests::test_simple
    ring_buf::tests::test_split_off
    ring_buf::tests::test_swap_front_back_remove
    slice::tests::test_element_swaps
    slice::tests::test_permutations

(looking)

Member

pnkfelix commented Feb 19, 2015

Now stdtest and serializetest both finish, and we hit the big one: collectionstest!:

failures:
    btree::map::test::test_basic_large
    btree::map::test::test_iter_mixed
    btree::map::test::test_range
    btree::map::test::test_range_1000
    btree::map::test::test_range_small
    ring_buf::tests::test_clone
    ring_buf::tests::test_insert
    ring_buf::tests::test_mut_rev_iter
    ring_buf::tests::test_mut_rev_iter_wrap
    ring_buf::tests::test_param_int
    ring_buf::tests::test_param_reccy
    ring_buf::tests::test_param_taggy
    ring_buf::tests::test_param_taggypar
    ring_buf::tests::test_remove
    ring_buf::tests::test_rev_iter
    ring_buf::tests::test_simple
    ring_buf::tests::test_split_off
    ring_buf::tests::test_swap_front_back_remove
    slice::tests::test_element_swaps
    slice::tests::test_permutations

(looking)

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Feb 19, 2015

Member

Addressed ring_buf, slice, and follow-on problems in librand.

The btree problem above seems like it might have been a legitimate bug (it seems like the size_hint in an iterator gets corrupted, and the tests are not using corrupted data), but I cannot seem to expose this problem in the playpen, so either its been fixed in the meantime, or my analysis/hypothesis is wrong. Anyway, I have not checked in my local workaround for btree, because it bears further investigation.

Next up:

run: x86_64-apple-darwin/stage1/test/coretesttest-x86_64-apple-darwin
...
failures:
    iter::test_random_access_rev
    num::test::test_int_from_str_overflow
Member

pnkfelix commented Feb 19, 2015

Addressed ring_buf, slice, and follow-on problems in librand.

The btree problem above seems like it might have been a legitimate bug (it seems like the size_hint in an iterator gets corrupted, and the tests are not using corrupted data), but I cannot seem to expose this problem in the playpen, so either its been fixed in the meantime, or my analysis/hypothesis is wrong. Anyway, I have not checked in my local workaround for btree, because it bears further investigation.

Next up:

run: x86_64-apple-darwin/stage1/test/coretesttest-x86_64-apple-darwin
...
failures:
    iter::test_random_access_rev
    num::test::test_int_from_str_overflow
@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Feb 20, 2015

Member

I think I've addressed the two problems above, but now I need to rebuild core.

Q: Was this a bug in iter ? (Or am I injecting one?) I cannot tell from the contract of RandomAccessIter whether the underflow wrap-around means that it should always be returning None in this case, or if some iterators (e.g. ones that repeat infinitely) would/should attempt to return a valid Some(thing) for a usize::MAX index. (My personal guess is that this is a legitimate bug fix.)

--- INDEX:/src/libcore/iter.rs
+++ WORKDIR:/src/libcore/iter.rs
@@ -1127,7 +1127,11 @@
 #[unstable(feature = "core", reason = "trait is experimental")]
 impl<I> RandomAccessIterator for Rev<I> where I: DoubleEndedIterator + RandomAccessIterator {
     #[inline]
     fn indexable(&self) -> usize { self.iter.indexable() }
     #[inline]
     fn idx(&mut self, index: usize) -> Option<<I as Iterator>::Item> {
         let amt = self.indexable();
-        self.iter.idx(amt - index - 1)
+        if amt > index {
+            self.iter.idx(amt - index - 1)
+        } else {
+            None
+        }
     }
 }

Member

pnkfelix commented Feb 20, 2015

I think I've addressed the two problems above, but now I need to rebuild core.

Q: Was this a bug in iter ? (Or am I injecting one?) I cannot tell from the contract of RandomAccessIter whether the underflow wrap-around means that it should always be returning None in this case, or if some iterators (e.g. ones that repeat infinitely) would/should attempt to return a valid Some(thing) for a usize::MAX index. (My personal guess is that this is a legitimate bug fix.)

--- INDEX:/src/libcore/iter.rs
+++ WORKDIR:/src/libcore/iter.rs
@@ -1127,7 +1127,11 @@
 #[unstable(feature = "core", reason = "trait is experimental")]
 impl<I> RandomAccessIterator for Rev<I> where I: DoubleEndedIterator + RandomAccessIterator {
     #[inline]
     fn indexable(&self) -> usize { self.iter.indexable() }
     #[inline]
     fn idx(&mut self, index: usize) -> Option<<I as Iterator>::Item> {
         let amt = self.indexable();
-        self.iter.idx(amt - index - 1)
+        if amt > index {
+            self.iter.idx(amt - index - 1)
+        } else {
+            None
+        }
     }
 }

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Feb 20, 2015

Member

@pnkfelix that looks like a legitimate bug fix to me

Member

alexcrichton commented Feb 20, 2015

@pnkfelix that looks like a legitimate bug fix to me

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Feb 20, 2015

Member

Okay, libcoretest works now (though I still need to look into the aforementioned btree issue).

Now I get to:

run doc-crate-std [x86_64-apple-darwin]
...
failures:
    rand::random_0
Member

pnkfelix commented Feb 20, 2015

Okay, libcoretest works now (though I still need to look into the aforementioned btree issue).

Now I get to:

run doc-crate-std [x86_64-apple-darwin]
...
failures:
    rand::random_0
@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Feb 20, 2015

Member

Well, this is a funny one; it obviously might overflow (about half the time, right?):

/// ```
/// use std::rand;
///
/// let x = rand::random();
/// println!("{}", 2u8 * x);
///
/// let y = rand::random::<f64>();
/// println!("{}", y);
///
/// if rand::random() { // generates a boolean
///     println!("Better lucky than good!");
/// }
/// ```
Member

pnkfelix commented Feb 20, 2015

Well, this is a funny one; it obviously might overflow (about half the time, right?):

/// ```
/// use std::rand;
///
/// let x = rand::random();
/// println!("{}", 2u8 * x);
///
/// let y = rand::random::<f64>();
/// println!("{}", y);
///
/// if rand::random() { // generates a boolean
///     println!("Better lucky than good!");
/// }
/// ```
@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Feb 20, 2015

Member

Current state now:

run rpass [x86_64-apple-darwin]: x86_64-apple-darwin/stage2/bin/compiletest
...
failures:
    [run-pass] run-pass/enum-discr.rs
    [run-pass] run-pass/issue-14021.rs

Update: oh, sweet, we've started hitting cases where rustc itself is causing the overflow exception! I have been looking forward to these. :)

Member

pnkfelix commented Feb 20, 2015

Current state now:

run rpass [x86_64-apple-darwin]: x86_64-apple-darwin/stage2/bin/compiletest
...
failures:
    [run-pass] run-pass/enum-discr.rs
    [run-pass] run-pass/issue-14021.rs

Update: oh, sweet, we've started hitting cases where rustc itself is causing the overflow exception! I have been looking forward to these. :)

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Feb 20, 2015

Member

Okay, we get past run-pass now. (Which was ... easier than I expected. I wonder if there are no run-pass tests that check handling of constant expressions that overflow...)

Next, compile-fail, we do surprisingly well there too (probably too well, again I say):

run cfail [x86_64-apple-darwin]: x86_64-apple-darwin/stage2/bin/compiletest
[...]
failures:
    [compile-fail] compile-fail/lifetime-elision-return-type-requires-explicit-lifetime.rs

test result: FAILED. 1583 passed; 1 failed; 20 ignored; 0 measured
Member

pnkfelix commented Feb 20, 2015

Okay, we get past run-pass now. (Which was ... easier than I expected. I wonder if there are no run-pass tests that check handling of constant expressions that overflow...)

Next, compile-fail, we do surprisingly well there too (probably too well, again I say):

run cfail [x86_64-apple-darwin]: x86_64-apple-darwin/stage2/bin/compiletest
[...]
failures:
    [compile-fail] compile-fail/lifetime-elision-return-type-requires-explicit-lifetime.rs

test result: FAILED. 1583 passed; 1 failed; 20 ignored; 0 measured
@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Feb 20, 2015

Member

Now we get past compile-fail. Next up, run-make:

run cfail-full [x86_64-apple-darwin]: x86_64-apple-darwin/stage2/bin/compiletest
[...]
maketest: alloc-extern-crates
maketest: allow-non-lint-warnings-cmdline
maketest: allow-warnings-cmdline-stability
maketest: c-dynamic-dylib
maketest: c-dynamic-rlib
maketest: c-link-to-rust-dylib
maketest: c-link-to-rust-staticlib
maketest: c-static-dylib
maketest: c-static-rlib
maketest: cannot-read-embedded-idents
maketest: codegen-options-parsing
maketest: compiler-lookup-paths-2
maketest: compiler-lookup-paths
maketest: crate-data-smoke
maketest: crate-name-priority
maketest: dep-info-spaces
[...]
maketest: save-analysis
maketest: sepcomp-cci-copies

and then rustc presumably hits an internal arithmetic overflow while trying to build that last test.


Update: Actually, I don't know what's going on with this test. It does not seem to be hitting a panic. Maybe I have broken something without realizing it.

Member

pnkfelix commented Feb 20, 2015

Now we get past compile-fail. Next up, run-make:

run cfail-full [x86_64-apple-darwin]: x86_64-apple-darwin/stage2/bin/compiletest
[...]
maketest: alloc-extern-crates
maketest: allow-non-lint-warnings-cmdline
maketest: allow-warnings-cmdline-stability
maketest: c-dynamic-dylib
maketest: c-dynamic-rlib
maketest: c-link-to-rust-dylib
maketest: c-link-to-rust-staticlib
maketest: c-static-dylib
maketest: c-static-rlib
maketest: cannot-read-embedded-idents
maketest: codegen-options-parsing
maketest: compiler-lookup-paths-2
maketest: compiler-lookup-paths
maketest: crate-data-smoke
maketest: crate-name-priority
maketest: dep-info-spaces
[...]
maketest: save-analysis
maketest: sepcomp-cci-copies

and then rustc presumably hits an internal arithmetic overflow while trying to build that last test.


Update: Actually, I don't know what's going on with this test. It does not seem to be hitting a panic. Maybe I have broken something without realizing it.

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Feb 20, 2015

Member

(I can't figure out why the sepcomp tests are failing for me. For now I'm going to skip them, under the assumption that something else i did to the rustc, e.g. stuff to turn off the inlining attribute for debugging, is the cause here and that this will fix itself when I remove those changes and test against a clean build.)

Member

pnkfelix commented Feb 20, 2015

(I can't figure out why the sepcomp tests are failing for me. For now I'm going to skip them, under the assumption that something else i did to the rustc, e.g. stuff to turn off the inlining attribute for debugging, is the cause here and that this will fix itself when I remove those changes and test against a clean build.)

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Feb 22, 2015

Member
  • I figured out why I had not been able to expose an underflow bug in btree: AFAICT, we only attempt to decrement 0usize during the range iterators, which explicitly say that they ignore the size field (by not passing through the fn size_method method to their inner AbsIter the way that the other adapters do). I addressed this by setting the unused size field to usize::MAX instead of 0.
  • I changed constant-expression evaluation to use OverflowingOps methods rather than blind arithmetic, to be able to handle the subsequent overflow cases rather than panicking in the stage2 compiler.
  • I revamped the error-handling during constant-expression evaluation.
  • I found some more sources of over- and underflow, and fixed them. No obvious bugs exposed by this, though I am at best suspicious of our constant evaluatior.
  • I revised a number of places that either ICE'd or issued fatal errors to instead issue a normal error, to allow computation to proceed. This was a little bit too aggressive; the way I did it here causes blowup in the amount of redundant error reporting.

However, all together, these changes appear to let me pass through make check-stage2!

Member

pnkfelix commented Feb 22, 2015

  • I figured out why I had not been able to expose an underflow bug in btree: AFAICT, we only attempt to decrement 0usize during the range iterators, which explicitly say that they ignore the size field (by not passing through the fn size_method method to their inner AbsIter the way that the other adapters do). I addressed this by setting the unused size field to usize::MAX instead of 0.
  • I changed constant-expression evaluation to use OverflowingOps methods rather than blind arithmetic, to be able to handle the subsequent overflow cases rather than panicking in the stage2 compiler.
  • I revamped the error-handling during constant-expression evaluation.
  • I found some more sources of over- and underflow, and fixed them. No obvious bugs exposed by this, though I am at best suspicious of our constant evaluatior.
  • I revised a number of places that either ICE'd or issued fatal errors to instead issue a normal error, to allow computation to proceed. This was a little bit too aggressive; the way I did it here causes blowup in the amount of redundant error reporting.

However, all together, these changes appear to let me pass through make check-stage2!

src/librustc_trans/trans/base.rs
+ let check_overflow = if let Some(v) = tcx.sess.opts.debugging_opts.force_overflow_checks {
+ v
+ } else {
+ !attr::contains_name(&krate.config[], "ndebug")

This comment has been minimized.

@pnkfelix

pnkfelix Feb 24, 2015

Member

(this may need to be changed to be based on optimization level rather than ndebug)

@pnkfelix

pnkfelix Feb 24, 2015

Member

(this may need to be changed to be based on optimization level rather than ndebug)

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Feb 26, 2015

Contributor

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

Contributor

bors commented Feb 26, 2015

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

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Feb 27, 2015

Member

@eddyb points out correctly that while this code is catching (and reporting) overflows of 64-bit integer arithmetic in const_eval.rs, it does not appear to catch e.g. an attempt to const-eval 255u8 + 1u8. I will file such as a separate ticket, since I am guessing that it is a serious P-backcompat-lang wart that we do not want to be stuck with.

Update: Oh, in fact, I already filed this as #22531.

Member

pnkfelix commented Feb 27, 2015

@eddyb points out correctly that while this code is catching (and reporting) overflows of 64-bit integer arithmetic in const_eval.rs, it does not appear to catch e.g. an attempt to const-eval 255u8 + 1u8. I will file such as a separate ticket, since I am guessing that it is a serious P-backcompat-lang wart that we do not want to be stuck with.

Update: Oh, in fact, I already filed this as #22531.

src/librustc_typeck/astconv.rs
+ Err(ref r) => {
+ let subspan =
+ ast_ty.span.lo <= r.span.lo && r.span.hi <= ast_ty.span.hi;
+ span_err!(tcx.sess, ast_ty.span, E0250,

This comment has been minimized.

@pnkfelix

pnkfelix Feb 27, 2015

Member

hmm, I am pretty sure I meant for this to pass in r.span here. (and then you only get the whole ast_ty.span if they are actually disconnected).

@pnkfelix

pnkfelix Feb 27, 2015

Member

hmm, I am pretty sure I meant for this to pass in r.span here. (and then you only get the whole ast_ty.span if they are actually disconnected).

This comment has been minimized.

@pnkfelix

pnkfelix Feb 27, 2015

Member

(clearly I need to add a test for this case. :)

@pnkfelix

pnkfelix Feb 27, 2015

Member

(clearly I need to add a test for this case. :)

src/librustc/middle/const_eval.rs
+ }
+}
+
+fn invalid_op_for_bools(e: &Expr, op: ast::BinOp_) -> ConstEvalErr {

This comment has been minimized.

@eddyb

eddyb Feb 27, 2015

Member

This and all the similar functions below could be replaced by a macro which expands to:

return Err(ConstEvalErr { span: $e.span, kind: ErrKind::$ctor(($args),*) }

(with a no-argument form, of course)

@eddyb

eddyb Feb 27, 2015

Member

This and all the similar functions below could be replaced by a macro which expands to:

return Err(ConstEvalErr { span: $e.span, kind: ErrKind::$ctor(($args),*) }

(with a no-argument form, of course)

src/librustc/middle/const_eval.rs
+ (err @ Err(..), _) |
+ (_, err @ Err(..)) => err,
+
+ _ => Err(misc_binary_op(e)),

This comment has been minimized.

@eddyb

eddyb Feb 27, 2015

Member

It's interesting that we do this, but isn't it worse than wrapping the calls to eval_const_expr_partial for a and b in try!?

@eddyb

eddyb Feb 27, 2015

Member

It's interesting that we do this, but isn't it worse than wrapping the calls to eval_const_expr_partial for a and b in try!?

This comment has been minimized.

@pnkfelix

pnkfelix Feb 27, 2015

Member

uh, you are referring to the catch-all at the end that returns Err(misc_binary_op(e)), right? I had assumed that this is trying to catch e.g. type-mismatches in the two results ... I do not understand what alternative you are suggesting to that here: the two sides could both hand back Ok's.

@pnkfelix

pnkfelix Feb 27, 2015

Member

uh, you are referring to the catch-all at the end that returns Err(misc_binary_op(e)), right? I had assumed that this is trying to catch e.g. type-mismatches in the two results ... I do not understand what alternative you are suggesting to that here: the two sides could both hand back Ok's.

This comment has been minimized.

@pnkfelix

pnkfelix Feb 27, 2015

Member

(Oh, are you instead referring to the (err, _) | (_ err) thing I put in? That could be replaced by try!'s, yes, I think so.)

@pnkfelix

pnkfelix Feb 27, 2015

Member

(Oh, are you instead referring to the (err, _) | (_ err) thing I put in? That could be replaced by try!'s, yes, I think so.)

This comment has been minimized.

@eddyb

eddyb Feb 27, 2015

Member

My bad, I was reading the pattern above wrong, I thought it was only matching (Ok(_), Err(_)) | (Err(_), Ok(_)).
I guess it's equivalent to using try!, so why not do that and get rid of some Ok pattern noise?

@eddyb

eddyb Feb 27, 2015

Member

My bad, I was reading the pattern above wrong, I thought it was only matching (Ok(_), Err(_)) | (Err(_), Ok(_)).
I guess it's equivalent to using try!, so why not do that and get rid of some Ok pattern noise?

This comment has been minimized.

@pnkfelix

pnkfelix Feb 27, 2015

Member

yes I think this is a good idea, I will try it out.

@pnkfelix

pnkfelix Feb 27, 2015

Member

yes I think this is a good idea, I will try it out.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Feb 27, 2015

Member

@pnkfelix When I've tried to adjust the results of operations (or just literals? not sure now) for their type, I ended up killing "literal outside of range" warnings (I used the same mechanism as uses in const_eval, which is much easier than handling it for each possible type in each possible case).
I recall @nikomatsakis agreeing on erroring early in such cases, and I think that with overflow checking finally here, we can start being more aggressive. I would also never infer 256 as an u8, but I'm getting ahead of myself.

Member

eddyb commented Feb 27, 2015

@pnkfelix When I've tried to adjust the results of operations (or just literals? not sure now) for their type, I ended up killing "literal outside of range" warnings (I used the same mechanism as uses in const_eval, which is much easier than handling it for each possible type in each possible case).
I recall @nikomatsakis agreeing on erroring early in such cases, and I think that with overflow checking finally here, we can start being more aggressive. I would also never infer 256 as an u8, but I'm getting ahead of myself.

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Feb 27, 2015

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Mar 1, 2015

Contributor

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

Contributor

bors commented Mar 1, 2015

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

+ _ => signal!(e, MiscCatchAll)
+ };
+
+ Ok(result)

This comment has been minimized.

@eddyb

eddyb Mar 1, 2015

Member

😍 Really liking how this function looks now.

@eddyb

eddyb Mar 1, 2015

Member

😍 Really liking how this function looks now.

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Mar 1, 2015

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Mar 1, 2015

Member

@bors r=pnkfelix,eddyb p=1

Member

pnkfelix commented Mar 1, 2015

@bors r=pnkfelix,eddyb p=1

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Mar 1, 2015

Contributor

🙀 p=1 is not a valid commit SHA. Please try again with 3cfe2f4.

Contributor

bors commented Mar 1, 2015

🙀 p=1 is not a valid commit SHA. Please try again with 3cfe2f4.

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Mar 1, 2015

Member

@bors p=1 r=pnkfelix,eddyb

Member

pnkfelix commented Mar 1, 2015

@bors p=1 r=pnkfelix,eddyb

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Mar 1, 2015

Contributor

@bors r=pnkfelix 3cfe2f4

Contributor

bors commented Mar 1, 2015

@bors r=pnkfelix 3cfe2f4

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Mar 1, 2015

Contributor

⌛️ Testing commit 3cfe2f4 with merge 4850180...

Contributor

bors commented Mar 1, 2015

⌛️ Testing commit 3cfe2f4 with merge 4850180...

bors added a commit that referenced this pull request Mar 1, 2015

Auto merge of #22532 - pnkfelix:arith-overflow, r=pnkfelix
Implements most of rust-lang/rfcs#560. Errors encountered from the checks during building were fixed.

The checks for division, remainder and bit-shifting have not been implemented yet.

cc @Aatch ; cc @nikomatsakis
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Mar 1, 2015

Contributor

💔 Test failed - auto-linux-64-nopt-t

Contributor

bors commented Mar 1, 2015

💔 Test failed - auto-linux-64-nopt-t

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Mar 1, 2015

Member

(I want to give this a whirl on a dedicated linux machine; it is an invasive enough change that I should try to go through make check-stage2 on the big three (mac/linux/windows) as best I can.)

Member

pnkfelix commented Mar 1, 2015

(I want to give this a whirl on a dedicated linux machine; it is an invasive enough change that I should try to go through make check-stage2 on the big three (mac/linux/windows) as best I can.)

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Mar 1, 2015

Member

I've initiated a build on a linux machine (check-stage1 for now). Might take a while, this particular machine is behind on llvm and the snapshot.

Member

Manishearth commented Mar 1, 2015

I've initiated a build on a linux machine (check-stage1 for now). Might take a while, this particular machine is behind on llvm and the snapshot.

Aatch and others added some commits Jan 9, 2015

Implement parse_opt_bool better
During my clean-up of rebase errors, I took the opportunity to implement
parse_opt_bool so that it isn't identical to parse_bool wrapped in
`Some`.

parse_opt_bool considers no value to be true, a value of 'y', 'yes' or
'on' to be true and 'n', 'no' or 'off' to be false. All other values are
an error.
Added `OverflowingOps` trait to core::num::wrapping.
These return the result of the operation *plus* an overflow/underflow bit.

This can make it easier to write operations where you want to chain
some arithmetic together, but also want to return a flag signalling if
overflow every occurred.
Fixes to collections to accommodate arith-overflow changes.
* `collections::btree::node`: accommodate (transient) underflow.

* `collections::btree::map`: avoid underflow during `fn next`
  for `BTreeMap::range` methods.

* `collections::slice`: note that pnkfelix deliberately used
  `new_pos_wrapping` only once; the other cases of arithmetic do not
  over- nor underflow, which is a useful property to leave implicitly
  checked/documented via the remaining calls to `fn new_pos(..)`.

* `collections::vec_deque` applied wrapping ops (somewhat blindly)
  to two implementation methods, and many tests.

* `std::collections:#️⃣:table` : Use `OverflowingOps` trait to
  track overflow during `calculate_offsets` and `calculate_allocation`
  functions.
Fix the overflowing tests in run-fail.
* The error patterns had a typo.

* Our current constant evaluation would silently allow the overflow
  (filed as Issue 22531).

* The overflowing-mul test was accidentally doing addition instead of
  multiplication.
Accommodate arith-overflow in `core::num`, `std::num`, `coretest::num`.
 * `core::num`: adjust `UnsignedInt::is_power_of_two`,
   `UnsignedInt::next_power_of_two`, `Int::pow`.

   In particular for `Int::pow`: (1.) do not panic when `base`
   overflows if `acc` never observes the overflowed `base`, and (2.)
   if `acc` does observe the overflowed `base`, make sure we only
   panic if we would have otherwise (e.g. during a computation of
   `base * base`).

 * also in `core::num`: avoid underflow during computation of `uint::MAX`.

 * `std::num`: adjust tests `uint::test_uint_from_str_overflow`,
   `uint::test_uint_to_str_overflow`, `strconv`

 * `coretest::num`: adjust `test::test_int_from_str_overflow`.
Accommodate arith-overflow in `rand` and `std::rand`.
Regarding the `rand` changes: It is unfortunate that Wrapping(T) does
not support the `+=` operator.  We may want to try to fix that before
1.0 to make porting code like this palatable.

Regarding `std::rand`, just arith-overflow in first example from
`std::rand::random()` doc.
`core::iter`: fix bug uncovered by arith-overflow.
(The bug was in `impl RandomAccessIterator for Rev`; it may or may not
have been innocuous, depending on what guarantees one has about the
behavior of `idx` for the underlying iterator.)
Catch arith-overflow explicitly during `rustc::middle::const_eval`.
This only replaces the conditional arith-overflow asserts with
unconditional errors from the guts of const-eval; it does *not*
attempt to sanely handle such errors e.g. with a nice error message
from `rustc`.  So the same test that led me to add this commit are
still failing, and must be addressed.
Avoid fatal errors in astconv; just err and return `ty_err` instead.
This allows computation to proceed and find further errors.

(However, this is also annoying at times when the subsequent errors
are just reporting that a ty_err occurred. I have thoughts on ways to
fix this that I will experiment with separately.)
Address arith-overflow and error-handling in `const_eval.rs`.
 1. Detect and report arithmetic overflow during const-expr eval.

 2. Instead `eval_const_expr_partial` returning `Err(String)`, it now
    has a dedicated enum of different cases. The main benefit of this
    is the ability to pass along an interpretable payload, namely the
    two inputs that caused an overlfow.

I attempted to minimize fallout to error output in tests, but some was
unavoidable. Those changes are in a follow-on commit.
Switch to eval_const_expr_partial in `trans::consts`.
However, I left the ICEs rather than switching to nicer error
reporting there; these cases *should* be detected prior to hitting
trans, and thus ICE'ing here is appropriate.

(So why switch to `eval_const_expr_partial`? Because I want to try to
eliminate all uses of `eval_const_expr` entirely.)
Fix span typo in the arithmetic overflow for array length reporting.
Added pair of regression tests that I should have made when I thought
of doing this in the first place.
@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Mar 3, 2015

Member

@bors p=1 r=pnkfelix,eddyb cefd82f

Member

pnkfelix commented Mar 3, 2015

@bors p=1 r=pnkfelix,eddyb cefd82f

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Mar 3, 2015

Member

I could just change the lines in question to be more robust, but I would prefer to observe the effect locally.

After reviewing the code and thinking it over, I decided the only sane thing to do here would be to act like either over- or underflow could be occurring, and to use saturated arithmetic in both cases. See commit 243c516

Member

pnkfelix commented Mar 3, 2015

I could just change the lines in question to be more robust, but I would prefer to observe the effect locally.

After reviewing the code and thinking it over, I decided the only sane thing to do here would be to act like either over- or underflow could be occurring, and to use saturated arithmetic in both cases. See commit 243c516

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Mar 3, 2015

Member

@bors p=1 r=pnkfelix,eddyb 243c516

Member

pnkfelix commented Mar 3, 2015

@bors p=1 r=pnkfelix,eddyb 243c516

bors added a commit that referenced this pull request Mar 3, 2015

Auto merge of #22532 - pnkfelix:arith-overflow, r=pnkfelix,eddyb
Rebase and follow-through on work done by @cmr and @Aatch.

Implements most of rust-lang/rfcs#560. Errors encountered from the checks during building were fixed.

The checks for division, remainder and bit-shifting have not been implemented yet.

See also PR #20795

cc @Aatch ; cc @nikomatsakis
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Mar 3, 2015

Contributor

⌛️ Testing commit 243c516 with merge 14f0942...

Contributor

bors commented Mar 3, 2015

⌛️ Testing commit 243c516 with merge 14f0942...

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Mar 3, 2015

Member

nuts, it does not reproduce in my VM image. :(

Aha! I have managed to now reproduce it: I had to run make check on a 64-bit VM cross-compiling and running a 32-bit rust binaries!

Member

pnkfelix commented Mar 3, 2015

nuts, it does not reproduce in my VM image. :(

Aha! I have managed to now reproduce it: I had to run make check on a 64-bit VM cross-compiling and running a 32-bit rust binaries!

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Mar 3, 2015

Member

Wait, we run our 32bit stuff on 64bit systems? Not 32bit systems/emulators? I can understand the crosscompile on 64bit, but I thought the builders would shove it into qemu or something for testing.

Member

Manishearth commented Mar 3, 2015

Wait, we run our 32bit stuff on 64bit systems? Not 32bit systems/emulators? I can understand the crosscompile on 64bit, but I thought the builders would shove it into qemu or something for testing.

@bors bors merged commit 243c516 into rust-lang:master Mar 3, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@Manishearth

This comment has been minimized.

Show comment
Hide comment
Member

Manishearth commented Mar 3, 2015

\o/

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Mar 3, 2015

Member

nuts, it does not reproduce in my VM image. :(

Aha! I have managed to now reproduce it: I had to run make check on a 64-bit VM cross-compiling and running a 32-bit rust binaries!

Mystery solved: It appears it was indeed an overflow triggered from this calcuation:

let top_plus_20k = my_stack_top + 20000;

(So, is 64-bit linux just allowing for the stack to start at much higher addresses than one would expect? I assume that it would stop a 32-bit stack pointer from actually overflowing...)

Member

pnkfelix commented Mar 3, 2015

nuts, it does not reproduce in my VM image. :(

Aha! I have managed to now reproduce it: I had to run make check on a 64-bit VM cross-compiling and running a 32-bit rust binaries!

Mystery solved: It appears it was indeed an overflow triggered from this calcuation:

let top_plus_20k = my_stack_top + 20000;

(So, is 64-bit linux just allowing for the stack to start at much higher addresses than one would expect? I assume that it would stop a 32-bit stack pointer from actually overflowing...)

SimonSapin added a commit to SimonSapin/rustc-serialize that referenced this pull request Mar 4, 2015

Fix integer overflow detection.
The previous code only detected overflow to exactly zero (`u64::MAX + 1`).

Tested on `multirust override nightly-2015-03-01`
(before rust-lang/rust#22532)

@aturon aturon referenced this pull request Mar 5, 2015

Closed

Stabilization for 1.0-beta #22500

75 of 91 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment