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

Remove the field field from Lvalue::Local #220

Merged
merged 2 commits into from
Jun 28, 2017
Merged

Remove the field field from Lvalue::Local #220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5dfaacf
Simplify all the code
oli-obk Jun 27, 2017
a724a39
Reword comments
oli-obk Jun 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 29 additions & 68 deletions src/eval_context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1003,12 +1003,11 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
lvalue: Lvalue<'tcx>,
) -> EvalResult<'tcx, Lvalue<'tcx>> {
let new_lvalue = match lvalue {
Lvalue::Local { frame, local, field } => {
Lvalue::Local { frame, local } => {
// -1 since we don't store the return value
match self.stack[frame].locals[local.index() - 1] {
None => return Err(EvalError::DeadLocal),
Some(Value::ByRef(ptr)) => {
assert!(field.is_none());
Lvalue::from_ptr(ptr)
},
Some(val) => {
Expand All @@ -1018,12 +1017,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
let ptr = self.alloc_ptr_with_substs(ty, substs)?;
self.stack[frame].locals[local.index() - 1] = Some(Value::ByRef(ptr)); // it stays live
self.write_value_to_ptr(val, PrimVal::Ptr(ptr), ty)?;
let lval = Lvalue::from_ptr(ptr);
if let Some((field, field_ty)) = field {
self.lvalue_field(lval, field, ty, field_ty)?
} else {
lval
}
Lvalue::from_ptr(ptr)
}
}
}
Expand Down Expand Up @@ -1110,11 +1104,11 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
self.write_value_to_ptr(src_val, ptr, dest_ty)
}

Lvalue::Local { frame, local, field } => {
let dest = self.stack[frame].get_local(local, field.map(|(i, _)| i))?;
Lvalue::Local { frame, local } => {
let dest = self.stack[frame].get_local(local)?;
self.write_value_possibly_by_val(
src_val,
|this, val| this.stack[frame].set_local(local, field.map(|(i, _)| i), val),
|this, val| this.stack[frame].set_local(local, val),
dest,
dest_ty,
)
Expand Down Expand Up @@ -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).

// Due to read_ptr ignoring the sign, we need to jump around some hoops
match self.memory.read_int(ptr, size) {
Err(EvalError::ReadPointerAsBytes) if size == self.memory.pointer_size() => self.memory.read_ptr(ptr)?,
other => PrimVal::from_i128(other?),
}
}

ty::TyUint(uint_ty) => {
Expand All @@ -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 an usize, reading it back into a primval shouldn't panic
self.memory.read_ptr(ptr)?
} else {
PrimVal::from_u128(self.memory.read_uint(ptr, size)?)
}
}

ty::TyFloat(FloatTy::F32) => PrimVal::from_f32(self.memory.read_f32(ptr)?),
Expand Down Expand Up @@ -1518,19 +1522,16 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {

pub(super) fn dump_local(&self, lvalue: Lvalue<'tcx>) {
// Debug output
if let Lvalue::Local { frame, local, field } = lvalue {
if let Lvalue::Local { frame, local } = lvalue {
let mut allocs = Vec::new();
let mut msg = format!("{:?}", local);
if let Some((field, _)) = field {
write!(msg, ".{}", field).unwrap();
}
let last_frame = self.stack.len() - 1;
if frame != last_frame {
write!(msg, " ({} frames up)", last_frame - frame).unwrap();
}
write!(msg, ":").unwrap();

match self.stack[frame].get_local(local, field.map(|(i, _)| i)) {
match self.stack[frame].get_local(local) {
Err(EvalError::DeadLocal) => {
write!(msg, " is dead").unwrap();
}
Expand Down Expand Up @@ -1575,14 +1576,13 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
&mut self,
frame: usize,
local: mir::Local,
field: Option<usize>,
f: F,
) -> EvalResult<'tcx>
where F: FnOnce(&mut Self, Value) -> EvalResult<'tcx, Value>,
{
let val = self.stack[frame].get_local(local, field)?;
let val = self.stack[frame].get_local(local)?;
let new_val = f(self, val)?;
self.stack[frame].set_local(local, field, new_val)?;
self.stack[frame].set_local(local, new_val)?;
// FIXME(solson): Run this when setting to Undef? (See previous version of this code.)
// if let Value::ByRef(ptr) = self.stack[frame].get_local(local) {
// self.memory.deallocate(ptr)?;
Expand All @@ -1598,59 +1598,20 @@ impl<'tcx> Frame<'tcx> {
_ => false,
}
}
pub fn get_local(&self, local: mir::Local, field: Option<usize>) -> EvalResult<'tcx, Value> {
pub fn get_local(&self, local: mir::Local) -> EvalResult<'tcx, Value> {
// Subtract 1 because we don't store a value for the ReturnPointer, the local with index 0.
if let Some(field) = field {
Ok(match self.locals[local.index() - 1] {
None => return Err(EvalError::DeadLocal),
Some(Value::ByRef(_)) => bug!("can't have lvalue fields for ByRef"),
Some(val @ Value::ByVal(_)) => {
assert_eq!(field, 0);
val
},
Some(Value::ByValPair(a, b)) => {
match field {
0 => Value::ByVal(a),
1 => Value::ByVal(b),
_ => bug!("ByValPair has only two fields, tried to access {}", field),
}
},
})
} else {
self.locals[local.index() - 1].ok_or(EvalError::DeadLocal)
}
self.locals[local.index() - 1].ok_or(EvalError::DeadLocal)
}

fn set_local(&mut self, local: mir::Local, field: Option<usize>, value: Value) -> EvalResult<'tcx> {
fn set_local(&mut self, local: mir::Local, value: Value) -> EvalResult<'tcx> {
// Subtract 1 because we don't store a value for the ReturnPointer, the local with index 0.
if let Some(field) = field {
match self.locals[local.index() - 1] {
None => return Err(EvalError::DeadLocal),
Some(Value::ByRef(_)) => bug!("can't have lvalue fields for ByRef"),
Some(Value::ByVal(_)) => {
assert_eq!(field, 0);
self.set_local(local, None, value)?;
},
Some(Value::ByValPair(a, b)) => {
let prim = match value {
Value::ByRef(_) => bug!("can't set ValPair field to ByRef"),
Value::ByVal(val) => val,
Value::ByValPair(_, _) => bug!("can't set ValPair field to ValPair"),
};
match field {
0 => self.set_local(local, None, Value::ByValPair(prim, b))?,
1 => self.set_local(local, None, Value::ByValPair(a, prim))?,
_ => bug!("ByValPair has only two fields, tried to access {}", field),
}
},
}
} else {
match self.locals[local.index() - 1] {
None => return Err(EvalError::DeadLocal),
Some(ref mut local) => { *local = value; }
match self.locals[local.index() - 1] {
None => Err(EvalError::DeadLocal),
Some(ref mut local) => {
*local = value;
Ok(())
}
}
return Ok(());
}

pub fn storage_live(&mut self, local: mir::Local) -> EvalResult<'tcx, Option<Value>> {
Expand Down
51 changes: 4 additions & 47 deletions src/lvalue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

},

/// An lvalue referring to a global
Expand Down Expand Up @@ -141,8 +139,8 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
assert_eq!(extra, LvalueExtra::None);
Ok(Value::ByRef(ptr.to_ptr()?))
}
Lvalue::Local { frame, local, field } => {
self.stack[frame].get_local(local, field.map(|(i, _)| i))
Lvalue::Local { frame, local } => {
self.stack[frame].get_local(local)
}
Lvalue::Global(cid) => {
Ok(self.globals.get(&cid).expect("global not cached").value)
Expand All @@ -154,7 +152,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
use rustc::mir::Lvalue::*;
let lvalue = match *mir_lvalue {
Local(mir::RETURN_POINTER) => self.frame().return_lvalue,
Local(local) => Lvalue::Local { frame: self.stack.len() - 1, local, field: None },
Local(local) => Lvalue::Local { frame: self.stack.len() - 1, local },

Static(ref static_) => {
let instance = ty::Instance::mono(self.tcx, static_.def_id);
Expand Down Expand Up @@ -235,48 +233,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
_ => bug!("field access on non-product type: {:?}", base_layout),
};

let (base_ptr, base_extra) = match base {
Lvalue::Ptr { ptr, extra } => (ptr, extra),
Lvalue::Local { frame, local, field } => match self.stack[frame].get_local(local, field.map(|(i, _)| i))? {
Value::ByRef(ptr) => {
assert!(field.is_none(), "local can't be ByRef and have a field offset");
(PrimVal::Ptr(ptr), LvalueExtra::None)
},
Value::ByVal(PrimVal::Undef) => {
// FIXME: allocate in fewer cases
if self.ty_to_primval_kind(base_ty).is_ok() {
return Ok(base);
} else {
(PrimVal::Ptr(self.force_allocation(base)?.to_ptr()?), LvalueExtra::None)
}
},
Value::ByVal(_) => {
if self.get_field_count(base_ty)? == 1 {
assert_eq!(field_index, 0, "ByVal can only have 1 non zst field with offset 0");
return Ok(base);
}
// this branch is taken when a union creates a large ByVal which is then
// accessed as a struct with multiple small fields
(PrimVal::Ptr(self.force_allocation(base)?.to_ptr()?), LvalueExtra::None)
},
Value::ByValPair(_, _) => {
let field_count = self.get_field_count(base_ty)?;
if field_count == 1 {
assert_eq!(field_index, 0, "{:?} has only one field", base_ty);
return Ok(base);
}
assert_eq!(field_count, 2);
assert!(field_index < 2);
return Ok(Lvalue::Local {
frame,
local,
field: Some((field_index, field_ty)),
});
},
},
// FIXME: do for globals what we did for locals
Lvalue::Global(_) => self.force_allocation(base)?.to_ptr_and_extra(),
};
let (base_ptr, base_extra) = self.force_allocation(base)?.to_ptr_and_extra();

let offset = match base_extra {
LvalueExtra::Vtable(tab) => {
Expand Down
2 changes: 1 addition & 1 deletion src/step.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
// Mark locals as dead or alive.
StorageLive(ref lvalue) | StorageDead(ref lvalue)=> {
let (frame, local) = match self.eval_lvalue(lvalue)? {
Lvalue::Local{ frame, local, field: None } if self.stack.len() == frame+1 => (frame, local),
Lvalue::Local{ frame, local } if self.stack.len() == frame+1 => (frame, local),
_ => return Err(EvalError::Unimplemented("Storage annotations must refer to locals of the topmost stack frame.".to_owned())) // FIXME maybe this should get its own error type
};
let old_val = match stmt.kind {
Expand Down
4 changes: 2 additions & 2 deletions src/terminator/intrinsic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
Ok(zero_val)
};
match dest {
Lvalue::Local { frame, local, field } => self.modify_local(frame, local, field.map(|(i, _)| i), init)?,
Lvalue::Local { frame, local } => self.modify_local(frame, local, init)?,
Lvalue::Ptr { ptr, extra: LvalueExtra::None } => self.memory.write_repeat(ptr.to_ptr()?, 0, size)?,
Lvalue::Ptr { .. } => bug!("init intrinsic tried to write to fat ptr target"),
Lvalue::Global(cid) => self.modify_global(cid, init)?,
Expand Down Expand Up @@ -419,7 +419,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
}
};
match dest {
Lvalue::Local { frame, local, field } => self.modify_local(frame, local, field.map(|(i, _)| i), uninit)?,
Lvalue::Local { frame, local } => self.modify_local(frame, local, uninit)?,
Lvalue::Ptr { ptr, extra: LvalueExtra::None } =>
self.memory.mark_definedness(ptr, size, false)?,
Lvalue::Ptr { .. } => bug!("uninit intrinsic tried to write to fat ptr target"),
Expand Down
2 changes: 1 addition & 1 deletion tests/compile-fail/pointer_byte_read_1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ fn main() {
let y = &x;
let z = &y as *const &i32 as *const usize;
let ptr_bytes = unsafe { *z }; // the actual deref is fine, because we read the entire pointer at once
let _ = ptr_bytes == 15; //~ ERROR: tried to access part of a pointer value as raw bytes
let _ = ptr_bytes % 432; //~ ERROR: tried to access part of a pointer value as raw bytes
}