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

Remove the field field from Lvalue::Local #220

Merged
merged 2 commits into from
Jun 28, 2017

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 27, 2017

Undoes #146 and #139 because this was just a lot of overhead working with the field accesses. They also only "helped" when the lvalue that was produced was immediately read again.

We should move to doing an lvalue-read operation similar to https://github.com/solson/miri/pull/139/files#diff-3596aeeadedc2ed352f2c376852f8fb7L117 which optimizes through those cases where you don't want to have a pointer in the end, but only a primval. So basically primval rvalues?

I did not reinstate the mentioned lvalue-read, since i want to get this PR through first and then write a more advanced version that supports more cases.

@oli-obk oli-obk requested a review from eddyb June 27, 2017 18:07
@@ -1353,7 +1347,12 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
I128 => 16,
Is => self.memory.pointer_size(),
};
PrimVal::from_i128(self.memory.read_int(ptr, size)?)
// if we cast a ptr to a usize reading it back into a primval shouldn't panic
Copy link
Member

Choose a reason for hiding this comment

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

What about isize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, copy paste.

@@ -1366,7 +1365,12 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
U128 => 16,
Us => self.memory.pointer_size(),
};
PrimVal::from_u128(self.memory.read_uint(ptr, size)?)
if size == self.memory.pointer_size() {
// if we cast a ptr to a usize reading it back into a primval shouldn't panic
Copy link
Member

Choose a reason for hiding this comment

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

What about isize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the usize cast ;)

@@ -24,8 +24,6 @@ pub enum Lvalue<'tcx> {
Local {
frame: usize,
local: mir::Local,
/// Optionally, this lvalue can point to a field of the stack value
field: Option<(usize, Ty<'tcx>)>,
Copy link
Member

Choose a reason for hiding this comment

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

Yay. :D TBH I never understood this field, so I'm happy I don't have to.

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

The title of the PR is not entirely precise; this is really just about removing the field, right?

@RalfJung
Copy link
Member

pub enum Lvalue<'tcx>

Looks like 'tcx is not used any more? I think it'd be great to get rid of it; run-time lvalues really should not depend on type information...

@eddyb
Copy link
Member

eddyb commented Jun 27, 2017

@RalfJung Looks like GlobalId depends on it though. Not sure if it should.

@RalfJung
Copy link
Member

Ah, GitHub fooled me. I pressed the "expand code" button and then what Is aw looked almost like a complete type, but it wasn't actually completely expanded. :/

@oli-obk oli-obk changed the title Simplify all the code Remove the field field from Lvalue::Local Jun 28, 2017
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 28, 2017

GlobalId has an Instance field: https://github.com/solson/miri/blob/8c6c6d7cadceb7ad6fc9c280674bb629b1e30003/src/lvalue.rs#L48 which is needed to differentiate between different globals. Maybe we can use something else? I don't think the DefId suffices, but I might be wrong.

@oli-obk oli-obk merged commit 42d3eda into rust-lang:master Jun 28, 2017
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 28, 2017

The Instance issue is separate, so I'll open a new PR for it.

@oli-obk oli-obk deleted the undo_single_field_opt branch June 28, 2017 08:15
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 28, 2017

I checked whether we could use the DefId, but it doesn't suffice, because we are caching promoteds, which can occur within autogenerated MIR. The only way to get autogenerated MIR is through the Instance: http://manishearth.github.io/rust-internals-docs/rustc/ty/struct.TyCtxt.html#method.instance_mir

@@ -1353,7 +1347,12 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
I128 => 16,
Is => self.memory.pointer_size(),
};
PrimVal::from_i128(self.memory.read_int(ptr, size)?)
// if we cast a ptr to an isize, reading it back into a primval shouldn't panic
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I am happy about using the word "cast" here. This is doing a transmute, i.e., it is reading the same bytes at a different type. The rules for the two should be somewhat consistent, but they are clearly not the same right now (as they are implemented in two different files).

@RalfJung
Copy link
Member

Fair enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants