[PATCH 03/16] rust: pass rustc_args when building all crates

Paolo Bonzini posted 16 patches 1 week ago
There is a newer version of this series
[PATCH 03/16] rust: pass rustc_args when building all crates
Posted by Paolo Bonzini 1 week ago
rustc_args is needed to smooth the difference in warnings between the various
versions of rustc.  Always include those arguments.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build                       | 18 +++++++++++-------
 rust/qemu-api/meson.build         |  2 +-
 rust/qemu-api/src/device_class.rs | 10 ++++++----
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/meson.build b/meson.build
index 37f94ab32aa..2545185014e 100644
--- a/meson.build
+++ b/meson.build
@@ -3317,6 +3317,17 @@ endif
 
 genh += configure_file(output: 'config-host.h', configuration: config_host_data)
 
+if have_rust and have_system
+  rustc_args = run_command(
+    find_program('scripts/rust/rustc_args.py'),
+    '--config-headers', meson.project_build_root() / 'config-host.h',
+    capture : true,
+    check: true).stdout().strip().split()
+  rustc_args += ['-D', 'unsafe_op_in_unsafe_fn']
+  add_project_arguments(rustc_args, native: false, language: 'rust')
+  add_project_arguments(rustc_args, native: true, language: 'rust')
+endif
+
 hxtool = find_program('scripts/hxtool')
 shaderinclude = find_program('scripts/shaderinclude.py')
 qapi_gen = find_program('scripts/qapi-gen.py')
@@ -3909,12 +3920,6 @@ common_all = static_library('common',
                             dependencies: common_ss.all_dependencies())
 
 if have_rust and have_system
-  rustc_args = run_command(
-    find_program('scripts/rust/rustc_args.py'),
-    '--config-headers', meson.project_build_root() / 'config-host.h',
-    capture : true,
-    check: true).stdout().strip().split()
-  rustc_args += ['-D', 'unsafe_op_in_unsafe_fn']
   bindgen_args = [
     '--disable-header-comment',
     '--raw-line', '// @generated',
@@ -4087,7 +4092,6 @@ foreach target : target_dirs
                             rlib_rs,
                             dependencies: target_rust.dependencies(),
                             override_options: ['rust_std=2021', 'build.rust_std=2021'],
-                            rust_args: rustc_args,
                             rust_abi: 'c')
       arch_deps += declare_dependency(link_whole: [rlib])
     endif
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index c72d34b607d..42ea815fa5a 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -10,7 +10,7 @@ _qemu_api_rs = static_library(
   ),
   override_options: ['rust_std=2021', 'build.rust_std=2021'],
   rust_abi: 'rust',
-  rust_args: rustc_args + [
+  rust_args: [
     '--cfg', 'MESON',
     # '--cfg', 'feature="allocator"',
   ],
diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
index 1ea95beb78d..b6b68cf9ce2 100644
--- a/rust/qemu-api/src/device_class.rs
+++ b/rust/qemu-api/src/device_class.rs
@@ -16,10 +16,12 @@ macro_rules! device_class_init {
         ) {
             let mut dc =
                 ::core::ptr::NonNull::new(klass.cast::<$crate::bindings::DeviceClass>()).unwrap();
-            dc.as_mut().realize = $realize_fn;
-            dc.as_mut().vmsd = &$vmsd;
-            $crate::bindings::device_class_set_legacy_reset(dc.as_mut(), $legacy_reset_fn);
-            $crate::bindings::device_class_set_props(dc.as_mut(), $props.as_mut_ptr());
+            unsafe {
+                dc.as_mut().realize = $realize_fn;
+                dc.as_mut().vmsd = &$vmsd;
+                $crate::bindings::device_class_set_legacy_reset(dc.as_mut(), $legacy_reset_fn);
+                $crate::bindings::device_class_set_props(dc.as_mut(), $props.as_mut_ptr());
+            }
         }
     };
 }
-- 
2.46.2
Re: [PATCH 03/16] rust: pass rustc_args when building all crates
Posted by Zhao Liu 1 day, 15 hours ago
On Tue, Oct 15, 2024 at 03:17:21PM +0200, Paolo Bonzini wrote:
> Date: Tue, 15 Oct 2024 15:17:21 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 03/16] rust: pass rustc_args when building all crates
> X-Mailer: git-send-email 2.46.2
> 
> rustc_args is needed to smooth the difference in warnings between the various
> versions of rustc.  Always include those arguments.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  meson.build                       | 18 +++++++++++-------
>  rust/qemu-api/meson.build         |  2 +-
>  rust/qemu-api/src/device_class.rs | 10 ++++++----
>  3 files changed, 18 insertions(+), 12 deletions(-)

LGTM (with one trivial comment inline)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

> diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
> index 1ea95beb78d..b6b68cf9ce2 100644
> --- a/rust/qemu-api/src/device_class.rs
> +++ b/rust/qemu-api/src/device_class.rs
> @@ -16,10 +16,12 @@ macro_rules! device_class_init {
>          ) {
>              let mut dc =
>                  ::core::ptr::NonNull::new(klass.cast::<$crate::bindings::DeviceClass>()).unwrap();
> -            dc.as_mut().realize = $realize_fn;
> -            dc.as_mut().vmsd = &$vmsd;
> -            $crate::bindings::device_class_set_legacy_reset(dc.as_mut(), $legacy_reset_fn);
> -            $crate::bindings::device_class_set_props(dc.as_mut(), $props.as_mut_ptr());
> +            unsafe {
> +                dc.as_mut().realize = $realize_fn;
> +                dc.as_mut().vmsd = &$vmsd;
> +                $crate::bindings::device_class_set_legacy_reset(dc.as_mut(), $legacy_reset_fn);
> +                $crate::bindings::device_class_set_props(dc.as_mut(), $props.as_mut_ptr());
> +            }

The issue exists because the unsafe_op_in_unsafe_fn is allowed in
rust/qemu-api/src/lib.rs. So should we wrap the bindings in a separate
lib (similar to the rust/bindings in the Linux kernel)?

This way, the special lint settings can be applied only to the binding
files, while the default lint checks can cover the other user
development code.

In addition, another thing that confuses me is why bindgen still
generates code that does not follow the unsafe_op_in_unsafe_fn
requirement. It seems that bindgen has supported unsafe_op_in_unsafe_fn
since v0.62 [1, 2], but binding code we generated still violates
unsafe_op_in_unsafe_fn. Is this a bug of bindgen?

[1]: https://github.com/rust-lang/rust-bindgen/pull/2266 
[2]: https://github.com/rust-lang/rust-bindgen/blob/main/CHANGELOG.md#changed-12

Regards,
Zhao
Re: [PATCH 03/16] rust: pass rustc_args when building all crates
Posted by Paolo Bonzini 1 day, 8 hours ago
On Mon, Oct 21, 2024 at 8:16 AM Zhao Liu <zhao1.liu@intel.com> wrote:
> unsafe_op_in_unsafe_fn is allowed in
> rust/qemu-api/src/lib.rs. So should we wrap the bindings in a separate
> lib (similar to the rust/bindings in the Linux kernel)?
>
> This way, the special lint settings can be applied only to the binding
> files, while the default lint checks can cover the other user
> development code.
>
> In addition, another thing that confuses me is why bindgen still
> generates code that does not follow the unsafe_op_in_unsafe_fn
> requirement. It seems that bindgen has supported unsafe_op_in_unsafe_fn
> since v0.62 [1, 2], but binding code we generated still violates
> unsafe_op_in_unsafe_fn. Is this a bug of bindgen?

The plan is to support older versions of bindgen (0.60.x) as long as
Debian has them. One possibility to fix this is, as you said, to use a
completely separate crate. Another is to add #![allow()] to just the
bindings module, for example by changing bindgen.rs to

#![allow(...)]
include!("bindgen.rs.inc")

This is related to the fact that we don't have yet a good way to run
"clippy", because "cargo clippy" needs the bindgen.rs file. So we
should probably look at these issues at once.

Paolo
Re: [PATCH 03/16] rust: pass rustc_args when building all crates
Posted by Junjie Mao 19 hours ago
Paolo Bonzini <pbonzini@redhat.com> writes:

> On Mon, Oct 21, 2024 at 8:16 AM Zhao Liu <zhao1.liu@intel.com> wrote:
>> unsafe_op_in_unsafe_fn is allowed in
>> rust/qemu-api/src/lib.rs. So should we wrap the bindings in a separate
>> lib (similar to the rust/bindings in the Linux kernel)?
>>
>> This way, the special lint settings can be applied only to the binding
>> files, while the default lint checks can cover the other user
>> development code.
>>
>> In addition, another thing that confuses me is why bindgen still
>> generates code that does not follow the unsafe_op_in_unsafe_fn
>> requirement. It seems that bindgen has supported unsafe_op_in_unsafe_fn
>> since v0.62 [1, 2], but binding code we generated still violates
>> unsafe_op_in_unsafe_fn. Is this a bug of bindgen?
>
> The plan is to support older versions of bindgen (0.60.x) as long as
> Debian has them. One possibility to fix this is, as you said, to use a
> completely separate crate. Another is to add #![allow()] to just the
> bindings module, for example by changing bindgen.rs to
>
> #![allow(...)]
> include!("bindgen.rs.inc")
>
> This is related to the fact that we don't have yet a good way to run
> "clippy", because "cargo clippy" needs the bindgen.rs file. So we
> should probably look at these issues at once.
>
> Paolo

Since meson 0.6.0 clippy-driver can be used as a wrapper of rustc. So we
can run clippy by:

   mkdir build.clippy && cd build.clippy
   RUSTC=clippy-driver ../configure --enable-rust ...
   ninja librust_x86_64_softmmu.a

--
Best Regards
Junjie Mao
Re: [PATCH 03/16] rust: pass rustc_args when building all crates
Posted by Paolo Bonzini 18 hours ago
Il mar 22 ott 2024, 04:35 Junjie Mao <junjie.mao@hotmail.com> ha scritto:

>
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
> > On Mon, Oct 21, 2024 at 8:16 AM Zhao Liu <zhao1.liu@intel.com> wrote:
> >> unsafe_op_in_unsafe_fn is allowed in
> >> rust/qemu-api/src/lib.rs. So should we wrap the bindings in a separate
> >> lib (similar to the rust/bindings in the Linux kernel)?
> >>
> >> This way, the special lint settings can be applied only to the binding
> >> files, while the default lint checks can cover the other user
> >> development code.
> >>
> >> In addition, another thing that confuses me is why bindgen still
> >> generates code that does not follow the unsafe_op_in_unsafe_fn
> >> requirement. It seems that bindgen has supported unsafe_op_in_unsafe_fn
> >> since v0.62 [1, 2], but binding code we generated still violates
> >> unsafe_op_in_unsafe_fn. Is this a bug of bindgen?
> >
> > The plan is to support older versions of bindgen (0.60.x) as long as
> > Debian has them. One possibility to fix this is, as you said, to use a
> > completely separate crate. Another is to add #![allow()] to just the
> > bindings module, for example by changing bindgen.rs to
> >
> > #![allow(...)]
> > include!("bindgen.rs.inc")
> >
> > This is related to the fact that we don't have yet a good way to run
> > "clippy", because "cargo clippy" needs the bindgen.rs file. So we
> > should probably look at these issues at once.
> >
> > Paolo
>
> Since meson 0.6.0 clippy-driver can be used as a wrapper of rustc. So we
> can run clippy by:
>
>    mkdir build.clippy && cd build.clippy
>    RUSTC=clippy-driver ../configure --enable-rust ...
>    ninja librust_x86_64_softmmu.a
>

True but it's less handy to have a separately-configured tree instead of
just "make clippy". Also the same is true of rustfmt and rustdoc (which
ideally would be part of the build so that doctests are also run by make
check-unit). So the question of how to emulate these other cargo tools is
open.

Paolo


> --
> Best Regards
> Junjie Mao
>
>
Re: [PATCH 03/16] rust: pass rustc_args when building all crates
Posted by Zhao Liu 1 day, 7 hours ago
On Mon, Oct 21, 2024 at 03:38:06PM +0200, Paolo Bonzini wrote:
> Date: Mon, 21 Oct 2024 15:38:06 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 03/16] rust: pass rustc_args when building all crates
> 
> On Mon, Oct 21, 2024 at 8:16 AM Zhao Liu <zhao1.liu@intel.com> wrote:
> > unsafe_op_in_unsafe_fn is allowed in
> > rust/qemu-api/src/lib.rs. So should we wrap the bindings in a separate
> > lib (similar to the rust/bindings in the Linux kernel)?
> >
> > This way, the special lint settings can be applied only to the binding
> > files, while the default lint checks can cover the other user
> > development code.
> >
> > In addition, another thing that confuses me is why bindgen still
> > generates code that does not follow the unsafe_op_in_unsafe_fn
> > requirement. It seems that bindgen has supported unsafe_op_in_unsafe_fn
> > since v0.62 [1, 2], but binding code we generated still violates
> > unsafe_op_in_unsafe_fn. Is this a bug of bindgen?
> 
> The plan is to support older versions of bindgen (0.60.x) as long as
> Debian has them. One possibility to fix this is, as you said, to use a
> completely separate crate. Another is to add #![allow()] to just the
> bindings module, for example by changing bindgen.rs to
> 
> #![allow(...)]
> include!("bindgen.rs.inc")
> 
> This is related to the fact that we don't have yet a good way to run
> "clippy", because "cargo clippy" needs the bindgen.rs file. So we
> should probably look at these issues at once.

Thank you. I agree, it's a better way.

Regards,
Zhao