Skip to content

Commit

Permalink
fixup! [move-vm] Removed most RCs from runtime
Browse files Browse the repository at this point in the history
  • Loading branch information
@tnowacki
tnowacki committed Apr 1, 2022
1 parent 4163634 commit e72b36a
Show file tree
Hide file tree
Showing 8 changed files with 187 additions and 319 deletions.
2 changes: 1 addition & 1 deletion language/move-stdlib/src/natives/bcs.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ pub fn native_to_bytes(
// delegate to the BCS serialization for `Value`
let serialized_value_opt = match context.type_to_type_layout(&arg_type)? {
None => None,
Some(layout) => ref_to_val.read_ref().simple_serialize(&layout),
Some(layout) => unsafe { ref_to_val.read_ref() }.simple_serialize(&layout),
};
let serialized_value = match serialized_value_opt {
None => {
Expand Down
2 changes: 1 addition & 1 deletion language/move-stdlib/src/natives/debug.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ pub fn native_print(
let r = pop_arg!(args, Reference);

let mut buf = String::new();
print_reference(&mut buf, &r)?;
unsafe { print_reference(&mut buf, &r)? };
println!("[debug] {}", buf);
}

Expand Down
15 changes: 8 additions & 7 deletions language/move-stdlib/src/natives/vector.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ pub fn native_length(

let r = pop_arg!(args, VectorRef);
let cost = native_gas(context.cost_table(), NativeCostIndex::LENGTH, 1);
NativeResult::map_partial_vm_result_one(cost, r.len(&ty_args[0]))
NativeResult::map_partial_vm_result_one(cost, unsafe { r.len(&ty_args[0]) })
}

pub fn native_push_back(
Expand All @@ -54,7 +54,7 @@ pub fn native_push_back(
NativeCostIndex::PUSH_BACK,
e.size().get() as usize,
);
NativeResult::map_partial_vm_result_empty(cost, r.push_back(e, &ty_args[0]))
NativeResult::map_partial_vm_result_empty(cost, unsafe { r.push_back(e, &ty_args[0]) })
}

pub fn native_borrow(
Expand All @@ -70,8 +70,7 @@ pub fn native_borrow(
let cost = native_gas(context.cost_table(), NativeCostIndex::BORROW, 1);
NativeResult::map_partial_vm_result_one(
cost,
r.borrow_elem(idx, &ty_args[0])
.map_err(native_error_to_abort),
unsafe { r.borrow_elem(idx, &ty_args[0]) }.map_err(native_error_to_abort),
)
}

Expand All @@ -85,7 +84,10 @@ pub fn native_pop(

let r = pop_arg!(args, VectorRef);
let cost = native_gas(context.cost_table(), NativeCostIndex::POP_BACK, 1);
NativeResult::map_partial_vm_result_one(cost, r.pop(&ty_args[0]).map_err(native_error_to_abort))
NativeResult::map_partial_vm_result_one(
cost,
unsafe { r.pop(&ty_args[0]) }.map_err(native_error_to_abort),
)
}

pub fn native_destroy_empty(
Expand Down Expand Up @@ -118,8 +120,7 @@ pub fn native_swap(
let cost = native_gas(context.cost_table(), NativeCostIndex::SWAP, 1);
NativeResult::map_partial_vm_result_empty(
cost,
r.swap(idx1, idx2, &ty_args[0])
.map_err(native_error_to_abort),
unsafe { r.swap(idx1, idx2, &ty_args[0]) }.map_err(native_error_to_abort),
)
}

Expand Down
4 changes: 3 additions & 1 deletion language/move-vm/runtime/src/debug.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,9 @@ impl DebugContext {
println!(" Locals:");
if function_desc.local_count() > 0 {
let mut s = String::new();
values::debug::print_locals(&mut s, locals).unwrap();
unsafe {
values::debug::print_locals(&mut s, locals).unwrap();
}
println!("{}", s);
} else {
println!(" (none)");
Expand Down
86 changes: 61 additions & 25 deletions language/move-vm/runtime/src/interpreter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,8 @@ impl Interpreter {
debug_writeln!(buf)?;
debug_writeln!(buf, " Locals:")?;
if func.local_count() > 0 {
values::debug::print_locals(buf, &frame.locals)?;
// see REFERENCE SAFETY EXPLANATION in values
unsafe { values::debug::print_locals(buf, &frame.locals)? };
debug_writeln!(buf)?;
} else {
debug_writeln!(buf, " (none)")?;
Expand All @@ -514,7 +515,8 @@ impl Interpreter {
// TODO: Currently we do not know the types of the values on the operand stack.
// Revisit.
debug_write!(buf, " [{}] ", idx)?;
values::debug::print_value(buf, val)?;
// see REFERENCE SAFETY EXPLANATION in values
unsafe { values::debug::print_value(buf, val)? };
debug_writeln!(buf)?;
}
Ok(())
Expand Down Expand Up @@ -558,10 +560,28 @@ impl Interpreter {
}
internal_state.push_str(format!("{}* {:?}\n", i, code[pc]).as_str());
}
internal_state.push_str(format!("Locals:\n{}\n", current_frame.locals).as_str());
let locals_str = {
let mut buf = String::new();
// see REFERENCE SAFETY EXPLANATION in values
let res = unsafe { values::debug::print_locals(&mut buf, &current_frame.locals) };
match res {
Ok(()) => buf,
Err(err) => format!("!!!ERRROR COULD NOT PRINT LOCALS {:?}!!!", err),
}
};
internal_state.push_str(format!("Locals:\n{}\n", locals_str).as_str());
internal_state.push_str("Operand Stack:\n");
for value in &self.operand_stack.0 {
internal_state.push_str(format!("{}\n", value).as_str());
let value_str = {
let mut buf = String::new();
// see REFERENCE SAFETY EXPLANATION in values
let res = unsafe { values::debug::print_value(&mut buf, value) };
match res {
Ok(()) => buf,
Err(err) => format!("!!!ERRROR COULD NOT PRINT VALUE {:?}!!!", err),
}
};
internal_state.push_str(format!("{}\n", value_str).as_str());
}
internal_state
}
Expand Down Expand Up @@ -906,16 +926,16 @@ impl Frame {
}
Bytecode::ReadRef => {
let reference = interpreter.operand_stack.pop_as::<Reference>()?;
let value = reference.read_ref();
// see REFERENCE SAFETY EXPLANATION in values
let value = unsafe { reference.read_ref() };
gas_status.charge_instr_with_size(Opcodes::READ_REF, value.size())?;
interpreter.operand_stack.push(value)?;
}
Bytecode::WriteRef => {
let reference = interpreter.operand_stack.pop_as::<Reference>()?;
let value = interpreter.operand_stack.pop()?;
gas_status.charge_instr_with_size(Opcodes::WRITE_REF, value.size())?;
// safe due as long as the code being executed passed the reference
// safety checks
// see REFERENCE SAFETY EXPLANATION in values
unsafe {
reference.write_ref(value)?;
}
Expand Down Expand Up @@ -1042,18 +1062,20 @@ impl Frame {
let rhs = interpreter.operand_stack.pop()?;
gas_status
.charge_instr_with_size(Opcodes::EQ, lhs.size().add(rhs.size()))?;
// see REFERENCE SAFETY EXPLANATION in values
interpreter
.operand_stack
.push(Value::bool(lhs.equals(&rhs)?))?;
.push(Value::bool(unsafe { lhs.equals(&rhs)? }))?;
}
Bytecode::Neq => {
let lhs = interpreter.operand_stack.pop()?;
let rhs = interpreter.operand_stack.pop()?;
gas_status
.charge_instr_with_size(Opcodes::NEQ, lhs.size().add(rhs.size()))?;
// see REFERENCE SAFETY EXPLANATION in values
interpreter
.operand_stack
.push(Value::bool(!lhs.equals(&rhs)?))?;
.push(Value::bool(unsafe { !lhs.equals(&rhs)? }))?;
}
Bytecode::MutBorrowGlobal(sd_idx) | Bytecode::ImmBorrowGlobal(sd_idx) => {
let addr = interpreter.operand_stack.pop_as::<AccountAddress>()?;
Expand Down Expand Up @@ -1100,11 +1122,14 @@ impl Frame {
Bytecode::MoveTo(sd_idx) => {
let resource = interpreter.operand_stack.pop()?;
let signer_reference = interpreter.operand_stack.pop_as::<StructRef>()?;
let addr = signer_reference
.borrow_field(0)?
.value_as::<Reference>()?
.read_ref()
.value_as::<AccountAddress>()?;
// see REFERENCE SAFETY EXPLANATION in values
let addr = unsafe {
signer_reference
.borrow_field(0)?
.value_as::<Reference>()?
.read_ref()
.value_as::<AccountAddress>()?
};
let ty = resolver.get_struct_type(*sd_idx);
// REVIEW: Can we simplify Interpreter::move_to?
let size = interpreter.move_to(data_store, addr, &ty, resource)?;
Expand All @@ -1113,11 +1138,14 @@ impl Frame {
Bytecode::MoveToGeneric(si_idx) => {
let resource = interpreter.operand_stack.pop()?;
let signer_reference = interpreter.operand_stack.pop_as::<StructRef>()?;
let addr = signer_reference
.borrow_field(0)?
.value_as::<Reference>()?
.read_ref()
.value_as::<AccountAddress>()?;
// see REFERENCE SAFETY EXPLANATION in values
let addr = unsafe {
signer_reference
.borrow_field(0)?
.value_as::<Reference>()?
.read_ref()
.value_as::<AccountAddress>()?
};
let ty = resolver.instantiate_generic_type(*si_idx, self.ty_args())?;
let size = interpreter.move_to(data_store, addr, &ty, resource)?;
gas_status.charge_instr_with_size(Opcodes::MOVE_TO_GENERIC, size)?;
Expand Down Expand Up @@ -1145,33 +1173,40 @@ impl Frame {
Bytecode::VecLen(si) => {
let vec_ref = interpreter.operand_stack.pop_as::<VectorRef>()?;
gas_status.charge_instr(Opcodes::VEC_LEN)?;
let value = vec_ref.len(resolver.single_type_at(*si))?;
// see REFERENCE SAFETY EXPLANATION in values
let value = unsafe { vec_ref.len(resolver.single_type_at(*si))? };
interpreter.operand_stack.push(value)?;
}
Bytecode::VecImmBorrow(si) => {
let idx = interpreter.operand_stack.pop_as::<u64>()? as usize;
let vec_ref = interpreter.operand_stack.pop_as::<VectorRef>()?;
gas_status.charge_instr(Opcodes::VEC_IMM_BORROW)?;
let value = vec_ref.borrow_elem(idx, resolver.single_type_at(*si))?;
// see REFERENCE SAFETY EXPLANATION in values
let value =
unsafe { vec_ref.borrow_elem(idx, resolver.single_type_at(*si))? };
interpreter.operand_stack.push(value)?;
}
Bytecode::VecMutBorrow(si) => {
let idx = interpreter.operand_stack.pop_as::<u64>()? as usize;
let vec_ref = interpreter.operand_stack.pop_as::<VectorRef>()?;
gas_status.charge_instr(Opcodes::VEC_MUT_BORROW)?;
let value = vec_ref.borrow_elem(idx, resolver.single_type_at(*si))?;
// see REFERENCE SAFETY EXPLANATION in values
let value =
unsafe { vec_ref.borrow_elem(idx, resolver.single_type_at(*si))? };
interpreter.operand_stack.push(value)?;
}
Bytecode::VecPushBack(si) => {
let elem = interpreter.operand_stack.pop()?;
let vec_ref = interpreter.operand_stack.pop_as::<VectorRef>()?;
gas_status.charge_instr_with_size(Opcodes::VEC_PUSH_BACK, elem.size())?;
vec_ref.push_back(elem, resolver.single_type_at(*si))?;
// see REFERENCE SAFETY EXPLANATION in values
unsafe { vec_ref.push_back(elem, resolver.single_type_at(*si))? };
}
Bytecode::VecPopBack(si) => {
let vec_ref = interpreter.operand_stack.pop_as::<VectorRef>()?;
gas_status.charge_instr(Opcodes::VEC_POP_BACK)?;
let value = vec_ref.pop(resolver.single_type_at(*si))?;
// see REFERENCE SAFETY EXPLANATION in values
let value = unsafe { vec_ref.pop(resolver.single_type_at(*si))? };
interpreter.operand_stack.push(value)?;
}
Bytecode::VecUnpack(si, num) => {
Expand All @@ -1188,7 +1223,8 @@ impl Frame {
let idx1 = interpreter.operand_stack.pop_as::<u64>()? as usize;
let vec_ref = interpreter.operand_stack.pop_as::<VectorRef>()?;
gas_status.charge_instr(Opcodes::VEC_SWAP)?;
vec_ref.swap(idx1, idx2, resolver.single_type_at(*si))?;
// see REFERENCE SAFETY EXPLANATION in values
unsafe { vec_ref.swap(idx1, idx2, resolver.single_type_at(*si))? };
}
}
// invariant: advance to pc +1 is iff instruction at pc executed without aborting
Expand Down
12 changes: 12 additions & 0 deletions language/move-vm/types/src/values/mod.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
// Copyright (c) The Diem Core Contributors
// SPDX-License-Identifier: Apache-2.0

//! Implementation for Move values
//!
//! REFERENCE SAFETY EXPLANATION
//! References in Move are proved safe statically in the reference safety check
//! (bytecode_verifier/reference_safety). This check guarantees that there are no dangling
//! references and no memory leaks. But the access pattern for reading/writing to Values must
//! abide by these safety rules (which the VM interpreter does. You should refrain from writing to
//! reference values outside of the VM or native functions (with special care))
//! However, it is **very important** that any instance of `Locals` is not dropped until after
//! all Values are consumed. If this is not followed, reading/writing to Values *will* read from
//! invalid pointers.
pub mod values_impl;

#[cfg(test)]
Expand Down
28 changes: 17 additions & 11 deletions language/move-vm/types/src/values/value_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@ fn locals() -> PartialVMResult<()> {
}
locals.store_loc(1, Value::u64(42))?;

assert!(locals.copy_loc(1)?.equals(&Value::u64(42))?);
let r = locals.borrow_loc(1)?.value_as::<Reference>()?;
assert!(r.read_ref().equals(&Value::u64(42))?);
assert!(locals.move_loc(1)?.equals(&Value::u64(42))?);
unsafe {
assert!(locals.copy_loc(1)?.equals(&Value::u64(42))?);
let r = locals.borrow_loc(1)?.value_as::<Reference>()?;
assert!(r.read_ref().equals(&Value::u64(42))?);
assert!(locals.move_loc(1)?.equals(&Value::u64(42))?);
}

assert!(locals.copy_loc(1).is_err());
assert!(locals.move_loc(1).is_err());
Expand All @@ -39,7 +41,9 @@ fn struct_pack_and_unpack() -> PartialVMResult<()> {

assert!(vals.len() == unpacked.len());
for (v1, v2) in vals.iter().zip(unpacked.iter()) {
assert!(v1.equals(v2)?);
unsafe {
assert!(v1.equals(v2)?);
}
}

Ok(())
Expand All @@ -54,7 +58,7 @@ fn struct_borrow_field() -> PartialVMResult<()> {
)?;
let r: StructRef = locals.borrow_loc(0)?.value_as()?;

{
unsafe {
let f: Reference = r.borrow_field(1)?.value_as()?;
assert!(f.read_ref().equals(&Value::bool(false))?);
}
Expand All @@ -64,7 +68,7 @@ fn struct_borrow_field() -> PartialVMResult<()> {
f.write_ref(Value::bool(true))?;
}

{
unsafe {
let f: Reference = r.borrow_field(1)?.value_as()?;
assert!(f.read_ref().equals(&Value::bool(true))?);
}
Expand All @@ -87,7 +91,7 @@ fn struct_borrow_nested() -> PartialVMResult<()> {
let r1: StructRef = locals.borrow_loc(0)?.value_as()?;
let r2: StructRef = r1.borrow_field(1)?.value_as()?;

{
unsafe {
let r3: Reference = r2.borrow_field(0)?.value_as()?;
assert!(r3.read_ref().equals(&Value::u64(20))?);
}
Expand All @@ -97,13 +101,15 @@ fn struct_borrow_nested() -> PartialVMResult<()> {
r3.write_ref(Value::u64(30))?;
}

{
unsafe {
let r3: Reference = r2.borrow_field(0)?.value_as()?;
assert!(r3.read_ref().equals(&Value::u64(30))?);
}

assert!(r2.into_ref().read_ref().equals(&inner(30))?);
assert!(r1.into_ref().read_ref().equals(&outer(30))?);
unsafe {
assert!(r2.into_ref().read_ref().equals(&inner(30))?);
assert!(r1.into_ref().read_ref().equals(&outer(30))?);
}

Ok(())
}
Expand Down
Loading

0 comments on commit e72b36a

Please sign in to comment.