From 4ca45ca4da9aadcde229aba9d0922c9d0dc7a9ae Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Fri, 15 May 2020 21:04:44 +0200 Subject: [PATCH] vtable crate: Some refactoring and simplifications --- api/sixtyfps-rs/lib.rs | 2 +- helper_crates/vtable/macro/macro.rs | 53 +++--- helper_crates/vtable/src/lib.rs | 161 +++++++----------- helper_crates/vtable/tests/test_vtable.rs | 5 +- .../corelib/abi/datastructures.rs | 13 +- sixtyfps_runtime/corelib/abi/primitives.rs | 1 - tools/viewer/main.rs | 8 +- 7 files changed, 97 insertions(+), 146 deletions(-) diff --git a/api/sixtyfps-rs/lib.rs b/api/sixtyfps-rs/lib.rs index 3f17f7ac5..ba37a94ba 100644 --- a/api/sixtyfps-rs/lib.rs +++ b/api/sixtyfps-rs/lib.rs @@ -3,7 +3,7 @@ pub use sixtyfps_rs_macro::sixtyfps; /// internal re_exports used by the macro generated pub mod re_exports { pub use const_field_offset::FieldOffsets; - pub use corelib::abi::datastructures::{Component, ComponentVTable, ItemTreeNode, ComponentTO}; + pub use corelib::abi::datastructures::{Component, ComponentTO, ComponentVTable, ItemTreeNode}; pub use corelib::abi::primitives::{Image, ImageVTable, Rectangle, RectangleVTable}; pub use gl::sixtyfps_runtime_run_component_with_gl_renderer; pub use once_cell::sync::Lazy; diff --git a/helper_crates/vtable/macro/macro.rs b/helper_crates/vtable/macro/macro.rs index d50a133c9..c2307f29f 100644 --- a/helper_crates/vtable/macro/macro.rs +++ b/helper_crates/vtable/macro/macro.rs @@ -52,7 +52,6 @@ pub fn vtable(_attr: TokenStream, item: TokenStream) -> TokenStream { let trait_name = Ident::new(&vtable_name[..vtable_name.len() - 6], input.ident.span()); let to_name = quote::format_ident!("{}TO", trait_name); - let impl_name = quote::format_ident!("{}Impl", trait_name); let module_name = quote::format_ident!("{}_vtable_mod", trait_name); let ref_name = quote::format_ident!("{}Ref", trait_name); let refmut_name = quote::format_ident!("{}RefMut", trait_name); @@ -209,10 +208,10 @@ pub fn vtable(_attr: TokenStream, item: TokenStream) -> TokenStream { })); let self_ty = ¶m.ty; let const_or_mut = mutability.map_or_else(|| quote!(const), |x| quote!(#x)); - call_code = Some(quote!(#call_code <#self_ty>::from_inner(*self),)); - self_call = Some( - quote!(&#mutability (*(<#self_ty>::get_ptr(&#arg_name).as_ptr() as *#const_or_mut T)),), - ); + call_code = + Some(quote!(#call_code <#self_ty>::from_raw(self.vtable, self.ptr),)); + self_call = + Some(quote!(&#mutability (*(#arg_name.as_ptr() as *#const_or_mut T)),)); has_self = true; continue; } @@ -271,10 +270,12 @@ pub fn vtable(_attr: TokenStream, item: TokenStream) -> TokenStream { drop_impl = Some(quote! { impl VTableMetaDrop for #vtable_name { - unsafe fn drop(ptr: #to_name) { + unsafe fn drop(ptr: *mut #to_name) { // Safety: The vtable is valid and inner is a type corresponding to the vtable, // which was allocated such that drop is expected. - unsafe { (ptr.vtable.as_ref().#ident)(VRefMut::from_inner(ptr)) } + unsafe { + let ptr = &*ptr; + (ptr.vtable.as_ref().#ident)(VRefMut::from_raw(ptr.vtable, ptr.ptr)) } } } @@ -291,7 +292,7 @@ pub fn vtable(_attr: TokenStream, item: TokenStream) -> TokenStream { generated_to_fn_trait.push(ImplItemMethod { attrs: vec![], - vis: Visibility::Inherited, + vis: Visibility::Public(VisPublic { pub_token: Default::default() }), defaultness: None, sig: sig.clone(), block: parse2(if has_self { @@ -412,36 +413,28 @@ pub fn vtable(_attr: TokenStream, item: TokenStream) -> TokenStream { #generated_trait #generated_trait_assoc_const - struct #impl_name { _private: [u8; 0] } - - /// This structure is highly unsafe, as it just has pointers. One could call trait functions - /// directly. However, it should not be possible, in safe code, to construct or to obtain a reference - /// to this structure, as it cannot be constructed safely. And none of the safe api allow accessing - /// a reference or a copy of this structure + /// Invariant, same as vtable::Inner: vtable and ptr has to be valid and ptr an instance macthcin the vtable #[doc(hidden)] - #[derive(Clone, Copy)] #[repr(C)] pub struct #to_name { vtable: core::ptr::NonNull<#vtable_name>, - ptr: core::ptr::NonNull<#impl_name>, + ptr: core::ptr::NonNull, + } + impl #to_name { + #(#generated_to_fn_trait)* + + pub fn get_vtable(&self) -> &#vtable_name { + unsafe { self.vtable.as_ref() } + } + + pub fn as_ptr(&self) -> *const u8 { + self.ptr.as_ptr() + } } - impl #trait_name for #to_name { #(#generated_to_fn_trait)* } unsafe impl VTableMeta for #vtable_name { - type Trait = dyn #trait_name; type VTable = #vtable_name; - type TraitObject = #to_name; - #[inline] - unsafe fn map_to(from: &Self::TraitObject) -> &Self::Trait { from } - #[inline] - unsafe fn map_to_mut(from: &mut Self::TraitObject) -> &mut Self::Trait { from } - #[inline] - unsafe fn get_ptr(from: &Self::TraitObject) -> core::ptr::NonNull { from.ptr.cast() } - #[inline] - unsafe fn get_vtable(from: &Self::TraitObject) -> &Self::VTable { from.vtable.as_ref() } - #[inline] - unsafe fn from_raw(vtable: core::ptr::NonNull, ptr: core::ptr::NonNull) -> Self::TraitObject - { #to_name { vtable, ptr : ptr.cast() } } + type Target = #to_name; } #drop_impl diff --git a/helper_crates/vtable/src/lib.rs b/helper_crates/vtable/src/lib.rs index a42784375..cb2a4993e 100644 --- a/helper_crates/vtable/src/lib.rs +++ b/helper_crates/vtable/src/lib.rs @@ -4,86 +4,72 @@ use core::ptr::NonNull; pub use vtable_macro::*; pub unsafe trait VTableMeta { - /// that's the rust trait. (e.g: `Hello`) - type Trait: ?Sized; - /// that's the vtable struct `HelloVTable` + /// That's the trait object that implements the functions + /// NOTE: the size must be 2*size_of:: + /// and a repr(C) with (vtable, ptr) so it has the same layout as + /// the inner and VBox/VRef/VRefMut + type Target; + + /// That's the VTable itself (so most likely Self) type VTable; - - /// That's the trait object that implements the trait. - /// NOTE: the size must be 2*size_of - type TraitObject: Copy; - - /// That maps from the tait object from the trait iteself - /// (In other word, return 'to' since 'to' implements trait, - /// but we can't represent that in rust right now, hence these helper) - /// - /// Safety: the trait object need to be pointing to valid pointer / vtable - unsafe fn map_to(to: &Self::TraitObject) -> &Self::Trait; - /// Same as map_to, but mutable - unsafe fn map_to_mut(to: &mut Self::TraitObject) -> &mut Self::Trait; - - /// Return a raw pointer to the inside of the impl - unsafe fn get_ptr(from: &Self::TraitObject) -> NonNull; - - /// Create a trait object from its raw parts - unsafe fn from_raw(vtable: NonNull, ptr: NonNull) -> Self::TraitObject; - - /// return a reference to the vtable - unsafe fn get_vtable(from: &Self::TraitObject) -> &Self::VTable; } pub trait VTableMetaDrop: VTableMeta { - /// Safety: the traitobject need to be pointing to a valid allocated pointer - unsafe fn drop(ptr: Self::TraitObject); + /// Safety: the Target need to be pointing to a valid allocated pointer + unsafe fn drop(ptr: *mut Self::Target); } -// These checks are not enough to ensure that this is not unsafe. -fn sanity_checks() { - debug_assert_eq!(core::mem::size_of::(), 2 * core::mem::size_of::()); +#[derive(Copy, Clone)] +/// The inner structure of VRef, VRefMut, and VBox. +/// +/// Invariant: _vtable and _ptr are valid pointer for the lifetime of the container. +/// _ptr is an instance of the object represented by _vtable +#[allow(dead_code)] +struct Inner { + vtable: *const u8, + ptr: *const u8, +} + +impl Inner { + /// Transmute a reference to self into a reference to T::Target. + fn deref(&self) -> *const T::Target { + debug_assert_eq!(core::mem::size_of::(), core::mem::size_of::()); + self as *const Inner as *const T::Target + } } #[repr(C)] pub struct VBox { - inner: T::TraitObject, + inner: Inner, + phantom: PhantomData, } impl Deref for VBox { - type Target = T::Trait; + type Target = T::Target; fn deref(&self) -> &Self::Target { - sanity_checks::(); - unsafe { T::map_to(&self.inner) } + unsafe { &*self.inner.deref::() } } } impl DerefMut for VBox { fn deref_mut(&mut self) -> &mut Self::Target { - sanity_checks::(); - unsafe { T::map_to_mut(&mut self.inner) } + unsafe { &mut *(self.inner.deref::() as *mut _) } } } impl Drop for VBox { fn drop(&mut self) { unsafe { - T::drop(self.inner); + T::drop(self.inner.deref::() as *mut _); } } } impl VBox { - pub unsafe fn from_inner(inner: T::TraitObject) -> Self { - Self { inner } - } - pub unsafe fn inner(x: &Self) -> T::TraitObject { - x.inner - } - pub unsafe fn get_ptr(x: &Self) -> NonNull { - T::get_ptr(&x.inner) - } pub unsafe fn from_raw(vtable: NonNull, ptr: NonNull) -> Self { - Self { inner: T::from_raw(vtable, ptr) } - } - pub fn get_vtable(&self) -> &T::VTable { - unsafe { T::get_vtable(&self.inner) } + Self { + inner: Inner { vtable: vtable.cast().as_ptr(), ptr: ptr.cast().as_ptr() }, + phantom: PhantomData, + } } pub fn borrow<'b>(&'b self) -> VRef<'b, T> { unsafe { VRef::from_inner(self.inner) } @@ -94,8 +80,8 @@ impl VBox { } pub struct VRef<'a, T: ?Sized + VTableMeta> { - inner: T::TraitObject, - _phantom: PhantomData<&'a T::Trait>, + inner: Inner, + phantom: PhantomData<&'a T::Target>, } // Need to implement manually otheriwse it is not implemented if T do not implement Copy / Clone @@ -103,80 +89,65 @@ impl<'a, T: ?Sized + VTableMeta> Copy for VRef<'a, T> {} impl<'a, T: ?Sized + VTableMeta> Clone for VRef<'a, T> { fn clone(&self) -> Self { - Self { inner: self.inner, _phantom: self._phantom } + Self { inner: self.inner, phantom: PhantomData } } } impl<'a, T: ?Sized + VTableMeta> Deref for VRef<'a, T> { - type Target = T::Trait; + type Target = T::Target; fn deref(&self) -> &Self::Target { - sanity_checks::(); - unsafe { T::map_to(&self.inner) } + unsafe { &*self.inner.deref::() } } } impl<'a, T: ?Sized + VTableMeta> VRef<'a, T> { - pub unsafe fn from_inner(inner: T::TraitObject) -> Self { - Self { inner, _phantom: PhantomData } - } - pub unsafe fn inner(x: &Self) -> T::TraitObject { - x.inner - } - pub unsafe fn get_ptr(x: &Self) -> NonNull { - T::get_ptr(&x.inner) + unsafe fn from_inner(inner: Inner) -> Self { + Self { inner, phantom: PhantomData } } pub unsafe fn from_raw(vtable: NonNull, ptr: NonNull) -> Self { - Self { inner: T::from_raw(vtable, ptr), _phantom: PhantomData } - } - pub fn get_vtable(&self) -> &T::VTable { - unsafe { T::get_vtable(&self.inner) } + Self { + inner: Inner { vtable: vtable.cast().as_ptr(), ptr: ptr.cast().as_ptr() }, + phantom: PhantomData, + } } } pub struct VRefMut<'a, T: ?Sized + VTableMeta> { - inner: T::TraitObject, - _phantom: PhantomData<&'a mut T::Trait>, + inner: Inner, + phantom: PhantomData<&'a mut T::Target>, } impl<'a, T: ?Sized + VTableMeta> Deref for VRefMut<'a, T> { - type Target = T::Trait; + type Target = T::Target; fn deref(&self) -> &Self::Target { - sanity_checks::(); - unsafe { T::map_to(&self.inner) } + unsafe { &*self.inner.deref::() } } } impl<'a, T: ?Sized + VTableMeta> DerefMut for VRefMut<'a, T> { fn deref_mut(&mut self) -> &mut Self::Target { - sanity_checks::(); - unsafe { T::map_to_mut(&mut self.inner) } + unsafe { &mut *(self.inner.deref::() as *mut _) } } } impl<'a, T: ?Sized + VTableMeta> VRefMut<'a, T> { - pub unsafe fn from_inner(inner: T::TraitObject) -> Self { - Self { inner, _phantom: PhantomData } - } - pub unsafe fn inner(x: &Self) -> T::TraitObject { - x.inner - } - pub unsafe fn get_ptr(x: &Self) -> NonNull { - T::get_ptr(&x.inner) - } - pub fn borrow<'b>(&'b self) -> VRef<'b, T> { - unsafe { VRef::from_inner(VRefMut::inner(self)) } - } - pub fn borrow_mut<'b>(&'b mut self) -> VRefMut<'b, T> { - unsafe { VRefMut::from_inner(VRefMut::inner(self)) } - } - pub fn into_ref(self) -> VRef<'a, T> { - unsafe { VRef::from_inner(VRefMut::inner(&self)) } + unsafe fn from_inner(inner: Inner) -> Self { + Self { inner, phantom: PhantomData } } pub unsafe fn from_raw(vtable: NonNull, ptr: NonNull) -> Self { - Self { inner: T::from_raw(vtable, ptr), _phantom: PhantomData } + Self { + inner: Inner { vtable: vtable.cast().as_ptr(), ptr: ptr.cast().as_ptr() }, + phantom: PhantomData, + } } - pub fn get_vtable(&self) -> &T::VTable { - unsafe { T::get_vtable(&self.inner) } + pub fn borrow<'b>(&'b self) -> VRef<'b, T> { + unsafe { VRef::from_inner(self.inner) } + } + pub fn borrow_mut<'b>(&'b mut self) -> VRefMut<'b, T> { + unsafe { VRefMut::from_inner(self.inner) } + } + pub fn into_ref(self) -> VRef<'a, T> { + unsafe { VRef::from_inner(self.inner) } } } diff --git a/helper_crates/vtable/tests/test_vtable.rs b/helper_crates/vtable/tests/test_vtable.rs index 81fff7087..2aa9b6656 100644 --- a/helper_crates/vtable/tests/test_vtable.rs +++ b/helper_crates/vtable/tests/test_vtable.rs @@ -26,7 +26,6 @@ impl Hello for SomeStruct { self.0 } - fn construct(init: u32) -> Self { println!("calling Construct {}", init); Self(init) @@ -40,7 +39,7 @@ impl HelloConsts for SomeStruct { const CONSTANT: usize = 88; } -static SOME_STRUCT_TYPE : HelloVTable = HelloVTable_static!(SomeStruct); +static SOME_STRUCT_TYPE: HelloVTable = HelloVTable_static!(SomeStruct); #[test] fn test() { @@ -53,5 +52,3 @@ fn test() { assert_eq!(bx.foo(2), 97); assert_eq!(bx.get_vtable().CONSTANT, 88); } - - diff --git a/sixtyfps_runtime/corelib/abi/datastructures.rs b/sixtyfps_runtime/corelib/abi/datastructures.rs index 143155007..6942d039b 100644 --- a/sixtyfps_runtime/corelib/abi/datastructures.rs +++ b/sixtyfps_runtime/corelib/abi/datastructures.rs @@ -121,14 +121,14 @@ pub type MouseEvent = (); pub fn cached_rendering_data(item: VRef<'_, ItemVTable>) -> &CachedRenderingData { unsafe { - &*(VRef::get_ptr(&item).as_ptr().offset(item.get_vtable().cached_rendering_data_offset) + &*(item.as_ptr().offset(item.get_vtable().cached_rendering_data_offset) as *const CachedRenderingData) } } pub fn cached_rendering_data_mut(item: VRefMut<'_, ItemVTable>) -> &mut CachedRenderingData { unsafe { - &mut *(VRefMut::get_ptr(&item).as_ptr().offset(item.get_vtable().cached_rendering_data_offset) + &mut *(item.as_ptr().offset(item.get_vtable().cached_rendering_data_offset) as *mut CachedRenderingData) } } @@ -156,9 +156,7 @@ fn visit_internal( let item = unsafe { ItemRef::from_raw( NonNull::new_unchecked(*vtable as *mut _), - NonNull::new_unchecked( - (VRef::get_ptr(&component).as_ptr()).offset(*offset) as *mut _ - ), + NonNull::new_unchecked(component.as_ptr().offset(*offset) as *mut _), ) }; let state = visitor(item, state); @@ -191,8 +189,7 @@ fn visit_internal_mut( ItemRefMut::from_raw( NonNull::new_unchecked(*vtable as *mut _), NonNull::new_unchecked( - (VRefMut::get_ptr(&component).as_ptr() as *mut u8).offset(*offset) - as *mut _, + (component.as_ptr() as *mut u8).offset(*offset) as *mut _ ), ) }; @@ -296,8 +293,6 @@ pub static QT_BUTTON_VTABLE: ItemVTable = ItemVTable { }; */ - - #[allow(non_upper_case_globals)] #[no_mangle] pub static RectangleVTable: ItemVTable = ItemVTable_static!(crate::abi::primitives::Rectangle); diff --git a/sixtyfps_runtime/corelib/abi/primitives.rs b/sixtyfps_runtime/corelib/abi/primitives.rs index 5a731c677..051684629 100644 --- a/sixtyfps_runtime/corelib/abi/primitives.rs +++ b/sixtyfps_runtime/corelib/abi/primitives.rs @@ -80,5 +80,4 @@ impl ItemConsts for Image { Image::field_offsets().cached_rendering_data as isize; } - pub use super::datastructures::{ImageVTable, RectangleVTable}; diff --git a/tools/viewer/main.rs b/tools/viewer/main.rs index f805debbc..1e9d7f956 100644 --- a/tools/viewer/main.rs +++ b/tools/viewer/main.rs @@ -69,14 +69,10 @@ struct MyComponentType { it: Vec, } -extern "C" fn item_tree( - r: ComponentRef<'_>, -) -> *const corelib::abi::datastructures::ItemTreeNode { +extern "C" fn item_tree(r: ComponentRef<'_>) -> *const corelib::abi::datastructures::ItemTreeNode { // FIXME! unsafe is not correct here, as the ComponentVTable might not be a MyComponentType // (one can safely take a copy of the vtable and call the create function to get a box) - unsafe { - (*(ComponentRef::get_vtable(&r) as *const ComponentVTable as *const MyComponentType)).it.as_ptr() - } + unsafe { (*(r.get_vtable() as *const ComponentVTable as *const MyComponentType)).it.as_ptr() } } struct RuntimeTypeInfo {