On Fri, 25 Oct 2024 at 15:01, Paolo Bonzini <pbonz...@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
>
