Skip to content

Commit

Permalink
Fix up stackframe allocation
Browse files Browse the repository at this point in the history
  • Loading branch information
@cgswords
cgswords committed Nov 6, 2024
1 parent 3d202c7 commit 6f6590e
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 100 deletions.
6 changes: 3 additions & 3 deletions external-crates/move/crates/move-vm-runtime/src/execution/interpreter/eval.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,9 @@ pub(super) fn run(

// Run until we're done or we produce an error and bail.
while step(&mut state, &mut run_context, gas_meter)? != StepStatus::Done {
println!("-------------------------------------");
println!("Call Frame:\n{:?}", state.call_stack.current_frame);
println!("{}", {
dbg_println!(flag: eval_step, "-------------------------------------");
dbg_println!(flag: eval_step, "Call Frame:\n{:?}", state.call_stack.current_frame);
dbg_println!(flag: eval_step, "{}", {
let mut buf = String::new();
let _ = state.debug_print_stack_trace(&mut buf, run_context.vtables);
buf
Expand Down
122 changes: 28 additions & 94 deletions external-crates/move/crates/move-vm-runtime/src/execution/interpreter/locals.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,16 +35,13 @@ pub struct BaseHeapId(usize);
/// stackframes.
#[derive(Debug)]
pub struct MachineHeap {
heap: Box<[Value]>,
current_index: u64,
}

/// A stack frame is an allocated frame. It was allocated starting at `start` in the heap. When it
/// is freed, we need to check that we are freeing the one on the end of the heap.
#[derive(Debug)]
pub struct StackFrame {
base_index: usize,
slice: *mut [Value],
slice: Vec<Value>,
}

// -------------------------------------------------------------------------------------------------
Expand Down Expand Up @@ -132,13 +129,7 @@ impl Default for MachineHeap {

impl MachineHeap {
pub fn new() -> Self {
let heap = (0..(1024 * 16))
.map(|_| Value::invalid())
.collect::<Vec<_>>()
.into_boxed_slice();
Self {
heap,
current_index: 0,
}
}

Expand All @@ -149,19 +140,6 @@ impl MachineHeap {
params: Vec<Value>,
size: usize,
) -> PartialVMResult<StackFrame> {
let base_index = self.current_index as usize;
let remaining_space = self.heap.len() - base_index;

// Check if there's enough space to allocate the frame
if size > remaining_space {
return Err(
PartialVMError::new(StatusCode::MEMORY_LIMIT_EXCEEDED).with_message(format!(
"Failed to allocate stack frame: requested size {}, remaining space {}",
size, remaining_space
)),
);
}

// Calculate how many invalid values need to be added
let invalids_len = size - params.len();

Expand All @@ -171,45 +149,11 @@ impl MachineHeap {
.chain((0..invalids_len).map(|_| Value::invalid())) // Fill the rest with `Invalid`
.collect::<Vec<Value>>();

// Create the stack frame
// SAFETY: We are manually creating a slice from the heap array with known bounds,
// and we ensure that the range does not exceed the heap size.
let slice = {
// This is safe because we already checked bounds above.
let slice = &self.heap[base_index..base_index + size];
slice as *const [Value] as *mut [Value]
};
{
let borrow_slice = arena::to_mut_ref_slice(slice);
for (ndx, value) in local_values.into_iter().enumerate() {
borrow_slice[ndx] = value;
}
}
let frame = StackFrame { base_index, slice };

// Move the current index forward
self.current_index += size as u64;

Ok(frame)
Ok(StackFrame { slice: local_values })
}

/// Frees the given stack frame, ensuring that it is the last frame on the heap.
pub fn free_stack_frame(&mut self, frame: StackFrame) -> PartialVMResult<()> {
let current_index = self.current_index as usize;

// Ensure that we are freeing the most recently allocated frame
if frame.base_index + frame.slice.len() != current_index {
return Err(
PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR).with_message(
"Attempt to free a stack frame that is not the last allocated frame"
.to_string(),
),
);
}

// Move the current index back, effectively freeing the space
self.current_index -= frame.slice.len() as u64;

pub fn free_stack_frame(&mut self, _frame: StackFrame) -> PartialVMResult<()> {
Ok(())
}
}
Expand All @@ -220,12 +164,7 @@ impl MachineHeap {

impl StackFrame {
pub fn iter(&self) -> std::slice::Iter<'_, Value> {
arena::mut_to_ref_slice(self.slice).iter()
}

/// Only used for debug prints
pub fn base_index(&self) -> usize {
self.base_index
self.slice.iter()
}

/// Makes a copy of the value, via `value.copy_value`
Expand All @@ -236,7 +175,7 @@ impl StackFrame {
.with_message("Cannot copy from an invalid memory location".to_string()),
);
}
arena::mut_to_ref_slice(self.slice)
self.slice
.get(ndx)
.ok_or_else(|| {
PartialVMError::new(StatusCode::INTERNAL_TYPE_ERROR)
Expand All @@ -255,7 +194,7 @@ impl StackFrame {
}

let value = std::mem::replace(
&mut arena::to_mut_ref_slice(self.slice)[ndx],
&mut self.slice[ndx],
Value::invalid(),
);
Ok(value)
Expand All @@ -267,7 +206,7 @@ impl StackFrame {
return Err(PartialVMError::new(StatusCode::INTERNAL_TYPE_ERROR)
.with_message(format!("Local index out of bounds: {}", ndx)));
}
arena::to_mut_ref_slice(self.slice)[ndx] = x;
self.slice[ndx] = x;
Ok(())
}

Expand All @@ -278,7 +217,7 @@ impl StackFrame {
.with_message("Cannot copy from an invalid memory location".to_string()),
);
}
arena::mut_to_ref_slice(self.slice)
self.slice
.get(ndx)
.ok_or_else(|| {
PartialVMError::new(StatusCode::INTERNAL_TYPE_ERROR)
Expand All @@ -289,7 +228,7 @@ impl StackFrame {

/// Checks if the value at the location is invalid
pub fn is_invalid(&self, ndx: usize) -> PartialVMResult<bool> {
arena::mut_to_ref_slice(self.slice)
self.slice
.get(ndx)
.map(|value| matches!(value, Value(ValueImpl::Invalid)))
.ok_or_else(|| {
Expand All @@ -303,22 +242,28 @@ impl StackFrame {
pub fn drop_all_values(&mut self) -> impl Iterator<Item = (usize, Value)> {
let mut res = vec![];

for ndx in 0..self.slice.len() {
match &arena::mut_to_ref_slice(self.slice)[ndx].0 {
for (ndx, value) in self.slice.iter_mut().enumerate() {
match &value.0 {
ValueImpl::Invalid => (),
ValueImpl::Reference(_) => {
arena::to_mut_ref_slice(self.slice)[ndx] = Value(ValueImpl::Invalid);
let _ = std::mem::replace(value, Value::invalid());
}
ValueImpl::U8(_) |
ValueImpl::U16(_) |
ValueImpl::U32(_) |
ValueImpl::U64(_) |
ValueImpl::U128(_) |
ValueImpl::U256(_) |
ValueImpl::Bool(_) |
ValueImpl::Address(_) |
ValueImpl::Container(_) => {
res.push((
ndx,
std::mem::replace(value, Value::invalid())
))
}
_ => res.push((
ndx,
std::mem::replace(
&mut arena::to_mut_ref_slice(self.slice)[ndx],
Value::invalid(),
),
)),
}
}

res.into_iter()
}
}
Expand All @@ -327,25 +272,14 @@ impl StackFrame {
// Display
// -------------------------------------------------------------------------------------------------

impl std::fmt::Display for MachineHeap {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
writeln!(f, "Heap(current_index: {})", self.current_index)?;
for (i, value) in self.heap.iter().enumerate() {
writeln!(f, " [{}]: {:?}", i, value)?;
}
Ok(())
}
}

impl std::fmt::Display for StackFrame {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
writeln!(
f,
"StackFrame(start: {}, size: {})",
self.base_index,
"StackFrame(size: {})",
self.slice.len()
)?;
for (i, value) in arena::mut_to_ref_slice(self.slice).iter().enumerate() {
for (i, value) in self.slice.iter().enumerate() {
writeln!(f, " [{}]: {:?}", i, value)?;
}
Ok(())
Expand Down
3 changes: 1 addition & 2 deletions external-crates/move/crates/move-vm-runtime/src/execution/interpreter/state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -339,8 +339,7 @@ impl MachineState {
}
internal_state.push_str(
format!(
"Locals (start: {}):\n{}\n",
self.call_stack.current_frame.stack_frame.base_index(),
"Locals:\n{}\n",
self.call_stack.current_frame.stack_frame
)
.as_str(),
Expand Down
2 changes: 1 addition & 1 deletion external-crates/move/crates/move-vm-runtime/src/execution/values/values_impl.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2278,7 +2278,7 @@ impl Display for ValueImpl {
Self::Bool(x) => write!(f, "{}", x),
Self::Address(addr) => write!(f, "Address({})", addr.short_str_lossless()),

Self::Container(r) => write!(f, "{}", r),
Self::Container(r) => write!(f, "Container({})", r),
Self::Reference(r) => write!(f, "(&){}", r),
}
}
Expand Down

0 comments on commit 6f6590e

Please sign in to comment.