From: Junjie Mao <junjie.mao@hotmail.com>
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!.
The actual implementation of offset_of! is done in a declarative macro,
but for simplicity and to avoid introducing an extra level of indentation,
the trigger is a procedural macro #[derive(offsets)].
The procedural macro is perhaps a bit overengineered, but it helps
introducing some idioms that will be useful in the future as well.
Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/Cargo.lock | 1 +
rust/hw/char/pl011/src/device.rs | 2 +-
rust/qemu-api-macros/Cargo.toml | 2 +-
rust/qemu-api-macros/src/lib.rs | 81 ++++++++-
rust/qemu-api/Cargo.toml | 6 +-
rust/qemu-api/build.rs | 9 +
rust/qemu-api/meson.build | 12 +-
rust/qemu-api/src/device_class.rs | 8 +-
rust/qemu-api/src/lib.rs | 4 +
rust/qemu-api/src/offset_of.rs | 161 ++++++++++++++++++
rust/qemu-api/src/vmstate.rs | 10 +-
rust/qemu-api/tests/tests.rs | 1 +
subprojects/packagefiles/syn-2-rs/meson.build | 1 +
13 files changed, 280 insertions(+), 18 deletions(-)
create mode 100644 rust/qemu-api/src/offset_of.rs
diff --git a/rust/Cargo.lock b/rust/Cargo.lock
index 9f43b33e8b8..c0c6069247a 100644
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -93,6 +93,7 @@ name = "qemu_api"
version = "0.1.0"
dependencies = [
"qemu_api_macros",
+ "version_check",
]
[[package]]
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 2d225d544de..bca727e37f0 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -55,7 +55,7 @@ impl DeviceId {
}
#[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/Cargo.toml b/rust/qemu-api-macros/Cargo.toml
index f8d6d03609f..a8f7377106b 100644
--- a/rust/qemu-api-macros/Cargo.toml
+++ b/rust/qemu-api-macros/Cargo.toml
@@ -19,4 +19,4 @@ proc-macro = true
[dependencies]
proc-macro2 = "1"
quote = "1"
-syn = "2"
+syn = { version = "2", features = ["extra-traits"] }
diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
index a4bc5d01ee8..c2ea22101e4 100644
--- a/rust/qemu-api-macros/src/lib.rs
+++ b/rust/qemu-api-macros/src/lib.rs
@@ -3,8 +3,34 @@
// SPDX-License-Identifier: GPL-2.0-or-later
use proc_macro::TokenStream;
-use quote::quote;
-use syn::{parse_macro_input, DeriveInput};
+use proc_macro2::Span;
+use quote::{quote, quote_spanned};
+use syn::{
+ parse_macro_input, parse_quote, punctuated::Punctuated, token::Comma, Data, DeriveInput, Field,
+ Fields, Ident, Type, Visibility,
+};
+
+struct CompileError(String, Span);
+
+impl From<CompileError> for proc_macro2::TokenStream {
+ fn from(err: CompileError) -> Self {
+ let CompileError(msg, span) = err;
+ quote_spanned! { span => compile_error!(#msg); }
+ }
+}
+
+fn is_c_repr(input: &DeriveInput, msg: &str) -> Result<(), CompileError> {
+ let expected = parse_quote! { #[repr(C)] };
+
+ if input.attrs.iter().any(|attr| attr == &expected) {
+ Ok(())
+ } else {
+ Err(CompileError(
+ format!("#[repr(C)] required for {}", msg),
+ input.ident.span(),
+ ))
+ }
+}
#[proc_macro_derive(Object)]
pub fn derive_object(input: TokenStream) -> TokenStream {
@@ -21,3 +53,48 @@ pub fn derive_object(input: TokenStream) -> TokenStream {
TokenStream::from(expanded)
}
+
+fn get_fields(input: &DeriveInput) -> Result<&Punctuated<Field, Comma>, CompileError> {
+ if let Data::Struct(s) = &input.data {
+ if let Fields::Named(fs) = &s.fields {
+ Ok(&fs.named)
+ } else {
+ Err(CompileError(
+ "Cannot generate offsets for unnamed fields.".to_string(),
+ input.ident.span(),
+ ))
+ }
+ } else {
+ Err(CompileError(
+ "Cannot generate offsets for union or enum.".to_string(),
+ input.ident.span(),
+ ))
+ }
+}
+
+#[rustfmt::skip::macros(quote)]
+fn derive_offsets_or_error(input: DeriveInput) -> Result<proc_macro2::TokenStream, CompileError> {
+ is_c_repr(&input, "#[derive(offsets)]")?;
+
+ 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_types: Vec<&Type> = fields.iter().map(|f| &f.ty).collect();
+ let field_vis: Vec<&Visibility> = fields.iter().map(|f| &f.vis).collect();
+
+ Ok(quote! {
+ ::qemu_api::with_offsets! {
+ struct #name {
+ #(#field_vis #field_names: #field_types,)*
+ }
+ }
+ })
+}
+
+#[proc_macro_derive(offsets)]
+pub fn derive_offsets(input: TokenStream) -> TokenStream {
+ let input = parse_macro_input!(input as DeriveInput);
+ let expanded = derive_offsets_or_error(input).unwrap_or_else(Into::into);
+
+ TokenStream::from(expanded)
+}
diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml
index e092f61e8f3..cc716d75d46 100644
--- a/rust/qemu-api/Cargo.toml
+++ b/rust/qemu-api/Cargo.toml
@@ -16,9 +16,13 @@ categories = []
[dependencies]
qemu_api_macros = { path = "../qemu-api-macros" }
+[build-dependencies]
+version_check = "~0.9"
+
[features]
default = []
allocator = []
[lints.rust]
-unexpected_cfgs = { level = "warn", check-cfg = ['cfg(MESON)', 'cfg(HAVE_GLIB_WITH_ALIGNED_ALLOC)'] }
+unexpected_cfgs = { level = "warn", check-cfg = ['cfg(MESON)', 'cfg(HAVE_GLIB_WITH_ALIGNED_ALLOC)',
+ 'cfg(has_offset_of)'] }
diff --git a/rust/qemu-api/build.rs b/rust/qemu-api/build.rs
index 419b154c2d2..20f8f718b90 100644
--- a/rust/qemu-api/build.rs
+++ b/rust/qemu-api/build.rs
@@ -4,6 +4,8 @@
use std::path::Path;
+use version_check as rustc;
+
fn main() {
if !Path::new("src/bindings.rs").exists() {
panic!(
@@ -11,4 +13,11 @@ fn main() {
(`ninja bindings.rs`) and copy them to src/bindings.rs, or build through meson."
);
}
+
+ // Check for available rustc features
+ if rustc::is_min_version("1.77.0").unwrap_or(false) {
+ println!("cargo:rustc-cfg=has_offset_of");
+ }
+
+ println!("cargo:rerun-if-changed=build.rs");
}
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index c950b008d59..6f637af7b1b 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(
@@ -6,6 +12,7 @@ _qemu_api_rs = static_library(
'src/c_str.rs',
'src/definitions.rs',
'src/device_class.rs',
+ 'src/offset_of.rs',
'src/vmstate.rs',
'src/zeroable.rs',
],
@@ -13,10 +20,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,
)
rust.test('rust-qemu-api-tests', _qemu_api_rs,
diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
index cb4573ca6ef..56608c7f7fc 100644
--- a/rust/qemu-api/src/device_class.rs
+++ b/rust/qemu-api/src/device_class.rs
@@ -23,23 +23,23 @@ 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 {
// use associated function syntax for type checking
name: ::std::ffi::CStr::as_ptr($name),
info: $prop,
- offset: ::core::mem::offset_of!($state, $field) as isize,
+ offset: $crate::offset_of!($state, $field) as isize,
set_default: true,
defval: $crate::bindings::Property__bindgen_ty_1 { u: $defval as u64 },
..$crate::zeroable::Zeroable::ZERO
}
};
- ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr$(,)*) => {
+ ($name:expr, $state:ty, $field:ident, $prop:expr, $type:expr$(,)*) => {
$crate::bindings::Property {
// use associated function syntax for type checking
name: ::std::ffi::CStr::as_ptr($name),
info: $prop,
- offset: ::core::mem::offset_of!($state, $field) as isize,
+ offset: $crate::offset_of!($state, $field) as isize,
set_default: false,
..$crate::zeroable::Zeroable::ZERO
}
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index e6bd953e10b..aa8d16ec94b 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -32,6 +32,7 @@ unsafe impl Sync for bindings::VMStateInfo {}
pub mod c_str;
pub mod definitions;
pub mod device_class;
+pub mod offset_of;
pub mod vmstate;
pub mod zeroable;
@@ -169,3 +170,6 @@ unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
}
}
}
+
+#[cfg(has_offset_of)]
+pub use core::mem::offset_of;
diff --git a/rust/qemu-api/src/offset_of.rs b/rust/qemu-api/src/offset_of.rs
new file mode 100644
index 00000000000..075e98f986b
--- /dev/null
+++ b/rust/qemu-api/src/offset_of.rs
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: MIT
+
+/// 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 wrapped with `with_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
+ };
+}
+
+/// A wrapper for struct declarations, that allows using `offset_of!` in
+/// versions of Rust prior to 1.77
+#[macro_export]
+macro_rules! with_offsets {
+ // This method 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
+ (
+ $(#[$struct_meta:meta])*
+ $struct_vis:vis
+ struct $StructName:ident {
+ $(
+ $(#[$field_meta:meta])*
+ $field_vis:vis
+ $field_name:ident : $field_ty:ty
+ ),*
+ $(,)?
+ }
+ ) => (
+ #[cfg(not(has_offset_of))]
+ const _: () = {
+ struct StructOffsetsHelper<T>(std::marker::PhantomData<T>);
+ const END_OF_PREV_FIELD: usize = 0;
+
+ // populate StructOffsetsHelper<T> with associated consts,
+ // one for each field
+ $crate::with_offsets! {
+ @struct $StructName
+ @names [ $($field_name)* ]
+ @tys [ $($field_ty ,)*]
+ }
+
+ // now turn StructOffsetsHelper<T>'s consts into a single struct,
+ // applying field visibility. This provides better error messages
+ // than if offset_of! used StructOffsetsHelper::<T> directly.
+ pub
+ struct StructOffsets {
+ $(
+ $field_vis
+ $field_name: usize,
+ )*
+ }
+ impl $StructName {
+ pub
+ const OFFSET_TO__: StructOffsets = StructOffsets {
+ $(
+ $field_name: StructOffsetsHelper::<$StructName>::$field_name,
+ )*
+ };
+ }
+ };
+ );
+
+ (
+ @struct $StructName:ident
+ @names []
+ @tys []
+ ) => ();
+
+ (
+ @struct $StructName:ident
+ @names [$field_name:ident $($other_names:tt)*]
+ @tys [$field_ty:ty , $($other_tys:tt)*]
+ ) => (
+ #[allow(non_local_definitions)]
+ #[allow(clippy::modulo_one)]
+ impl StructOffsetsHelper<$StructName> {
+ #[allow(nonstandard_style)]
+ const $field_name: usize = {
+ const ALIGN: usize = std::mem::align_of::<$field_ty>();
+ const TRAIL: usize = END_OF_PREV_FIELD % ALIGN;
+ END_OF_PREV_FIELD + (if TRAIL == 0 { 0usize } else { ALIGN - TRAIL })
+ };
+ }
+ const _: () = {
+ const END_OF_PREV_FIELD: usize =
+ StructOffsetsHelper::<$StructName>::$field_name +
+ std::mem::size_of::<$field_ty>()
+ ;
+ $crate::with_offsets! {
+ @struct $StructName
+ @names [$($other_names)*]
+ @tys [$($other_tys)*]
+ }
+ };
+ );
+}
+
+#[cfg(test)]
+mod tests {
+ use crate::offset_of;
+
+ #[repr(C)]
+ struct Foo {
+ a: u16,
+ b: u32,
+ c: u64,
+ d: u16,
+ }
+
+ #[repr(C)]
+ struct Bar {
+ pub a: u16,
+ pub b: u64,
+ c: Foo,
+ d: u64,
+ }
+
+ crate::with_offsets! {
+ #[repr(C)]
+ struct Bar {
+ pub a: u16,
+ pub b: u64,
+ c: Foo,
+ d: u64,
+ }
+ }
+
+ #[repr(C)]
+ pub struct Baz {
+ b: u32,
+ a: u8,
+ }
+ crate::with_offsets! {
+ #[repr(C)]
+ pub struct Baz {
+ b: u32,
+ a: u8,
+ }
+ }
+
+ #[test]
+ fn test_offset_of() {
+ 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);
+
+ assert_eq!(offset_of!(Baz, b), 0);
+ assert_eq!(offset_of!(Baz, a), 4);
+ }
+}
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 0aaaef1c7c8..b452d01ae3e 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -57,7 +57,7 @@ macro_rules! vmstate_single_test {
.as_bytes()
.as_ptr() as *const ::std::os::raw::c_char,
err_hint: ::core::ptr::null(),
- offset: ::core::mem::offset_of!($struct_name, $field_name),
+ offset: $crate::offset_of!($struct_name, $field_name),
size: $size,
start: 0,
num: 0,
@@ -134,7 +134,7 @@ macro_rules! vmstate_array {
.as_bytes()
.as_ptr() as *const ::std::os::raw::c_char,
err_hint: ::core::ptr::null(),
- offset: ::core::mem::offset_of!($struct_name, $field_name),
+ offset: $crate::offset_of!($struct_name, $field_name),
size: $size,
start: 0,
num: $length as _,
@@ -182,7 +182,7 @@ macro_rules! vmstate_struct_pointer_v {
.as_bytes()
.as_ptr() as *const ::std::os::raw::c_char,
err_hint: ::core::ptr::null(),
- offset: ::core::mem::offset_of!($struct_name, $field_name),
+ offset: $crate::offset_of!($struct_name, $field_name),
size: ::core::mem::size_of::<*const $type>(),
start: 0,
num: 0,
@@ -211,7 +211,7 @@ macro_rules! vmstate_array_of_pointer {
info: $info,
size: ::core::mem::size_of::<*const $type>(),
flags: VMStateFlags(VMStateFlags::VMS_ARRAY.0 | VMStateFlags::VMS_ARRAY_OF_POINTER.0),
- offset: ::core::mem::offset_of!($struct_name, $field_name),
+ offset: $crate::offset_of!($struct_name, $field_name),
err_hint: ::core::ptr::null(),
start: 0,
num_offset: 0,
@@ -240,7 +240,7 @@ macro_rules! vmstate_array_of_pointer_to_struct {
| VMStateFlags::VMS_STRUCT.0
| VMStateFlags::VMS_ARRAY_OF_POINTER.0,
),
- offset: ::core::mem::offset_of!($struct_name, $field_name),
+ offset: $crate::offset_of!($struct_name, $field_name),
err_hint: ::core::ptr::null(),
start: 0,
num_offset: 0,
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index 381ac84657b..7442f695646 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -21,6 +21,7 @@ fn test_device_decl_macros() {
..Zeroable::ZERO
};
+ #[derive(qemu_api_macros::offsets)]
#[repr(C)]
#[derive(qemu_api_macros::Object)]
pub struct DummyState {
diff --git a/subprojects/packagefiles/syn-2-rs/meson.build b/subprojects/packagefiles/syn-2-rs/meson.build
index a53335f3092..9f56ce1c24d 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.47.0
Paolo Bonzini <pbonzini@redhat.com> writes:
> From: Junjie Mao <junjie.mao@hotmail.com>
>
> 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!.
>
> The actual implementation of offset_of! is done in a declarative macro,
> but for simplicity and to avoid introducing an extra level of indentation,
> the trigger is a procedural macro #[derive(offsets)].
>
> The procedural macro is perhaps a bit overengineered, but it helps
> introducing some idioms that will be useful in the future as well.
>
> Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Generally looks good to me. Thanks for integrating this!
It seems Rust does not have builtin support for unit tests expecting
compilation failures. There is a crate (named trybuild [1]) for that
purpose but it requires introducing a dozen of new dependencies (see
below). Do you think it worth the effort? If so, I can take a closer
look and cook something for initial review (probably post 9.2).
trybuild v1.0.101
├── glob v0.3.1
├── serde v1.0.210
├── serde_derive v1.0.210 (proc-macro)
│ ├── proc-macro2 v1.0.84 (*)
│ ├── quote v1.0.36 (*)
│ └── syn v2.0.66 (*)
├── serde_json v1.0.132
│ ├── itoa v1.0.11
│ ├── memchr v2.7.4
│ ├── ryu v1.0.18
│ └── serde v1.0.210
├── target-triple v0.1.3
├── termcolor v1.4.1
└── toml v0.8.19
├── serde v1.0.210
├── serde_spanned v0.6.8
│ └── serde v1.0.210
├── toml_datetime v0.6.8
│ └── serde v1.0.210
└── toml_edit v0.22.22
├── indexmap v2.6.0
│ ├── equivalent v1.0.1
│ └── hashbrown v0.15.0
├── serde v1.0.210
├── serde_spanned v0.6.8 (*)
├── toml_datetime v0.6.8 (*)
└── winnow v0.6.20
[1] https://docs.rs/trybuild/latest/trybuild/
[snip]
> diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
> index a4bc5d01ee8..c2ea22101e4 100644
> --- a/rust/qemu-api-macros/src/lib.rs
> +++ b/rust/qemu-api-macros/src/lib.rs
> @@ -3,8 +3,34 @@
> // SPDX-License-Identifier: GPL-2.0-or-later
>
> use proc_macro::TokenStream;
> -use quote::quote;
> -use syn::{parse_macro_input, DeriveInput};
> +use proc_macro2::Span;
> +use quote::{quote, quote_spanned};
> +use syn::{
> + parse_macro_input, parse_quote, punctuated::Punctuated, token::Comma, Data, DeriveInput, Field,
> + Fields, Ident, Type, Visibility,
> +};
> +
> +struct CompileError(String, Span);
> +
> +impl From<CompileError> for proc_macro2::TokenStream {
> + fn from(err: CompileError) -> Self {
> + let CompileError(msg, span) = err;
> + quote_spanned! { span => compile_error!(#msg); }
The documentation [2] says "there should be no space before the =>
token" and that is by intention to tell that `span` is "evaluated in the
context of proc macro" while those after the arm "in the generated
code". Should we follow that convention (even though the extra white
space does not impact building)?
[2] https://docs.rs/quote/latest/quote/macro.quote_spanned.html
--
Best Regards
Junjie Mao
On 11/3/24 10:54, Junjie Mao wrote:
>
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> From: Junjie Mao <junjie.mao@hotmail.com>
>>
>> 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!.
>>
>> The actual implementation of offset_of! is done in a declarative macro,
>> but for simplicity and to avoid introducing an extra level of indentation,
>> the trigger is a procedural macro #[derive(offsets)].
>>
>> The procedural macro is perhaps a bit overengineered, but it helps
>> introducing some idioms that will be useful in the future as well.
>>
>> Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
>> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Generally looks good to me. Thanks for integrating this!
>
> It seems Rust does not have builtin support for unit tests expecting
> compilation failures. There is a crate (named trybuild [1]) for that
> purpose but it requires introducing a dozen of new dependencies (see
> below). Do you think it worth the effort? If so, I can take a closer
> look and cook something for initial review (probably post 9.2).
>
> trybuild v1.0.101
> ├── glob v0.3.1
> ├── serde v1.0.210
> ├── serde_derive v1.0.210 (proc-macro)
> │ ├── proc-macro2 v1.0.84 (*)
> │ ├── quote v1.0.36 (*)
> │ └── syn v2.0.66 (*)
> ├── serde_json v1.0.132
> │ ├── itoa v1.0.11
> │ ├── memchr v2.7.4
> │ ├── ryu v1.0.18
> │ └── serde v1.0.210
> ├── target-triple v0.1.3
> ├── termcolor v1.4.1
> └── toml v0.8.19
> ├── serde v1.0.210
> ├── serde_spanned v0.6.8
> │ └── serde v1.0.210
> ├── toml_datetime v0.6.8
> │ └── serde v1.0.210
> └── toml_edit v0.22.22
> ├── indexmap v2.6.0
> │ ├── equivalent v1.0.1
> │ └── hashbrown v0.15.0
> ├── serde v1.0.210
> ├── serde_spanned v0.6.8 (*)
> ├── toml_datetime v0.6.8 (*)
> └── winnow v0.6.20
>
> [1] https://docs.rs/trybuild/latest/trybuild/
>
> [snip]
>> diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
>> index a4bc5d01ee8..c2ea22101e4 100644
>> --- a/rust/qemu-api-macros/src/lib.rs
>> +++ b/rust/qemu-api-macros/src/lib.rs
>> @@ -3,8 +3,34 @@
>> // SPDX-License-Identifier: GPL-2.0-or-later
>>
>> use proc_macro::TokenStream;
>> -use quote::quote;
>> -use syn::{parse_macro_input, DeriveInput};
>> +use proc_macro2::Span;
>> +use quote::{quote, quote_spanned};
>> +use syn::{
>> + parse_macro_input, parse_quote, punctuated::Punctuated, token::Comma, Data, DeriveInput, Field,
>> + Fields, Ident, Type, Visibility,
>> +};
>> +
>> +struct CompileError(String, Span);
>> +
>> +impl From<CompileError> for proc_macro2::TokenStream {
>> + fn from(err: CompileError) -> Self {
>> + let CompileError(msg, span) = err;
>> + quote_spanned! { span => compile_error!(#msg); }
>
> The documentation [2] says "there should be no space before the =>
> token" and that is by intention to tell that `span` is "evaluated in the
> context of proc macro" while those after the arm "in the generated
> code". Should we follow that convention (even though the extra white
> space does not impact building)?
Ah, forgot to reply about this. Personally I think it's clear enough
with the space around both sides of "=>", but if there's agreement on
removing the space I don't oppose it.
Paolo
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 11/3/24 10:54, Junjie Mao wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
>>> index a4bc5d01ee8..c2ea22101e4 100644
>>> --- a/rust/qemu-api-macros/src/lib.rs
>>> +++ b/rust/qemu-api-macros/src/lib.rs
>>> @@ -3,8 +3,34 @@
>>> // SPDX-License-Identifier: GPL-2.0-or-later
>>>
>>> use proc_macro::TokenStream;
>>> -use quote::quote;
>>> -use syn::{parse_macro_input, DeriveInput};
>>> +use proc_macro2::Span;
>>> +use quote::{quote, quote_spanned};
>>> +use syn::{
>>> + parse_macro_input, parse_quote, punctuated::Punctuated, token::Comma, Data, DeriveInput, Field,
>>> + Fields, Ident, Type, Visibility,
>>> +};
>>> +
>>> +struct CompileError(String, Span);
>>> +
>>> +impl From<CompileError> for proc_macro2::TokenStream {
>>> + fn from(err: CompileError) -> Self {
>>> + let CompileError(msg, span) = err;
>>> + quote_spanned! { span => compile_error!(#msg); }
>> The documentation [2] says "there should be no space before the =>
>> token" and that is by intention to tell that `span` is "evaluated in the
>> context of proc macro" while those after the arm "in the generated
>> code". Should we follow that convention (even though the extra white
>> space does not impact building)?
>
> Ah, forgot to reply about this. Personally I think it's clear enough with the
> space around both sides of "=>", but if there's agreement on removing the space
> I don't oppose it.
>
> Paolo
I don't have any preference, either. I think we can keep it as is and
make sure future calls to quote_spanned! have consistent style. Thanks.
--
Best Regards
Junjie Mao
On 11/3/24 10:54, Junjie Mao wrote:
>
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> From: Junjie Mao <junjie.mao@hotmail.com>
>>
>> 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!.
>>
>> The actual implementation of offset_of! is done in a declarative macro,
>> but for simplicity and to avoid introducing an extra level of indentation,
>> the trigger is a procedural macro #[derive(offsets)].
>>
>> The procedural macro is perhaps a bit overengineered, but it helps
>> introducing some idioms that will be useful in the future as well.
>>
>> Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
>> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Generally looks good to me. Thanks for integrating this!
>
> It seems Rust does not have builtin support for unit tests expecting
> compilation failures. There is a crate (named trybuild [1]) for that
> purpose but it requires introducing a dozen of new dependencies (see
> below). Do you think it worth the effort? If so, I can take a closer
> look and cook something for initial review (probably post 9.2).
>
> trybuild v1.0.101
> ├── glob v0.3.1
> ├── serde v1.0.210
> ├── serde_derive v1.0.210 (proc-macro)
> │ ├── proc-macro2 v1.0.84 (*)
> │ ├── quote v1.0.36 (*)
> │ └── syn v2.0.66 (*)
> ├── serde_json v1.0.132
> │ ├── itoa v1.0.11
> │ ├── memchr v2.7.4
> │ ├── ryu v1.0.18
> │ └── serde v1.0.210
> ├── target-triple v0.1.3
> ├── termcolor v1.4.1
> └── toml v0.8.19
> ├── serde v1.0.210
> ├── serde_spanned v0.6.8
> │ └── serde v1.0.210
> ├── toml_datetime v0.6.8
> │ └── serde v1.0.210
> └── toml_edit v0.22.22
> ├── indexmap v2.6.0
> │ ├── equivalent v1.0.1
> │ └── hashbrown v0.15.0
> ├── serde v1.0.210
> ├── serde_spanned v0.6.8 (*)
> ├── toml_datetime v0.6.8 (*)
> └── winnow v0.6.20
>
> [1] https://docs.rs/trybuild/latest/trybuild/
>
> [snip]
>> diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
>> index a4bc5d01ee8..c2ea22101e4 100644
>> --- a/rust/qemu-api-macros/src/lib.rs
>> +++ b/rust/qemu-api-macros/src/lib.rs
>> @@ -3,8 +3,34 @@
>> // SPDX-License-Identifier: GPL-2.0-or-later
>>
>> use proc_macro::TokenStream;
>> -use quote::quote;
>> -use syn::{parse_macro_input, DeriveInput};
>> +use proc_macro2::Span;
>> +use quote::{quote, quote_spanned};
>> +use syn::{
>> + parse_macro_input, parse_quote, punctuated::Punctuated, token::Comma, Data, DeriveInput, Field,
>> + Fields, Ident, Type, Visibility,
>> +};
>> +
>> +struct CompileError(String, Span);
>> +
>> +impl From<CompileError> for proc_macro2::TokenStream {
>> + fn from(err: CompileError) -> Self {
>> + let CompileError(msg, span) = err;
>> + quote_spanned! { span => compile_error!(#msg); }
>
> The documentation [2] says "there should be no space before the =>
> token" and that is by intention to tell that `span` is "evaluated in the
> context of proc macro" while those after the arm "in the generated
> code". Should we follow that convention (even though the extra white
> space does not impact building)?
I think it'd be a bit too much to add 18 crates (of which 2 for both
native and non-native).
Assuming you can make it work at all (because it's a lot of crates,
because there might be dependencies on Cargo, and because the toml crate
changes MSRV very often), it would increase build time a lot. Plus, the
only time I worked with a crate that used trybuild, it tended to break a
lot due to changes in compiler errors.
We're only adding macros for our own convenience, it's not like we're a
general purpose crate, so we can live with the limitation. But thanks
for getting such a clear dep tree!
Paolo
On 11/3/24 10:54, Junjie Mao wrote:
>
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> From: Junjie Mao <junjie.mao@hotmail.com>
>>
>> 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!.
>>
>> The actual implementation of offset_of! is done in a declarative macro,
>> but for simplicity and to avoid introducing an extra level of indentation,
>> the trigger is a procedural macro #[derive(offsets)].
>>
>> The procedural macro is perhaps a bit overengineered, but it helps
>> introducing some idioms that will be useful in the future as well.
>>
>> Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
>> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Generally looks good to me. Thanks for integrating this!
>
> It seems Rust does not have builtin support for unit tests expecting
> compilation failures. There is a crate (named trybuild [1]) for that
> purpose but it requires introducing a dozen of new dependencies (see
> below). Do you think it worth the effort? If so, I can take a closer
> look and cook something for initial review (probably post 9.2).
>
> trybuild v1.0.101
> ├── glob v0.3.1
> ├── serde v1.0.210
> ├── serde_derive v1.0.210 (proc-macro)
> │ ├── proc-macro2 v1.0.84 (*)
> │ ├── quote v1.0.36 (*)
> │ └── syn v2.0.66 (*)
> ├── serde_json v1.0.132
> │ ├── itoa v1.0.11
> │ ├── memchr v2.7.4
> │ ├── ryu v1.0.18
> │ └── serde v1.0.210
> ├── target-triple v0.1.3
> ├── termcolor v1.4.1
> └── toml v0.8.19
> ├── serde v1.0.210
> ├── serde_spanned v0.6.8
> │ └── serde v1.0.210
> ├── toml_datetime v0.6.8
> │ └── serde v1.0.210
> └── toml_edit v0.22.22
> ├── indexmap v2.6.0
> │ ├── equivalent v1.0.1
> │ └── hashbrown v0.15.0
> ├── serde v1.0.210
> ├── serde_spanned v0.6.8 (*)
> ├── toml_datetime v0.6.8 (*)
> └── winnow v0.6.20
>
> [1] https://docs.rs/trybuild/latest/trybuild/
>
> [snip]
>> diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
>> index a4bc5d01ee8..c2ea22101e4 100644
>> --- a/rust/qemu-api-macros/src/lib.rs
>> +++ b/rust/qemu-api-macros/src/lib.rs
>> @@ -3,8 +3,34 @@
>> // SPDX-License-Identifier: GPL-2.0-or-later
>>
>> use proc_macro::TokenStream;
>> -use quote::quote;
>> -use syn::{parse_macro_input, DeriveInput};
>> +use proc_macro2::Span;
>> +use quote::{quote, quote_spanned};
>> +use syn::{
>> + parse_macro_input, parse_quote, punctuated::Punctuated, token::Comma, Data, DeriveInput, Field,
>> + Fields, Ident, Type, Visibility,
>> +};
>> +
>> +struct CompileError(String, Span);
>> +
>> +impl From<CompileError> for proc_macro2::TokenStream {
>> + fn from(err: CompileError) -> Self {
>> + let CompileError(msg, span) = err;
>> + quote_spanned! { span => compile_error!(#msg); }
>
> The documentation [2] says "there should be no space before the =>
> token" and that is by intention to tell that `span` is "evaluated in the
> context of proc macro" while those after the arm "in the generated
> code". Should we follow that convention (even though the extra white
> space does not impact building)?
Ah, forgot to reply about this. Personally I think it's clear enough
with the "=>", but if there's agreement on removing the space I don't
oppose it.
Paolo
© 2016 - 2024 Red Hat, Inc.