On Mon, Jul 08, 2024 at 06:26:22PM +0200, Paolo Bonzini wrote:
> On 7/4/24 14:15, Manos Pitsidianakis wrote:
> > Changes from v3->v4:
> > - Add rust-specific files to .gitattributes
> > - Added help text to scripts/cargo_wrapper.py arguments (thanks Stephan)
> > - Split bindings separate crate
> > - Add declarative macros for symbols exported to QEMU to said crate
> > - Lowered MSRV to 1.77.2
> > - Removed auto-download and install of bindgen-cli
> > - Fixed re-compilation of Rust objects in case they are missing from
> > filesystem
> > - Fixed optimized builds by adding #[used] (thanks Pierrick for the help
> > debugging this)
>
> I think the largest issue is that I'd rather have a single cargo build using
> a virtual manifest, because my hunch is that it'd be the easiest path
> towards Kconfig integration. But it's better to do this after merge, as the
> changes are pretty large. It's also independent from any other changes
> targeted at removing unsafe code, so no need to hold back on merging.
>
> Other comments I made that should however be addressed before merging, from
> most to least important:
>
> - TODO comments when the code is doing potential undefined behavior
>
> - module structure should IMO resemble the C part of the tree
>
> - only generate bindings.rs.inc once
>
> - a couple abstractions that I'd like to have now: a trait to store the CStr
> corresponding to the structs, and one to generate all-zero structs without
> having to type "unsafe { MaybeUninit::zeroed().assume_init() }"
>
> - I pointed out a couple lints that are too broad and should be enabled
> per-file, even if right now it's basically all files that include them.
>
> - add support for --cargo and CARGO environment variables (if my patch works
> without too much hassle)
>
> - I'd like to use ctor instead of non-portable linker magic, and the cstr
> crate instead of CStr statics or c""
>
> - please check if -Wl,--whole-archive can be replaced with link_whole:
>
> - probably, until Rust is enabled by default we should treat dependencies as
> a moving target and not commit Cargo.lock files. In the meanwhile we can
> discuss how to handle them.
>
> And a few aesthetic changes on top of this.
This series is still missing changes to enable build on all targets
during CI, including cross-compiles, to prove that we're doing the
correct thing on all our targetted platforms. That's a must have
before considering it suitable for merge.
I also believe we should default to enabling rust toolchain by
default in configure, and require and explicit --without-rust
to disable it, *despite* it not technically being a mandatory
feature....yet.
This is to give users a clear message that Rust is likely to
become a fundamental part of QEMU, so they need to give feedback
if they hit any problems / have use cases we've not anticipated
that are problematic wrt Rust.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
