| Message ID | 20241209123717.99077-14-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: bundle of prerequisites for HPET implementation | expand |
On Mon, Dec 09, 2024 at 01:37:04PM +0100, Paolo Bonzini wrote: > Date: Mon, 9 Dec 2024 13:37:04 +0100 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: [PATCH 13/26] rust: qom: automatically use Drop trait to implement > instance_finalize > X-Mailer: git-send-email 2.47.1 > > Replace the customizable INSTANCE_FINALIZE with a generic function > that drops the Rust object. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > rust/qemu-api/src/definitions.rs | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > Great idea. It nicely balances the differences between Rust and C QOM conventions. Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
On Tue, Dec 10, 2024 at 4:58 PM Zhao Liu <zhao1.liu@intel.com> wrote: > Great idea. It nicely balances the differences between Rust and C QOM > conventions. Except it does not work. :( Suppose you have pub struct MySuperclass { parent: DeviceState, field: Box<MyData>, ... } impl Drop for MySuperclass { ... } pub struct MySubclass { parent: MySuperclass, ... } When instance_finalize is called for MySubclass, it will walk the struct's list of fields and call the drop method for MySuperclass. Then, object_deinit recurses to the superclass and calls the same drop method again. This will cause double-freeing of the Box<MyData>, or more in general double-dropping. What's happening here is that QOM wants to control the drop order of MySuperclass and MySubclass's fields. To do so, the parent field must be marked ManuallyDrop<>, which is quite ugly. Perhaps we can add a wrapper type ParentField<> that is specific to QOM. This hides the implementation detail of *what* is special about the ParentField, and it will also be easy to check for in the #[derive(Object)] macro. Maybe in the future it will even make sense to have special functions implemented on ParentField, I don't know... Paolo
On Wed, Dec 11, 2024 at 01:42:32PM +0100, Paolo Bonzini wrote: > Date: Wed, 11 Dec 2024 13:42:32 +0100 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: Re: [PATCH 13/26] rust: qom: automatically use Drop trait to > implement instance_finalize > > On Tue, Dec 10, 2024 at 4:58 PM Zhao Liu <zhao1.liu@intel.com> wrote: > > Great idea. It nicely balances the differences between Rust and C QOM > > conventions. > > Except it does not work. :( Suppose you have > > pub struct MySuperclass { > parent: DeviceState, > field: Box<MyData>, > ... > } > > impl Drop for MySuperclass { > ... > } > > pub struct MySubclass { > parent: MySuperclass, > ... > } > > When instance_finalize is called for MySubclass, it will walk the > struct's list of fields and call the drop method for MySuperclass. > Then, object_deinit recurses to the superclass and calls the same drop > method again. This will cause double-freeing of the Box<MyData>, or > more in general double-dropping. Good catch! Yes, there is indeed such an issue... The above example could become a test case :-), which I supposed could trigger a double- dropping error when compiling. > What's happening here is that QOM wants to control the drop order of > MySuperclass and MySubclass's fields. To do so, the parent field must > be marked ManuallyDrop<>, which is quite ugly. Perhaps we can add a > wrapper type ParentField<> that is specific to QOM. This hides the > implementation detail of *what* is special about the ParentField, and > it will also be easy to check for in the #[derive(Object)] macro. > Maybe in the future it will even make sense to have special functions > implemented on ParentField, I don't know... I also looked into the implementation of ManuallyDrop, and I agree with a new ParentField (or simply ObjectParent). This wrapper is simple enough but also useful for QOM. I will pay more attention to the recursed relationships in QOM in review as well... Thanks, Zhao
diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs index 0467e6290e0..d64a581a5cc 100644 --- a/rust/qemu-api/src/definitions.rs +++ b/rust/qemu-api/src/definitions.rs @@ -8,6 +8,13 @@ use crate::bindings::{Object, ObjectClass, TypeInfo}; +unsafe extern "C" fn drop_object<T: ObjectImpl>(obj: *mut Object) { + // SAFETY: obj is an instance of T, since drop_object<T> + // is called from QOM core as the instance_finalize function + // for class T + unsafe { std::ptr::drop_in_place(obj.cast::<T>()) } +} + /// Trait a type must implement to be registered with QEMU. pub trait ObjectImpl: ClassInitImpl + Sized { type Class; @@ -16,7 +23,6 @@ pub trait ObjectImpl: ClassInitImpl + Sized { const ABSTRACT: bool = false; const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = None; const INSTANCE_POST_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = None; - const INSTANCE_FINALIZE: Option<unsafe extern "C" fn(obj: *mut Object)> = None; const TYPE_INFO: TypeInfo = TypeInfo { name: Self::TYPE_NAME.as_ptr(), @@ -29,7 +35,7 @@ pub trait ObjectImpl: ClassInitImpl + Sized { instance_align: core::mem::align_of::<Self>(), instance_init: Self::INSTANCE_INIT, instance_post_init: Self::INSTANCE_POST_INIT, - instance_finalize: Self::INSTANCE_FINALIZE, + instance_finalize: Some(drop_object::<Self>), abstract_: Self::ABSTRACT, class_size: core::mem::size_of::<Self::Class>(), class_init: <Self as ClassInitImpl>::CLASS_INIT,
Replace the customizable INSTANCE_FINALIZE with a generic function that drops the Rust object. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- rust/qemu-api/src/definitions.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)