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

[move-vm][closures] Interpreter #15680

Open
wants to merge 1 commit into
base: wrwg/clos_values
Choose a base branch
from
+184 −27
Open

[move-vm][closures] Interpreter #15680

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 10 additions & 0 deletions third_party/move/move-core/types/src/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ impl ClosureMask {
/// stack than this number.
pub const MAX_ARGS: usize = 64;

pub fn empty() -> Self {
Self(0)
}

pub fn new(mask: u64) -> Self {
Self(mask)
}
Expand All @@ -48,6 +52,12 @@ impl ClosureMask {
self.0
}

/// Returns true if the i'th argument is captured. If `i` is out of range, false will
/// be returned.
pub fn is_captured(&self, i: usize) -> bool {
i < 64 && self.0 & (1 << i) != 0
}

/// Apply a closure mask to a list of elements, returning only those
/// where position `i` is set in the mask (if `collect_captured` is true) or not
/// set (otherwise).
Expand Down
200 changes: 174 additions & 26 deletions third_party/move/move-vm/runtime/src/interpreter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,22 +8,25 @@ use crate::{
frame_type_cache::{
AllRuntimeCaches, FrameTypeCache, NoRuntimeCaches, PerInstructionCache, RuntimeCacheTraits,
},
loader::{LegacyModuleStorageAdapter, Loader, Resolver},
loader::{LazyLoadedFunction, LegacyModuleStorageAdapter, Loader, Resolver},
module_traversal::TraversalContext,
native_extensions::NativeContextExtensions,
native_functions::NativeContext,
runtime_type_checks::{FullRuntimeTypeCheck, NoRuntimeTypeCheck, RuntimeTypeCheck},
storage::ty_tag_converter::TypeTagConverter,
trace, LoadedFunction, ModuleStorage,
};
use fail::fail_point;
use move_binary_format::{
errors::*,
file_format::{
AccessKind, Bytecode, FunctionHandleIndex, FunctionInstantiationIndex, LocalIndex,
SignatureIndex,
},
};
use move_core_types::{
account_address::AccountAddress,
function::ClosureMask,
gas_algebra::{NumArgs, NumBytes, NumTypeNodes},
language_storage::{ModuleId, TypeTag},
vm_status::{StatusCode, StatusType},
Expand All @@ -37,8 +40,8 @@ use move_vm_types::{
},
natives::function::NativeResult,
values::{
self, GlobalValue, IntegerValue, Locals, Reference, SignerRef, Struct, StructRef,
VMValueCast, Value, Vector, VectorRef,
self, AbstractFunction, Closure, GlobalValue, IntegerValue, Locals, Reference, SignerRef,
Struct, StructRef, VMValueCast, Value, Vector, VectorRef,
},
views::TypeView,
};
Expand Down Expand Up @@ -293,7 +296,7 @@ impl InterpreterImpl {
};

let mut current_frame = self
.make_new_frame(gas_meter, loader, function, locals, frame_cache)
.make_new_frame(gas_meter, loader, function, locals, frame_cache.clone())
wrwg marked this conversation as resolved.
Show resolved Hide resolved
.map_err(|err| self.set_location(err))?;

// Access control for the new frame.
Expand Down Expand Up @@ -419,6 +422,8 @@ impl InterpreterImpl {
traversal_context,
extensions,
&function,
ClosureMask::empty(),
vec![],
)?;
continue;
}
Expand All @@ -429,6 +434,8 @@ impl InterpreterImpl {
loader,
function,
frame_cache,
ClosureMask::empty(),
vec![],
)?;
},
ExitCode::CallGeneric(idx) => {
Expand Down Expand Up @@ -518,6 +525,8 @@ impl InterpreterImpl {
traversal_context,
extensions,
&function,
ClosureMask::empty(),
vec![],
)?;
continue;
}
Expand All @@ -528,8 +537,73 @@ impl InterpreterImpl {
loader,
function,
frame_cache,
ClosureMask::empty(),
vec![],
)?;
},
ExitCode::CallClosure(_sig_idx) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we plan to charge gas for dependencies? In the current system, you should call check_dependencies_and_charge_gas for the function's module ID, to ensure that the modules loaded with closure are charged for. You probably should do it after popping the closure from thr stack, since you know function's name and module.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, you should charge for ty tags (see my recent PR for that which you approved)

// TODO(#15664): runtime check whether closure value on stack is same as sig_idx
let (fun, captured) = self
.operand_stack
.pop_as::<Closure>()
.map_err(|e| set_err_info!(current_frame, e))?
.unpack();
let lazy_function = LazyLoadedFunction::expect_this_impl(fun.as_ref())
.map_err(|e| set_err_info!(current_frame, e))?;
let mask = lazy_function.closure_mask();
let function = lazy_function
.with_resolved_function(module_storage, |f| Ok(f.clone()))
.map_err(|e| set_err_info!(current_frame, e))?;

