Implement arithmetic overflow changes #20795

Aatch · 2015-01-09T05:41:40Z

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

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

Adds overflow checking to integer addition, multiplication, and subtraction when `-Z force-overflow-checks` is true, or if `--cfg ndebug` is not passed to the compiler. On overflow, it panics with `arithmatic operation overflowed`. Also adds `overflowing_add`, `overflowing_sub`, and `overflowing_mul` intrinsics for doing unchecked arithmatic. [breaking-change]

Adds a Wrapping<T> wrapper type and a WrappingOps trait for allowing people to explictly opt-in to wrapping integer arithmetic. This commit currently disables overflow checking in the build due to cases of expected overflow triggering the panic.

Many of the core rust libraries have places that rely on integer wrapping behaviour. These places have been altered to use the wrapping_* methods: * core::hash::sip - A number of macros * core::str - The `maximal_suffix` method in `TwoWaySearcher` * rustc::util::nodemap - Implementation of FnvHash * rustc_back::sha2 - A number of macros and other places * rand::isaac - Isaac64Rng, changed to use the Wrapping helper type Some places had "benign" underflow. This is when underflow or overflow occurs, but the unspecified value is not used due to other conditions. * collections::bit::Bitv - underflow when `self.nbits` is zero. * collections::hash::{map,table} - Underflow when searching an empty table. Did cause undefined behaviour in this case due to an out-of-bounds ptr::offset based on the underflowed index. However the resulting pointers would never be read from. * syntax::ext::deriving::encodable - Underflow when calculating the index of the last field in a variant with no fields. These cases were altered to avoid the underflow, often by moving the underflowing operation to a place where underflow could not happen. There was one case that relied on the fact that unsigned arithmetic and two's complement arithmetic are identical with wrapping semantics. This was changed to use the wrapping_* methods. Finally, the calculation of variant discriminants could overflow if the preceeding discriminant was `U64_MAX`. The logic in `rustc::middle::ty` for this was altered to avoid the overflow completely, while the remaining places were changed to use wrapping methods. This is because `rustc::middle::ty::enum_variants` now throws an error when the calculated discriminant value overflows a `u64`. This behaviour can be triggered by the following code: ``` enum Foo { A = U64_MAX, B } ``` This commit also implements the remaining integer operators for Wrapped<T>.

rust-highfive · 2015-01-09T05:41:51Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nick29581 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

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.

sfackler · 2015-01-09T05:47:45Z

src/libcore/intrinsics.rs

