Skip to content

Commit bd9e6cb

Browse files
youknowoneyouknowone
authored
Clear frame locals and stack on generator close + Add dir_fd support for rmdir, remove/unlink, scandir (RustPython#7222)
* Clear frame locals and stack on generator close Add Frame::clear_locals_and_stack() to release references held by closed generators/coroutines, matching _PyFrame_ClearLocals behavior. Call it from Coro::close() after marking the coroutine as closed. Update test_generators.py expectedFailure markers accordingly. * Add dir_fd support for rmdir, remove/unlink, scandir - rmdir: use unlinkat(fd, path, AT_REMOVEDIR) when dir_fd given - remove/unlink: use unlinkat(fd, path, 0) when dir_fd given - scandir: accept fd via fdopendir, add ScandirIteratorFd - listdir: rewrite fd path to use raw readdir instead of nix::dir::Dir - DirEntry: add d_type and dir_fd fields for fd-based scandir - Update supports_fd/supports_dir_fd entries accordingly * cells_free
1 parent 3b394f1 commit bd9e6cb

10 files changed

Lines changed: 450 additions & 93 deletions

File tree

‎Lib/test/test_generators.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -660,7 +660,6 @@ def f():
660660
with self.assertRaises(RuntimeError):
661661
gen.close()
662662

663-
@unittest.expectedFailure # TODO: RUSTPYTHON; no deterministic GC finalization
664663
def test_close_releases_frame_locals(self):
665664
# See gh-118272
666665

@@ -684,6 +683,7 @@ def genfn():
684683

685684
# See https://github.com/python/cpython/issues/125723
686685
class GeneratorDeallocTest(unittest.TestCase):
686+
@unittest.expectedFailure # TODO: RUSTPYTHON; frame uses shared Arc, no ownership transfer
687687
def test_frame_outlives_generator(self):
688688
def g1():
689689
a = 42

‎Lib/test/test_os.py‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5312,7 +5312,6 @@ def test_bytes_like(self):
53125312
with self.assertRaises(TypeError):
53135313
os.scandir(path_bytes)
53145314

5315-
@unittest.expectedFailure # TODO: RUSTPYTHON; AssertionError: <builtin_function_or_method object at 0xba3106920> not found in {<builtin_function_or_method object at 0xba31078e0>, <builtin_function_or_method object at 0xba31079c0>, <builtin_function_or_method object at 0xba3107b10>, <builtin_function_or_method object at 0xba3159500>, <builtin_function_or_method object at 0xba3159570>, <builtin_function_or_method object at 0xba3107800>, <builtin_function_or_method object at 0xba3106760>, <builtin_function_or_method object at 0xba3106a00>, <builtin_function_or_method object at 0xba3106990>, <builtin_function_or_method object at 0xba3107330>, <builtin_function_or_method object at 0xba31072c0>, <builtin_function_or_method object at 0xba31064c0>}
53165315
@unittest.skipUnless(os.listdir in os.supports_fd,
53175316
'fd support for listdir required for this test.')
53185317
def test_fd(self):

‎crates/common/src/os.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ pub mod ffi {
118118

119119
fn from_bytes(slice: &[u8]) -> &OsStr {
120120
// WASI strings are guaranteed to be UTF-8
121-
OsStr::new(std::str::from_utf8(slice).expect("wasip2 strings are UTF-8"))
121+
OsStr::new(core::str::from_utf8(slice).expect("wasip2 strings are UTF-8"))
122122
}
123123
}
124124

‎crates/vm/src/builtins/frame.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -179,8 +179,8 @@ impl Py<Frame> {
179179
}
180180
}
181181

182-
// Clear the evaluation stack
183-
self.clear_value_stack();
182+
// Clear the evaluation stack and cell references
183+
self.clear_stack_and_cells();
184184

185185
// Clear temporary refs
186186
self.temporary_refs.lock().clear();

