On Wed, Jul 24, 2024 at 11:58 AM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>In my opinion we should start with cargo workspaces as the
>known-imperfect (but good enough) solution, so that it could be evolved
>later. It is important that any change that deviates from common Rust
>conventions is documented, and v4 provided a nice basis to build upon,
>with documentation coming as things settle. This is why I explicitly
>didn't include Kconfig in the TODO list in my review of v4.
After working with the latest meson releases, it seems we will soon have
a good enough way of handling all this with meson. It makes me sceptical
of adding cargo wrappers and using a build system out of meson when we
might be able to drop all that soonish. We might as well bite the bullet
now to avoid working on something we know we will remove.
Ehh, as you say below it's complicated. Sometimes worse is better.
Personally I wouldn't have minded keeping the v4 approach as a "known
evil"; but if you can make the Cargo subprojects work, that would be
fine for me. What I don't like is the vendoring and handwritten (I
think?) meson.build, I think that's worse than the v4.
>Also, of the code changes (as opposed to the build system changes) that
>I had asked for in the review of v4, almost none of them have been
>applied. I'm copying them here for future reference:
Thanks, this helps a lot.
>❌ TODO comments when the code is doing potential undefined behavior
Do you mean like Rust's safety comments?
No I meant comments where we have known instances of undefined
behavior. The two I had in my emails are
(in pl011_init):
// TODO: this assumes that "all zeroes" is a valid state for all fields of
// PL011State. This is not necessarily true of any #[repr(Rust)] structs,
// including bilge-generated types. It should instead use MaybeUninit.
(before the call to qemu_chr_fe_accept_input):
// TODO: this causes a callback that creates another "&mut self".
// This is forbidden by Rust aliasing rules and has to be fixed
// using interior mutability.
https://std-dev-guide.rust-lang.org/policy/safety-comments.html
These can be required by lints which is really helpful. At this point
the UART library has safety comments (which needs to be reviewed for
validity). I plan on adding some at the macros in qemu-api as well.
>
>❌ a trait to store the CStr corresponding to the structs
I don't know yet if that is helpful in our usecase, because the strings
must be visible from C, thus be (rust, not c) statics, unmangled and
marked as #[used] for the linker. It makes sense from the Rust POV but
must also be FFI-accessible.
Why do they have to be #[used]? You have
+ #[used]
+ static TYPE_NAME: &::core::ffi::CStr = $tname;
+ $tname.as_ptr()
but since the cstr crate (and c"" literal) promise to return a
&'static CStr, I thought it could be just
$tname.as_ptr()
About traits, I meant something like
pub unsafe trait ObjectType: Sized {
const TYPE_NAME: &'static CStr;
}
So that you can put the trait declaration in the pl011 crate and the
type_info! macro can do
<$t as ObjectType>::TYPE_NAME.as_ptr()
(also for the parent).
>❌ a trait to generate all-zero structs without having to type "unsafe {
>MaybeUninit::zeroed().assume_init() }"
Traits cannot have const fns at the moment (traits cannot have
type-level effects like const or async but it's WIP to get them into
rustc), so this cannot be used for statics and consts.
Ah, I see. Anyhow, I've been looking at the Linux kernel's pinned-init
crate (https://rust-for-linux.com/pinned-init) and it provides a
Zeroable macro including #[derive] support. So that has gone lower in
my priority.
>❌ I'd like to use ctor instead of non-portable linker magic,
The linker sections are pretty much standard and in fact ctor uses the
same linker attributes. Would writing our own constructor macro be a
solution for you? My reasoning is that 1) we support our own specific
platforms and it's better for portability to reflect that in our source
tree and 2) it avoids the external dependency, linker sections do not
change so any ctor update would be in the API or adding more platform
support, not fixes in what we target.
I'd still like to give someone else the burden. :) Writing our own
constructor macro would be more work for little gain.