X Tutup
Skip to content

Remove cells_frees duplicate storage from Frame #7351

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
youknowone wants to merge 2 commits into from
Closed

Remove cells_frees duplicate storage from Frame #7351

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d5514ef
Remove cells_frees duplicate storage from Frame
youknowone Mar 4, 2026
577f474
Auto-format: cargo fmt --all
github-actions[bot] Mar 4, 2026
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
55 changes: 33 additions & 22 deletions crates/vm/src/frame.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,8 +581,6 @@ pub struct Frame {
/// tracer function for this frame (usually is None)
pub trace: PyMutex<PyObjectRef>,

/// Cell and free variable references (cellvars + freevars).
cells_frees: FrameUnsafeCell<Box<[PyCellRef]>>,
/// Previous line number for LINE event suppression.
prev_line: FrameUnsafeCell<u32>,

Expand Down Expand Up @@ -627,7 +625,6 @@ unsafe impl Traverse for Frame {
// SAFETY: GC traversal does not run concurrently with frame execution.
unsafe {
(*self.localsplus.get()).traverse(tracer_fn);
(*self.cells_frees.get()).traverse(tracer_fn);
}
self.locals.traverse(tracer_fn);
self.globals.traverse(tracer_fn);
Expand Down Expand Up @@ -660,12 +657,6 @@ impl Frame {
let num_cells = code.cellvars.len();
let nfrees = closure.len();

let cells_frees: Box<[PyCellRef]> =
core::iter::repeat_with(|| PyCell::default().into_ref(&vm.ctx))
.take(num_cells)
.chain(closure.iter().cloned())
.collect();

let nlocalsplus = nlocals
.checked_add(num_cells)
.and_then(|v| v.checked_add(nfrees))
Expand All @@ -677,9 +668,13 @@ impl Frame {
LocalsPlus::new(nlocalsplus, max_stackdepth)
};

// Store cell objects at cellvars and freevars positions
for (i, cell) in cells_frees.iter().enumerate() {
localsplus.fastlocals_mut()[nlocals + i] = Some(cell.clone().into());
// Store cell/free variable objects directly in localsplus
let fastlocals = localsplus.fastlocals_mut();
for i in 0..num_cells {
fastlocals[nlocals + i] = Some(PyCell::default().into_ref(&vm.ctx).into());
}
for (i, cell) in closure.iter().enumerate() {
fastlocals[nlocals + num_cells + i] = Some(cell.clone().into());
}

Self {
Expand All @@ -696,7 +691,6 @@ impl Frame {
code,
func_obj,
lasti: Radium::new(0),
cells_frees: FrameUnsafeCell::new(cells_frees),
prev_line: FrameUnsafeCell::new(0),
trace: PyMutex::new(vm.ctx.none()),
trace_lines: PyMutex::new(true),
Expand Down Expand Up @@ -746,9 +740,9 @@ impl Frame {
/// For full local/cell cleanup, call `clear_locals_and_stack()`.
pub(crate) fn clear_stack_and_cells(&self) {
// SAFETY: Called when frame is not executing (generator closed).
// Cell refs in fastlocals[nlocals..] are cleared by clear_locals_and_stack().
unsafe {
Comment on lines +743 to 744
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 5, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, find the clear_stack_and_cells method definition and all callsites
rg -nP --type=rust -B2 -A8 'fn clear_stack_and_cells' crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 607

🏁 Script executed:

#!/bin/bash
# Find all callsites of clear_stack_and_cells
rg -nP --type=rust '\bclear_stack_and_cells\s*\(' --max-count=50

Repository: RustPython/RustPython

Length of output: 279

🏁 Script executed:

#!/bin/bash
# Look at lines 743-744 and surrounding context to understand the comment
sed -n '735,755p' crates/vm/src/frame.rs | cat -n

Repository: RustPython/RustPython

Length of output: 1210

🏁 Script executed:

#!/bin/bash
# Check the callsite at crates/vm/src/builtins/frame.rs:702
sed -n '695,710p' crates/vm/src/builtins/frame.rs | cat -n

Repository: RustPython/RustPython

Length of output: 554

🏁 Script executed:

#!/bin/bash
# Also check context around line 752 in frame.rs for the other callsite
sed -n '748,760p' crates/vm/src/frame.rs | cat -n

Repository: RustPython/RustPython

Length of output: 704

Rename clear_stack_and_cells to clarify misleading semantics.

The method name and docstring claim it clears cell/free references, but the implementation only calls stack_clear(). The comment at line 743 explicitly states cells are cleared by clear_locals_and_stack() instead. Both callsites handle cells correctly (either cleared before the call or by the wrapper method), so no correctness issue exists, but renaming to clear_stack() would eliminate confusion about the method's actual responsibilities.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/frame.rs` around lines 743 - 744, Rename the misleading method
clear_stack_and_cells() to clear_stack() and update its docstring to state it
only clears the value stack (delegating cell/free clearing to
clear_locals_and_stack()); inside the function keep calling stack_clear() as
before, and update all callsites and references (including any uses alongside
clear_locals_and_stack()) to the new name to maintain behavior; ensure public
API/export names are adjusted and tests/uses compile.

(*self.localsplus.get()).stack_clear();
let _old = core::mem::take(&mut *self.cells_frees.get());
}
}

Expand Down Expand Up @@ -777,8 +771,14 @@ impl Frame {

/// Set cell contents by cell index. Only safe to call before frame execution starts.
pub(crate) fn set_cell_contents(&self, cell_idx: usize, value: Option<PyObjectRef>) {
let nlocals = self.code.varnames.len();
// SAFETY: Called before frame execution starts.
unsafe { (*self.cells_frees.get())[cell_idx].set(value) };
let fastlocals = unsafe { (*self.localsplus.get()).fastlocals() };
fastlocals[nlocals + cell_idx]
.as_ref()
.and_then(|obj| obj.downcast_ref::<PyCell>())
.expect("cell slot empty or not a PyCell")
.set(value);
}

/// Store a borrowed back-reference to the owning generator/coroutine.
Expand Down Expand Up @@ -917,7 +917,6 @@ impl Py<Frame> {
},
lasti: &self.lasti,
object: self,
cells_frees: unsafe { &mut *self.cells_frees.get() },
prev_line: unsafe { &mut *self.prev_line.get() },
monitoring_mask: 0,
};
Expand Down Expand Up @@ -969,7 +968,6 @@ impl Py<Frame> {
builtins_dict: None,
lasti: &self.lasti,
object: self,
cells_frees: unsafe { &mut *self.cells_frees.get() },
prev_line: unsafe { &mut *self.prev_line.get() },
monitoring_mask: 0,
};
Expand Down Expand Up @@ -1010,7 +1008,6 @@ struct ExecutingFrame<'a> {
builtins_dict: Option<&'a PyExact<PyDict>>,
object: &'a Py<Frame>,
lasti: &'a PyAtomic<u32>,
cells_frees: &'a mut Box<[PyCellRef]>,
prev_line: &'a mut u32,
/// Cached monitoring events mask. Reloaded at Resume instruction only,
monitoring_mask: u32,
Expand Down Expand Up @@ -1038,6 +1035,18 @@ impl ExecutingFrame<'_> {
self.lasti.load(Relaxed)
}

/// Access the PyCellRef at the given cell/free variable index.
/// `cell_idx` is 0-based: 0..ncells for cellvars, ncells.. for freevars.
#[inline(always)]
fn cell_ref(&self, cell_idx: usize) -> &PyCell {
let nlocals = self.code.varnames.len();
self.localsplus.fastlocals()[nlocals + cell_idx]
.as_ref()
.expect("cell slot empty")
.downcast_ref::<PyCell>()
.expect("cell slot is not a PyCell")
}

/// Perform deferred stack unwinding after set_f_lineno.
///
/// set_f_lineno cannot pop the value stack directly because the execution
Expand Down Expand Up @@ -1837,7 +1846,7 @@ impl ExecutingFrame<'_> {
}
Instruction::DeleteAttr { namei: idx } => self.delete_attr(vm, idx.get(arg)),
Instruction::DeleteDeref { i } => {
self.cells_frees[i.get(arg) as usize].set(None);
self.cell_ref(i.get(arg) as usize).set(None);
Ok(None)
}
Instruction::DeleteFast { var_num: idx } => {
Expand Down Expand Up @@ -2272,7 +2281,8 @@ impl ExecutingFrame<'_> {
};
self.push_value(match value {
Some(v) => v,
None => self.cells_frees[i]
None => self
.cell_ref(i)
.get()
.ok_or_else(|| self.unbound_cell_exception(i, vm))?,
});
Expand Down Expand Up @@ -2329,7 +2339,8 @@ impl ExecutingFrame<'_> {
}
Instruction::LoadDeref { i } => {
let idx = i.get(arg) as usize;
let x = self.cells_frees[idx]
let x = self
.cell_ref(idx)
.get()
.ok_or_else(|| self.unbound_cell_exception(idx, vm))?;
self.push_value(x);
Expand Down Expand Up @@ -2942,7 +2953,7 @@ impl ExecutingFrame<'_> {
}
Instruction::StoreDeref { i } => {
let value = self.pop_value();
self.cells_frees[i.get(arg) as usize].set(Some(value));
self.cell_ref(i.get(arg) as usize).set(Some(value));
Ok(None)
}
Instruction::StoreFast { var_num: idx } => {
Expand Down
Loading
X Tutup