Skip to content
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

arithmetic-overflow checks during const-eval #23863

Merged
merged 16 commits into from Apr 1, 2015

Conversation

Projects
None yet
7 participants
@pnkfelix
Copy link
Member

pnkfelix commented Mar 30, 2015

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

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Mar 30, 2015

r? @alexcrichton

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

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

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Mar 30, 2015

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

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Mar 30, 2015

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Mar 30, 2015

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).

if !oflo { Ok(const_uint(ret)) } else { signal!(e, SubuWithOverflow(a, b)) }

pub_fn_checked_op!{ const_int_checked_add(a: i64, b: i64,.. IntTy) {
int_arith_body overflowing_add const_int AddiWithOverflow(a, b)

This comment has been minimized.

@eddyb

eddyb Mar 30, 2015

Member

Are the indentation levels here intentional?

This comment has been minimized.

@pnkfelix

pnkfelix Mar 30, 2015

Author Member

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

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 31, 2015

r+ modulo the FIXME

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Mar 31, 2015

@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

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Mar 31, 2015

@bors r=nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 31, 2015

📌 Commit e40d0de has been approved by nikomatsakis

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Mar 31, 2015

@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

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Mar 31, 2015

@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 pnkfelix force-pushed the pnkfelix:arith-oflo-const-eval branch 2 times, most recently to 7506c3d Mar 31, 2015

@aturon aturon referenced this pull request Mar 31, 2015

Closed

Tracking issue for Integer Overflow (RFC 560) #22020

11 of 15 tasks complete
@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Mar 31, 2015

(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

This comment has been minimized.

Copy link
Contributor

bors commented Mar 31, 2015

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

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Mar 31, 2015

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. :)

pnkfelix added some commits Mar 27, 2015

Added type-specific overflow checks when computing enum discriminant …
…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 #23030

Fix #23221

Fix #23235
rustc::middle::const_eval : add overflow-checking for {+, -, *}.
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.
rust_llvm: Add way to reflectively ask if a ValueRef is a known const…
…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.)

@pnkfelix pnkfelix force-pushed the pnkfelix:arith-oflo-const-eval branch from 4298dc5 to 2a9de1d Apr 1, 2015

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Apr 1, 2015

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

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 1, 2015

@bors: r=nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 1, 2015

📌 Commit 2a9de1d has been approved by nikomatsakis

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Apr 1, 2015

rollup merge of rust-lang#23863: pnkfelix/arith-oflo-const-eval
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

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@andersk

This comment has been minimized.

Copy link
Contributor

andersk commented Apr 2, 2015

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

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.