Re: [PATCH 03/11] rust/qemu-api-macros: introduce Device proc macro

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

On Fri, 25 Oct 2024 at 15:01, Paolo Bonzini <pbonzini@redhat.com> wrote:

>
> (Generally, don't read too much in the code - the syntax might have
> issues but you get the idea).
>
>
> Anyhow, going forward to the property attribute:
>
> > +    #[property(name = c"migrate-clk", qdev_prop = qdev_prop_bool)]
>
> There are two issues here:
>
> First, the default is true, so 1) this has to be fixed in QEMU (will
> do), 2) it is important to support it in #[property()].


TODO, it was not ignored just planned as next


>
> Second, this provides a false sense of safety, because I could specify
> qdev_prop_chr here.  Instead, the qdev_prop type should be derived by
> the field type.

TODO, if you recall we had that discussion about external statics,
that was what I was looking into back then.


> Third, since we are at it there's no need to use c"" in the attribute.
> The c_str!() macro that I am adding for backwards compatibility to old
> versions of Rust might actually come in handy here.

TODO, you can you use both LitStr and LitCStr in the macro


>
> The part where I have most comments, and some ideas of how to make your
> work a little more maintainable, is the implementation of class_init
> (and all that depends on it).
>
> Let's start with these generated functions:
>
> > +        pub unsafe extern "C" fn #realize_fn(
> > +            dev: *mut ::qemu_api::bindings::DeviceState,
> > +            errp: *mut *mut ::qemu_api::bindings::Error,
> > +        ) {
> > +            let mut instance = NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a non-null pointer of type ", stringify!(#name)));
> > +            unsafe {
> > +                ::qemu_api::objects::DeviceImpl::realize(instance.as_mut());
> > +            }
> > +        }
> > +
> > +        #[no_mangle]
> > +        pub unsafe extern "C" fn #reset_fn(
> > +            dev: *mut ::qemu_api::bindings::DeviceState,
> > +        ) {
> > +            let mut instance = NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a non-null pointer of type ", stringify!(#name)));
> > +            unsafe {
> > +                ::qemu_api::objects::DeviceImpl::reset(instance.as_mut());
> > +            }
> > +        }
>
> This can be handled entirely in Rust code, outside the macro.  If you add:

Why? I don't understand what this solves. These are *just* trampoline
functions to call the Rust-abi code.


>
> unsafe extern "C" fn realize_fn_unsafe<T: DeviceImpl>(
>      dev: *mut DeviceState,
>      errp: *mut *mut Error,
> ) {
>      let mut instance = NonNull::new(dev.cast::<T>()).
>          expect("Expected dev to be a non-null pointer");
>      unsafe {
>          ::qemu_api::objects::DeviceImpl::realize(instance.as_mut());
>      }
> }
>
> unsafe extern "C" fn reset_fn_unsafe<T: DeviceImpl>(
>      dev: *mut ::qemu_api::bindings::DeviceState,
> ) {
>      let mut instance = NonNull::new(dev.cast::<T>()).
>          expect("Expected dev to be a non-null pointer");
>      unsafe {
>          ::qemu_api::objects::DeviceImpl::reset(instance.as_mut());
>      }
> }
>
> then the functions can be used directly instead of #realize_fn and
> #reset_fn with a blanket implementation of DeviceImplUnsafe:
>

So just rename them and put a generic argument. Still not seeing any gain here.


