From d49dc5dbaaa6a18b6496a5281c8c7018b3c4f54f Mon Sep 17 00:00:00 2001 From: Cameron Swords Date: Tue, 3 Sep 2024 14:39:08 -0700 Subject: [PATCH] Swap *const Function to ArenaPointer everywhere, minor arena.rs reorg --- .../crates/move-vm-runtime/src/interpreter.rs | 10 +-- .../move-vm-runtime/src/loader/arena.rs | 82 ++++++++++++++----- .../crates/move-vm-runtime/src/loader/ast.rs | 6 +- .../crates/move-vm-runtime/src/loader/mod.rs | 9 +- .../move-vm-runtime/src/loader/translate.rs | 22 ++--- .../crates/move-vm-runtime/src/runtime.rs | 4 +- 6 files changed, 87 insertions(+), 46 deletions(-) diff --git a/external-crates/move/crates/move-vm-runtime/src/interpreter.rs b/external-crates/move/crates/move-vm-runtime/src/interpreter.rs index 6d0768e93b97d..bdae97f277975 100644 --- a/external-crates/move/crates/move-vm-runtime/src/interpreter.rs +++ b/external-crates/move/crates/move-vm-runtime/src/interpreter.rs @@ -4,7 +4,7 @@ use crate::{ loader::{ - arena, + arena::{self, ArenaPointer}, ast::{Bytecode, Function}, Loader, Resolver, }, @@ -108,7 +108,7 @@ impl Interpreter { /// Entrypoint into the interpreter. All external calls need to be routed through this /// function. pub(crate) fn entrypoint( - fun_ref: *const Function, + funtction: ArenaPointer, ty_args: Vec, args: Vec, data_store: &mut impl DataStore, @@ -121,7 +121,7 @@ impl Interpreter { call_stack: CallStack::new(), runtime_limits_config: loader.vm_config().runtime_limits_config.clone(), }; - let fun_ref = arena::to_ref(fun_ref); + let fun_ref = funtction.to_ref(); profile_open_frame!(gas_meter, fun_ref.pretty_string()); if fun_ref.is_native() { @@ -203,7 +203,7 @@ impl Interpreter { } } ExitCode::Call(function) => { - let func = arena::to_ref(function); + let func = function.to_ref(); #[cfg(feature = "gas-profiler")] let func_name = func.pretty_string(); profile_open_frame!(gas_meter, func_name.clone()); @@ -765,7 +765,7 @@ struct Frame { #[derive(Debug)] enum ExitCode { Return, - Call(*const Function), + Call(ArenaPointer), CallGeneric(FunctionInstantiationIndex), } diff --git a/external-crates/move/crates/move-vm-runtime/src/loader/arena.rs b/external-crates/move/crates/move-vm-runtime/src/loader/arena.rs index 6126e798b8c3c..23f8f73ef5224 100644 --- a/external-crates/move/crates/move-vm-runtime/src/loader/arena.rs +++ b/external-crates/move/crates/move-vm-runtime/src/loader/arena.rs @@ -1,16 +1,30 @@ // Copyright (c) The Move Contributors // SPDX-License-Identifier: Apache-2.0 -// Arena Definitions - #![allow(unsafe_code)] use std::mem::MaybeUninit; use bumpalo::Bump; +// ------------------------------------------------------------------------------------------------- +// ARENA DEFINITIONS +// ------------------------------------------------------------------------------------------------- + +// ----------------------------------------------- +// Types +// ----------------------------------------------- + pub struct Arena(Bump); +/// An Arena Pointer, which allows conversion to references and const. Equality is defined as +/// pointer equality, and clone/copy are shallow. +pub struct ArenaPointer(*const T); + +// ----------------------------------------------- +// Impls +// ----------------------------------------------- + impl Arena { pub fn new() -> Self { Arena(Bump::new()) @@ -26,12 +40,26 @@ impl Arena { } } -// SAFETY: these are okay, if callers follow the documented safety requirements for `Arena`'s -// unsafe methods. -unsafe impl Send for Arena {} -unsafe impl Sync for Arena {} +impl ArenaPointer { + pub fn new(value: *const T) -> Self { + ArenaPointer(value) + } + + #[allow(dead_code)] + pub fn to_const(self) -> *const T { + self.0 + } + + pub fn to_ref<'a>(self) -> &'a T { + to_ref(self.0) + } +} -/// Returns a pointer to a slice, but nulled. This must be set before use. +// ----------------------------------------------- +// Pointer Operations +// ----------------------------------------------- + +///// Returns a pointer to a slice, but nulled. This must be set before use. pub fn null_ptr() -> *const [T] { unsafe { MaybeUninit::<*const [T]>::zeroed().assume_init() } } @@ -52,22 +80,18 @@ pub fn to_ref<'a, T>(value: *const T) -> &'a T { unsafe { &*value as &T } } -#[derive(Clone, Copy)] -pub struct ArenaPointer(*const T); +// ----------------------------------------------- +// Trait Implementations +// ----------------------------------------------- -impl ArenaPointer { - pub fn new(value: *const T) -> Self { - ArenaPointer(value) - } +// SAFETY: these are okay, if callers follow the documented safety requirements for `Arena`'s +// unsafe methods. - pub fn to_const(&self) -> *const T { - self.0 - } +unsafe impl Send for Arena {} +unsafe impl Sync for Arena {} - pub fn to_ref<'a>(&self) -> &'a T { - to_ref(self.0) - } -} +unsafe impl Send for ArenaPointer {} +unsafe impl Sync for ArenaPointer {} impl ::std::fmt::Debug for ArenaPointer { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { @@ -75,5 +99,19 @@ impl ::std::fmt::Debug for ArenaPointer { } } -unsafe impl Send for ArenaPointer {} -unsafe impl Sync for ArenaPointer {} +// Pointer equality +impl PartialEq for ArenaPointer { + fn eq(&self, other: &Self) -> bool { + std::ptr::eq(self.0, other.0) + } +} + +impl Eq for ArenaPointer {} + +impl Clone for ArenaPointer { + fn clone(&self) -> Self { + *self + } +} + +impl Copy for ArenaPointer {} diff --git a/external-crates/move/crates/move-vm-runtime/src/loader/ast.rs b/external-crates/move/crates/move-vm-runtime/src/loader/ast.rs index ed70854d59b01..fceed9d331418 100644 --- a/external-crates/move/crates/move-vm-runtime/src/loader/ast.rs +++ b/external-crates/move/crates/move-vm-runtime/src/loader/ast.rs @@ -214,7 +214,7 @@ pub(crate) enum DatatypeTagType { /// /// Bytecodes operate on a stack machine and each bytecode has side effect on the stack and the /// instruction stream. -#[derive(Clone, Hash, Eq, PartialEq)] +#[derive(Clone, Eq, PartialEq)] pub enum Bytecode { /// Pop and discard the value at the top of the stack. /// The value on the stack must be an copyable type. @@ -336,7 +336,7 @@ pub enum Bytecode { /// /// ```..., arg(1), arg(2), ..., arg(n) -> ..., return_value(1), return_value(2), ..., /// return_value(k)``` - Call(*const Function), + Call(ArenaPointer), CallGeneric(FunctionInstantiationIndex), /// Create an instance of the type specified via `DatatypeHandleIndex` and push it on the stack. /// The values of the fields of the struct, in the order they appear in the struct declaration, @@ -936,7 +936,7 @@ impl ::std::fmt::Debug for Bytecode { Bytecode::CopyLoc(a) => write!(f, "CopyLoc({})", a), Bytecode::MoveLoc(a) => write!(f, "MoveLoc({})", a), Bytecode::StLoc(a) => write!(f, "StLoc({})", a), - Bytecode::Call(a) => write!(f, "Call({})", arena::to_ref(*a).name), + Bytecode::Call(a) => write!(f, "Call({})", a.to_ref().name), Bytecode::CallGeneric(ndx) => write!(f, "CallGeneric({})", ndx), Bytecode::Pack(a) => write!(f, "Pack({})", a), Bytecode::PackGeneric(a) => write!(f, "PackGeneric({})", a), diff --git a/external-crates/move/crates/move-vm-runtime/src/loader/mod.rs b/external-crates/move/crates/move-vm-runtime/src/loader/mod.rs index 3bfafab242e5e..37721fbb688fb 100644 --- a/external-crates/move/crates/move-vm-runtime/src/loader/mod.rs +++ b/external-crates/move/crates/move-vm-runtime/src/loader/mod.rs @@ -2,6 +2,7 @@ // Copyright (c) The Move Contributors // SPDX-License-Identifier: Apache-2.0 +#[allow(unsafe_code)] pub mod arena; pub mod ast; pub mod translate; @@ -584,13 +585,13 @@ impl ModuleCache { func_name: &IdentStr, runtime_id: &ModuleId, link_context: AccountAddress, - ) -> PartialVMResult<*const Function> { + ) -> PartialVMResult> { match self .loaded_modules .get(&(link_context, runtime_id.clone())) .and_then(|module| module.function_map.get(func_name)) { - Some(func_idx) => Ok(func_idx.to_ref()), + Some(func_idx) => Ok(*func_idx), None => Err( PartialVMError::new(StatusCode::FUNCTION_RESOLUTION_FAILURE).with_message(format!( "Cannot find {:?}::{:?} in cache for context {:?}", @@ -849,7 +850,7 @@ impl Loader { ) -> VMResult<( Arc, Arc, - *const Function, + ArenaPointer, LoadedFunctionInstantiation, )> { let link_context = data_store.link_context(); @@ -860,7 +861,7 @@ impl Loader { .resolve_function_by_name(function_name, runtime_id, link_context) .map_err(|err| err.finish(Location::Undefined))?; - let fun_ref = arena::to_ref(function); + let fun_ref = function.to_ref(); let parameters = compiled .signature_at(fun_ref.parameters) diff --git a/external-crates/move/crates/move-vm-runtime/src/loader/translate.rs b/external-crates/move/crates/move-vm-runtime/src/loader/translate.rs index dfa211d58ffa8..b67f185e82b9f 100644 --- a/external-crates/move/crates/move-vm-runtime/src/loader/translate.rs +++ b/external-crates/move/crates/move-vm-runtime/src/loader/translate.rs @@ -28,7 +28,7 @@ struct Context<'a> { link_context: AccountAddress, cache: &'a ModuleCache, module: &'a CompiledModule, - function_map: HashMap, + function_map: HashMap>, single_signature_token_map: BTreeMap, } @@ -77,7 +77,7 @@ pub fn module( let mut struct_instantiations = vec![]; let mut enums = vec![]; let mut enum_instantiations = vec![]; - let mut function_refs: Vec<*const Function> = vec![]; + let mut function_refs: Vec> = vec![]; let mut function_instantiations = vec![]; let mut field_handles = vec![]; let mut field_instantiations: Vec = vec![]; @@ -172,7 +172,12 @@ pub fn module( let function_map = arena::mut_to_ref_slice(loaded_functions) .iter() - .map(|function| (function.name.clone(), function as *const Function)) + .map(|function| { + ( + function.name.clone(), + ArenaPointer::new(function as *const Function), + ) + }) .collect::>(); if DEBUG_FLAGS.function_list_sizes { @@ -200,7 +205,7 @@ pub fn module( if DEBUG_FLAGS.function_resolution { println!("pushing {}", function.to_ref().name.as_ident_str()); } - function_refs.push(function.to_const()); + function_refs.push(*function); break; } } @@ -257,7 +262,7 @@ pub fn module( cache, )?; function_instantiations.push(FunctionInstantiation { - handle: arena::ArenaPointer::new(handle), + handle, instantiation_idx, }); } @@ -287,10 +292,7 @@ pub fn module( function_instantiations, field_handles, field_instantiations, - function_map: function_map - .into_iter() - .map(|(key, value)| (key, arena::ArenaPointer::new(value))) - .collect::>(), + function_map, single_signature_token_map, instantiation_signatures, variant_handles: module.variant_handles().to_vec(), @@ -498,7 +500,7 @@ fn bytecode(context: &mut Context, bytecode: &FF::Bytecode) -> PartialVMResult PartialVMResult<*const Function> { +) -> PartialVMResult> { let func_handle = context.module.function_handle_at(function_handle_index); let func_name = context.module.identifier_at(func_handle.name); let module_handle = context.module.module_handle_at(func_handle.module); diff --git a/external-crates/move/crates/move-vm-runtime/src/runtime.rs b/external-crates/move/crates/move-vm-runtime/src/runtime.rs index a3643e1a77457..7e140ca6da225 100644 --- a/external-crates/move/crates/move-vm-runtime/src/runtime.rs +++ b/external-crates/move/crates/move-vm-runtime/src/runtime.rs @@ -5,7 +5,7 @@ use crate::{ data_cache::TransactionDataCache, interpreter::Interpreter, - loader::{ast::Function, Loader}, + loader::{arena::ArenaPointer, ast::Function, Loader}, native_extensions::NativeContextExtensions, native_functions::{NativeFunction, NativeFunctions}, session::{LoadedFunctionInstantiation, SerializedReturnValues, Session}, @@ -312,7 +312,7 @@ impl VMRuntime { fn execute_function_impl( &self, - func: *const Function, + func: ArenaPointer, ty_args: Vec, param_types: Vec, return_types: Vec,