Skip to content

feature: Double drop check in debug mode #4242

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
discord9 wants to merge 4 commits into from
Closed

feature: Double drop check in debug mode #4242

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d817694
feat: support `#[attr]` in `partially_init!`
discord9 Oct 23, 2022
09c5d06
feat: add double drop check in debug mode
discord9 Oct 23, 2022
e0c9be0
fix: use Mutex instead of PyMutex in `ID2TYPE`
discord9 Oct 23, 2022
fbc80ca
refactor: cfg cond for `Drop` trait instead
discord9 Oct 26, 2022
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
76 changes: 73 additions & 3 deletions vm/src/object/core.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,13 @@ use std::{
ptr::{self, NonNull},
};

#[cfg(debug_assertions)]
use once_cell::sync::Lazy;

#[cfg(debug_assertions)]
pub static ID2TYPE: Lazy<std::sync::Mutex<std::collections::HashMap<TypeId, String>>> =
Lazy::new(|| std::sync::Mutex::new(std::collections::HashMap::new()));

// so, PyObjectRef is basically equivalent to `PyRc<PyInner<dyn PyObjectPayload>>`, except it's
// only one pointer in width rather than 2. We do that by manually creating a vtable, and putting
// a &'static reference to it inside the `PyRc` rather than adjacent to it, like trait objects do.
Expand Down Expand Up @@ -109,6 +116,9 @@ impl PyObjVTable {
#[repr(C)]
struct PyInner<T> {
ref_count: RefCount,
/// flag to prevent double drop(might not always work, and might lead to seg fault if double drop really happened)
#[cfg(debug_assertions)]
is_drop: PyMutex<bool>,
// TODO: move typeid into vtable once TypeId::of is const
typeid: TypeId,
vtable: &'static PyObjVTable,
Expand Down Expand Up @@ -433,6 +443,8 @@ impl<T: PyObjectPayload> PyInner<T> {
let member_count = typ.slots.member_count;
Box::new(PyInner {
ref_count: RefCount::new(),
#[cfg(debug_assertions)]
is_drop: PyMutex::new(false),
typeid: TypeId::of::<T>(),
vtable: PyObjVTable::of::<T>(),
typ: PyAtomicRef::from(typ),
Expand Down Expand Up @@ -794,6 +806,8 @@ impl PyObject {
// abort drop for whatever reason
return;
}

*ptr.as_ref().0.is_drop.lock() = true;
let drop_dealloc = ptr.as_ref().0.vtable.drop_dealloc;
// call drop only when there are no references in scope - stacked borrows stuff
drop_dealloc(ptr.as_ptr())
Expand Down Expand Up @@ -846,6 +860,28 @@ impl<'a, T: PyObjectPayload> From<&'a Py<T>> for &'a PyObject {
}
}

#[cfg(debug_assertions)]
impl Drop for PyObjectRef {
#[inline]
fn drop(&mut self) {

@youknowone youknowone Oct 25, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Because PyObjectRef::drop is very small, I'd like to suggest to add a whole debug drop , rather than adding a lot bigger debug features inside of the functions.

#[cfg(debug_assertions)]
fn drop(&mut self) {
    // debug drop here
}

#[cfg(not(debug_assertions))
#[inline]
fn drop(&mut self) {
    // production drop here
}

@discord9 discord9 Oct 25, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because PyObjectRef::drop is very small, I'd like to suggest to add a whole debug drop , rather than adding a lot bigger debug features inside of the functions.

#[cfg(debug_assertions)]
fn drop(&mut self) {
    // debug drop here
}

#[cfg(not(debug_assertions))
#[inline]
fn drop(&mut self) {
    // production drop here
}

Doing the refactor right now, after I done refactor, should I amend&force push or create a new commit?

@discord9 discord9 Oct 25, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because PyObjectRef::drop is very small, I'd like to suggest to add a whole debug drop , rather than adding a lot bigger debug features inside of the functions.

#[cfg(debug_assertions)]
fn drop(&mut self) {
    // debug drop here
}

#[cfg(not(debug_assertions))
#[inline]
fn drop(&mut self) {
    // production drop here
}

Also I refactor GC PR(https://github.com/discord9/RustPython/blob/7a3ba7884aec20f42f1bddf290a0e93beb13e87e/vm/src/object/core.rs#L1099) on top of this drop check, which, you can see, make drop() a much larger function, should it be spilted the same way?

impl Drop for PyObjectRef {
    #[inline]
    fn drop(&mut self) {
        #[cfg(debug_assertions)]
        if *self.0.is_drop.lock() {
            error!(
                "Double drop on PyObjectRef with typeid={:?}(Type={:?})",
                self.0.typeid,
                ID2TYPE
                    .lock()
                    .expect("can't read ID2TYPE")
                    .get(&self.0.typeid),
            );
            return;
        }
        #[cfg(feature = "gc")]
        {
            let stat = self.decrement();
            match stat {
                GcStatus::ShouldDrop => unsafe {
                    *self.0.is_drop.lock() = true;
                    PyObject::drop_slow(self.ptr);
                },
                GcStatus::ShouldDropOnly | GcStatus::BufferedDrop => unsafe {
                    *self.0.is_drop.lock() = true;
                    PyObject::drop_only(self.ptr);
                },
                GcStatus::ShouldKeep | GcStatus::DoNothing => (),
            }
        }
        #[cfg(not(feature = "gc"))]
        if self.0.ref_count.dec() {
            #[cfg(debug_assertions)]
            {
                *self.0.is_drop.lock() = true;
            }
            unsafe { PyObject::drop_slow(self.ptr) }
        }
    }
}

@youknowone youknowone Oct 26, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Because they don't share any logic, having different drop function for gc feature looks reasonable.

if *self.0.is_drop.lock() {
error!(
"Double drop on PyObjectRef with typeid={:?}(Type={:?})",
self.0.typeid,
ID2TYPE
.lock()
.expect("can't read ID2TYPE")
.get(&self.0.typeid),
);
return;
}
if self.0.ref_count.dec() {
unsafe { PyObject::drop_slow(self.ptr) }
}
}
}

#[cfg(not(debug_assertions))]
impl Drop for PyObjectRef {
#[inline]
fn drop(&mut self) {
Expand Down Expand Up @@ -950,6 +986,31 @@ impl<T: PyObjectPayload> fmt::Debug for PyRef<T> {
}
}

#[cfg(debug_assertions)]
impl<T: PyObjectPayload> Drop for PyRef<T> {
#[inline]
fn drop(&mut self) {
if *self.0.is_drop.lock() {
error!(
"Double drop on PyRef<{}>",
std::any::type_name::<T>().to_string()
);
return;
}
let tid = TypeId::of::<T>();
ID2TYPE
.lock()
.expect("can't insert into ID2TYPE")
.entry(tid)
.or_insert_with(|| std::any::type_name::<T>().to_string());

if self.0.ref_count.dec() {
unsafe { PyObject::drop_slow(self.ptr.cast::<PyObject>()) }
}
}
}

#[cfg(not(debug_assertions))]
impl<T: PyObjectPayload> Drop for PyRef<T> {
#[inline]
fn drop(&mut self) {
Expand Down Expand Up @@ -1081,21 +1142,26 @@ impl<T: PyObjectPayload> PyWeakRef<T> {
/// either given values or explicitly left uninitialized
macro_rules! partially_init {
(
$ty:path {$($init_field:ident: $init_value:expr),*$(,)?},
$ty:path {$($(#[$attr:meta])? $init_field:ident: $init_value:expr),*$(,)?},
Uninit { $($uninit_field:ident),*$(,)? }$(,)?
) => {{
// check all the fields are there but *don't* actually run it
if false {
#[allow(invalid_value, dead_code, unreachable_code)]
let _ = {$ty {
$($init_field: $init_value,)*
$(
$(#[$attr])?
$init_field: $init_value,
)*
$($uninit_field: unreachable!(),)*
}};
}
let mut m = ::std::mem::MaybeUninit::<$ty>::uninit();
#[allow(unused_unsafe)]
unsafe {
$(::std::ptr::write(&mut (*m.as_mut_ptr()).$init_field, $init_value);)*
$(
$(#[$attr])?
::std::ptr::write(&mut (*m.as_mut_ptr()).$init_field, $init_value);)*
}
m
}};
Expand Down Expand Up @@ -1136,6 +1202,8 @@ pub(crate) fn init_type_hierarchy() -> (PyTypeRef, PyTypeRef, PyTypeRef) {
let type_type_ptr = Box::into_raw(Box::new(partially_init!(
PyInner::<PyType> {
ref_count: RefCount::new(),
#[cfg(debug_assertions)]
is_drop: PyMutex::new(false),
typeid: TypeId::of::<PyType>(),
vtable: PyObjVTable::of::<PyType>(),
dict: None,
Expand All @@ -1148,6 +1216,8 @@ pub(crate) fn init_type_hierarchy() -> (PyTypeRef, PyTypeRef, PyTypeRef) {
let object_type_ptr = Box::into_raw(Box::new(partially_init!(
PyInner::<PyType> {
ref_count: RefCount::new(),
#[cfg(debug_assertions)]
is_drop: PyMutex::new(false),
typeid: TypeId::of::<PyType>(),
vtable: PyObjVTable::of::<PyType>(),
dict: None,
Expand Down