>
> unsafe impl DeviceImplUnsafe for T: DeviceImpl {
>      const REALIZE: ... = Some(realize_fn_unsafe::<T>);
>      const RESET: ... = Some(realize_fn_unsafe::<T>);
> }
>
> Going on to the implementation of the safe functions:
>
> > +impl DeviceImpl for PL011State {
> > +    fn realize(&mut self) {
>
> These are not quite traits.  First, you can implement only some of the
> functions.

This is called "default implementations" in Rust

 > Second, if you don't implement them they are not overwritten
> by the class_init method.

WYM overwritten? That we hook up the empty stub instead of a NULL
function pointer?

> So this points to a different implementation as an attribute macro,
> which is able to rewrite everything in the body of the impl.  For example:
>
> #[qom_class_init]
> impl DeviceImpl for PL011State {
>      fn realize(&mut self) { ... }
>      fn reset(&mut self) { ... }
>
>      const VMSTATE: ... = {}
>      const CATEGORY: ... = {}
> }
>
> can be transformed into:
>
> #[qom_class_init]
> impl DeviceImpl for PL011State {
>      // fns are wrapped and transformed into consts
>      const REALIZE: Option<fn(&mut self)> = {
>          fn realize(&mut self) { ... }
>          Some(realize)
>      };
>      const RESET: Option<fn(&mut self)> = {
>          fn reset(&mut self) { ... }
>          Some(reset)
>      };
>
>      // while associated consts (and perhaps types?) remain as is
>      const VMSTATE: ... = {}
>      const CATEGORY: ... = {}
> }
>
> The above blanket implementation of DeviceImplUnsafe is easily adjusted
> to support non-overridden methods:
>
> unsafe impl DeviceImplUnsafe for T: DeviceImpl {
>      const REALIZE: ... = <T as DeviceImpl>::REALIZE::map(
>          || realize_fn_unsafe::<T>);
>      const RESET: ... = <T as DeviceImpl>::RESET::map(
>          || realize_fn_unsafe::<T>);
> }
>
>
> You can also keep out of the macro the class_init method itself.  Here
> I'm adding it to DeviceImplUnsafe:
>
> pub trait DeviceImplUnsafe {
>      unsafe fn class_init(klass: *mut ObjectClass, data: *mut c_void) {
>          let mut dc = NonNull::new(klass.cast::<DeviceClass>()).unwrap();
>          unsafe {
>              dc.as_mut().realize = Self::REALIZE;
>              bindings::device_class_set_legacy_reset(
>                                   dc.as_mut(), Self::RESET);
>              device_class_set_props(dc.as_mut(),
>                                     <Self as DeviceInfo>::PROPS);
>              if let Some(vmsd) = <Self as DeviceInfo>::VMSTATE {
>                  dc.as_mut().vmsd = vmsd;
>              }
>              if let Some(cat) = <Self as DeviceInfo>::CATEGORY {
>                  dc.as_mut().category = cat;
>              }
>
>              // maybe in the future for unparent
>              // <Self as ObjectImplUnsafe>::class_init(klass, data)
>          }
>      }
> }
>
> And with all this machinery in place the device_class_init! macro
> becomes simply
>
>      device_class_init!(PL011State);
>
> (because the arguments have moved to DeviceInfo or DeviceImpl).
>
>
> Why is this important? Because the only code transformation is the
> generation of properties and vtables, and the bindings can be developed
> almost entirely in qemu_api instead of qemu_api_macros.  This has
> several advantages:
>
> 1) it's much easier: simpler error messages, no macro indirection, no
> super long global crate paths

Hard no, sorry. Error messages can definitely be generated from proc
macros. Long crate paths; that's just a matter of using imports or
changing names.
>
> 2) it allows simplifying the pl011 code piecewise, even before
> introducing procedural macro code

Not sure how that is relevant can you explain?

>
> 3) it's easier to add comments
>
>
> It also becomes much easier to separate the work in separate patches, or
> even separate series.  Replacing the arguments to device_class_init!()
> with DeviceImpl + DeviceImplUnsafe can be introduced first: posted,
> reviewed, merged.  All the remaining tasks are pretty much independent:
>
> 1) splitting out ObjectInfo and introducing #[object] to automate it
> (i.e. extending derive(Object))
>
> 2) splitting out DeviceInfo and introducing #[property] to automate it
> (i.e. derive(Device))
>
> 3) the #[qom_class_init] macro


I disagree with this approach at the moment. This patch is in an
acceptable state albeit a bit longish while all these suggestions
would merely cause  more running around making changes for no real
gain while also delaying review and merging.


>
> A final word: I absolutely don't want you to think that your work is of
> no value.  It's totally okay to throw away the first version of
> something.  You showed that it _is_ possible to have idiomatic code with
> the help of procedural macros.  Even if the implementation can be
> improved, the _idea_ remains yours.

I don't know what this is in reference to :P What work and throwing
away are you talking about?

> Paolo
>