// In difference to regular calls, we skip visibility check.
// It is possible to call a private function of another module via
// a closure.

// Charge gas
let module_id = function.module_id().ok_or_else(|| {
wrwg marked this conversation as resolved.
Show resolved Hide resolved
let err =
PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR)
.with_message(
"Failed to get native function module id".to_string(),
wrwg marked this conversation as resolved.
Show resolved Hide resolved
);
set_err_info!(current_frame, err)
})?;
gas_meter
.charge_call(
module_id,
function.name(),
self.operand_stack
.last_n(function.param_tys().len())
.map_err(|e| set_err_info!(current_frame, e))?,
(function.local_tys().len() as u64).into(),
)
.map_err(|e| set_err_info!(current_frame, e))?;

// Call function
if function.is_native() {
wrwg marked this conversation as resolved.
Show resolved Hide resolved
self.call_native::<RTTCheck, RTCaches>(
&mut current_frame,
&resolver,
data_store,
gas_meter,
traversal_context,
extensions,
&function,
mask,
captured.collect(),
)?
} else {
self.set_new_call_frame::<RTTCheck, RTCaches>(
&mut current_frame,
gas_meter,
loader,
function,
frame_cache.clone(),
mask,
captured.collect(),
)?
}
},
}
}
}
Expand All @@ -541,6 +615,8 @@ impl InterpreterImpl {
loader: &Loader,
function: Rc<LoadedFunction>,
frame_cache: Rc<RefCell<FrameTypeCache>>,
mask: ClosureMask,
captured: Vec<Value>,
) -> VMResult<()> {
match (function.module_id(), current_frame.function.module_id()) {
(Some(module_id), Some(current_module_id)) if module_id != current_module_id => {
Expand All @@ -563,7 +639,14 @@ impl InterpreterImpl {
}

let mut frame = self
.make_call_frame::<RTTCheck, RTCaches>(gas_meter, loader, function, frame_cache)
.make_call_frame::<RTTCheck, RTCaches>(
gas_meter,
loader,
function,
frame_cache,
mask,
captured,
)
.map_err(|err| {
self.attach_state_if_invariant_violation(self.set_location(err), current_frame)
})?;
Expand Down Expand Up @@ -593,21 +676,29 @@ impl InterpreterImpl {
loader: &Loader,
function: Rc<LoadedFunction>,
frame_cache: Rc<RefCell<FrameTypeCache>>,
mask: ClosureMask,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nicer to have frame setups for closures being completely separate, also no need to pass empty mask and empty captured args everywhere. Whatever is here inside can be factored out into helpers.

mut captured: Vec<Value>,
) -> PartialVMResult<Frame> {
let mut locals = Locals::new(function.local_tys().len());
let num_param_tys = function.param_tys().len();

for i in 0..num_param_tys {
locals.store_loc(
num_param_tys - i - 1,
self.operand_stack.pop()?,
loader.vm_config().check_invariant_in_swap_loc,
)?;
for i in (0..num_param_tys).rev() {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could probably use the same helper function you had in ClosureMask to avoid the for loops here? Simialar to call_native

let is_captured = mask.is_captured(i);
let value = if is_captured {
captured.pop().ok_or_else(|| {
PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR)
.with_message("inconsistent closure mask".to_string())
})?
} else {
self.operand_stack.pop()?
};
locals.store_loc(i, value, loader.vm_config().check_invariant_in_swap_loc)?;
graphite-app[bot] marked this conversation as resolved.
Show resolved Hide resolved

let ty_args = function.ty_args();
if RTTCheck::should_perform_checks() {
if RTTCheck::should_perform_checks() && !is_captured {
// Only perform paranoid type check for actual operands on the stack.
// Captured arguments are already verified against function signature.
let ty = self.operand_stack.pop_ty()?;
let expected_ty = &function.local_tys()[num_param_tys - i - 1];
let expected_ty = &function.local_tys()[i];
if !ty_args.is_empty() {
let expected_ty = loader
.ty_builder()
Expand Down Expand Up @@ -673,6 +764,8 @@ impl InterpreterImpl {
traversal_context: &mut TraversalContext,
extensions: &mut NativeContextExtensions,
function: &LoadedFunction,
mask: ClosureMask,
captured: Vec<Value>,
) -> VMResult<()> {
// Note: refactor if native functions push a frame on the stack
self.call_native_impl::<RTTCheck, RTCaches>(
Expand All @@ -683,6 +776,8 @@ impl InterpreterImpl {
traversal_context,
extensions,
function,
mask,
captured,
)
.map_err(|e| match function.module_id() {
Some(id) => {
Expand Down Expand Up @@ -711,28 +806,41 @@ impl InterpreterImpl {
traversal_context: &mut TraversalContext,
extensions: &mut NativeContextExtensions,
function: &LoadedFunction,
mask: ClosureMask,
mut captured: Vec<Value>,
) -> PartialVMResult<()> {
let ty_builder = resolver.loader().ty_builder();

let mut args = VecDeque::new();
let num_param_tys = function.param_tys().len();
for _ in 0..num_param_tys {
args.push_front(self.operand_stack.pop()?);
let mut args = VecDeque::new();
for i in (0..num_param_tys).rev() {
if mask.is_captured(i) {
args.push_front(captured.pop().ok_or_else(|| {
PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR)
.with_message("inconsistent number of captured arguments".to_string())
})?)
} else {
args.push_front(self.operand_stack.pop()?)
}
}
let mut arg_tys = VecDeque::new();

let mut arg_tys = VecDeque::new();
let ty_args = function.ty_args();
if RTTCheck::should_perform_checks() {
for i in 0..num_param_tys {
let ty = self.operand_stack.pop_ty()?;
let expected_ty = &function.param_tys()[num_param_tys - i - 1];
if !ty_args.is_empty() {
let expected_ty = ty_builder.create_ty_with_subst(expected_ty, ty_args)?;
ty.paranoid_check_eq(&expected_ty)?;
for i in (0..num_param_tys).rev() {
let expected_ty = &function.param_tys()[i];
if !mask.is_captured(i) {
let ty = self.operand_stack.pop_ty()?;
if !ty_args.is_empty() {
let expected_ty = ty_builder.create_ty_with_subst(expected_ty, ty_args)?;
ty.paranoid_check_eq(&expected_ty)?;
} else {
ty.paranoid_check_eq(expected_ty)?;
}
arg_tys.push_front(ty);
} else {
ty.paranoid_check_eq(expected_ty)?;
arg_tys.push_front(expected_ty.clone())
}
arg_tys.push_front(ty);
}
}

Expand Down Expand Up @@ -890,6 +998,8 @@ impl InterpreterImpl {
resolver.loader(),
Rc::new(target_func),
frame_cache,
ClosureMask::empty(),
vec![],
)
.map_err(|err| err.to_partial())
},
Expand Down Expand Up @@ -1649,6 +1759,7 @@ enum ExitCode {
Return,
Call(FunctionHandleIndex),
CallGeneric(FunctionInstantiationIndex),
CallClosure(SignatureIndex),
}

impl AccessSpecifierEnv for Frame {
Expand Down Expand Up @@ -1859,6 +1970,7 @@ impl Frame {
Bytecode::CallGeneric(idx) => {
return Ok(ExitCode::CallGeneric(*idx));
},
Bytecode::CallClosure(idx) => return Ok(ExitCode::CallClosure(*idx)),
Bytecode::MutBorrowLoc(idx) | Bytecode::ImmBorrowLoc(idx) => {
let instr = match instruction {
Bytecode::MutBorrowLoc(_) => S::MutBorrowLoc,
Expand Down Expand Up @@ -2199,6 +2311,42 @@ impl Frame {
.operand_stack
.push(reference.test_variant(info.variant)?)?;
},
Bytecode::PackClosure(fh_idx, mask) => {
// TODO(#15664): runtime checks. We should verify that the popped
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW I think @ziaptos refactored the runtime checks so that you wouldn't need to add the checks in the core interpretation loop.

// argument types match the function.
let function = resolver
.build_loaded_function_from_handle_and_ty_args(*fh_idx, vec![])?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will load the function if it is defined outside of this module, we should be charging gas for module loading here as well.

let captured = interpreter.operand_stack.popn(mask.captured_count())?;
let lazy_function = LazyLoadedFunction::new_resolved(
&TypeTagConverter::new(resolver.module_storage().runtime_environment()),
Rc::new(function),
*mask,
)?;
interpreter
.operand_stack
.push(Value::closure(Box::new(lazy_function), captured))?
},
Bytecode::PackClosureGeneric(fi_idx, mask) => {
// TODO(#15664): runtime checks
let ty_args = resolver.instantiate_generic_function(
Some(gas_meter),
*fi_idx,
self.function.ty_args(),
)?;
let function = resolver
.build_loaded_function_from_instantiation_and_ty_args(
*fi_idx, ty_args,
)?;
let captured = interpreter.operand_stack.popn(mask.captured_count())?;
let lazy_function = LazyLoadedFunction::new_resolved(
&TypeTagConverter::new(resolver.module_storage().runtime_environment()),
Rc::new(function),
*mask,
)?;
interpreter
.operand_stack
.push(Value::closure(Box::new(lazy_function), captured))?
},
Bytecode::ReadRef => {
let reference = interpreter.operand_stack.pop_as::<Reference>()?;
gas_meter.charge_read_ref(reference.value_view())?;
Expand Down
1 change: 0 additions & 1 deletion third_party/move/move-vm/runtime/src/loader/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ impl LazyLoadedFunction {
})))
}

#[allow(unused)]
pub(crate) fn new_resolved(
converter: &TypeTagConverter,
fun: Rc<LoadedFunction>,
Expand Down
Loading