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.

Sign up for GitHub

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

Jump to bottom

interpret: fix CheckedBinOp behavior when overflow checking is disabled #98888

Merged
merged 2 commits into from
Jul 5, 2022
Merged

interpret: fix CheckedBinOp behavior when overflow checking is disabled #98888

merged 2 commits into from
Jul 5, 2022

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jul 4, 2022

Adjusts the interpreter to #98738.

r? @oli-obk

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 4, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 4, 2022

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 4, 2022
@RalfJung RalfJung mentioned this pull request Jul 4, 2022
Merged
@RalfJung
Copy link
Member Author

RalfJung commented Jul 4, 2022

Uh, ConstProp is not happy with this...

@tmiasko
Copy link
Contributor

tmiasko commented Jul 4, 2022

I don't think CTFE should change the behaviour based on the options from the session. This would risk introducing cross-crate non-determinism in the execution.

Conceptually I was considering CTFE to simply have overflow checking enabled, and so already in agreement with wording from #98738, but I can make this explicit.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member Author

RalfJung commented Jul 4, 2022

I don't think CTFE should change the behaviour based on the options from the session. This would risk introducing cross-crate non-determinism in the execution.

You specified MIR behavior to depend on the session options. CTFE should accurately implement MIR behavior.

If this is fine for codegen, then why not for CTFE?

@RalfJung
Copy link
Member Author

RalfJung commented Jul 4, 2022

FWIW I would much prefer if CheckedBinOp would just always check for overflow. Also note that the code here is not just for CTFE, it is also for Miri.

I don't understand these CI failures though, ConstProp does not even call this function, it directly calls overflowing_binary_op...

@RalfJung
Copy link
Member Author

RalfJung commented Jul 4, 2022

This would risk introducing cross-crate non-determinism in the execution.

I guess it does mean that the same query executed in different crates can have different results. Which indeed sounds bad.
But the same seems to be true for the codegen queries, so... not sure what to make of this.

Also is it really so easy to break the query system? I have no idea if our other accesses to tcx.sess are any better...

@RalfJung
Copy link
Member Author

RalfJung commented Jul 4, 2022

I don't understand these CI failures though, ConstProp does not even call this function, it directly calls overflowing_binary_op...

Oh, these are not actually lints. Those are const-evaluations that used to fail because we always overflow-check arithmetic in const/static (we always generate CheckedBinaryOp), but now that we honor the same flag as codegen, we don't check this any more.

I suppose I could add a machine flag to control this, so Miri can behave like codegen and CTFE can behave like before.

@bors
Copy link
Contributor

bors commented Jul 4, 2022

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

@oli-obk
Copy link
Contributor

oli-obk commented Jul 5, 2022

rustfmt doesn't like you again ^^ (I run all my commands as ./x.py fmt && ./x.py asdft because I ran into this all the time, too)

@oli-obk
Copy link
Contributor

oli-obk commented Jul 5, 2022

r=me with CI happy

@tmiasko
Copy link
Contributor

tmiasko commented Jul 5, 2022 •
edited
Loading

This would risk introducing cross-crate non-determinism in the execution.

I guess it does mean that the same query executed in different crates can have different results. Which indeed sounds bad. But the same seems to be true for the codegen queries, so... not sure what to make of this.

Also is it really so easy to break the query system? I have no idea if our other accesses to tcx.sess are any better...

I wasn't referring to the determinism in context of a query system and I don' see any immediate issue there since overflow checking is controlled by tracked options.

I was referring to the determinism as a fundamental property that CTFE should have. It would be rather bad if non-deterministic computation is lifted into the type system and different crates have a different view of what the types are.

In the specific case of overflow checking, maybe there is some argument to be made that given how MIR is built the only other behaviour would be an evaluation error ...

The proposed changes looks good to me. Thanks for updating this.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 5, 2022

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Jul 5, 2022

📌 Commit 2f6e996 has been approved by oli-obk

@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 Jul 5, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 5, 2022
Merged
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 5, 2022
@Dylan-DPC
Rollup merge of rust-lang#98888 - RalfJung:interpret-checked-bin, r=o…
e1305c2 
…li-obk

interpret: fix CheckedBinOp behavior when overflow checking is disabled

Adjusts the interpreter to rust-lang#98738.

r? ``@oli-obk``
@Dylan-DPC Dylan-DPC mentioned this pull request Jul 5, 2022
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 5, 2022
@bors
Auto merge of rust-lang#98936 - matthiaskrgr:rollup-dvr0ucm, r=matthi…
41ad4d9 
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#98860 (adjust dangling-int-ptr error message)
 - rust-lang#98888 (interpret: fix CheckedBinOp behavior when overflow checking is disabled)
 - rust-lang#98889 (Add regression test for rust-lang#79467)
 - rust-lang#98895 (bootstrap.py: Always use `.exe` for Windows)
 - rust-lang#98920 (adapt issue-37945 codegen test to accept any order of ops)
 - rust-lang#98921 (Refactor: remove a redundant mutable variable)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cca43fe into rust-lang:master Jul 5, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 5, 2022
@RalfJung RalfJung deleted the interpret-checked-bin branch July 5, 2022 21:49
@RalfJung RalfJung mentioned this pull request Jul 5, 2022
Merged
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 6, 2022
@Dylan-DPC
Rollup merge of rust-lang#98964 - RalfJung:typo, r=Dylan-DPC
56667b5 
fix typo in function name

I don't know what I was doing when I named that function...
follow-up to rust-lang#98888
r? `@oli-obk`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.64.0
Development

Successfully merging this pull request may close these issues.
7 participants
@RalfJung @rustbot @tmiasko @rust-log-analyzer @bors @oli-obk @rust-highfive