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

[econoplas]

I encountered 'arithmetic operation overflowed' panick when calling std::i32::MIN.abs() and similarly for i8, i16, and i64.

The docs for 1.0.0-beta.5 std::num abs function says:

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

I suspect && self != <$T>::min_value() is needed in the if condition for the block that returns "!self + 1" inabs implementation such as:

        pub fn abs(self) -> $T {
            if self.is_negative() && self != $T::min_value() {
                !self + 1 // wrapping_neg
            } else {
                self
            }
        }

I tried this code:

fn main() {
    println!("{}", (std::i8::MIN).abs());   // Expected i8::MIN
    println!("{}", (std::i16::MIN).abs());  // Expected i16::MIN
    println!("{}", (std::i32::MIN).abs());  // Expected i32::MIN
    println!("{}", (std::i64::MIN).abs());  // Expected i64::MIN
}

I expected to see this happen:
-128
-32768
-2147483648
-9223372036854775808

Instead, this happened:
thread '<main>' panicked at 'arithmetic operation overflowed', /tmp/build/rustc-1.0.0-beta.5/src/libcore/num/mod.rs:509

Meta

rustc --version --verbose:
rustc 1.0.0-dev (built 2015-05-13)
binary: rustc
commit-hash: unknown
commit-date: unknown
build-date: 2015-05-13
host: x86_64-unknown-linux-gnu
release: 1.0.0-dev

$ RUST_BACKTRACE=1 ./try_abs2

thread '<main>' panicked at 'arithmetic operation overflowed', /tmp/build/rustc-1.0.0-beta.5/src/libcore/num/mod.rs:509
stack backtrace:
   1:     0x7f181b0a98f9 - sys::backtrace::write::he573c8167e01081cd4r
   2:     0x7f181b0acab8 - panicking::on_panic::had2e9609e9ec19cdjrw
   3:     0x7f181b0a5f32 - rt::unwind::begin_unwind_inner::h2b37885f8310bdebt6v
   4:     0x7f181b0a61ec - rt::unwind::begin_unwind_fmt::h8504c97e3506491474v
   5:     0x7f181b0ac426 - rust_begin_unwind
   6:     0x7f181b0d5094 - panicking::panic_fmt::hdf7ef67474c35dd3wwy
   7:     0x7f181b0d5014 - panicking::panic::hf43ff458bd7d8b953uy
   8:     0x7f181b0a470d - num::i8::abs::h3652d8ae9a3c4a64Qhc
   9:     0x7f181b0a432b - main::h50fc8aac82f30ec4eaa
  10:     0x7f181b0b0a48 - rust_try_inner
  11:     0x7f181b0b0a35 - rust_try
  12:     0x7f181b0ae20b - rt::lang_start::hed504002616c7361Olw
  13:     0x7f181b0a4b7e - main
  14:     0x7f181a29daf4 - __libc_start_main
  15:     0x7f181b0a41d8 - <unknown>
  16:                0x0 - <unknown>

As a side-node, I tried this with C (gcc 4.8.3) on the same platform (CentOS 7 x86_64) and abs(INT_MIN) returns INT_MIN, and labs(LONG_MIN) returns LONG_MIN in case there is question whether Rust's documented behavior of abs() returning Int::min_value() is consistent with common C/C++ library implementation (and it is).

Disclaimer: This is C code:

   printf("%d %d\n", INT_MIN, abs(INT_MIN));
   printf("%ld %ld\n", LONG_MIN, labs(LONG_MIN));

Output:

    -2147483648 -2147483648
    -9223372036854775808 -9223372036854775808

[alexcrichton]

I believe that the documentation has not been updated since the new overflow semantics landed, and this is otherwise correctly panicking on overflow for debug builds.

I'm going to tag this as A-docs as it sounds like we need to update the docs. Thanks for the report!

[econoplas]

Fair enough. So this is intentional.

Sadly, I am now 0-for-2 on new issue reports today. I guess I don't understand rust as well as I thought yet :-( Thanks for the clarification in both cases, and sorry for the extra noise on the issues list.

