diff mbox series

[04/17] rust/vmstate: Use ident instead of expr to parse vmsd in vmstate_struct macro

Message ID 20250317151236.536673-5-zhao1.liu@intel.com (mailing list archive)
State New
Headers show
Series rust/vmstate: Clean up, fix, enhance & test | expand

Commit Message

Zhao Liu March 17, 2025, 3:12 p.m. UTC 
When specify an array field in vmstate_struct macro, there will be an
error:

> local ambiguity when calling macro `vmstate_struct`: multiple parsing
> options: built-in NTs expr ('vmsd') or 1 other option.

This is because "expr" can't recognize the "vmsd" field correctly, so
that it gets confused with the previous array field.

To fix the above issue, use "ident" for "vmsd" field, and explicitly
refer to it in the macro.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 rust/hw/char/pl011/src/device_class.rs | 2 +-
 rust/qemu-api/src/vmstate.rs           | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Paolo Bonzini March 17, 2025, 5:17 p.m. UTC | #1 
On Mon, Mar 17, 2025 at 3:52 PM Zhao Liu <zhao1.liu@intel.com> wrote:
>
> When specify an array field in vmstate_struct macro, there will be an
> error:
>
> > local ambiguity when calling macro `vmstate_struct`: multiple parsing
> > options: built-in NTs expr ('vmsd') or 1 other option.
>
> This is because "expr" can't recognize the "vmsd" field correctly, so
> that it gets confused with the previous array field.
>
> To fix the above issue, use "ident" for "vmsd" field, and explicitly
> refer to it in the macro.

I think this is not needed if the varray case is left as is, and other
cases use .with_...() instead of arguments?

Paolo

> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  rust/hw/char/pl011/src/device_class.rs | 2 +-
>  rust/qemu-api/src/vmstate.rs           | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs
> index 0b2076ddaa0f..e43a5d6cd063 100644
> --- a/rust/hw/char/pl011/src/device_class.rs
> +++ b/rust/hw/char/pl011/src/device_class.rs
> @@ -72,7 +72,7 @@ extern "C" fn pl011_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
>      post_load: Some(pl011_post_load),
>      fields: vmstate_fields! {
>          vmstate_unused!(core::mem::size_of::<u32>()),
> -        vmstate_struct!(PL011State, regs, &VMSTATE_PL011_REGS, BqlRefCell<PL011Registers>),
> +        vmstate_struct!(PL011State, regs, VMSTATE_PL011_REGS, BqlRefCell<PL011Registers>),
>      },
>      subsections: vmstate_subsections! {
>          VMSTATE_PL011_CLOCK
> diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
> index 94efbd8bb735..3f95d4825149 100644
> --- a/rust/qemu-api/src/vmstate.rs
> +++ b/rust/qemu-api/src/vmstate.rs
> @@ -435,7 +435,7 @@ macro_rules! vmstate_unused {
>  #[doc(alias = "VMSTATE_STRUCT")]
>  #[macro_export]
>  macro_rules! vmstate_struct {
> -    ($struct_name:ty, $field_name:ident $(, [0 .. $num:ident $(* $factor:expr)?])?, $vmsd:expr, $type:ty $(,)?) => {
> +    ($struct_name:ty, $field_name:ident $(, [0 .. $num:ident $(* $factor:expr)?])?, $vmsd:ident, $type:ty $(,)?) => {
>          $crate::bindings::VMStateField {
>              name: ::core::concat!(::core::stringify!($field_name), "\0")
>                  .as_bytes()
> @@ -447,7 +447,7 @@ macro_rules! vmstate_struct {
>              },
>              size: ::core::mem::size_of::<$type>(),
>              flags: $crate::bindings::VMStateFlags::VMS_STRUCT,
> -            vmsd: $vmsd,
> +            vmsd: &$vmsd,
>              ..$crate::zeroable::Zeroable::ZERO $(
>                  .with_varray_flag($crate::call_func_with_field!(
>                      $crate::vmstate::vmstate_varray_flag,
> --
> 2.34.1
>
Zhao Liu March 18, 2025, 2:46 a.m. UTC | #2 
On Mon, Mar 17, 2025 at 06:17:07PM +0100, Paolo Bonzini wrote:
> Date: Mon, 17 Mar 2025 18:17:07 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 04/17] rust/vmstate: Use ident instead of expr to parse
>  vmsd in vmstate_struct macro
> 
> On Mon, Mar 17, 2025 at 3:52 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> >
> > When specify an array field in vmstate_struct macro, there will be an
> > error:
> >
> > > local ambiguity when calling macro `vmstate_struct`: multiple parsing
> > > options: built-in NTs expr ('vmsd') or 1 other option.
> >
> > This is because "expr" can't recognize the "vmsd" field correctly, so
> > that it gets confused with the previous array field.
> >
> > To fix the above issue, use "ident" for "vmsd" field, and explicitly
> > refer to it in the macro.
> 
> I think this is not needed if the varray case is left as is, and other
> cases use .with_...() instead of arguments?
> 

Yes! With a[0..num], the `vmsd` could be parsed correctly. I'll drop this
patch as well and refresh unit tests.

Thanks,
Zhao
Zhao Liu March 18, 2025, 6:14 a.m. UTC | #3 
On Tue, Mar 18, 2025 at 10:46:10AM +0800, Zhao Liu wrote:
> Date: Tue, 18 Mar 2025 10:46:10 +0800
> From: Zhao Liu <zhao1.liu@intel.com>
> Subject: Re: [PATCH 04/17] rust/vmstate: Use ident instead of expr to parse
>  vmsd in vmstate_struct macro
> 
> On Mon, Mar 17, 2025 at 06:17:07PM +0100, Paolo Bonzini wrote:
> > Date: Mon, 17 Mar 2025 18:17:07 +0100
> > From: Paolo Bonzini <pbonzini@redhat.com>
> > Subject: Re: [PATCH 04/17] rust/vmstate: Use ident instead of expr to parse
> >  vmsd in vmstate_struct macro
> > 
> > On Mon, Mar 17, 2025 at 3:52 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> > >
> > > When specify an array field in vmstate_struct macro, there will be an
> > > error:
> > >
> > > > local ambiguity when calling macro `vmstate_struct`: multiple parsing
> > > > options: built-in NTs expr ('vmsd') or 1 other option.
> > >
> > > This is because "expr" can't recognize the "vmsd" field correctly, so
> > > that it gets confused with the previous array field.
> > >
> > > To fix the above issue, use "ident" for "vmsd" field, and explicitly
> > > refer to it in the macro.
> > 
> > I think this is not needed if the varray case is left as is, and other
> > cases use .with_...() instead of arguments?
> > 
> 
> Yes! With a[0..num], the `vmsd` could be parsed correctly. I'll drop this
> patch as well and refresh unit tests.
> 

Additionally, at present, IMO it is not suitable to replace the vmsd argument
with .with_vmsd() method because VMS_STRUCT requires a vmsd field, and
.with_vmsd() is optional.

There is no way to ensure that vmsd is not omitted... unless VMS_STRUCT is
also set within .with_vmsd(). This would be akin to merging vmstate_struct
and vmstate_of, but I suppose that would be a long way as for now.

So I prefer vmsd argument at the moment :-)

Thanks,
Zhao
diff mbox series

Patch

diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs
index 0b2076ddaa0f..e43a5d6cd063 100644
--- a/rust/hw/char/pl011/src/device_class.rs
+++ b/rust/hw/char/pl011/src/device_class.rs
@@ -72,7 +72,7 @@  extern "C" fn pl011_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
     post_load: Some(pl011_post_load),
     fields: vmstate_fields! {
         vmstate_unused!(core::mem::size_of::<u32>()),
-        vmstate_struct!(PL011State, regs, &VMSTATE_PL011_REGS, BqlRefCell<PL011Registers>),
+        vmstate_struct!(PL011State, regs, VMSTATE_PL011_REGS, BqlRefCell<PL011Registers>),
     },
     subsections: vmstate_subsections! {
         VMSTATE_PL011_CLOCK
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 94efbd8bb735..3f95d4825149 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -435,7 +435,7 @@  macro_rules! vmstate_unused {
 #[doc(alias = "VMSTATE_STRUCT")]
 #[macro_export]
 macro_rules! vmstate_struct {
-    ($struct_name:ty, $field_name:ident $(, [0 .. $num:ident $(* $factor:expr)?])?, $vmsd:expr, $type:ty $(,)?) => {
+    ($struct_name:ty, $field_name:ident $(, [0 .. $num:ident $(* $factor:expr)?])?, $vmsd:ident, $type:ty $(,)?) => {
         $crate::bindings::VMStateField {
             name: ::core::concat!(::core::stringify!($field_name), "\0")
                 .as_bytes()
@@ -447,7 +447,7 @@  macro_rules! vmstate_struct {
             },
             size: ::core::mem::size_of::<$type>(),
             flags: $crate::bindings::VMStateFlags::VMS_STRUCT,
-            vmsd: $vmsd,
+            vmsd: &$vmsd,
             ..$crate::zeroable::Zeroable::ZERO $(
                 .with_varray_flag($crate::call_func_with_field!(
                     $crate::vmstate::vmstate_varray_flag,