Re: [PATCH v1 2/3] rust: add pin-init API

[y86-dev]

On Thursday, March 16th, 2023 at 18:38, Gary Guo <gary@xxxxxxxxxxx> wrote:
> On Thu, 16 Mar 2023 09:38:16 +0000
> y86-dev <y86-dev@xxxxxxxxxxxxxx> 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