| Message ID | SY0P300MB10265E25557DB71EE426525795432@SY0P300MB1026.AUSP300.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: introduce alternative implementation of offset_of! | expand |
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
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
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 > >
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
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,