Skip to content

/rust

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from May 19, 2015

Conversation

Reviewers
No reviews
Assignees

@aturon aturon

Labels
T-libs beta-accepted
Projects
None yet
Milestone
No milestone
6 participants
@alexcrichton @rust-highfive @bors @petrochenkov @aturon @brson
@alexcrichton
Copy link
Member

alexcrichton commented May 15, 2015

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

@rust-highfive rust-highfive assigned brson May 15, 2015

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

rust-highfive commented May 15, 2015

r? @brson

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

@alexcrichton alexcrichton referenced this pull request May 15, 2015

Closed

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

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented May 15, 2015

r? @aturon

@rust-highfive rust-highfive assigned aturon and unassigned brson May 15, 2015

src/libcore/num/mod.rs Outdated
@@ -569,7 +569,7 @@ macro_rules! int_impl {
#[inline]
pub fn abs(self) -> $T {
if self.is_negative() {
self.wrapping_neg()
-self

This comment has been minimized.

Sign in to view
Copy link
@aturon

aturon May 18, 2015

Member

Can you leave a comment saying that this has the correct overflow semantics only if kept as inline given how std is distributed? I hope we can do something more robust in the future.

Otherwise r=me.
@alexcrichton
std: Make abs() panic on overflow in debug mode 
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
Loading status checks…
5f39ceb

@alexcrichton alexcrichton force-pushed the alexcrichton:debug-panic-neg branch from 2a05f77 to 5f39ceb May 19, 2015

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented May 19, 2015

@bors: r=aturon 5f39ceb
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented May 19, 2015

⌛️ Testing commit 5f39ceb with merge 8dbc369...

bors added a commit that referenced this pull request May 19, 2015

@bors
Auto merge of #25441 - alexcrichton:debug-panic-neg, r=aturon 
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
Loading status checks…
8dbc369
@bors

@bors bors merged commit 5f39ceb into rust-lang:master May 19, 2015
2 checks passed

2 checks passed

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

@alexcrichton alexcrichton added beta-nominated T-libs labels May 19, 2015

@alexcrichton alexcrichton deleted the alexcrichton:debug-panic-neg branch May 19, 2015

@alexcrichton alexcrichton restored the alexcrichton:debug-panic-neg branch May 19, 2015

@alexcrichton alexcrichton deleted the alexcrichton:debug-panic-neg branch May 19, 2015

@alexcrichton alexcrichton restored the alexcrichton:debug-panic-neg branch May 19, 2015

@alexcrichton alexcrichton deleted the alexcrichton:debug-panic-neg branch May 19, 2015

@petrochenkov

This comment has been minimized.

Sign in to view
Copy link
Contributor

petrochenkov commented May 19, 2015

Hey, it broke the compiler
Edit: I'm fixing it

$ RUST_BACKTRACE=1 make
cfg: version 1.2.0-dev (b301e02f3 2015-05-19) (built 2015-05-19)
cfg: build triple x86_64-pc-windows-gnu
cfg: host triples x86_64-pc-windows-gnu
cfg: target triples x86_64-pc-windows-gnu
cfg: enabling debug assertions (CFG_ENABLE_DEBUG_ASSERTIONS)
cfg: enabling debuginfo (CFG_ENABLE_DEBUGINFO)
cfg: host for x86_64-pc-windows-gnu is x86_64
cfg: os for x86_64-pc-windows-gnu is pc-windows-gnu
cfg: good valgrind for x86_64-pc-windows-gnu is
cfg: using CC=gcc (CFG_CC)
cfg: disabling valgrind run-pass tests
cfg: no xelatex found, disabling LaTeX docs
cfg: no pandoc found, omitting PDF and EPUB docs
rustc: x86_64-pc-windows-gnu/stage1/bin/rustlib/x86_64-pc-windows-gnu/lib/libcore
error: internal compiler error: unexpected panic
note: the compiler unexpectedly panicked. this is a bug.
note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
note: run with `RUST_BACKTRACE=1` for a backtrace
thread 'rustc' panicked at 'attempted to negate with overflow', C:/msys64/home/we/rust/src/libcore\num/mod.rs:581

stack backtrace:
   1:         0x6d2b3fa1 - sys::backtrace::write::h410e8b1c02e6a4cb8fs
   2:         0x6d2bca98 - rt::unwind::register::h1ca1862cef28bdc0xNv
   3:         0x6d284e14 - rt::unwind::begin_unwind_inner::h6525e25cf712ad9fHKv
   4:         0x6d2857f9 - rt::unwind::begin_unwind_fmt::ha2ec3cbf348c0730NJv
   5:         0x6d2bc6a0 - rust_begin_unwind
   6:         0x6d2da032 - panicking::panic_fmt::h3bec9bbe2b23bab6pZB
   7:         0x6d2d2e2f - panicking::panic::hcc1bf6d590d7a6bfWXB
   8:         0x71201f22 - builtin::TypeLimits.LintPass::check_expr::h3cbdea1f95212ea22ea
   9:           0xac16b0 - lint::context::Context<'a, 'tcx>.Visitor<'v>::visit_expr::ha294eff0288554d123s
  10:           0xac1ae6 - lint::context::Context<'a, 'tcx>.Visitor<'v>::visit_expr::ha294eff0288554d123s
  11:           0xac190b - lint::context::Context<'a, 'tcx>.Visitor<'v>::visit_expr::ha294eff0288554d123s
  12:           0xac1ae6 - lint::context::Context<'a, 'tcx>.Visitor<'v>::visit_expr::ha294eff0288554d123s
  13:           0xac18f6 - lint::context::Context<'a, 'tcx>.Visitor<'v>::visit_expr::ha294eff0288554d123s
  14:           0xabd984 - lint::context::Context<'a, 'tcx>.Visitor<'v>::visit_item::h97cd569f2a202a39k0s
  15:           0xac21c9 - lint::context::Context<'a, 'tcx>.Visitor<'v>::visit_mod::ha15baa921fa04facaft
  16:           0xabda37 - lint::context::Context<'a, 'tcx>.Visitor<'v>::visit_item::h97cd569f2a202a39k0s
  17:           0xac21c9 - lint::context::Context<'a, 'tcx>.Visitor<'v>::visit_mod::ha15baa921fa04facaft
  18:           0xacf212 - lint::context::check_crate::h008b1bd3f8cb5d749yt
  19:         0x65399d3d - driver::phase_3_run_analysis_passes::h4302356ee8fb62b5tGa
  20:         0x6538322f - driver::compile_input::h2cf0fc5773828de8Qba
  21:         0x6541babd - run_compiler::hce52d824a1c83d3675b
  22:         0x65419a70 - run::h315a1ba5be8f35fbN5b
  23:         0x6541921e - run::h315a1ba5be8f35fbN5b
  24:         0x6d2fc07c - rust_try
  25:         0x6d2fc059 - rust_try
  26:         0x654194dc - run::h315a1ba5be8f35fbN5b
  27:         0x6d2bac4d - sys::process::Command::cwd::ha96a7c671c94f9abquu
  28:     0x7ffbd1b113d2 - BaseThreadInitThunk

@petrochenkov petrochenkov referenced this pull request May 19, 2015

Merged

Fix panic in lint for out of range literals #25612

bors added a commit that referenced this pull request May 21, 2015

@bors
Auto merge of #25612 - petrochenkov:lintabs, r=@alexcrichton 
Reported here #25441 (comment)
Loading status checks…
9ee4ead

@alexcrichton alexcrichton added the beta-accepted label Jun 9, 2015

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Jun 9, 2015

Accepting for a beta backport

@alexcrichton alexcrichton referenced this pull request Jun 10, 2015

Merged

Backport accepted PRs to beta #26161

@alexcrichton alexcrichton removed the beta-nominated label Jun 11, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.