Implement arithmetic overflow changes

[pnkfelix]

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]

r? @aturon

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

[pnkfelix]

(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]

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]

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

[pnkfelix]

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]

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]

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]

@pnkfelix that looks like a legitimate bug fix to me

[pnkfelix]

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]

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]

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]

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]

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]

(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]

• 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!

[pnkfelix]

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

[pnkfelix]

@bors p=1 r=pnkfelix,eddyb cefd82f

[pnkfelix]

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]

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

[bors]

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

[pnkfelix]

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]

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]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

[Manishearth]

\o/

[pnkfelix]

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...)