arithmetic-overflow checks during const-eval by pnkfelix · Pull Request #23863 · rust-lang/rust

pnkfelix · 2015-03-30T17:00:12Z

const_eval : add overflow-checking for {+, -, *, /, <<, >>}.

One tricky detail here: There is some duplication of labor between rustc::middle::const_eval and rustc_trans::trans::consts. It might be good to explore ways to try to factor out the common structure to the two passes (by abstracting over the particular value-representation used in the compile-time interpreter).

Update: Rebased atop #23841

Fix #22531

Fix #23030

Fix #23221

Fix #23235

rust-highfive · 2015-03-30T17:00:23Z

r? @alexcrichton

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

pnkfelix · 2015-03-30T17:02:54Z

cc @eddyb

(actually, I'd be happy to take a review from eddyb)

Also, eddyb: You may be concerned about collisions here with your own const-fn work. Feel free to recommend some ordering on how the two PR's land, or suggest some other strategy to avoid unnecessary rebasing effort.

pnkfelix · 2015-03-30T17:30:29Z

r? @nikomatsakis

eddyb · 2015-03-30T20:28:54Z

const fn is pretty simple, really, there won't be much churn I would have to deal with (I don't think).

As for the checks in trans: I did something else for div/rem: check_const invokes const_eval for those binops on integers and errors if evaluation fails.
Sadly, that doesn't work well in the presence of const fn and associated constants.

@nikomatsakis I wonder if recording "obligations" on constant evaluation would make sense - like where clauses, but implicit and working on values (either arguments of const fns or associated constants on type parameters).

eddyb · 2015-03-30T20:31:12Z

src/librustc/middle/const_eval.rs

Are the indentation levels here intentional?

yeah it was so that the operator name (e.g. "add") would line up in both the method name and in the operator invocation.

Maybe I would have been better off using some well-placed calls to stringify!...

nikomatsakis · 2015-03-31T08:55:12Z

r+ modulo the FIXME

eddyb · 2015-03-31T10:01:16Z

@nikomatsakis Should we open a discussion about the interactions between trans::consts and const_eval somewhere else?

I personally prefer checking somewhere higher than trans, but in the end, what I am not particularly fond of is extracting values from LLVM for checking.

It works for this PR, and it's not enough to stop it from merging (we need those checks in beta), but I hope we can find a cleaner solution.

pnkfelix · 2015-03-31T12:27:40Z

@bors r=nikomatsakis

bors · 2015-03-31T12:27:41Z

📌 Commit e40d0de has been approved by nikomatsakis

pnkfelix · 2015-03-31T12:32:19Z

@eddyb @nikomatsakis

Should we open a discussion about the interactions between trans::consts and const_eval somewhere else?

I just made this post on internals: http://internals.rust-lang.org/t/removing-const-eval-duplication-of-labor-between-librustc-and-librustc-trans/1786

so we can continue that conversation in a focused fashion there.

pnkfelix · 2015-03-31T13:38:05Z

@bors r-

I want to check this in a cross-compiling environment, and perhaps land it in tandem with #23841 and perhaps a fix for #23890

pnkfelix · 2015-03-31T17:39:19Z

