Intptrcast model#61668
Closed
pvdrz wants to merge 9 commits intorust-lang:masterfrom
pvdrz:intptrcast-model
Closed
Intptrcast model#61668pvdrz wants to merge 9 commits intorust-lang:masterfrom pvdrz:intptrcast-model
pvdrz wants to merge 9 commits intorust-lang:masterfrom
pvdrz:intptrcast-model
Conversation
Contributor
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Contributor
Author
oli-obk
reviewed
Jun 8, 2019
Contributor
There was a problem hiding this comment.
why was this change done? It seems good, but unrelated to this PR right now
Contributor
Author
There was a problem hiding this comment.
It was done per @RalfJung suggestions. Here are his comments on the matter:
basically, if we have integer addresses, we have two cases:
either left.layout.ty.is_integral() && right.layout.ty.is_integral(). then both sides should be ptr-to-int-cast, and we proceed with binary_int_op.
or bin_op == mir::BinOp::Offset, then call M::ptr_op.
if we dont have integer addresses, behavior should be unchanged.
Contributor
There was a problem hiding this comment.
ah, this is then indeed where your test output changes come from: force_bits is guaranteed to fail, since we already know that either left or right is a Scalar::Ptr (as per the if condition in line 313 and 314)
I don't think we should do this here, but instead miri can do this in ptr_op once it actually supports intptr. If we merged this, there would be no way for miri to do a change that will fix its test suite other than implementing intptr.
It's also useless for const eval, so I think it should not be in the engine.
@RalfJung or am I missing something?
Member
There was a problem hiding this comment.
Well now I can't look any more because the diff is broken. :/
But I somehow didn't like going through Miri and back when we have ptr-int-casts available. The code flow would be somewhat weird: Miri would do force_bits and then call back to here, and endless recursion would be prevented because things would then not be pointers any more.
Notice that I specifically said "if we dont have integer addresses, behavior should be unchanged" in my plan; the idea was that we would have some way to detect that (maybe check the force functions return value for Err?). But maybe you are right and it makes more sense to do all of that in Miri.
I just really don't like seeing any value-based dispatching in the code here. Dispatching should be purely type-based...
oli-obk
reviewed
Jun 8, 2019
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
oli-obk
reviewed
Jun 12, 2019
Contributor
oli-obk
left a comment
oli-obk
left a comment
There was a problem hiding this comment.
oops forgot to send off my review. why can't github tell me aggressively I have written reviews but not sent them?
src/librustc_mir/interpret/memory.rs
Outdated
Contributor
There was a problem hiding this comment.
Otherwise you'll get a "read undef bytes" error when interpreting 32 bit code
Suggested change
| _ => M::int_to_ptr(scalar.to_u64()?, &self.extra) | |
| _ => M::int_to_ptr(scalar.to_usize(self)?, &self.extra) |
Contributor
Author
There was a problem hiding this comment.
We talked about this on zulip and I thought I've done it, I'll fix it
Contributor
There was a problem hiding this comment.
ah, this is then indeed where your test output changes come from: force_bits is guaranteed to fail, since we already know that either left or right is a Scalar::Ptr (as per the if condition in line 313 and 314)
I don't think we should do this here, but instead miri can do this in ptr_op once it actually supports intptr. If we merged this, there would be no way for miri to do a change that will fix its test suite other than implementing intptr.
It's also useless for const eval, so I think it should not be in the engine.
@RalfJung or am I missing something?
This uses a well-known branchless implementation of `signum`. Its `const`-ness is unstable and requires `#![feature(const_int_sign)]`.
Contributor
Author
|
I messed up solving the conflicts, I'll redo the changes on a fresh branch and do another PR |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jun 14, 2019
…=oli-obk prepare for Intptrcast model rust-lang#61668 done right (I hope so). r? @RalfJung @oli-obk
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jun 14, 2019
…=oli-obk prepare for Intptrcast model rust-lang#61668 done right (I hope so). r? @RalfJung @oli-obk
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jun 16, 2019
…=oli-obk prepare for Intptrcast model rust-lang#61668 done right (I hope so). r? @RalfJung @oli-obk
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
rust-lang/miri#224