8000 fix: run `__del__`&drop separately · RustPython/RustPython@ce4a863 · GitHub
[go: up one dir, main page]
Skip to content

Commit ce4a863

Browse files
discord9discord9
committed
fix: run __del__&drop separately
1 parent 51f2aea commit ce4a863

File tree

4 files changed

+64
-31
lines changed

4 files changed

+64
-31
lines changed

‎< 8000 !-- -->vm/src/object/core.rs

Lines changed: 35 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ macro_rules! partially_drop {
111111
}
112112

113113
/// drop only(doesn't deallocate)
114-
/// TODO: not drop `header` to prevent UB
114+
/// NOTE: `header` is not drop to prevent UB
115115
unsafe fn drop_only_obj<T: PyObjectPayload>(x: *mut PyObject) {
116116
let obj = x.cast::<PyInner<T>>().as_ref().expect("Non-Null Pointer");
117117
#[cfg(feature = "gc")]
@@ -1020,7 +1020,15 @@ impl PyObject {
10201020
Ok(())
10211021
}
10221022

1023-
/// Can only be called when ref_count has dropped to zero. `ptr` must be valid
1023+
/// only run `__del__`(or `slot_del` depends on the actual object), doesn't drop or dealloc
1024+
pub(in crate::object) unsafe fn del_only(ptr: NonNull<Self>) -> bool {
1025+
if let Err(()) = ptr.as_ref().drop_slow_inner() {
1026+
return false;
1027+
}
1028+
true
1029+
}
1030+
1031+
/// Can only be called when ref_count has dropped to zero. `ptr` must be valid, it run __del__ then drop&dealloc
10241032
#[inline(never)]
10251033
pub(in crate::object) unsafe fn drop_slow(ptr: NonNull<PyObject>) -> bool {
10261034
if let Err(()) = ptr.as_ref().drop_slow_inner() {
@@ -1042,21 +1050,26 @@ impl PyObject {
10421050
true
10431051
}
10441052

1053+
/// only run rust RAII destructor, no __del__ neither dealloc
10451054
pub(in crate::object) unsafe fn drop_only(ptr: NonNull<PyObject>) -> bool {
1046-
if let Err(()) = ptr.as_ref().drop_slow_inner() {
1047-
// abort drop for whatever reason
1048-
return false;
1049-
}
1050-
10511055
if !ptr.as_ref().header().check_set_drop_only() {
10521056
return false;
10531057
}
1054-
// not set is_drop because still havn't dealloc
1058+
// not set PyInner's is_drop because still havn't dealloc
10551059
let drop_only = ptr.as_ref().0.vtable.drop_only;
10561060

10571061
drop_only(ptr.as_ptr());
10581062
true
10591063
}
1064+
/// run object's __del__ and then rust's destructor but doesn't dealloc
1065+
pub(in crate::object) unsafe fn del_drop(ptr: NonNull<PyObject>) -> bool {
1066+
if let Err(()) = ptr.as_ref().drop_slow_inner() {
1067+
// abort drop for whatever reason
1068+
return false;
1069+
}
1070+
1071+
Self::drop_only(ptr)
1072+
}
10601073

10611074
pub(in crate::object) unsafe fn dealloc_only(ptr: NonNull<PyObject>) -> bool {
10621075
#[cfg(feature = "gc")]
@@ -1187,12 +1200,16 @@ impl Drop for PyObjectRef {
11871200
return;
11881201
}
11891202
let stat = self.decrement();
1203+
let ptr = self.ptr;
11901204
match stat {
11911205
GcStatus::ShouldDrop => unsafe {
1192-
PyObject::drop_slow(self.ptr);
1206+
PyObject::drop_slow(ptr);
11931207
},
1194-
GcStatus::GarbageCycle | GcStatus::BufferedDrop => unsafe {
1195-
PyObject::drop_only(self.ptr);
1208+
GcStatus::BufferedDrop => unsafe {
1209+
PyObject::del_drop(ptr);
1210+
},
1211+
GcStatus::GarbageCycle => unsafe {
1212+
PyObject::del_only(ptr);
11961213
},
11971214
GcStatus::ShouldKeep | GcStatus::DoNothing => (),
11981215
}
@@ -1357,12 +1374,16 @@ impl<T: PyObjectPayload> Drop for PyRef<T> {
13571374
.or_insert_with(|| std::any::type_name::<T>().to_string());
13581375

13591376
let stat = self.as_object().decrement();
1377+
let ptr = self.ptr.cast::<PyObject>();
13601378
match stat {
13611379
GcStatus::ShouldDrop => unsafe {
1362-
PyObject::drop_slow(self.ptr.cast::<PyObject>());
1380+
PyObject::drop_slow(ptr);
1381+
},
1382+
GcStatus::BufferedDrop => unsafe {
1383+
PyObject::del_drop(ptr);
13631384
},
1364-
GcStatus::GarbageCycle | GcStatus::BufferedDrop => unsafe {
1365-
PyObject::drop_only(self.ptr.cast::<PyObject>());
1385+
GcStatus::GarbageCycle => unsafe {
1386+
PyObject::del_only(ptr);
13661387
},
13671388
GcStatus::ShouldKeep | GcStatus::DoNothing => (),
13681389
}

‎vm/src/object/gc/collector.rs

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -97,12 +97,6 @@ impl Collector {
9797
PyObject::drop_slow(obj)
9898
}
9999
*/
100-
unsafe fn drop_only(obj: NonNull<PyObject>) -> bool {
101-
PyObject::drop_only(obj)
102-
}
103-
unsafe fn dealloc_only(obj: NonNull<PyObject>) -> bool {
104-
PyObject::dealloc_only(obj)
105-
}
106100
fn collect_cycles(&self, force: bool) -> GcResult {
107101
if Self::IS_GC_THREAD.with(|v| v.get()) {
108102
return (0, 0).into();
@@ -171,7 +165,7 @@ impl Collector {
171165
unsafe {
172166
// only dealloc here, because already drop(only) in Object's impl Drop
173167
// PyObject::dealloc_only(ptr.cast::<PyObject>());
174-
Self::dealloc_only(**ptr);
168+
PyObject::dealloc_only(**ptr);
175169
// obj is dangling after this line?
176170
}
177171
}
@@ -278,14 +272,22 @@ impl Collector {
278272
});
279273
}
280274

281-
// drop all for once in seperate loop to avoid certain cycle ref double drop bug
275+
// first only run __del__ to prevent accesss dropped object hence UB
276+
let can_drops: Vec<_> = white
277+
.iter()
278+
.map(|i| unsafe { PyObject::del_only(*i) })
279+
.collect();
280+
281+
// drop all for once at seperate loop to avoid certain cycle ref double drop bug
282282
// TODO: check and add detailed explain(could be something with __del__ func do)
283-
let can_dealloc: Vec<_> = white
283+
let can_deallocs: Vec<_> = white
284284
.iter()
285-
.map(|i| {
286-
unsafe {
287-
Self::drop_only(*i)
288-
// PyObject::drop_only(i.cast::<PyObject>());
285+
.zip(can_drops)
286+
.map(|(i, can_drop)| {
287+
if can_drop {
288+
unsafe { PyObject::drop_only(*i) }
289+
} else {
290+
false
289291
}
290292
})
291293
.collect();
@@ -295,11 +297,11 @@ impl Collector {
295297
// access pointer of already dropped value's memory region
296298
white
297299
.iter()
298-
.zip(can_dealloc)
300+
.zip(can_deallocs)
299301
.map(|(i, can_dealloc)| {
300302
if can_dealloc {
301303
unsafe {
302-
Self::dealloc_only(*i);
304+
PyObject::dealloc_only(*i);
303305
}
304306
}
305307
})
@@ -435,6 +437,8 @@ impl Collector {
435437
#[inline]
436438
#[allow(unreachable_code)]
437439
pub fn should_gc(&self) -> bool {
440+
// TODO: use "Optimal Heap Limits for Reducing Browser Memory Use"(http://arxiv.org/abs/2204.10455)
441+
// to optimize gc condition
438442
if !self.is_enabled() {
439443
return false;
440444
}

‎vm/src/object/gc/header.rs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,15 @@ use rustpython_common::{
1313
#[derive(Debug)]
1414
pub struct GcHeader {
1515
ref_cnt: PyAtomic<usize>,
16+
/// TODO(discord9): compact color(2bit)+in_cycle(1)+buffered(1)+is_drop(1)+is_dealloc(1)+is_leak(1)=7bit into one byte
1617
color: PyMutex<Color>,
1718
/// prevent RAII to drop&dealloc when in cycle where should be drop&NOT dealloc
1819
in_cycle: PyMutex<bool>,
1920
buffered: PyMutex<bool>,
2021
is_drop: PyMutex<bool>,
2122
/// check for soundness
2223
is_dealloc: PyMutex<bool>,
24+
is_leak: PyMutex<bool>,
2325
exclusive: PyMutex<()>,
2426
gc: PyRc<Collector>,
2527
}
@@ -33,6 +35,7 @@ impl Default for GcHeader {
3335
buffered: PyMutex::new(false),
3436
is_drop: PyMutex::new(false),
3537
is_dealloc: PyMutex::new(false),
38+
is_leak: PyMutex::new(false),
3639
exclusive: PyMutex::new(()),
3740
/// when threading, using a global GC
3841
#[cfg(feature = "threading")]
@@ -207,14 +210,18 @@ impl GcHeader {
207210
// warn!("Try to leak a already leaked object!");
208211
return;
209212
}
213+
*self.is_leak.lock() = true;
214+
/*
210215
const BIT_MARKER: usize = (std::isize::MAX as usize) + 1;
211216
debug_assert_eq!(BIT_MARKER.count_ones(), 1);
212217
debug_assert_eq!(BIT_MARKER.leading_zeros(), 0);
213218
self.ref_cnt.fetch_add(BIT_MARKER, Ordering::Relaxed);
219+
*/
214220
}
215221

216222
pub fn is_leaked(&self) -> bool {
217-
(self.ref_cnt.load(Ordering::Acquire) as isize) < 0
223+
// (self.ref_cnt.load(Ordering::Acquire) as isize) < 0
224+
*self.is_leak.lock()
218225
}
219226
}
220227

‎vm/src/object/gc/mod.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,9 @@ pub enum GcStatus {
1616
/// should be drop by caller
1717
ShouldDrop,
1818
/// because object is part of a garbage cycle, we don't want double dealloc
19+
/// or use after drop, so run `__del__` only. Drop(destructor)&dealloc is handle by gc
1920
GarbageCycle,
20-
/// already buffered, will be dealloc by collector, caller should call `drop_only` to run destructor only but not dealloc memory region
21+
/// already buffered, will be dealloc by collector, caller should call [`PyObject::del_Drop`] to run destructor only but not dealloc memory region
2122
BufferedDrop,
2223
/// should keep and not drop by caller
2324
ShouldKeep,

0 commit comments

Comments
 (0)
0