+#[cfg(not(stage0))]
+extern "rust-intrinsic" {
+    /// Returns (a + b) mod 2^N, where N is the width of N in bits.
+    pub fn overflowing_add<T>(a: T, b: T) -> T;


Any reason these are overflowing_foo while the methods are wrapping_foo? I think wrapping_foo is a bit more descriptive, fwiw.

Yes, we ought to give them consistent names.

brson · 2015-01-09T05:55:07Z

Thanks @Aatch and @cmr

gereeter · 2015-01-09T14:31:59Z

src/test/run-fail/overflowing-mul.rs

+// error-pattern:thread '<main>' panicked at 'arithmatic operation overflowed'
+
+fn main() {
+    let x = 200u8 + 4u8;


Should this be multiplication?

Probably, yes. Blame copy-paste. (And @cmr, since it's his code :P)

nikomatsakis · 2015-01-10T17:16:12Z

This looks overall really nice. I'll give it a more detailed look soon.

nikomatsakis · 2015-01-13T01:59:30Z

Argh, slipped off my radar! I'm sorry about that. Tomorrow. (Please feel free to ping me.)

nikomatsakis · 2015-01-13T09:53:31Z

src/libcore/num/wrapping.rs

+pub trait WrappingOps {
+    fn wrapping_add(self, rhs: Self) -> Self;
+    fn wrapping_sub(self, rhs: Self) -> Self;
+    fn wrapping_mul(self, rhs: Self) -> Self;


What about wrapping_div and wrapping_mod? (same with the intrinsics)

These two methods only make sense for f32 and f64 (since they will never overflow and already panic for division by 0)… which are not part of the checking scheme.

floats don't panic on div by zero. they return inf.

Err, I surely meant that integers’ division panics and never overflows when dividing.

steveklabnik · 2015-01-31T08:49:43Z

This doesn't currently merge.

nikomatsakis · 2015-02-10T11:18:54Z

Now that the RFC has been accepted, we can rebase, updated, and land this PR. @Aatch are you stll interested in pursuing that? Can you talk to me on IRC (I've looked for you a few times but not found you, bad timing I guess)?

mahkoh · 2015-02-11T12:57:03Z

This patch doesn't address atomic operations.

mahkoh · 2015-02-11T13:05:26Z

More precisely: If overflow caused by fetch_add or fetch_sub is detected the process must be aborted. Panic is not possible because other threads might already have consumed the value.

nikomatsakis · 2015-02-11T20:43:16Z

@mahkoh and I talked briefly about atomics on IRC. The options seem to be:

Just wrap. Document it, perhaps change name of methods.
Abort process as @mahkoh suggests.
Abort the thread, although the observed value may have been seen elsewhere.
Wrap but return some indication of when wrapping occurred.
Fallback to a CAS loop.

Each has its own downsides. The first option is obviously prone to programmer error, particularly if someone is naively using an atomic to achieve some kind of global counter. That said, atomics are fairly specialized, and perhaps it should be expected. (Note: If we don't wrap by default, then clearly we need explicit wrapping options whatever we do.)

The second process is heavy-handed. I am generally reluctant to abort an entire process as it is irrecoverable. However, this is only in debug mode -- but it impacts users who want to use checked overflowing even in optimized code, as aborting the process may not be an option for them.

Aborting the thread is insufficient in general since others may observe the values.

Another option is to wrap but return an enum that indicates if wrapping occurred. This seems to add some overhead and mainly serves to warn users that wrapping can occur, thus encouraging them to design a recovery strategy. Probably not very appealing.

Falling back to a CAS loop gives irregular performance similarly may impact users who wish to turn on checked overflow by default (even outside debug mode).

nikomatsakis · 2015-02-12T18:49:17Z

Having given the question of atomics some more thought (and talked it over with @aturon), my feeling is that the best option is simply to clearly document the semantics of AtomicInt and friends as wrapping on overflow. Naturally this is something of a tradeoff. As I see it, using atomic ints and the like is somewhat advanced, and always rather isolated: it is much more reasonable to expect users to be aware of the possibility of overflow in that scenario, as compared to general arithmetic throughout the program (atomics are also much easier to audit). Finally, given that the recovery options are typically either drastic or expensive, there isn't really an appealing alternative.

pnkfelix · 2015-02-18T15:04:57Z

@nikomatsakis maybe we should at least name them AtomicWrappingInt or something. (but I don't know if they're even marked stable at the moment).

pnkfelix · 2015-02-19T11:58:26Z

closing in favor of rebased version here; #22532

@cmr

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

emberian and others added 3 commits January 9, 2015 16:41

rust-highfive assigned nrc Jan 9, 2015

Aatch force-pushed the arith-overflow branch from 9f1512d to 33ba612 Compare January 9, 2015 05:44

Aatch force-pushed the arith-overflow branch from 33ba612 to afc2c16 Compare January 9, 2015 05:46

sfackler reviewed Jan 9, 2015
View reviewed changes

gereeter reviewed Jan 9, 2015
View reviewed changes

nrc assigned nikomatsakis and unassigned nrc Jan 11, 2015

klutzy mentioned this pull request Jan 13, 2015

Numeric traits inconsistent with integer type system #21069

Closed

nikomatsakis reviewed Jan 13, 2015
View reviewed changes

mkaufmann mentioned this pull request Jan 17, 2015

Integer overflow rust-lang/rfcs#560

Merged

glaebhoerl mentioned this pull request Feb 12, 2015

Tracking issue for Integer Overflow (RFC 560) #22020

Closed

15 tasks

pnkfelix mentioned this pull request Feb 19, 2015

constant evaluation should not mask overflow for cases that are otherwise checked #22531

Closed

pnkfelix closed this Feb 19, 2015

pnkfelix mentioned this pull request Feb 19, 2015

Implement arithmetic overflow changes #22532

Merged

pnkfelix mentioned this pull request Mar 4, 2015

Integer overflow RFC needs more wrapper types rust-lang/rfcs#928

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement arithmetic overflow changes #20795

Implement arithmetic overflow changes #20795

Aatch commented Jan 9, 2015

rust-highfive commented Jan 9, 2015

sfackler Jan 9, 2015

nikomatsakis Jan 13, 2015

brson commented Jan 9, 2015

gereeter Jan 9, 2015

Aatch Jan 9, 2015

nikomatsakis commented Jan 10, 2015

nikomatsakis commented Jan 13, 2015

nikomatsakis Jan 13, 2015

nagisa Jan 14, 2015

oli-obk Feb 19, 2015

nagisa Feb 19, 2015

steveklabnik commented Jan 31, 2015

nikomatsakis commented Feb 10, 2015

mahkoh commented Feb 11, 2015

mahkoh commented Feb 11, 2015

nikomatsakis commented Feb 11, 2015

nikomatsakis commented Feb 12, 2015

pnkfelix commented Feb 18, 2015

pnkfelix commented Feb 19, 2015

Implement arithmetic overflow changes #20795

Implement arithmetic overflow changes #20795

Conversation

Aatch commented Jan 9, 2015

rust-highfive commented Jan 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brson commented Jan 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Jan 10, 2015

nikomatsakis commented Jan 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklabnik commented Jan 31, 2015

nikomatsakis commented Feb 10, 2015

mahkoh commented Feb 11, 2015

mahkoh commented Feb 11, 2015

nikomatsakis commented Feb 11, 2015

nikomatsakis commented Feb 12, 2015

pnkfelix commented Feb 18, 2015

pnkfelix commented Feb 19, 2015