(I have now rebased this atop #23841, but I am pretty sure that it is going to conflict with other PR's in the queue, namely #23549, so I am still holding off on putting back the r+ until I see if that lands or not.)

bors · 2015-03-31T18:29:53Z

☔ The latest upstream changes (presumably #23549) made this pull request unmergeable. Please resolve the merge conflicts.

pnkfelix · 2015-03-31T22:47:38Z

annoying: the error messages on some of my tests are different for 64-bit compile versus a cross-compiling 64-bit host/32-bit target. :(

Update: Well, being forced to fix this may yield an overall code cleanup. :)

…values. Moved such overflow checking into one place (in `rustc::middle::ty`, since it needs to be run on-demand during `const_eval` in some scenarios), and revised `rustc_typeck` accordingly. (Note that we only check for overflow if program did not provide a discriminant value explicitly.) Fix rust-lang#23030 Fix rust-lang#23221 Fix rust-lang#23235

…tch to target type.

…gOps.

… eval.

…ant int. Add option-returning variants to `const_to_int`/`const_to_uint` that never assert fail. (These will be used for overflow checking from rustc_trans::trans::consts.)

The overflow-checking attempts to accommodate early evaluation where we do not have type information yet. Also, add fixme note about something that has been bothering me.

… 64-bit targets. See also rust-lang#23926.

pnkfelix · 2015-04-01T01:00:08Z

r=nikomatsakis ; @alexcrichton says he will merge into next rollup.

alexcrichton · 2015-04-01T01:06:49Z

@bors: r=nikomatsakis

bors · 2015-04-01T01:06:50Z

📌 Commit 2a9de1d has been approved by nikomatsakis

const_eval : add overflow-checking for {`+`, `-`, `*`, `/`, `<<`, `>>`}. One tricky detail here: There is some duplication of labor between `rustc::middle::const_eval` and `rustc_trans::trans::consts`. It might be good to explore ways to try to factor out the common structure to the two passes (by abstracting over the particular value-representation used in the compile-time interpreter). ---- Update: Rebased atop rust-lang#23841 Fix rust-lang#22531 Fix rust-lang#23030 Fix rust-lang#23221 Fix rust-lang#23235

andersk · 2015-04-02T11:49:22Z

I think this caused #23968: !0u32/2 gives “error: attempted to divide with overflow in a constant expression [E0020]”.

rust-highfive assigned alexcrichton Mar 30, 2015

pnkfelix changed the title ~~Arith oflo const eval~~ arithmetic-overflow checks during const-eval Mar 30, 2015

rust-highfive assigned nikomatsakis and unassigned alexcrichton Mar 30, 2015

eddyb reviewed Mar 30, 2015
View reviewed changes

pnkfelix mentioned this pull request Mar 31, 2015

const_eval::cast_const should use target's int/uint types for isize/usize #23890

Closed

pnkfelix mentioned this pull request Mar 31, 2015

repr-type specific oflo checks for enum discrim values. #23841

Closed

pnkfelix force-pushed the arith-oflo-const-eval branch 2 times, most recently from ee14d46 to 7506c3d Compare March 31, 2015 16:21

aturon mentioned this pull request Mar 31, 2015

Tracking issue for Integer Overflow (RFC 560) #22020

Closed

15 tasks

pnkfelix added 7 commits April 1, 2015 02:55

Added tests for discriminant overflows.

36b6d0e

fix post rebase.

0b57df7

Fix rust-lang#23890: const-eval _ as usize, _ as isize must dispa…

792f6aa

…tch to target type.

Added overflowing_{div,rem,shl,shr} method implementations to Wrappin…

60289ac

…gOps.

Refactored ty::ctxt so node_types mutations must go through ty methods.

2f7658a

ty.rs improve error feedback when const-eval errs during repeat count…

7875dae

… eval.

pnkfelix added 9 commits April 1, 2015 02:56

rust_llvm: Add way to reflectively ask if a ValueRef is a known const…

2e93e38

…ant int. Add option-returning variants to `const_to_int`/`const_to_uint` that never assert fail. (These will be used for overflow checking from rustc_trans::trans::consts.)

rustc::middle::const_eval : add overflow-checking for {+, -, *}.

b02f7d2

The overflow-checking attempts to accommodate early evaluation where we do not have type information yet. Also, add fixme note about something that has been bothering me.

rustc_trans::trans::consts add overflow checking

6808e41

Fallout from changes for overflow-checking during constant evaluation.

8d54ea3

Test cases for checking arithmetic overflow during const eval.

41b3c86

Include feature core to get access to wrapping_add.

1f5e45b

Fixes to compile-fail error messages post-rebase.

1973f7e

Unify handling of checking repeat-count validity.

2243a2c

dealing with fallout to the tests, in particular diffs between 32- vs…

2a9de1d

… 64-bit targets. See also rust-lang#23926.

pnkfelix force-pushed the arith-oflo-const-eval branch from 4298dc5 to 2a9de1d Compare April 1, 2015 00:58

bors merged commit 2a9de1d into rust-lang:master Apr 1, 2015

Uh oh!

Conversation

pnkfelix commented Mar 30, 2015

Uh oh!

rust-highfive commented Mar 30, 2015

Uh oh!

pnkfelix commented Mar 30, 2015

Uh oh!

pnkfelix commented Mar 30, 2015

Uh oh!

eddyb commented Mar 30, 2015

Uh oh!

eddyb Mar 30, 2015

Choose a reason for hiding this comment

Uh oh!

pnkfelix Mar 30, 2015

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Mar 31, 2015

Uh oh!

eddyb commented Mar 31, 2015

Uh oh!

pnkfelix commented Mar 31, 2015

Uh oh!

bors commented Mar 31, 2015

Uh oh!

pnkfelix commented Mar 31, 2015

Uh oh!

pnkfelix commented Mar 31, 2015

Uh oh!

pnkfelix commented Mar 31, 2015

Uh oh!

bors commented Mar 31, 2015

Uh oh!

pnkfelix commented Mar 31, 2015

Uh oh!

pnkfelix commented Apr 1, 2015

Uh oh!

alexcrichton commented Apr 1, 2015

Uh oh!

bors commented Apr 1, 2015

Uh oh!

andersk commented Apr 2, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants