From: Martin Kletzander <mkletzan@redhat.com>

In some cases (e.g. in vmstate_tests.rs) the second argument to
impl_vmstate_struct! is actually an existing struct which is then
copied (since VMStateDescription implements Copy) when saved into the
static VMSD using .get().  That is not a problem because it is part of
the data segment and the pointers are not being free'd since they point
to static data.  But it is a problem when tests rely on comparing the
VMState descriptions as pointers rather than contents.  And it also
wastes space, more or less.

Introduce second variant of the macro which can, instead of the
expression, take an identifier or what looks like a reference.  This
second variant is added before the current variant so that it has
preference, and only references the existing static data from it.

This way tests are fixed and space is saved.

And now that the VMStateDescription checking is fixed we can also check
for the right value in test_vmstate_struct_varray_uint8_wrapper().

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
I'm not sure whether this is caused by different utility on my system or bash
version or whatever, but without this patch these three tests fail for me and
this patch fixes it.

 rust/qemu-api/src/vmstate.rs         | 11 +++++++++++
 rust/qemu-api/tests/vmstate_tests.rs |  1 +
 2 files changed, 12 insertions(+)

diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index b5c6b764fbba..716e52afe740 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -449,6 +449,17 @@ macro_rules! vmstate_validate {
 /// description of the struct.
 #[macro_export]
 macro_rules! impl_vmstate_struct {
+    ($type:ty, $(&)?$vmsd:ident) => {
+        unsafe impl $crate::vmstate::VMState for $type {
+            const BASE: $crate::bindings::VMStateField =
+                $crate::bindings::VMStateField {
+                    vmsd: $vmsd.as_ref(),
+                    size: ::core::mem::size_of::<$type>(),
+                    flags: $crate::bindings::VMStateFlags::VMS_STRUCT,
+                    ..$crate::zeroable::Zeroable::ZERO
+                };
+        }
+    };
     ($type:ty, $vmsd:expr) => {
         unsafe impl $crate::vmstate::VMState for $type {
             const BASE: $crate::bindings::VMStateField = {
diff --git a/rust/qemu-api/tests/vmstate_tests.rs b/rust/qemu-api/tests/vmstate_tests.rs
index 2c0670ba0eed..7d3180e6c2ea 100644
--- a/rust/qemu-api/tests/vmstate_tests.rs
+++ b/rust/qemu-api/tests/vmstate_tests.rs
@@ -320,6 +320,7 @@ fn test_vmstate_struct_varray_uint8_wrapper() {
         b"arr_a_wrap\0"
     );
     assert_eq!(foo_fields[5].num_offset, 228);
+    assert_eq!(foo_fields[5].vmsd, VMSTATE_FOOA.as_ref());
     assert!(unsafe { foo_fields[5].field_exists.unwrap()(foo_b_p, 0) });
 
     // The last VMStateField in VMSTATE_FOOB.
-- 
2.50.1

Il ven 1 ago 2025, 17:00 Martin Kletzander <mkletzan@redhat.com> ha scritto:

> From: Martin Kletzander <mkletzan@redhat.com>
>
> In some cases (e.g. in vmstate_tests.rs) the second argument to
> impl_vmstate_struct! is actually an existing struct which is then
> copied (since VMStateDescription implements Copy) when saved into the
> static VMSD using .get().  That is not a problem because it is part of
> the data segment and the pointers are not being free'd since they point
> to static data.  But it is a problem when tests rely on comparing the
> VMState descriptions as pointers rather than contents.  And it also
> wastes space, more or less.
>
> Introduce second variant of the macro which can, instead of the
> expression, take an identifier or what looks like a reference.  This
> second variant is added before the current variant so that it has
> preference, and only references the existing static data from it.
>
> This way tests are fixed and space is saved.
>
> And now that the VMStateDescription checking is fixed we can also check
> for the right value in test_vmstate_struct_varray_uint8_wrapper().
>
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
> I'm not sure whether this is caused by different utility on my system or
> bash
> version or whatever, but without this patch these three tests fail for me
> and
> this patch fixes it.
>

I found something similar, though I wasn't sure if it was broken in master
as well or only in the rust-next branch.

If that works in master as well, I would remove completely the possibility
of using &FOO, and always use .as_ref(). It's more efficient as you said,
and there's no reason that I know to use the less efficient one.

Paolo

 rust/qemu-api/src/vmstate.rs         | 11 +++++++++++
>  rust/qemu-api/tests/vmstate_tests.rs |  1 +
>  2 files changed, 12 insertions(+)
>
> diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
> index b5c6b764fbba..716e52afe740 100644
> --- a/rust/qemu-api/src/vmstate.rs
> +++ b/rust/qemu-api/src/vmstate.rs
> @@ -449,6 +449,17 @@ macro_rules! vmstate_validate {
>  /// description of the struct.
>  #[macro_export]
>  macro_rules! impl_vmstate_struct {
> +    ($type:ty, $(&)?$vmsd:ident) => {
> +        unsafe impl $crate::vmstate::VMState for $type {
> +            const BASE: $crate::bindings::VMStateField =
> +                $crate::bindings::VMStateField {
> +                    vmsd: $vmsd.as_ref(),
> +                    size: ::core::mem::size_of::<$type>(),
> +                    flags: $crate::bindings::VMStateFlags::VMS_STRUCT,
> +                    ..$crate::zeroable::Zeroable::ZERO
> +                };
> +        }
> +    };
>      ($type:ty, $vmsd:expr) => {
>          unsafe impl $crate::vmstate::VMState for $type {
>              const BASE: $crate::bindings::VMStateField = {
> diff --git a/rust/qemu-api/tests/vmstate_tests.rs b/rust/qemu-api/tests/
> vmstate_tests.rs
> index 2c0670ba0eed..7d3180e6c2ea 100644
> --- a/rust/qemu-api/tests/vmstate_tests.rs
> +++ b/rust/qemu-api/tests/vmstate_tests.rs
> @@ -320,6 +320,7 @@ fn test_vmstate_struct_varray_uint8_wrapper() {
>          b"arr_a_wrap\0"
>      );
>      assert_eq!(foo_fields[5].num_offset, 228);
> +    assert_eq!(foo_fields[5].vmsd, VMSTATE_FOOA.as_ref());
>      assert!(unsafe { foo_fields[5].field_exists.unwrap()(foo_b_p, 0) });
>
>      // The last VMStateField in VMSTATE_FOOB.
> --
> 2.50.1
>
>

On Fri, Aug 01, 2025 at 11:44:03PM +0200, Paolo Bonzini wrote:
>Il ven 1 ago 2025, 17:00 Martin Kletzander <mkletzan@redhat.com> ha scritto:
>
>> From: Martin Kletzander <mkletzan@redhat.com>
>>
>> In some cases (e.g. in vmstate_tests.rs) the second argument to
>> impl_vmstate_struct! is actually an existing struct which is then
>> copied (since VMStateDescription implements Copy) when saved into the
>> static VMSD using .get().  That is not a problem because it is part of
>> the data segment and the pointers are not being free'd since they point
>> to static data.  But it is a problem when tests rely on comparing the
>> VMState descriptions as pointers rather than contents.  And it also
>> wastes space, more or less.
>>
>> Introduce second variant of the macro which can, instead of the
>> expression, take an identifier or what looks like a reference.  This
>> second variant is added before the current variant so that it has
>> preference, and only references the existing static data from it.
>>
>> This way tests are fixed and space is saved.
>>
>> And now that the VMStateDescription checking is fixed we can also check
>> for the right value in test_vmstate_struct_varray_uint8_wrapper().
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>> I'm not sure whether this is caused by different utility on my system or
>> bash
>> version or whatever, but without this patch these three tests fail for me
>> and
>> this patch fixes it.
>>
>
>I found something similar, though I wasn't sure if it was broken in master
>as well or only in the rust-next branch.
>

It is not broken on master, but it *was* broken on rust-next.

>If that works in master as well, I would remove completely the possibility
>of using &FOO, and always use .as_ref(). It's more efficient as you said,
>and there's no reason that I know to use the less efficient one.
>

That would mean that you cannot do what's done in e.g. pl011

impl_vmstate_struct!(
     PL011Registers,
     VMStateDescriptionBuilder::<PL011Registers>::new()
     ...
     .build()
);

requiring you to always do:

static/const VMSTATE_PL011_REGISTERS: VMStateDescription<PL011Registers> =
     VMStateDescriptionBuilder::<PL011Registers>::new()
     ...
     .build();

impl_vmstate_struct!(PL011Registers, VMSTATE_PL011_REGISTERS);

*BUT* of course I had to rebase the patches on top of current rust-next
on Friday and there were some of your commits from Thursday which I now
see actually fix all what I tried fixing before as well.  I tried
finding the previous commit on which I saw all the issues and after some
rebuilding I could not.  So it is now not even broken on rust-next.

This way I completely wasted your time, but at least learned something
that's happening in the code.  Sorry for that.

Martin

>Paolo
>
> rust/qemu-api/src/vmstate.rs         | 11 +++++++++++
>>  rust/qemu-api/tests/vmstate_tests.rs |  1 +
>>  2 files changed, 12 insertions(+)
>>
>> diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
>> index b5c6b764fbba..716e52afe740 100644
>> --- a/rust/qemu-api/src/vmstate.rs
>> +++ b/rust/qemu-api/src/vmstate.rs
>> @@ -449,6 +449,17 @@ macro_rules! vmstate_validate {
>>  /// description of the struct.
>>  #[macro_export]
>>  macro_rules! impl_vmstate_struct {
>> +    ($type:ty, $(&)?$vmsd:ident) => {
>> +        unsafe impl $crate::vmstate::VMState for $type {
>> +            const BASE: $crate::bindings::VMStateField =
>> +                $crate::bindings::VMStateField {
>> +                    vmsd: $vmsd.as_ref(),
>> +                    size: ::core::mem::size_of::<$type>(),
>> +                    flags: $crate::bindings::VMStateFlags::VMS_STRUCT,
>> +                    ..$crate::zeroable::Zeroable::ZERO
>> +                };
>> +        }
>> +    };
>>      ($type:ty, $vmsd:expr) => {
>>          unsafe impl $crate::vmstate::VMState for $type {
>>              const BASE: $crate::bindings::VMStateField = {
>> diff --git a/rust/qemu-api/tests/vmstate_tests.rs b/rust/qemu-api/tests/
>> vmstate_tests.rs
>> index 2c0670ba0eed..7d3180e6c2ea 100644
>> --- a/rust/qemu-api/tests/vmstate_tests.rs
>> +++ b/rust/qemu-api/tests/vmstate_tests.rs
>> @@ -320,6 +320,7 @@ fn test_vmstate_struct_varray_uint8_wrapper() {
>>          b"arr_a_wrap\0"
>>      );
>>      assert_eq!(foo_fields[5].num_offset, 228);
>> +    assert_eq!(foo_fields[5].vmsd, VMSTATE_FOOA.as_ref());
>>      assert!(unsafe { foo_fields[5].field_exists.unwrap()(foo_b_p, 0) });
>>
>>      // The last VMStateField in VMSTATE_FOOB.
>> --
>> 2.50.1
>>
>>

Il lun 4 ago 2025, 10:56 Martin Kletzander <mkletzan@redhat.com> ha scritto:

> *BUT* of course I had to rebase the patches on top of current rust-next
> on Friday and there were some of your commits from Thursday which I now
> see actually fix all what I tried fixing before as well.  I tried
> finding the previous commit on which I saw all the issues and after some
> rebuilding I could not.  So it is now not even broken on rust-next.
>
> This way I completely wasted your time, but at least learned something
> that's happening in the code.  Sorry for that.
>

Uh no you didn't. It was broken.

Paolo

Martin
>
> >Paolo
> >
> > rust/qemu-api/src/vmstate.rs         | 11 +++++++++++
> >>  rust/qemu-api/tests/vmstate_tests.rs |  1 +
> >>  2 files changed, 12 insertions(+)
> >>
> >> diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/
> vmstate.rs
> >> index b5c6b764fbba..716e52afe740 100644
> >> --- a/rust/qemu-api/src/vmstate.rs
> >> +++ b/rust/qemu-api/src/vmstate.rs
> >> @@ -449,6 +449,17 @@ macro_rules! vmstate_validate {
> >>  /// description of the struct.
> >>  #[macro_export]
> >>  macro_rules! impl_vmstate_struct {
> >> +    ($type:ty, $(&)?$vmsd:ident) => {
> >> +        unsafe impl $crate::vmstate::VMState for $type {
> >> +            const BASE: $crate::bindings::VMStateField =
> >> +                $crate::bindings::VMStateField {
> >> +                    vmsd: $vmsd.as_ref(),
> >> +                    size: ::core::mem::size_of::<$type>(),
> >> +                    flags: $crate::bindings::VMStateFlags::VMS_STRUCT,
> >> +                    ..$crate::zeroable::Zeroable::ZERO
> >> +                };
> >> +        }
> >> +    };
> >>      ($type:ty, $vmsd:expr) => {
> >>          unsafe impl $crate::vmstate::VMState for $type {
> >>              const BASE: $crate::bindings::VMStateField = {
> >> diff --git a/rust/qemu-api/tests/vmstate_tests.rs
> b/rust/qemu-api/tests/
> >> vmstate_tests.rs
> >> index 2c0670ba0eed..7d3180e6c2ea 100644
> >> --- a/rust/qemu-api/tests/vmstate_tests.rs
> >> +++ b/rust/qemu-api/tests/vmstate_tests.rs
> >> @@ -320,6 +320,7 @@ fn test_vmstate_struct_varray_uint8_wrapper() {
> >>          b"arr_a_wrap\0"
> >>      );
> >>      assert_eq!(foo_fields[5].num_offset, 228);
> >> +    assert_eq!(foo_fields[5].vmsd, VMSTATE_FOOA.as_ref());
> >>      assert!(unsafe { foo_fields[5].field_exists.unwrap()(foo_b_p, 0)
> });
> >>
> >>      // The last VMStateField in VMSTATE_FOOB.
> >> --
> >> 2.50.1
> >>
> >>
>

On Mon, Aug 04, 2025 at 12:08:15PM +0200, Paolo Bonzini wrote:
>Il lun 4 ago 2025, 10:56 Martin Kletzander <mkletzan@redhat.com> ha scritto:
>
>> *BUT* of course I had to rebase the patches on top of current rust-next
>> on Friday and there were some of your commits from Thursday which I now
>> see actually fix all what I tried fixing before as well.  I tried
>> finding the previous commit on which I saw all the issues and after some
>> rebuilding I could not.  So it is now not even broken on rust-next.
>>
>> This way I completely wasted your time, but at least learned something
>> that's happening in the code.  Sorry for that.
>>
>
>Uh no you didn't. It was broken.
>

But it is not now, neither master, nor rust-next, nor anything I tried
from the reflog, which makes me suspicious about what I was developing
this on.  I distinctly remember the `$vmsd.get()` call in the macro
which I presume was causing the copying due to VMStateDescription
automatically implementing the Copy trait due to the bindgen invocation.

But that's nowhere to be found, including git log --walk-reflog and
manually searching various history trees.  And I doubted that you
rewrote the history of the rust-next branch.  Neither do I remember
changing the macro until I found out that other in code changes did not
help to fix it.

After some time I now managed to find it.  It was the previous version
of a commit 4cb0670e12c4, and that is nowhere to be found in rust-next
at the moment, I guess fixes were incorporated while rebasing the branch
on current master.

That was a wild ride, but I'm glad it all works (apart from the bash
version indentation) on rust-next.  I'll try to read up on what's next
to help with, if anything.

Have a nice day,
Martin

>Paolo
>
>Martin
>>
>> >Paolo
>> >
>> > rust/qemu-api/src/vmstate.rs         | 11 +++++++++++
>> >>  rust/qemu-api/tests/vmstate_tests.rs |  1 +
>> >>  2 files changed, 12 insertions(+)
>> >>
>> >> diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/
>> vmstate.rs
>> >> index b5c6b764fbba..716e52afe740 100644
>> >> --- a/rust/qemu-api/src/vmstate.rs
>> >> +++ b/rust/qemu-api/src/vmstate.rs
>> >> @@ -449,6 +449,17 @@ macro_rules! vmstate_validate {
>> >>  /// description of the struct.
>> >>  #[macro_export]
>> >>  macro_rules! impl_vmstate_struct {
>> >> +    ($type:ty, $(&)?$vmsd:ident) => {
>> >> +        unsafe impl $crate::vmstate::VMState for $type {
>> >> +            const BASE: $crate::bindings::VMStateField =
>> >> +                $crate::bindings::VMStateField {
>> >> +                    vmsd: $vmsd.as_ref(),
>> >> +                    size: ::core::mem::size_of::<$type>(),
>> >> +                    flags: $crate::bindings::VMStateFlags::VMS_STRUCT,
>> >> +                    ..$crate::zeroable::Zeroable::ZERO
>> >> +                };
>> >> +        }
>> >> +    };
>> >>      ($type:ty, $vmsd:expr) => {
>> >>          unsafe impl $crate::vmstate::VMState for $type {
>> >>              const BASE: $crate::bindings::VMStateField = {
>> >> diff --git a/rust/qemu-api/tests/vmstate_tests.rs
>> b/rust/qemu-api/tests/
>> >> vmstate_tests.rs
>> >> index 2c0670ba0eed..7d3180e6c2ea 100644
>> >> --- a/rust/qemu-api/tests/vmstate_tests.rs
>> >> +++ b/rust/qemu-api/tests/vmstate_tests.rs
>> >> @@ -320,6 +320,7 @@ fn test_vmstate_struct_varray_uint8_wrapper() {
>> >>          b"arr_a_wrap\0"
>> >>      );
>> >>      assert_eq!(foo_fields[5].num_offset, 228);
>> >> +    assert_eq!(foo_fields[5].vmsd, VMSTATE_FOOA.as_ref());
>> >>      assert!(unsafe { foo_fields[5].field_exists.unwrap()(foo_b_p, 0)
>> });
>> >>
>> >>      // The last VMStateField in VMSTATE_FOOB.
>> >> --
>> >> 2.50.1
>> >>
>> >>
>>

Il lun 4 ago 2025, 14:04 Martin Kletzander <mkletzan@redhat.com> ha scritto:

> But that's nowhere to be found, including git log --walk-reflog and
> manually searching various history trees.  And I doubted that you
> rewrote the history of the rust-next branch


Yes, rust-next has its history rewritten.

By the way the bug was probably related to some compiler changes, because I
am almost certain it worked a couple months ago.

Paolo