Skip to content

[move][move-vm][rewrite][cleanup 1/n] Replace signature index references with VM Pointers #20965

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

Closed
wants to merge 3 commits into from
Closed

[move][move-vm][rewrite][cleanup 1/n] Replace signature index references with VM Pointers #20965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
460b03a
Fix up ointer equality
cgswords Jan 22, 2025
9c1da9b
Add cache signatures to the arena
cgswords Jan 24, 2025
87b80cc
Clean up TODOs
cgswords Jan 28, 2025
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
2 changes: 1 addition & 1 deletion external-crates/move/crates/move-cli/tests/sandbox_tests/print_stack_trace/args.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ Call Stack:

Code:
[10] LdU64(1)
[11] VecImmBorrow(2)
[11] VecImmBorrow(Bool)
[12] StLoc(0)
> [13] CallGeneric(0)
[14] StLoc(2)
Expand Down
8 changes: 8 additions & 0 deletions external-crates/move/crates/move-vm-runtime/src/cache/arena.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ impl Arena {
let slice = self.0.alloc_slice_fill_iter(items);
slice as *mut [T]
}

/// SAFETY: it is the caller's responsibility to ensure that `self` is not shared across
/// threads during this call. This should be fine as the translation step that uses an arena
/// should happen in a thread that holds that arena, with no other contention for allocation
/// into it, and nothing should allocate into a LoadedModule after it is loaded.
pub fn alloc_item<T>(&self, item: T) -> *mut T {
self.0.alloc(item) as *mut T
}
}