I agree a document update would be helpful, possibly even mentioning that that abs(Int::min_value()) will produce an arithmetic overflow panick.

Thanks and keep up the good work!

[pnkfelix]

See also rust-lang/rfcs#1017 for a very relevant discussion / potential API addition.

[econoplas]

Thanks! Very enlightening points of view. This is a sticky issue indeed.

If you will allow me to add my 2 cents as a newbie and potential adopter of Rust in "real life" in the near future.

1. I am impressed with what I have seen of Rust's internal implementation so far. Rust macros rock.
2. If I understand correctly... Rust's philosophy is a departure from the "unsafety" of C/C++ to the "safety" partly thorough compile-time checks? I think the proposal to return an Option is the best alternative to promote the concept of "safety" rather than producing a runtime panick.
3. Runtime check producing a panick & crash for common operations like abs() does not feel very safe for newbies, most of whom are by definition polyglots looking at Rust through the filter of their understanding of other C-like languages but may have possibly dabbled with Haskell or Ocaml. Developers from primarily C/C++/Java background will likely (possibly unknowingly) intuitively expect abs(min_value()) to return min_value(), but not cause a crash. I only accidentally discovered I would have built a timebomb into my app when trying abs(i32::MIN) as part of my Rust learning process.
4. For complex dynamic systems (I am thinking about robot sensor input), the sequence of events that lead to producing the data that causes abs to panick could be difficult to reproduce... especially with limited debugger support on some platforms.
5. Returning an Option would force the developer to explicitly decide (presumably using a match) how they want to handle either Some or None return value, so they aren't surprised by runtime panick.
6. In practice, with an Option return value I would probably implement my own safe_abs helper function returning i32::MIN when abs returns None. I would use safe_abs in 99% of my applications. By the same token, now that I know how abs behaves currently... it is not a big deal to instead just have my safe_abs pre-check if value == min_value() and return i32::MIN, otherwise return abs(value). Either way safe_abs would perform exactly how I would have expected abs to work coming from other language backgrounds, and I don't have to worry about the panick situation anymore. I can always decide when to deal with i32::MIN return result from safe_abs in those parts of my code where it matters.
7. Even without changing abs, I agree that a thorough documentation of abs may be sufficient for 1.0.0 release to avoid tagging abs() as unstable. Assuming developers (especially newbies) read the docs :-O You may want to add the abs panick behavior or the "new overflow semantics" Alex mentioned in general to the FAQ as well. This will be a source of angst for many newbies.
8. Classifying abs as unstable would be a big put-off to new adopters in my opinion. Many will ask... "what do you mean you haven't even decided in rev 1.0.0 how to implement something as simple as abs() yet?"
9. Not sure I am a fan of abs returning an unsigned value for reasons others mentioned. Also I think it muddies up an otherwise elegant & cohesive set of macros y'all are using to implement the i8/16/32/64 variations (kudos to the Rust team on the implementation by the way). Returning unsigned value only makes sense as an "ivory tower" argument, but pragmatically speaking nearly every library I've used (POSIX C, C#, Java, Haskell, Ocaml) returns the same type as the input argument.
10. I can't pass up the obvious Haskell "zen moment", since (as usual) the Haskell prelude says it most succinctly and elegantly...

abs :: a -> a 

[econoplas]

OK... I should have looked at master branch's implementation of abs, which has been improved compared to 1.0.0-beta.5 and in fact should return i32::MIN when I call i32::MIN.abs(). Not sure why it was changed, but master should exhibit the behavior as documented. I am building nightly now so I can test this theory.

So this may not be a documentation issue with 1.0.0-beta.5 after all, but rather something that was changed/fixed subsequently in the master and not back-ported to 1.0.0-beta.5. The proposed change to abs in my original post above (adding some variation of && self != $T::min_value() check) would cause 1.0.0-beta.5 to behave the same way as the master branch without having to backport the entirety of the new wrapping_neg and overflowing_neg implementation into 1.0.0-beta.5, and eliminate the panic for i32::MIN.abs(). And then the documentation would be correct as well.

Here is the code from master to which I am referring:

libcore/num/mod.rs:570

pub fn abs(self) -> $T {
    if self.is_negative() {
        self.wrapping_neg()
    } else {
        self
    }
}

libcore/num/mod.rs:1068

pub fn wrapping_neg(self) -> $T {
    self.overflowing_neg().0
}

libcore/num/wrapping.rs:231

fn overflowing_neg(self) -> ($t, bool) {
    if self == $t::MIN {
        ($t::MIN, true)
    } else {
        (-self, false)
    }
}

What do you think? Am I headed down the right path with my investigation here, or just wasting my time?

Will post my results after I finish my testing with nightly later this evening.

[pnkfelix]

@econoplas I strongly recommend using git blame in these situations...

beta was deliberately switched from using .wrapping_neg to use !x + 1 instead, in #24785.

I think there was probably a mistake somewhere here, but switching it back to wrapping_neg without knowing why it was changed would probably be a further mistake.

History I'm referencing:

• When checked negation landed in master, I deliberately avoiding changing the behavior of abs : Check for overflow in arithmetic negation #24500
• Checked negation was then backported to beta, keeping the same semantics (where abs would not panic, but instead was the identity on int::MIN: Cherry-picking beta-nominated in to beta #24708
• But then Fix fallout from wrapping_neg revert #24785 came in and got rid of the wrapping_neg invocation. That PR unfortunately lacks a description explaining the motivation, and I cannot remember the situation offhand.

cc @brson @alexcrichton

[alexcrichton]

@pnkfelix the wrapping_neg function does not exist in the beta branch, so this sounds like it's a bug in beta (not using wrapping_add). If we want this function to explicitly not panic (which is fine by me), then it sounds like this was just a mistake in backporting.

Overall it sounds like the behavior on nightly is as intended (using wrapping semantics and not panicking), and the behavior on beta is a mistake because it's using + 1 instead of .wrapping_add(1). At this point it's too late to backport to beta, so I'm going to close this as "fixed" :)

Sorry for giving you the runaround @econoplas!

[econoplas]

Not a problem. Thanks for looking into this and listening to my noobie feedback! And for not laughing at my ignorance :-)

Confirmed that the code from my original post above works with no panic with 1.1.0-dev nightly 20150513.

I am OK with closing this without resolving the strange abs behavior in 1.0.0.

But I should mention what started me down this path to begin with was my 1.0.0-beta.5 build crashed in abs on CentOS 6.5 with gcc 4.9.2 which I built myself from source. I chalked it up to some bone-head mistake on my part.

But that also led me to investigate why abs was misbehaving and submitting this issue as a result. I will try to reproduce on CentOS 6.5 again and maybe file a separate issue.

My concern is that if abs is possibly used in other library code (such as in hashmap), then user programs might experience a crash if they insert an item whose hash value evaluates to i32::MIN into a hashmap. I would need to read a bunch of code or try to come up with test cases for various containers to see if I can trigger this behavior.

Knowing what I know now about the issue submittal process, I wish I would have kept the stack trace from my failed 1.0.0-beta.5 compile on CentOS 6.5. I will try to reproduce the build crashing in abs again.

Thanks again!

[alexcrichton]

@econoplas we actually discussed this a bit more at triage yesterday and the conclusion was that we may actually want i32::min_value().abs() to check for overflow in debug mode, so I've opened #25441 and added a summary there as well.

[econoplas]

Thanks for the update! I tracked down the place in the 1.0.0 code where i32::MIN::abs() was causing compilation from source to panic compiling stage1 libcore, and verified it still occurs with the 1.0.0 release.

See #25492 which includes my analysis as well as a patch.

I also spent a couple of hours searching for all possible calls to MIN::abs() and reading those sections of code, and this was the only place I could find where MIN::abs() is explicitly called.

The thought being that if MIN::abs() in 1.0.0 is still vulnerable to panic behaviour, then someone should look at all calls to abs() in the code base to see if any obvious panics will occur.

Hopefully I didn't miss any :-)