Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 16 Mar 2023 23:08:09 +0000 | From | y86-dev <> | Subject | Re: [PATCH v1 2/3] rust: add pin-init API |
| |
On Thursday, March 16th, 2023 at 18:38, Gary Guo <gary@garyguo.net> wrote: > On Thu, 16 Mar 2023 09:38:16 +0000 > y86-dev <y86-dev@protonmail.com> wrote: > > > > > + > > > > +/// Trait facilitating pinned destruction. > > > > +/// > > > > +/// Use [`pinned_drop`] to implement this trait safely: > > > > +/// > > > > +/// ```rust > > > > +/// # use kernel::sync::Mutex; > > > > +/// use kernel::macros::pinned_drop; > > > > +/// use core::pin::Pin; > > > > +/// #[pin_data(PinnedDrop)] > > > > +/// struct Foo { > > > > +/// #[pin] > > > > +/// mtx: Mutex<usize>, > > > > +/// } > > > > +/// > > > > +/// #[pinned_drop] > > > > +/// impl PinnedDrop for Foo { > > > > +/// fn drop(self: Pin<&mut Self>) { > > > > +/// pr_info!("Foo is being dropped!"); > > > > +/// } > > > > +/// } > > > > +/// ``` > > > > +/// > > > > +/// # Safety > > > > +/// > > > > +/// This trait must be implemented with [`pinned_drop`]. > > > > +/// > > > > +/// [`pinned_drop`]: kernel::macros::pinned_drop > > > > +pub unsafe trait PinnedDrop: __PinData { > > > > + /// Executes the pinned destructor of this type. > > > > + /// > > > > + /// # Safety > > > > + /// > > > > + /// Only call this from `<Self as Drop>::drop`. > > > > + unsafe fn drop(self: Pin<&mut Self>); > > > > + > > > > + // Used by the `pinned_drop` proc-macro to ensure that only safe operations are used in `drop`. > > > > + // the function should not be called. > > > > + #[doc(hidden)] > > > > + fn __ensure_no_unsafe_op_in_drop(self: Pin<&mut Self>); > > > > > > One idea to avoid this extra function is to have an unsafe token to the > > > drop function. > > > > > > fn drop(self: Pin<&mut Self>, token: TokenThatCanOnlyBeCreatedUnsafely); > > > > What is wrong with having this extra function? If the problem is that this > > function might be called, then we could add a parameter with an > > unconstructable type. > > > > I think that `drop` should be `unsafe`, since it really does have > > the requirement of only being called in the normal drop impl. > > The point to avoid having two functions with the same body. This would > require double the amount of checks needed by the compiler (and make > error message worth if anything's wrong in the body of `drop`). > > This current approach is really just a hack to avoid code from doing > unsafe stuff without using `unsafe` block -- and the best solution is > just to avoid make `drop` function unsafe. However we don't want drop > function to be actually called from safe code, and that's the point of > a token that can only be created unsafely is force `drop` to *not* be > called by safe code. The token is a proof that `unsafe` is being used. > > This way the `__ensure_no_unsafe_op_in_drop` function would not be > needed.
That makes sense.
> > > > +// This trait is only implemented via the `#[pin_data]` proc-macro. It is used to facilitate > > > > +// the pin projections within the initializers. > > > > +#[doc(hidden)] > > > > +pub unsafe trait __PinData { > > > > + type __PinData; > > > > +} > > > > + > > > > +/// Stack initializer helper type. Use [`stack_pin_init`] instead of this primitive. > > > > > > `#[doc(hidden)]`? > > > > This trait is implementation detail of the `#[pin_data]` macro. Why should > > it be visible in the rust-docs? > > I am commenting about `stack_pin_init` (note the doc comment above my > comment). `StackInit` is an implementation detail of `stack_pin_init` > and shouldn't be exposed, IMO. Or do you think manual use of > `StackInit` is needed?
I thought that it could be used by something else, but I will hide it for now.
Cheers, Benno
| ![\](/images/icornerr.gif) |