// -----------------------------------------------
Expand Down
6 changes: 4 additions & 2 deletions external-crates/move/crates/move-vm-runtime/src/dev_utils/in_memory_test_adapter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,8 @@ impl VMTestAdapter<InMemoryStorage> for InMemoryTestAdapter {
package: SerializedPackage,
) -> VMResult<(verif_ast::Package, MoveVM<'extensions>)> {
let Some(storage_id) = package.linkage_table.get(&runtime_id).cloned() else {
// TODO: VM error instead?
// NB: This is a panic because it means something went really wrong -- we should always
// panic if we expect a package to exist and it is not there.
panic!("Did not find runtime ID {runtime_id} in linkage context.");
};
assert_eq!(storage_id, package.storage_id);
Expand All @@ -140,7 +141,8 @@ impl VMTestAdapter<InMemoryStorage> for InMemoryTestAdapter {
package: verif_ast::Package,
) -> VMResult<()> {
let Some(storage_id) = package.linkage_table.get(&runtime_id).cloned() else {
// TODO: VM error instead?
// NB: This is a panic because it means something went really wrong -- we should always
// panic if we expect a package to exist and it is not there.
panic!("Did not find runtime ID {runtime_id} in linkage context.");
};
assert!(storage_id == package.storage_id);
Expand Down
32 changes: 16 additions & 16 deletions external-crates/move/crates/move-vm-runtime/src/execution/interpreter/eval.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,104 +733,104 @@ fn op_step_impl(
Bytecode::Nop => {
gas_meter.charge_simple_instr(S::Nop)?;
}
Bytecode::VecPack(si, num) => {
Bytecode::VecPack(ty_ptr, num) => {
let ty = state
.call_stack
.current_frame
.resolver
.instantiate_single_type(*si, state.call_stack.current_frame.ty_args())?;
.instantiate_single_type(ty_ptr, state.call_stack.current_frame.ty_args())?;
check_depth_of_type(run_context, &ty)?;
gas_meter.charge_vec_pack(make_ty!(&ty), state.last_n_operands(*num as usize)?)?;
let elements = state.pop_n_operands(*num as u16)?;
let value = Vector::pack(&ty, elements)?;
state.push_operand(value)?;
}
Bytecode::VecLen(si) => {
Bytecode::VecLen(ty_ptr) => {
let vec_ref = state.pop_operand_as::<VectorRef>()?;
let ty = &state
.call_stack
.current_frame
.resolver
.instantiate_single_type(*si, state.call_stack.current_frame.ty_args())?;
.instantiate_single_type(ty_ptr, state.call_stack.current_frame.ty_args())?;
gas_meter.charge_vec_len(ResolvableType {
ty,
vtables: run_context.vtables,
})?;
let value = vec_ref.len(ty)?;
state.push_operand(value)?;
}
Bytecode::VecImmBorrow(si) => {
Bytecode::VecImmBorrow(ty_ptr) => {
let idx = state.pop_operand_as::<u64>()? as usize;
let vec_ref = state.pop_operand_as::<VectorRef>()?;
let ty = state
.call_stack
.current_frame
.resolver
.instantiate_single_type(*si, state.call_stack.current_frame.ty_args())?;
.instantiate_single_type(ty_ptr, state.call_stack.current_frame.ty_args())?;
let res = vec_ref.borrow_elem(idx, &ty);
gas_meter.charge_vec_borrow(false, make_ty!(&ty), res.is_ok())?;
state.push_operand(res?)?;
}
Bytecode::VecMutBorrow(si) => {
Bytecode::VecMutBorrow(ty_ptr) => {
let idx = state.pop_operand_as::<u64>()? as usize;
let vec_ref = state.pop_operand_as::<VectorRef>()?;
let ty = &state
.call_stack
.current_frame
.resolver
.instantiate_single_type(*si, state.call_stack.current_frame.ty_args())?;
.instantiate_single_type(ty_ptr, state.call_stack.current_frame.ty_args())?;
let res = vec_ref.borrow_elem(idx, ty);
gas_meter.charge_vec_borrow(true, make_ty!(ty), res.is_ok())?;
state.push_operand(res?)?;
}
Bytecode::VecPushBack(si) => {
Bytecode::VecPushBack(ty_ptr) => {
let elem = state.pop_operand()?;
let vec_ref = state.pop_operand_as::<VectorRef>()?;
let ty = &state
.call_stack
.current_frame
.resolver
.instantiate_single_type(*si, state.call_stack.current_frame.ty_args())?;
.instantiate_single_type(ty_ptr, state.call_stack.current_frame.ty_args())?;
gas_meter.charge_vec_push_back(make_ty!(ty), &elem)?;
vec_ref.push_back(
elem,
ty,
run_context.vm_config.runtime_limits_config.vector_len_max,
)?;
}
Bytecode::VecPopBack(si) => {
Bytecode::VecPopBack(ty_ptr) => {
let vec_ref = state.pop_operand_as::<VectorRef>()?;
let ty = &state
.call_stack
.current_frame
.resolver
.instantiate_single_type(*si, state.call_stack.current_frame.ty_args())?;
.instantiate_single_type(ty_ptr, state.call_stack.current_frame.ty_args())?;
let res = vec_ref.pop(ty);
gas_meter.charge_vec_pop_back(make_ty!(ty), res.as_ref().ok())?;
state.push_operand(res?)?;
}
Bytecode::VecUnpack(si, num) => {
Bytecode::VecUnpack(ty_ptr, num) => {
let vec_val = state.pop_operand_as::<Vector>()?;
let ty = &state
.call_stack
.current_frame
.resolver
.instantiate_single_type(*si, state.call_stack.current_frame.ty_args())?;
.instantiate_single_type(ty_ptr, state.call_stack.current_frame.ty_args())?;
gas_meter.charge_vec_unpack(make_ty!(ty), NumArgs::new(*num), vec_val.elem_views())?;
let elements = vec_val.unpack(ty, *num)?;
for value in elements {
state.push_operand(value)?;
}
}
Bytecode::VecSwap(si) => {
Bytecode::VecSwap(ty_ptr) => {
let idx2 = state.pop_operand_as::<u64>()? as usize;
let idx1 = state.pop_operand_as::<u64>()? as usize;
let vec_ref = state.pop_operand_as::<VectorRef>()?;
let ty = &state
.call_stack
.current_frame
.resolver
.instantiate_single_type(*si, state.call_stack.current_frame.ty_args())?;
.instantiate_single_type(ty_ptr, state.call_stack.current_frame.ty_args())?;
gas_meter.charge_vec_swap(make_ty!(ty))?;
vec_ref.swap(idx1, idx2, ty)?;
}
Expand Down
16 changes: 8 additions & 8 deletions external-crates/move/crates/move-vm-runtime/src/execution/interpreter/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,8 +566,9 @@ impl ModuleDefinitionResolver {
) -> PartialVMResult<Vec<Type>> {
let loaded_module = &*self.module;
let func_inst = loaded_module.function_instantiation_at(idx.0);
let instantiation: Vec<_> = loaded_module
.instantiation_signature_at(func_inst.instantiation_idx)?
let instantiation: Vec<_> = func_inst
.instantiation_signature
.to_ref()
.iter()
.map(|ty| subst(ty, type_params))
.collect::<PartialVMResult<_>>()?;
Expand Down Expand Up @@ -606,8 +607,7 @@ impl ModuleDefinitionResolver {
) -> PartialVMResult<Type> {
let loaded_module = &*self.module;
let struct_inst = loaded_module.struct_instantiation_at(idx.0);
let instantiation =
loaded_module.instantiation_signature_at(struct_inst.instantiation_idx)?;
let instantiation = &struct_inst.instantiation_signature.to_ref();
self.instantiate_type_common(&struct_inst.def, instantiation, ty_args)
}

Expand All @@ -619,8 +619,7 @@ impl ModuleDefinitionResolver {
let loaded_module = &*self.module;
let handle = loaded_module.variant_instantiation_handle_at(vidx);
let enum_inst = loaded_module.enum_instantiation_at(handle.enum_def);
let instantiation =
loaded_module.instantiation_signature_at(enum_inst.instantiation_idx)?;
let instantiation = &enum_inst.instantiation_signature.to_ref();
self.instantiate_type_common(&enum_inst.def, instantiation, ty_args)
}

Expand Down Expand Up @@ -651,16 +650,17 @@ impl ModuleDefinitionResolver {
))))
}

#[allow(dead_code)]
fn single_type_at(&self, idx: SignatureIndex) -> &Type {
self.module.single_type_at(idx)
}

pub(crate) fn instantiate_single_type(
&self,
idx: SignatureIndex,
ty_ptr: &VMPointer<Type>,
ty_args: &[Type],
) -> PartialVMResult<Type> {
let ty = self.single_type_at(idx);
let ty = ty_ptr.to_ref();
if !ty_args.is_empty() {
subst(ty, ty_args)
} else {
Expand Down
13 changes: 10 additions & 3 deletions external-crates/move/crates/move-vm-runtime/src/execution/tracing/tracer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1245,10 +1245,10 @@ impl<'a> VMTracer<'a> {
.instruction(instruction, ty_args, effects, *remaining_gas, pc);
}

B::VecPack(tok, n) => {
B::VecPack(ty_ptr, n) => {
let resolver = &machine.call_stack.current_frame.resolver;
let ty = resolver
.instantiate_single_type(*tok, &machine.call_stack.current_frame.ty_args)
.instantiate_single_type(ty_ptr, &machine.call_stack.current_frame.ty_args)
.ok()?;
let ty = vtables.type_to_fully_annotated_layout(&ty).ok()?;
let ty = AnnotatedTypeLayout::Vector(Box::new(ty));
Expand Down Expand Up @@ -1646,15 +1646,22 @@ impl FunctionTypeInfo {
let layout = (type_layout, ref_type);
Some(TagWithLayoutInfoOpt { tag, layout })
};
let locals = if let Some(locals) = function.locals {
locals.to_ref()
} else {
&vec![]
};
let local_types = function
.parameters
.to_ref()
.iter()
.chain(function.locals.iter())
.chain(locals.iter())
.map(subst_and_layout_type)
.collect::<Option<Vec<_>>>()?;

let return_types = function
.return_
.to_ref()
.iter()
.map(subst_and_layout_type)
.collect::<Option<Vec<_>>>()?;
Expand Down
4 changes: 2 additions & 2 deletions external-crates/move/crates/move-vm-runtime/src/execution/vm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,9 +277,9 @@ impl<'extensions> MoveVM<'extensions> {
let fun_ref = function.to_ref();

// See TODO on LoadedModule to avoid this work
let parameters = fun_ref.parameters.clone();
let parameters = fun_ref.parameters.to_ref().clone();

let return_ = fun_ref.return_.clone();
let return_ = fun_ref.return_.to_ref().clone();

// verify type arguments
self.virtual_tables
Expand Down
Loading
Loading