Check for overflow in arithmetic negation #24500

pnkfelix · 2015-04-16T14:52:03Z

Add conditional overflow-checking to signed negate operator.

I argue this can land independently of #24420 , because one can write the implementation of wrapped_neg() inline if necessary (as illustrated in two cases on this PR).

[breaking-change]

If you are relying on the prior behavior of negate on Integer::MIN_VALUE, use the wrapping_neg method instead.

This needs to go into beta channel.

rust-highfive · 2015-04-16T14:52:08Z

r? @eddyb

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

pnkfelix · 2015-04-16T14:52:21Z

r? @alexcrichton

pnkfelix · 2015-04-16T14:55:03Z

(oh, but I am going to give this a whirl on try...)

pnkfelix · 2015-04-16T14:55:08Z

@bors try

bors · 2015-04-16T14:55:10Z

⌛ Trying commit f9c6780 with merge 2e51e09...

Add conditional overflow-checking to signed negate operator. I argue this can land independently of #24420 , because one can write the implementation of `wrapped_neg()` inline if necessary (as illustrated in two cases on this PR). This needs to go into beta channel.

bors · 2015-04-16T15:53:04Z

💔 Test failed - try-mac

pnkfelix · 2015-04-17T12:44:57Z

(I'm going to rebase so that I can make use of the methods that landed in #24420 ; after that's done, this should be ready to land.)

nikomatsakis · 2015-04-17T13:38:44Z

src/libcore/num/mod.rs

