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: dispatch first on the type #62946

Merged
merged 13 commits into from
Aug 3, 2019

Conversation

RalfJung
Copy link
Member

Based on the fact that Miri now always has intptrcast available, we can change binops and casts to first check the type of the source operand and then decide based on that what to do, instead of considering the value (pointer vs bits) first.

@rust-highfive
Copy link
Collaborator

r? @estebank

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 24, 2019
@RalfJung
Copy link
Member Author

r? @oli-obk @eddyb

@RalfJung
Copy link
Member Author

The Miri side of this is at rust-lang/miri#856. I verified that the test suite passes.

@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

Just some nits

src/librustc_mir/interpret/cast.rs Outdated Show resolved Hide resolved
src/librustc_mir/interpret/cast.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member Author

RalfJung commented Jul 24, 2019

Actually, the PR as-is makes this fail:

const X2: bool = unsafe { 42 as *const i32 == 43 as *const i32 };

The reason is that == on pointer types now always errors, whereas previously it worked if both operands had integer value.

I suppose this is something we have to keep working, for backwards compatibility?

@RalfJung
Copy link
Member Author

If we do want this, I think the cleanest way would be to move https://github.com/rust-lang/miri/blob/5a9ea5d1b61a860b95916366b17cce8349450332/src/operator.rs#L52-L78 into the engine. It will still fail in CTFE if either input is a pointer because force_bits will fail.

What do you think?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 24, 2019

No, just break it. Pointer comparisons are unstable at the type level for a reason

@RalfJung
Copy link
Member Author

@oli-obk I think I took care of your nits. I also had to update some test files and rebase. Please check.

@RalfJung RalfJung force-pushed the miri_type_dispatch_first branch 2 times, most recently from 54f900f to 3cd4079 Compare July 24, 2019 22:38
@rust-highfive

This comment has been minimized.

Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

nits

src/librustc_mir/interpret/cast.rs Outdated Show resolved Hide resolved
src/librustc_mir/interpret/operator.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@RalfJung
Copy link
Member Author

Now that's surprising, why does this affect mir-opt...

@RalfJung
Copy link
Member Author

Turns out const_prop was doing ptr-to-int casts based on CTFE doing that. With CTFE not doing that any more, const_prop cannot do it any more, either.

Cc @wesleywiser

@oli-obk
Copy link
Contributor

oli-obk commented Jul 31, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Jul 31, 2019

📌 Commit d7b9bbf 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 31, 2019
@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 2, 2019
@RalfJung
Copy link
Member Author

RalfJung commented Aug 2, 2019

Rebased.

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Aug 2, 2019

📌 Commit b9db95e 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 2, 2019
@bors
Copy link
Contributor

bors commented Aug 3, 2019

⌛ Testing commit b9db95e with merge 8e917f4...

bors added a commit that referenced this pull request Aug 3, 2019
Miri: dispatch first on the type

Based on the fact that Miri now always has intptrcast available, we can change binops and casts to first check the type of the source operand and then decide based on that what to do, instead of considering the value (pointer vs bits) first.
@bors
Copy link
Contributor

bors commented Aug 3, 2019

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing 8e917f4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 3, 2019
@bors bors merged commit b9db95e into rust-lang:master Aug 3, 2019
bors added a commit to rust-lang/miri that referenced this pull request Aug 3, 2019
Adjust for ptr_op changes

This is the Miri side of rust-lang/rust#62946.
bors added a commit to rust-lang/miri that referenced this pull request Aug 3, 2019
Adjust for ptr_op changes

This is the Miri side of rust-lang/rust#62946.
@RalfJung RalfJung deleted the miri_type_dispatch_first branch August 5, 2019 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

None yet

6 participants