[PATCH] rust: introduce alternative implementation of offset_of!

Junjie Mao posted 1 patch 1 day, 15 hours ago
There is a newer version of this series
rust/hw/char/pl011/src/device.rs              |   2 +-
rust/qemu-api-macros/src/lib.rs               | 103 +++++++++++++++++-
rust/qemu-api/meson.build                     |  11 +-
rust/qemu-api/src/device_class.rs             |   8 +-
rust/qemu-api/src/lib.rs                      |  16 +++
rust/qemu-api/src/tests.rs                    |  30 ++++-
subprojects/packagefiles/syn-2-rs/meson.build |   1 +
7 files changed, 160 insertions(+), 11 deletions(-)
[PATCH] rust: introduce alternative implementation of offset_of!
Posted by Junjie Mao 1 day, 15 hours ago
offset_of! was stabilized in Rust 1.77.0. Use an alternative implemenation
that was found on the Rust forums, and whose author agreed to license as
MIT for use in QEMU.

The alternative allows only one level of field access, but apart from this
can be used just by replacing core::mem::offset_of! with
qemu_api::offset_of!.

Using offset_of! prior Rust 1.77.0 requires the structure to have the
derive(qemu_api_macros::Offsets) attribute.

Apply as a replacement of 10/16 of Paolo's RFC series [1].

Also remove subprojects/syn-2.0.66 if there is an existing build. An additional
feature cfg is added to packagefiles/syn-2-rs/meson.build, which requires meson
to re-checkout the subproject.

[1] https://lore.kernel.org/qemu-devel/20241015131735.518771-1-pbonzini@redhat.com

Co-authored-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
---
 rust/hw/char/pl011/src/device.rs              |   2 +-
 rust/qemu-api-macros/src/lib.rs               | 103 +++++++++++++++++-
 rust/qemu-api/meson.build                     |  11 +-
 rust/qemu-api/src/device_class.rs             |   8 +-
 rust/qemu-api/src/lib.rs                      |  16 +++
 rust/qemu-api/src/tests.rs                    |  30 ++++-
 subprojects/packagefiles/syn-2-rs/meson.build |   1 +
 7 files changed, 160 insertions(+), 11 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 55d933ee5e..e9c0932712 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -29,7 +29,7 @@
 pub const PL011_FIFO_DEPTH: usize = 16_usize;
 
 #[repr(C)]