@@ -1321,7 +1321,11 @@ macro_rules! int_impl {
        #[stable(feature = "rust1", since = "1.0.0")]
        #[inline]
        pub fn abs(self) -> $T {
-            if self.is_negative() { -self } else { self }
+            if self.is_negative() {
+                self.wrapping_neg()


Hmm, I guess this is assuming we want abs(INT_IN) to be INT_MIN? I kind of feel like I want it to be the original def'n -- but I guess this is a question for the RFC as much as anything. Still, given that -self was apparently unchecked before, it feels like we can "change" the behavior here and (legitimately) call it a bug fix.

Never mind, pnkfelix points out the result for min-value is documented.

the docs that lie just out of reach of the diff say:

Int::min_value() will be returned if the number is Int::min_value().

So I was just preserving that. (Plus I think there are tests that check it.)

But yeah, seems like a Q for rust-lang/rfcs#1017

nikomatsakis · 2015-04-17T16:58:06Z

r+ we can settle abs() later

pnkfelix · 2015-04-17T16:58:57Z

@bors r=nikomatsakis

bors · 2015-04-17T16:58:58Z

📌 Commit b8ec7e8 has been approved by nikomatsakis

pnkfelix · 2015-04-17T16:59:17Z

@bors p=1

Add conditional overflow-checking to signed negate operator. I argue this can land independently of #24420 , because one can write the implementation of `wrapped_neg()` inline if necessary (as illustrated in two cases on this PR). This needs to go into beta channel.

bors · 2015-04-17T22:45:11Z

⌛ Testing commit b8ec7e8 with merge b08d6cf...

bors · 2015-04-18T00:48:33Z

☀️ 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

pnkfelix · 2015-04-23T21:47:24Z

going from nominated to (nominated, accepted)

Debug overflow checks for arithmetic negation landed in rust-lang#24500, at which time the `abs` method on signed integers was changed to using `wrapping_neg` to ensure that the function never panicked. This implied that `abs` of `INT_MIN` would return `INT_MIN`, another negative value. When this change was back-ported to beta, however, in rust-lang#24708, the `wrapping_neg` function had not yet been backported, so the implementation was changed in rust-lang#24785 to `!self + 1`. This change had the unintended side effect of enabling debug overflow checks for the `abs` function. Consequently, the current state of affairs is that the beta branch checks for overflow in debug mode for `abs` and the nightly branch does not. This commit alters the behavior of nightly to have `abs` always check for overflow in debug mode. This change is more consistent with the way the standard library treats overflow as well, and it is also not a breaking change as it's what the beta branch currently does (albeit if by accident). cc rust-lang#25378

Debug overflow checks for arithmetic negation landed in #24500, at which time the `abs` method on signed integers was changed to using `wrapping_neg` to ensure that the function never panicked. This implied that `abs` of `INT_MIN` would return `INT_MIN`, another negative value. When this change was back-ported to beta, however, in #24708, the `wrapping_neg` function had not yet been backported, so the implementation was changed in #24785 to `!self + 1`. This change had the unintended side effect of enabling debug overflow checks for the `abs` function. Consequently, the current state of affairs is that the beta branch checks for overflow in debug mode for `abs` and the nightly branch does not. This commit alters the behavior of nightly to have `abs` always check for overflow in debug mode. This change is more consistent with the way the standard library treats overflow as well, and it is also not a breaking change as it's what the beta branch currently does (albeit if by accident). cc #25378

Debug overflow checks for arithmetic negation landed in rust-lang#24500, at which time the `abs` method on signed integers was changed to using `wrapping_neg` to ensure that the function never panicked. This implied that `abs` of `INT_MIN` would return `INT_MIN`, another negative value. When this change was back-ported to beta, however, in rust-lang#24708, the `wrapping_neg` function had not yet been backported, so the implementation was changed in rust-lang#24785 to `!self + 1`. This change had the unintended side effect of enabling debug overflow checks for the `abs` function. Consequently, the current state of affairs is that the beta branch checks for overflow in debug mode for `abs` and the nightly branch does not. This commit alters the behavior of nightly to have `abs` always check for overflow in debug mode. This change is more consistent with the way the standard library treats overflow as well, and it is also not a breaking change as it's what the beta branch currently does (albeit if by accident). cc rust-lang#25378

rust-highfive assigned eddyb Apr 16, 2015

rust-highfive assigned alexcrichton and unassigned eddyb Apr 16, 2015

aturon mentioned this pull request Apr 17, 2015

Tracking issue for Integer Overflow (RFC 560) #22020

Closed

15 tasks

pnkfelix force-pushed the oflo-checked-neg branch from f9c6780 to a8b1776 Compare April 17, 2015 12:41

pnkfelix added 4 commits April 17, 2015 14:45

factor out useful helper.

9a6d7fb

Add conditional overflow-checking to signed negate operator.

c34fa8b

side-step potentially panic'ing negate in fn abs.

e9f892a

Workaround deliberate overflowing negation in serialize::json.

5e7785c

pnkfelix force-pushed the oflo-checked-neg branch from a8b1776 to 5e7785c Compare April 17, 2015 12:55

unit test for checked overflow during signed negation.

b8ec7e8

nikomatsakis reviewed Apr 17, 2015
View reviewed changes

bors merged commit b8ec7e8 into rust-lang:master Apr 18, 2015

pnkfelix mentioned this pull request Apr 18, 2015

PR title in the queue doesn't get updated when PR title changes in the repo barosl/homu#51

Closed

pnkfelix added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 21, 2015

alexcrichton mentioned this pull request Apr 23, 2015

Cherry-picking beta-nominated in to beta #24708

Merged

pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 23, 2015

alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 23, 2015

pnkfelix mentioned this pull request May 14, 2015

std::i32::MIN.abs() results in panicked at 'arithmetic operation overflowed' #25378

Closed

alexcrichton mentioned this pull request May 15, 2015

std: Make abs() panic on overflow in debug mode #25441

Merged

nagisa mentioned this pull request Aug 5, 2015

clarify extreme operator behaviour rust-lang/rfcs#1237

Merged

Check for overflow in arithmetic negation #24500

Check for overflow in arithmetic negation #24500

Uh oh!

Conversation

pnkfelix commented Apr 16, 2015

Uh oh!

rust-highfive commented Apr 16, 2015

Uh oh!

pnkfelix commented Apr 16, 2015

Uh oh!

pnkfelix commented Apr 16, 2015

Uh oh!

pnkfelix commented Apr 16, 2015

Uh oh!

bors commented Apr 16, 2015

Uh oh!

bors commented Apr 16, 2015

Uh oh!

pnkfelix commented Apr 17, 2015

Uh oh!

nikomatsakis Apr 17, 2015

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Apr 17, 2015

Choose a reason for hiding this comment

Uh oh!

pnkfelix Apr 17, 2015

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Apr 17, 2015

Uh oh!

pnkfelix commented Apr 17, 2015

Uh oh!

bors commented Apr 17, 2015

Uh oh!

pnkfelix commented Apr 17, 2015

Uh oh!

bors commented Apr 17, 2015

Uh oh!

bors commented Apr 18, 2015

Uh oh!

pnkfelix commented Apr 23, 2015

Uh oh!

Uh oh!