Re: vtables and procedural macros (was Re: [PATCH] rust: pl011: convert pl011_create to safe Rust)

[Junjie Mao]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 2/10/25 10:59, Zhao Liu wrote:
>> On Thu, Feb 06, 2025 at 12:15:14PM +0100, Paolo Bonzini wrote:
>>> Not a major change but, as a small but significant step in creating
>>> qdev bindings, show how pl011_create can be written without "unsafe"
>>> calls (apart from converting pointers to references).
>>>
>>> This also provides a starting point for creating Error** bindings.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>   rust/hw/char/pl011/src/device.rs | 39 ++++++++++++++++----------------
>>>   rust/qemu-api/src/sysbus.rs      | 34 +++++++++++++++++++++++++---
>>>   2 files changed, 50 insertions(+), 23 deletions(-)
>> ...
>>
>>> +    fn realize(&self) {
>> What about renaming this as "realize_with_sysbus"?
>> Elsewhere, the device's own realize method is often used to set
>> DeviceImpl::REALIZE.
>
> I *think* this is not a problem in practice because trait methods are public 
> (if
> the trait is in scope) whereas the device's realize method if private.

I would suggest to keep the "sysbus" prefix in the method name, or in
general, keep the class prefix in the method names in XXXClassMethods
traits. Otherwise APIs from different parent classes may also
conflict. As an example, in the following class hierarchy:

  Object -> DeviceState -> PCIDevice -> PCIBridge -> ...

For PCIDevice instances there is an API pci_device_reset(), and for
PCIBridge ones pci_bridge_reset(). Without class prefixes both will be
wrapped (as blanket impl in two traits) as reset() and a dev.reset()
call will lead to a "multiple applicable items in scope" build error.

In addition, it is quite common to see static functions named
xxx_device_type_reset() in C today. Thus, it is quite natural to expect
(private) methods named XXXDeviceState::reset() in future Rust devices,
which will cause even more trouble as the compiler will no longer
complain and always pick that method for dev.reset() calls.

More general names can be found in multiple device classes, such as
write_config (pci_default_write_config() and pci_bridge_write_config()).

There are alternatives to solve such conflicts, such as requiring proc
macro attributes above methods with such general names, and requiring
ambiguous parent class API call to use fully qualified syntax, e.g.,
SysBusDeviceMethods::realize(self). But I think the former can be
error-prone because the list of general names vary among different
device types and required attributes can be easily neglected since no
build error is triggered without them, and the later is more tedious
than self.sysbus_realize().

>
> I agree that the naming conflict is unfortunate though, if only because it can
> cause confusion.  I don't know if this can be solved by procedural macros; for
> example a #[vtable] attribute that changes
>
>     #[qemu_api_macros::vtable]
>     fn realize(...) {
>     }
>
> into
>
>     const fn REALIZE: ... = Some({
>         fn realize(...) {
>         }
>         realize
>     }
>
> This way, the REALIZE item would be included in the "impl DeviceImpl for
> PL011State" block instead of "impl PL011State".  It's always a fine line
> between procedural macros cleaning vs. messing things up, which is why until 
> now
> I wanted to see what things look like with pure Rust code; but I guess now 
> it's
> the time to evaluate this kind of thing.
>
> Adding Junjie since he contributed the initial proc macro infrastructure and 
> may
> have opinions on this.

I agree that uses of proc macros should be carefully evaluated to see if
they really help or hurt. In this specific case, I'm not sure if using
attributes solves the root cause, though.

--
Best Regards
Junjie Mao

>
> Paolo