Skip to content

[move-vm] Removed most RCs from runtime #160

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[move-vm] Removed most RCs from runtime #160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
495f9b6
[move-vm] Removed most RCs from runtime
Feb 15, 2022
42a4540
fixup! [move-vm] Removed most RCs from runtime
tnowacki Apr 1, 2022
d52353e
fixup! fixup! [move-vm] Removed most RCs from runtime
tnowacki Apr 6, 2022
5ffed79
fixup! fixup! fixup! [move-vm] Removed most RCs from runtime
tnowacki Apr 6, 2022
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
12 changes: 7 additions & 5 deletions language/extensions/async/move-async-vm/src/async_vm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,11 +259,13 @@ impl<'r, 'l, S: MoveResolver> AsyncSession<'r, 'l, S> {
.get_data_store()
.load_resource(actor_addr, &state_type)
.map_err(partial_vm_error_to_async)?;
let actor_state = actor_state_global
.borrow_global()
.and_then(|v| v.value_as::<Reference>())
.and_then(|r| r.read_ref())
.map_err(partial_vm_error_to_async)?;
let actor_state = unsafe {
actor_state_global
.borrow_global()
.and_then(|v| v.value_as::<Reference>())
.map_err(partial_vm_error_to_async)?
.read_ref()
};
args.insert(
0,
self.to_bcs(actor_state, &state_type_tag)
Expand Down
4 changes: 2 additions & 2 deletions language/extensions/async/move-async-vm/src/natives.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use move_vm_types::{
loaded_data::runtime_types::Type,
natives::function::{native_gas, NativeResult},
pop_arg,
values::{Value, Vector},
values::Value,
};
use smallvec::smallvec;
use std::collections::VecDeque;
Expand Down Expand Up @@ -75,7 +75,7 @@ fn native_send(
let ext = context.extensions_mut().get_mut::<AsyncExtension>();
let mut bcs_args = vec![];
while args.len() > 2 {
bcs_args.push(pop_arg!(args, Vector).to_vec_u8()?);
bcs_args.push(pop_arg!(args, Vec<u8>));
Copy link
Contributor

@wrwg wrwg Apr 11, 2022

Choose a reason for hiding this comment

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

Ah! That actually works, great. Problem with that kind of magic is that the APIs aren't clearly documented and buried somewhere in all those trait impls.

Copy link
Contributor Author

@tnowacki tnowacki Apr 11, 2022

Choose a reason for hiding this comment

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

agreed its a bit hard to discover, not sure if @vgao1996 has any thoughts on that (to be clear the VMValueCast stuff has been around a while, not new to this PR)

}
bcs_args.reverse();
let message_hash = pop_arg!(args, u64);
Expand Down
31 changes: 10 additions & 21 deletions language/extensions/move-table-extension/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,12 +395,9 @@ fn native_add_box(
let table_context = context.extensions().get::<NativeTableContext>();
let mut table_data = table_context.table_data.borrow_mut();

// see REFERENCE SAFETY EXPLANATION in values
let val = args.pop_back().unwrap();
let key = args
.pop_back()
.unwrap()
.value_as::<Reference>()?
.read_ref()?;
let key = unsafe { args.pop_back().unwrap().value_as::<Reference>()?.read_ref() };
Copy link
Contributor

@wrwg wrwg Apr 11, 2022

Choose a reason for hiding this comment

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

Wondering: can't we isolate the unsafe somewhere in the value impl? Really don't like to see this in user code.

let handle = get_table_handle(&pop_arg!(args, StructRef))?;

let table = table_data.get_or_create_table(context, handle, &ty_args[0], &ty_args[2])?;
Expand Down Expand Up @@ -450,11 +447,8 @@ fn native_borrow_box(
let table_context = context.extensions().get::<NativeTableContext>();
let mut table_data = table_context.table_data.borrow_mut();

let key = args
.pop_back()
.unwrap()
.value_as::<Reference>()?
.read_ref()?;
// see REFERENCE SAFETY EXPLANATION in values
let key = unsafe { args.pop_back().unwrap().value_as::<Reference>()?.read_ref() };
let handle = get_table_handle(&pop_arg!(args, StructRef))?;

let table = table_data.get_or_create_table(context, handle, &ty_args[0], &ty_args[2])?;
Expand All @@ -479,11 +473,8 @@ fn native_contains_box(
let table_context = context.extensions().get::<NativeTableContext>();
let mut table_data = table_context.table_data.borrow_mut();

let key = args
.pop_back()
.unwrap()
.value_as::<Reference>()?
.read_ref()?;
// see REFERENCE SAFETY EXPLANATION in values
let key = unsafe { args.pop_back().unwrap().value_as::<Reference>()?.read_ref() };
let handle = get_table_handle(&pop_arg!(args, StructRef))?;

let table = table_data.get_or_create_table(context, handle, &ty_args[0], &ty_args[2])?;
Expand All @@ -508,11 +499,8 @@ fn native_remove_box(
let table_context = context.extensions().get::<NativeTableContext>();
let mut table_data = table_context.table_data.borrow_mut();

let key = args
.pop_back()
.unwrap()
.value_as::<Reference>()?
.read_ref()?;
// see REFERENCE SAFETY EXPLANATION in values
let key = unsafe { args.pop_back().unwrap().value_as::<Reference>()?.read_ref() };
let handle = get_table_handle(&pop_arg!(args, StructRef))?;
let table = table_data.get_or_create_table(context, handle, &ty_args[0], &ty_args[2])?;
let (val, key_size, val_size) = table.remove(table_context, &key)?;
Expand Down Expand Up @@ -568,7 +556,8 @@ fn get_table_handle(table: &StructRef) -> PartialVMResult<TableHandle> {
let field_ref = table
.borrow_field(HANDLE_FIELD_INDEX)?
.value_as::<Reference>()?;
field_ref.read_ref()?.value_as::<u128>().map(TableHandle)
// see REFERENCE SAFETY EXPLANATION in values
unsafe { field_ref.read_ref().value_as::<u128>().map(TableHandle) }
}

fn serialize(layout: &MoveTypeLayout, val: &Value) -> PartialVMResult<Vec<u8>> {
Expand Down
2 changes: 1 addition & 1 deletion language/move-stdlib/src/natives/bcs.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
100 changes: 74 additions & 26 deletions language/move-vm/runtime/src/interpreter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,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 @@ -515,7 +516,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 @@ -559,10 +561,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 @@ -927,15 +947,19 @@ 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())?;
reference.write_ref(value)?;
// see REFERENCE SAFETY EXPLANATION in values
unsafe {
reference.write_ref(value)?;
}
}
Bytecode::CastU8 => {
gas_status.charge_instr(Opcodes::CAST_U8)?;
Expand Down Expand Up @@ -1052,18 +1076,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 @@ -1110,11 +1136,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 @@ -1123,11 +1152,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 All @@ -1149,46 +1181,60 @@ impl Frame {
let elements = interpreter.operand_stack.popn(*num as u16)?;
let size = AbstractMemorySize::new(*num);
gas_status.charge_instr_with_size(Opcodes::VEC_PACK, size)?;
let value = Vector::pack(resolver.single_type_at(*si), elements)?;
let value = Vector::pack(
&resolver.instantiate_single_type(*si, self.ty_args())?,
elements,
)?;
interpreter.operand_stack.push(value)?;
}
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 vec_ty_arg = &resolver.instantiate_single_type(*si, self.ty_args())?;
let value = unsafe { vec_ref.len(vec_ty_arg)? };
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 vec_ty_arg = &resolver.instantiate_single_type(*si, self.ty_args())?;
let value = unsafe { vec_ref.borrow_elem(idx, vec_ty_arg)? };
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 vec_ty_arg = &resolver.instantiate_single_type(*si, self.ty_args())?;
let value = unsafe { vec_ref.borrow_elem(idx, vec_ty_arg)? };
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
let vec_ty_arg = &resolver.instantiate_single_type(*si, self.ty_args())?;
unsafe { vec_ref.push_back(elem, vec_ty_arg)? };
}
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 vec_ty_arg = &resolver.instantiate_single_type(*si, self.ty_args())?;
let value = unsafe { vec_ref.pop(vec_ty_arg)? };
interpreter.operand_stack.push(value)?;
}
Bytecode::VecUnpack(si, num) => {
let vec_val = interpreter.operand_stack.pop_as::<Vector>()?;
let size = AbstractMemorySize::new(*num);
gas_status.charge_instr_with_size(Opcodes::VEC_UNPACK, size)?;
let elements = vec_val.unpack(resolver.single_type_at(*si), *num)?;
let vec_ty_arg = &resolver.instantiate_single_type(*si, self.ty_args())?;
let elements = vec_val.unpack(vec_ty_arg, *num)?;
for value in elements {
interpreter.operand_stack.push(value)?;
}
Expand All @@ -1198,7 +1244,9 @@ 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
let vec_ty_arg = &resolver.instantiate_single_type(*si, self.ty_args())?;
unsafe { vec_ref.swap(idx1, idx2, vec_ty_arg)? };
}
}
// invariant: advance to pc +1 is iff instruction at pc executed without aborting
Expand Down
Loading