‎crates/vm/src/builtins/function.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -431,7 +431,7 @@ impl PyFunction {
431431
if let Some(cell2arg) = code.cell2arg.as_deref() {
432432
for (cell_idx, arg_idx) in cell2arg.iter().enumerate().filter(|(_, i)| **i != -1) {
433433
let x = fastlocals[*arg_idx as usize].take();
434-
frame.cells_frees[cell_idx].set(x);
434+
frame.set_cell_contents(cell_idx, x);
435435
}
436436
}
437437

‎crates/vm/src/builtins/super.rs‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -85,15 +85,17 @@ impl Initializer for PySuper {
8585
if frame.code.arg_count == 0 {
8686
return Err(vm.new_runtime_error("super(): no arguments"));
8787
}
88-
let obj = frame.fastlocals.lock()[0]
89-
.clone()
88+
// Lock fastlocals briefly to get arg[0], then drop the guard
89+
// before calling get_cell_contents (which also locks fastlocals).
90+
let first_arg = frame.fastlocals.lock()[0].clone();
91+
let obj = first_arg
9092
.or_else(|| {
9193
if let Some(cell2arg) = frame.code.cell2arg.as_deref() {
9294
cell2arg[..frame.code.cellvars.len()]
9395
.iter()
9496
.enumerate()
9597
.find(|(_, arg_idx)| **arg_idx == 0)
96-
.and_then(|(cell_idx, _)| frame.cells_frees[cell_idx].get())
98+
.and_then(|(cell_idx, _)| frame.get_cell_contents(cell_idx))
9799
} else {
98100
None
99101
}
@@ -104,8 +106,8 @@ impl Initializer for PySuper {
104106
for (i, var) in frame.code.freevars.iter().enumerate() {
105107
if var.as_bytes() == b"__class__" {
106108
let i = frame.code.cellvars.len() + i;
107-
let class = frame.cells_frees[i]
108-
.get()
109+
let class = frame
110+
.get_cell_contents(i)
109111
.ok_or_else(|| vm.new_runtime_error("super(): empty __class__ cell"))?;
110112
typ = Some(class.downcast().map_err(|o| {
111113
vm.new_type_error(format!(

‎crates/vm/src/coroutine.rs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,9 @@ impl Coro {
207207
)
208208
});
209209
self.closed.store(true);
210+
// Release frame locals and stack to free references held by the
211+
// closed generator, matching gen_send_ex2 with close_on_completion.
212+
self.frame.clear_locals_and_stack();
210213
match result {
211214
Ok(ExecutionResult::Yield(_)) => {
212215
Err(vm.new_runtime_error(format!("{} ignored GeneratorExit", gen_name(jen, vm))))

‎crates/vm/src/frame.rs‎

Lines changed: 57 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,8 @@ struct FrameState {
5353
// We need 1 stack per frame
5454
/// The main data frame of the stack machine
5555
stack: BoxVec<Option<PyObjectRef>>,
56+
/// Cell and free variable references (cellvars + freevars).
57+
cells_frees: Box<[PyCellRef]>,
5658
/// index of last instruction ran
5759
#[cfg(feature = "threading")]
5860
lasti: u32,
@@ -93,7 +95,6 @@ pub struct Frame {
9395
pub func_obj: Option<PyObjectRef>,
9496

9597
pub fastlocals: PyMutex<Box<[Option<PyObjectRef>]>>,
96-
pub(crate) cells_frees: Box<[PyCellRef]>,
9798
pub locals: ArgMapping,
9899
pub globals: PyDictRef,
99100
pub builtins: PyObjectRef,
@@ -135,6 +136,7 @@ impl PyPayload for Frame {
135136
unsafe impl Traverse for FrameState {
136137
fn traverse(&self, tracer_fn: &mut TraverseFn<'_>) {
137138
self.stack.traverse(tracer_fn);
139+
self.cells_frees.traverse(tracer_fn);
138140
}
139141
}
140142

@@ -143,7 +145,6 @@ unsafe impl Traverse for Frame {
143145
self.code.traverse(tracer_fn);
144146
self.func_obj.traverse(tracer_fn);
145147
self.fastlocals.traverse(tracer_fn);
146-
self.cells_frees.traverse(tracer_fn);
147148
self.locals.traverse(tracer_fn);
148149
self.globals.traverse(tracer_fn);
149150
self.builtins.traverse(tracer_fn);
@@ -193,13 +194,13 @@ impl Frame {
193194

194195
let state = FrameState {
195196
stack: BoxVec::new(code.max_stackdepth as usize),
197+
cells_frees,
196198
#[cfg(feature = "threading")]
197199
lasti: 0,
198200
};
199201

200202
Self {
201203
fastlocals: PyMutex::new(fastlocals_vec.into_boxed_slice()),
202-
cells_frees,
203204
locals: scope.locals,
204205
globals: scope.globals,
205206
builtins,
@@ -218,9 +219,38 @@ impl Frame {
218219
}
219220
}
220221

221-
/// Clear the evaluation stack. Used by frame.clear() to break reference cycles.
222-
pub(crate) fn clear_value_stack(&self) {
223-
self.state.lock().stack.clear();
222+
/// Clear evaluation stack and state-owned cell/free references.
223+
/// For full local/cell cleanup, call `clear_locals_and_stack()`.
224+
pub(crate) fn clear_stack_and_cells(&self) {
225+
let mut state = self.state.lock();
226+
state.stack.clear();
227+
let _old = core::mem::take(&mut state.cells_frees);
228+
}
229+
230+
/// Clear locals and stack after generator/coroutine close.
231+
/// Releases references held by the frame, matching _PyFrame_ClearLocals.
232+
pub(crate) fn clear_locals_and_stack(&self) {
233+
self.clear_stack_and_cells();
234+
let mut fastlocals = self.fastlocals.lock();
235+
for slot in fastlocals.iter_mut() {
236+
*slot = None;
237+
}
238+
}
239+
240+
/// Get cell contents by cell index. Reads through fastlocals (no state lock needed).
241+
pub(crate) fn get_cell_contents(&self, cell_idx: usize) -> Option<PyObjectRef> {
242+
let nlocals = self.code.varnames.len();
243+
let fastlocals = self.fastlocals.lock();
244+
fastlocals
245+
.get(nlocals + cell_idx)
246+
.and_then(|slot| slot.as_ref())
247+
.and_then(|obj| obj.downcast_ref::<PyCell>())
248+
.and_then(|cell| cell.get())
249+
}
250+
251+
/// Set cell contents by cell index. Only safe to call before frame execution starts.
252+
pub(crate) fn set_cell_contents(&self, cell_idx: usize, value: Option<PyObjectRef>) {
253+
self.state.lock().cells_frees[cell_idx].set(value);
224254
}
225255

226256
/// Store a borrowed back-reference to the owning generator/coroutine.
@@ -296,23 +326,25 @@ impl Frame {
296326
}
297327
}
298328
if !code.cellvars.is_empty() || !code.freevars.is_empty() {
299-
let map_to_dict = |keys: &[&PyStrInterned], values: &[PyCellRef]| {
300-
for (&k, v) in zip(keys, values) {
301-
if let Some(value) = v.get() {
302-
locals.mapping().ass_subscript(k, Some(value), vm)?;
303-
} else {
304-
match locals.mapping().ass_subscript(k, None, vm) {
305-
Ok(()) => {}
306-
Err(e) if e.fast_isinstance(vm.ctx.exceptions.key_error) => {}
307-
Err(e) => return Err(e),
308-
}
309-
}
329+
// Access cells through fastlocals to avoid locking state
330+
// (state may be held by with_exec during frame execution).
331+
for (i, &k) in code.cellvars.iter().enumerate() {
332+
let cell_value = self.get_cell_contents(i);
333+
match locals.mapping().ass_subscript(k, cell_value, vm) {
334+
Ok(()) => {}
335+
Err(e) if e.fast_isinstance(vm.ctx.exceptions.key_error) => {}
336+
Err(e) => return Err(e),
310337
}
311-
Ok(())
312-
};
313-
map_to_dict(&code.cellvars, &self.cells_frees)?;
338+
}
314339
if code.flags.contains(bytecode::CodeFlags::OPTIMIZED) {
315-
map_to_dict(&code.freevars, &self.cells_frees[code.cellvars.len()..])?;
340+
for (i, &k) in code.freevars.iter().enumerate() {
341+
let cell_value = self.get_cell_contents(code.cellvars.len() + i);
342+
match locals.mapping().ass_subscript(k, cell_value, vm) {
343+
Ok(()) => {}
344+
Err(e) if e.fast_isinstance(vm.ctx.exceptions.key_error) => {}
345+
Err(e) => return Err(e),
346+
}
347+
}
316348
}
317349
}
318350
Ok(locals.clone())
@@ -326,7 +358,6 @@ impl Py<Frame> {
326358
let exec = ExecutingFrame {
327359
code: &self.code,
328360
fastlocals: &self.fastlocals,
329-
cells_frees: &self.cells_frees,
330361
locals: &self.locals,
331362
globals: &self.globals,
332363
builtins: &self.builtins,
@@ -372,7 +403,6 @@ impl Py<Frame> {
372403
let exec = ExecutingFrame {
373404
code: &self.code,
374405
fastlocals: &self.fastlocals,
375-
cells_frees: &self.cells_frees,
376406
locals: &self.locals,
377407
globals: &self.globals,
378408
builtins: &self.builtins,
@@ -407,7 +437,6 @@ impl Py<Frame> {
407437
struct ExecutingFrame<'a> {
408438
code: &'a PyRef<PyCode>,
409439
fastlocals: &'a PyMutex<Box<[Option<PyObjectRef>]>>,
410-
cells_frees: &'a [PyCellRef],
411440
locals: &'a ArgMapping,
412441
globals: &'a PyDictRef,
413442
builtins: &'a PyObjectRef,
@@ -1011,7 +1040,7 @@ impl ExecutingFrame<'_> {
10111040
}
10121041
Instruction::DeleteAttr { idx } => self.delete_attr(vm, idx.get(arg)),
10131042
Instruction::DeleteDeref(i) => {
1014-
self.cells_frees[i.get(arg) as usize].set(None);
1043+
self.state.cells_frees[i.get(arg) as usize].set(None);
10151044
Ok(None)
10161045
}
10171046
Instruction::DeleteFast(idx) => {
@@ -1436,7 +1465,7 @@ impl ExecutingFrame<'_> {
14361465
};
14371466
self.push_value(match value {
14381467
Some(v) => v,
1439-
None => self.cells_frees[i]
1468+
None => self.state.cells_frees[i]
14401469
.get()
14411470
.ok_or_else(|| self.unbound_cell_exception(i, vm))?,
14421471
});
@@ -1493,7 +1522,7 @@ impl ExecutingFrame<'_> {
14931522
}
14941523
Instruction::LoadDeref(i) => {
14951524
let idx = i.get(arg) as usize;
1496-
let x = self.cells_frees[idx]
1525+
let x = self.state.cells_frees[idx]
14971526
.get()
14981527
.ok_or_else(|| self.unbound_cell_exception(idx, vm))?;
14991528
self.push_value(x);
@@ -2083,7 +2112,7 @@ impl ExecutingFrame<'_> {
20832112
Instruction::StoreAttr { idx } => self.store_attr(vm, idx.get(arg)),
20842113
Instruction::StoreDeref(i) => {
20852114
let value = self.pop_value();
2086-
self.cells_frees[i.get(arg) as usize].set(Some(value));
2115+
self.state.cells_frees[i.get(arg) as usize].set(Some(value));
20872116
Ok(None)
20882117
}
20892118
Instruction::StoreFast(idx) => {

0 commit comments

Comments
 (0)