-#[derive(Debug, qemu_api_macros::Object)]
+#[derive(Debug, qemu_api_macros::Object, qemu_api_macros::Offsets)]
 /// PL011 Device Model in QEMU
 pub struct PL011State {
     pub parent_obj: SysBusDevice,
diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
index 59aba592d9..edf987dc37 100644
--- a/rust/qemu-api-macros/src/lib.rs
+++ b/rust/qemu-api-macros/src/lib.rs
@@ -4,7 +4,10 @@
 
 use proc_macro::TokenStream;
 use quote::{format_ident, quote};
-use syn::{parse_macro_input, DeriveInput};
+use syn::{
+    parse_macro_input, parse_quote, punctuated::Punctuated, token::Comma, Data, DeriveInput, Field,
+    Fields, Ident, Visibility,
+};
 
 #[proc_macro_derive(Object)]
 pub fn derive_object(input: TokenStream) -> TokenStream {
@@ -41,3 +44,101 @@ extern "C" fn __load() {
 
     TokenStream::from(expanded)
 }
+
+fn is_c_repr(input: &DeriveInput) -> Result<(), proc_macro2::TokenStream> {
+    let expected = parse_quote! { #[repr(C)] };
+
+    for attr in &input.attrs {
+        if attr == &expected {
+            return Ok(());
+        }
+    }
+
+    Err(quote! { "Can only generate offsets for structs with a C representation." })
+}
+
+fn get_fields(input: &DeriveInput) -> Result<&Punctuated<Field, Comma>, proc_macro2::TokenStream> {
+    if let Data::Struct(s) = &input.data {
+        if let Fields::Named(fs) = &s.fields {
+            Ok(&fs.named)
+        } else {
+            Err(quote! { "Cannot generate offsets for unnamed fields." })
+        }
+    } else {
+        Err(quote! { "Cannot generate offsets for union or enum." })
+    }
+}
+
+#[rustfmt::skip::macros(quote)]
+fn derive_offsets_or_error(
+    input: DeriveInput,
+) -> Result<proc_macro2::TokenStream, proc_macro2::TokenStream> {
+    // The way to generate field offset constants comes from:
+    //
+    //     https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=10a22a9b8393abd7b541d8fc844bc0df
+    //
+    // used under MIT license with permission of Yandros aka Daniel Henry-Mantilla
+
+    is_c_repr(&input)?;
+
+    let name = &input.ident;
+    let fields = get_fields(&input)?;
+    let field_names: Vec<&Ident> = fields.iter().map(|f| f.ident.as_ref().unwrap()).collect();
+    let field_vis: Vec<&Visibility> = fields.iter().map(|f| &f.vis).collect();
+
+    let mut field_offsets = quote! {};
+    let mut end_of_prev_field = quote! { 0 };
+    for field in fields {
+        let name = field.ident.as_ref().unwrap();
+        let ty = &field.ty;
+
+        field_offsets.extend(
+            quote! {
+                const #name: usize = {
+                    let end_of_prev_field: usize = #end_of_prev_field;
+                    let align = std::mem::align_of::<#ty>();
+                    let trail = end_of_prev_field % align;
+                    end_of_prev_field + (align - trail) * [1, 0][(trail == 0) as usize]
+                };
+            }
+            .into_iter(),
+        );
+
+        end_of_prev_field = quote! {
+            Helper::#name + std::mem::size_of::<#ty>()
+        }
+    }
+
+    Ok(quote! {
+        #[cfg(not(has_offset_of))]
+        #[allow(nonstandard_style)]
+        const _: () = {
+            pub struct StructOffsets {
+                #(#field_vis #field_names: usize,)*
+            }
+            struct Helper;
+            impl #name {
+                pub const offset_to: StructOffsets = StructOffsets {
+                    #(#field_names: Helper::#field_names,)*
+                };
+            }
+            impl Helper {
+                #field_offsets
+            }
+        };
+    })
+}
+
+/// A derive macro that generate field offsets for using `offset_of!` in
+/// versions of Rust prior to 1.77
+#[proc_macro_derive(Offsets)]
+pub fn derive_offsets(input: TokenStream) -> TokenStream {
+    let input = parse_macro_input!(input as DeriveInput);
+
+    let expanded = match derive_offsets_or_error(input) {
+        Ok(ts) => ts,
+        Err(msg) => quote! { compile_error!(#msg); },
+    };
+
+    proc_macro::TokenStream::from(expanded)
+}
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index b55931c649..9a16b2872f 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -1,3 +1,9 @@
+_qemu_api_cfg = ['--cfg', 'MESON']
+# _qemu_api_cfg += ['--cfg', 'feature="allocator"']
+if rustc.version().version_compare('>=1.77.0')
+  _qemu_api_cfg += ['--cfg', 'has_offset_of']
+endif
+
 _qemu_api_rs = static_library(
   'qemu_api',
   structured_sources(
@@ -12,10 +18,7 @@ _qemu_api_rs = static_library(
   ),
   override_options: ['rust_std=2021', 'build.rust_std=2021'],
   rust_abi: 'rust',
-  rust_args: [
-    '--cfg', 'MESON',
-    # '--cfg', 'feature="allocator"',
-  ],
+  rust_args: _qemu_api_cfg,
   dependencies: [
     qemu_api_macros,
   ],
diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
index 871063d4a9..d4fa544df3 100644
--- a/rust/qemu-api/src/device_class.rs
+++ b/rust/qemu-api/src/device_class.rs
@@ -26,7 +26,7 @@ macro_rules! device_class_init {
 
 #[macro_export]
 macro_rules! define_property {
-    ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr, default = $defval:expr$(,)*) => {
+    ($name:expr, $state:ty, $field:ident, $prop:expr, $type:expr, default = $defval:expr$(,)*) => {
         $crate::bindings::Property {
             name: {
                 #[used]
@@ -34,7 +34,7 @@ macro_rules! define_property {
                 _TEMP.as_ptr()
             },
             info: $prop,
-            offset: ::core::mem::offset_of!($state, $field)
+            offset: $crate::offset_of!($state, $field)
                 .try_into()
                 .expect("Could not fit offset value to type"),
             bitnr: 0,
@@ -47,7 +47,7 @@ macro_rules! define_property {
             link_type: ::core::ptr::null(),
         }
     };
-    ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr$(,)*) => {
+    ($name:expr, $state:ty, $field:ident, $prop:expr, $type:expr$(,)*) => {
         $crate::bindings::Property {
             name: {
                 #[used]
@@ -55,7 +55,7 @@ macro_rules! define_property {
                 _TEMP.as_ptr()
             },
             info: $prop,
-            offset: ::core::mem::offset_of!($state, $field)
+            offset: $crate::offset_of!($state, $field)
                 .try_into()
                 .expect("Could not fit offset value to type"),
             bitnr: 0,
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index 9b2483fbfa..fd37d68eb5 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -167,3 +167,19 @@ unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
         }
     }
 }
+
+/// This macro provides the same functionality as `core::mem::offset_of`,
+/// except that only one level of field access is supported. The declaration
+/// of the struct must be attributed `derive(qemu_api_macros::Offsets)`.
+///
+/// It is needed because `offset_of!` was only stabilized in Rust 1.77.
+#[cfg(not(has_offset_of))]
+#[macro_export]
+macro_rules! offset_of {
+    ($container:ty, $field:ident) => {
+        <$container>::offset_to.$field
+    };
+}
+
+#[cfg(has_offset_of)]
+pub use std::mem::offset_of;
diff --git a/rust/qemu-api/src/tests.rs b/rust/qemu-api/src/tests.rs
index d34b8d2418..479dd9b450 100644
--- a/rust/qemu-api/src/tests.rs
+++ b/rust/qemu-api/src/tests.rs
@@ -3,7 +3,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 
 use crate::{
-    bindings::*, c_str, declare_properties, define_property, device_class_init, vm_state_description,
+    bindings::*, c_str, declare_properties, define_property, device_class_init, offset_of, vm_state_description,
 };
 
 #[test]
@@ -16,6 +16,7 @@ fn test_device_decl_macros() {
     }
 
     #[repr(C)]
+    #[derive(qemu_api_macros::Offsets)]
     pub struct DummyState {
         pub char_backend: CharBackend,
         pub migrate_clock: bool,
@@ -47,3 +48,30 @@ pub struct DummyState {
         vmsd => VMSTATE,
     }
 }
+
+#[test]
+fn test_offset_of() {
+    #[repr(C)]
+    struct Foo {
+	a: u16,
+	b: u32,
+	c: u64,
+	d: u16,
+    }
+
+    #[repr(C)]
+    #[derive(qemu_api_macros::Offsets)]
+    struct Bar {
+	pub a: u16,
+	pub b: u64,
+	c: Foo,
+	d: u64,
+    }
+
+    const OFFSET_TO_C: usize = offset_of!(Bar, c);
+
+    assert_eq!(offset_of!(Bar, a), 0);
+    assert_eq!(offset_of!(Bar, b), 8);
+    assert_eq!(OFFSET_TO_C, 16);
+    assert_eq!(offset_of!(Bar, d), 40);
+}
diff --git a/subprojects/packagefiles/syn-2-rs/meson.build b/subprojects/packagefiles/syn-2-rs/meson.build
index a53335f309..9f56ce1c24 100644
--- a/subprojects/packagefiles/syn-2-rs/meson.build
+++ b/subprojects/packagefiles/syn-2-rs/meson.build
@@ -24,6 +24,7 @@ _syn_rs = static_library(
     '--cfg', 'feature="printing"',
     '--cfg', 'feature="clone-impls"',
     '--cfg', 'feature="proc-macro"',
+    '--cfg', 'feature="extra-traits"',
   ],
   dependencies: [
     quote_dep,
-- 
2.34.1
Re: [PATCH] rust: introduce alternative implementation of offset_of!
Posted by Paolo Bonzini 1 day, 14 hours ago
On 10/21/24 07:40, Junjie Mao wrote:
> offset_of! was stabilized in Rust 1.77.0. Use an alternative implemenation
> that was found on the Rust forums, and whose author agreed to license as
> MIT for use in QEMU.
> 
> The alternative allows only one level of field access, but apart from this
> can be used just by replacing core::mem::offset_of! with
> qemu_api::offset_of!.
> 
> Using offset_of! prior Rust 1.77.0 requires the structure to have the
> derive(qemu_api_macros::Offsets) attribute.
> 
> Apply as a replacement of 10/16 of Paolo's RFC series [1].
> 
> Also remove subprojects/syn-2.0.66 if there is an existing build. An additional
> feature cfg is added to packagefiles/syn-2-rs/meson.build, which requires meson
> to re-checkout the subproject.
> 
> [1] https://lore.kernel.org/qemu-devel/20241015131735.518771-1-pbonzini@redhat.com
> 
> Co-authored-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>

Thanks.  I still prefer to keep the procedural macro code minimal, and 
have the code generation in a separate macro, but this is a nice start!

> +fn is_c_repr(input: &DeriveInput) -> Result<(), proc_macro2::TokenStream> {
> +    let expected = parse_quote! { #[repr(C)] };
> +
> +    for attr in &input.attrs {
> +        if attr == &expected {
> +            return Ok(());
> +        }
> +    }
> +
> +    Err(quote! { "Can only generate offsets for structs with a C representation." })
> +}

Probably can use any() here.

> +/// A derive macro that generate field offsets for using `offset_of!` in
> +/// versions of Rust prior to 1.77
> +#[proc_macro_derive(Offsets)]
> +pub fn derive_offsets(input: TokenStream) -> TokenStream {
> +    let input = parse_macro_input!(input as DeriveInput);
> +
> +    let expanded = match derive_offsets_or_error(input) {
> +        Ok(ts) => ts,
> +        Err(msg) => quote! { compile_error!(#msg); },

It should use quote_spanned! here.

Paolo
Re: [PATCH] rust: introduce alternative implementation of offset_of!
Posted by Junjie Mao 1 day, 14 hours ago
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 10/21/24 07:40, Junjie Mao wrote:
>> offset_of! was stabilized in Rust 1.77.0. Use an alternative implemenation
>> that was found on the Rust forums, and whose author agreed to license as
>> MIT for use in QEMU.
>> The alternative allows only one level of field access, but apart from this
>> can be used just by replacing core::mem::offset_of! with
>> qemu_api::offset_of!.
>> Using offset_of! prior Rust 1.77.0 requires the structure to have the
>> derive(qemu_api_macros::Offsets) attribute.
>> Apply as a replacement of 10/16 of Paolo's RFC series [1].
>> Also remove subprojects/syn-2.0.66 if there is an existing build. An
>> additional
>> feature cfg is added to packagefiles/syn-2-rs/meson.build, which requires meson
>> to re-checkout the subproject.
>> [1]
>> https://lore.kernel.org/qemu-devel/20241015131735.518771-1-pbonzini@redhat.com
>> Co-authored-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
>
> Thanks.  I still prefer to keep the procedural macro code minimal, and have the
> code generation in a separate macro, but this is a nice start!
>

I'm not sure if I get your point right.

My understanding is that preferring minimizing proc macros is because
they generate a big, arbitrary block of code that is hard to read and
debug directly (which requires cargo expand the whole crate). That is
thus more error-prone and makes maintenance harder.

As for having "the code generation in a separate macro", are you
referring to `macro_rules!`?

>> +fn is_c_repr(input: &DeriveInput) -> Result<(), proc_macro2::TokenStream> {
>> +    let expected = parse_quote! { #[repr(C)] };
>> +
>> +    for attr in &input.attrs {
>> +        if attr == &expected {
>> +            return Ok(());
>> +        }
>> +    }
>> +
>> +    Err(quote! { "Can only generate offsets for structs with a C representation." })
>> +}
>
> Probably can use any() here.
>

Sure.

>> +/// A derive macro that generate field offsets for using `offset_of!` in
>> +/// versions of Rust prior to 1.77
>> +#[proc_macro_derive(Offsets)]
>> +pub fn derive_offsets(input: TokenStream) -> TokenStream {
>> +    let input = parse_macro_input!(input as DeriveInput);
>> +
>> +    let expanded = match derive_offsets_or_error(input) {
>> +        Ok(ts) => ts,
>> +        Err(msg) => quote! { compile_error!(#msg); },
>
> It should use quote_spanned! here.

Sure. Will use quote_spanned! here and make reported errors being a
tuple of error msg and span to use.

>
> Paolo

--
Best Regards
Junjie Mao
Re: [PATCH] rust: introduce alternative implementation of offset_of!
Posted by Paolo Bonzini 1 day, 12 hours ago
Il lun 21 ott 2024, 09:24 Junjie Mao <junjie.mao@hotmail.com> ha scritto:

> > Thanks.  I still prefer to keep the procedural macro code minimal, and
> have the
> > code generation in a separate macro, but this is a nice start!
> >
>
> I'm not sure if I get your point right.
>
> My understanding is that preferring minimizing proc macros is because
> they generate a big, arbitrary block of code that is hard to read and
> debug directly (which requires cargo expand the whole crate). That is
> thus more error-prone and makes maintenance harder.
>
> As for having "the code generation in a separate macro", are you
> referring to `macro_rules!`?
>

Yes, keeping the generation of the impl block in with_offsets!. Then you
can get the best of both worlds in my opinion, for example the #[repr(C)]
check is more robust in the procedural macro.

Sure. Will use quote_spanned! here and make reported errors being a
> tuple of error msg and span to use.
>

If it's okay for you let's first get the fixes in, and then I can repost
the MSRV series and include your procedural macro and unit tests.

Paolo


> >
> > Paolo
>
> --
> Best Regards
> Junjie Mao
>
>
Re: [PATCH] rust: introduce alternative implementation of offset_of!
Posted by Junjie Mao 18 hours ago
Paolo Bonzini <pbonzini@redhat.com> writes:

> Il lun 21 ott 2024, 09:24 Junjie Mao <junjie.mao@hotmail.com> ha scritto:
>
>  > Thanks.  I still prefer to keep the procedural macro code minimal, and have the
>  > code generation in a separate macro, but this is a nice start!
>  >
>
>  I'm not sure if I get your point right.
>
>  My understanding is that preferring minimizing proc macros is because
>  they generate a big, arbitrary block of code that is hard to read and
>  debug directly (which requires cargo expand the whole crate). That is
>  thus more error-prone and makes maintenance harder.
>
>  As for having "the code generation in a separate macro", are you
>  referring to `macro_rules!`?
>
> Yes, keeping the generation of the impl block in with_offsets!. Then you can get the best of both worlds in my opinion, for example the #[repr(C)] check is more robust in the procedural macro.

Got it. Will try that approach in v2.

>
>  Sure. Will use quote_spanned! here and make reported errors being a
>  tuple of error msg and span to use.
>
> If it's okay for you let's first get the fixes in, and then I can repost the MSRV series and include your procedural macro and unit tests.

The v2 fixes look good to me overall. Thanks!

--
Best Regards
Junjie Mao