[snipping to concentrate on the QOM code generation]
On Fri, Oct 25, 2024 at 4:05 PM Manos Pitsidianakis
<manos.pitsidiana...@linaro.org> wrote:
> > 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 {
> > > + 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 {
> > > + 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.
Yes, and there is no need to place them in the procedural macros.
Generally it's a good rule to do as little as possible in procedural
macros, and move as much code as possible outside; this often means to
generic functions or a declarative macros. For some examples you can
look at Linux; in rust/macros/zeroable.rs for example the only
"quote!" is
quote! {
::kernel::__derive_zeroable!(
parse_input:
@sig(#(#rest)*),
@impl_generics(#(#new_impl_generics)*),
@ty_generics(#(#ty_generics)*),
@body(#last),
);
}
For more information, see https://www.youtube.com/watch?v=DMLBBZBlKis
> > unsafe extern "C" fn realize_fn_unsafe<T: DeviceImpl>(...) {}
> > unsafe extern "C" fn reset_fn_unsafe<T: DeviceImpl>(...) {}
>
> So just rename them and put a generic argument. Still not seeing any gain
> here.
The gain is that you have a much shorter macro implementation, a
shorter expansion which makes it easier to debug the macro, and an
implementation of realize_fn_unsafe<>() and reset_fn_unsafe<>() that
is isolated.
> > 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
Sort of. Yes, this is related to default implementations, but the
default implementation of a QOM method is not "call whatever is
declared in the trait". It's "call whatever is defined by the
superclass", which is represented by None.
> > 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?
Not really the NULL function pointer: the value left by the
superclass. In other words, the class_init function should not have
// realize is a function in a trait
dc.as_mut().realize = Self::realize;
but rather
// realize is a constant Option<fn(...)> in a trait
if let Some(realize_fn) = Self::REALIZE {
dc.as_mut().realize = Self::realize_fn;
}
> > 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.
I don't mean error messages from proc macros. I mean that debugging
macros that are this complex (does not matter if procedural or
declarative) is hell. If you move code outside the macro, to generic
functions and blanket trait implementations, error messages from the
compiler are simpler.
One additional benefit is that the code is compiled _always_, not just
if the macro hits a specific code path. It simplifies the testing
noticeably.
Finally, without -Zmacro-backtrace (nightly only) debugging procedural
macros is a tedious matter of using "cargo expand". With
-Zmacro-backtrace it's better but we don't want developers to install
nightly to develop QEMU. Note that this code is not write-once; it
will be extended for example as soon as we have a device that can fail
realization.
> Long crate paths; that's just a matter of using imports or
> changing names.
Only to some extent. See how many ::core::ffi:: or ::core::mem:: are
there in the current macros.
> > 2) it allows simplifying the pl011 code piecewise, even before
> > introducing procedural macro code
>
> Not sure how that is relevant can you explain?
Alex asked for a way to validate the expansion of the macro. If the
procedural macro is simple and the code is outside, then reviewers can
easily compare the pl011 code and the qemu-api-macros code, and see
that they expand to the same thing.
Try to put yourself into the mindset of the reviewer. If I see a patch like
+#[qom_class_init]
impl DeviceImpl for PL011State {
- const REALIZE: Option<fn(&mut self)> = {
fn realize(&mut self) { ... }
- Some(realize)
- };
- const RESET: Option<fn(&mut self)> = {
fn reset(&mut self) { ... }
- Some(reset)
- };
const VMSTATE: ... = ...;
const CATEGORY: ... = ...;
}
... then it's quite obvious what to expect from the expansion of the
#[qom_class_init] attribute macro.
> > 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.
I will be very clear: no, this patch is very far from an acceptable
state. It's a wonderful prototype, just like hw/char/pl011, but at
this point we're not accepting prototype-quality Rust code anymore:
we're turning the existing prototype, which has a manageable but still
large size, into the real thing.
To sum up the basic points underlying the suggestions, my review comments are:
- the syntax of #[device(...)] is arbitrary. You are keeping things
that obviously refer to the QOM object (type name, parent type name)
far from #[derive(Object)]. You are putting in #[device()] things that
apply to the QOM object class (e.g. class_name_override) rather than
the QOM device class. Finally, you are putting in #[device()] the
vmstate, which is not self-contained (due to subsections).
- the split in modules needs more work. For example, why is
Migrateable under qemu_api::objects instead of qemu_api::device_class?
- the part that would require the fewest changes is probably
#[property], but we need to be careful about not introducing "fake
safety".
- to simplify splitting the patches, start by moving code out of the
existing declarative macros and into generic code or declarative
macros
- to simply reviewing the patches, only use the procedural macros as a
syntactic sugar, and do as little code generation as possible in the
procedural macros themselves
- splitting the ObjectImpl/DeviceImpl traits in two (one for code
generated by derive-macros; one for stuff that is definde outside the
struct declaration) could be a way to work incrementally, adding more
and more parts of the class definition to the procedural macro but
without doing everything at once
- it must be possible to specify non-overridden DeviceClass or
ObjectClass functions
The suggestions are just a way to achieve these objectives. In
particular, the suggestions are about how to achieve the bullets in
the above list starting from the third.
Also, this series *must* come after the other cleanups and CI
integration that have been posted, for several reasons.
As to the cleanups, right now we *already* have in the tree code that
is duplicate, dead or untested. Removing that, and ensuring that it
does not regress, is a priority over everything else.
As to CI integration, first, changes like this one must be fully
bisectable, which requires having working CI before. Second, if we
want to advertise 9.2 as supporting --enable-rust (even if
experimental), we need a way to run automated tests on at least _some_
of the platforms that we support.
So my plan right now is to post a pull request for part 1 next week,
and to include part 2 (including your work on migration and Luminary,
but without the procedural macro) in -rc1. This work is for 10.0.
> > 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?
I was talking in general. When working in Rust I often found that the
first thing I came up with was crap, and the main value of it was in
learning. Sometimes I came up with the second version on my own,
sometimes it was pointed out during review. But even if it's pointed
out during review, the reviewer doesn't in any way think bad of you or
your work.
Paolo
