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

miri: improve and simplify overflow detection #69002

Merged
merged 8 commits into from
Feb 13, 2020

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Feb 9, 2020

This simplifies the overflow detection for signed binary operators, and adds overflow detection to unary operators so that const-prop doesn't have to crudely hand-roll that.

It also fixes some bugs in the operator implementation that however, I think, were not observable.

r? @oli-obk @wesleywiser

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 9, 2020
if r == -1 && l == (1 << (size.bits() - 1)) {
return Ok((Scalar::from_uint(l, size), true, left_layout.ty));
return Ok((Scalar::from_int(0, size), true, left_layout.ty));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what this used to do for Rem is plain wrong -- it returned the wrong result. However, even in release builds, remainder is checked for overflow. So, during normal execution in CTFE/Miri, the overflowing case here is unreachable. It is reachable in const-prop, which however does not care about the return value, just about whether there is an overflow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With control flow const prop we could stop propping in arms that are only reachable by a failing assert terminator.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also declare int_min / -1 and int_min % -1 to be UB. This is consistent with MIR building always checking that, and it should resolve the rem/div part of #69020.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though OTOH, it is probably more consistent to handle all overflow the same way... so maybe not. I hope to play with this next weekend.

@wesleywiser
Copy link
Member

wesleywiser commented Feb 10, 2020

Let's do a perf run just to be sure there's no regressions and then we can land this.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Feb 10, 2020

⌛ Trying commit d6c5a04 with merge 5bf0888774eedd0302d15303b594839d4c70873e...

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Feb 10, 2020

⌛ Trying commit d6c5a04 with merge 0f7a6fbaf793dfb724765f4d67fe0fa0addb53fb...

@bors
Copy link
Contributor

bors commented Feb 10, 2020

☀️ Try build successful - checks-azure
Build commit: 0f7a6fbaf793dfb724765f4d67fe0fa0addb53fb (0f7a6fbaf793dfb724765f4d67fe0fa0addb53fb)

@wesleywiser
Copy link
Member

@rust-timer build 0f7a6fbaf793dfb724765f4d67fe0fa0addb53fb

@rust-timer
Copy link
Collaborator

Queued 0f7a6fbaf793dfb724765f4d67fe0fa0addb53fb with parent 4d1241f, future comparison URL.

@RalfJung
Copy link
Member Author

Looks like the auto-queueing thing doesn't work any more?

@wesleywiser
Copy link
Member

It's been buggy lately but I think it got confused when I edited my comment.

Perf looks unaffected.

@bors r=oli-obk,wesleywiser

@bors
Copy link
Contributor

bors commented Feb 11, 2020

📌 Commit c561d23 has been approved by oli-obk,wesleywiser

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 11, 2020
@RalfJung
Copy link
Member Author

It's been buggy lately

Is there a bug report for this? I can't find any at https://github.com/rust-lang-nursery/rustc-perf/issues

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 12, 2020
…k,wesleywiser

miri: improve and simplify overflow detection

This simplifies the overflow detection for signed binary operators, and adds overflow detection to unary operators so that const-prop doesn't have to crudely hand-roll that.

It also fixes some bugs in the operator implementation that however, I think, were not observable.

r? @oli-obk @wesleywiser
bors added a commit that referenced this pull request Feb 12, 2020
Rollup of 7 pull requests

Successful merges:

 - #67954 (Support new LLVM pass manager)
 - #68981 ( Account for type params on method without parentheses)
 - #69002 (miri: improve and simplify overflow detection)
 - #69038 (Add initial debug fmt for Backtrace)
 - #69040 (Cleanup SGX entry code)
 - #69086 (Update compiler-builtins to 0.1.25)
 - #69095 (Minified theme check)

Failed merges:

r? @ghost
@bors bors merged commit c561d23 into rust-lang:master Feb 13, 2020
@RalfJung RalfJung deleted the miri-op-overflow branch February 13, 2020 10:13
@RalfJung RalfJung mentioned this pull request Feb 13, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 13, 2020
miri: fix exact_div

Turns out `exact_div` was relying on the broken behavior of `Rem` for `int_min % -1` that was fixed in rust-lang#69002. This PR fixes `exact_div`.

Inside rustc, `exact_div` is only used in a single place where the divisor is always positive (in `ptr_offset_from`), so we cannot test the fix in rustc. The Miri test suite covers this through the `exact_div` intrinsic, though (and it is how I found out).

One step to rust-lang#69117 (then we also need to address build failures introduced by rust-lang#68969)

r? @oli-obk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants