Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce custom derive for ByteValued/replace ByteValued with zerocopy #246

Open
Manishearth opened this issue Jul 18, 2023 · 6 comments
Open
Labels
good first issue Good for newcomers

Comments

@Manishearth
Copy link

ByteValued is an unsafe trait and implementing it is tricky with the padding requirement. But I think it can be autoderived.

Basically, a custom derive that does two things:

  • Emits something like const __ASSERT_SIZES_Foo: [(); mem::size_of::<Foo>()] = [(); mem::size_of::<Field1>() + mem::size_of::<Field2>() + ...] ] ] to assert that there is no padding
  • Emit some test function that ends up asserting bounds on all subfields.

This might significantly reduce the amount of unsafe needed by crate users.

@Ablu
Copy link
Contributor

Ablu commented Nov 23, 2023

rust-vmm/vhost#215 and rust-vmm/vhost#208 are an examples where this would have helped.

The helper should probably also check that the types of the fields are only integeral types that can be safely assigning from byte slices. Checking repr(C) would also help.

@Ablu
Copy link
Contributor

Ablu commented Dec 7, 2023

As @roypat suggested on #274, there is https://docs.rs/zerocopy/latest/zerocopy that may be useful here. However, it only allows (de)serializing from and to &[u8]. That does not work for our volatile memory scenarios. But maybe one could contribute a pointer based API towards that crate? 🤔

@roypat
Copy link
Collaborator

roypat commented Dec 22, 2023

I don't think we'd need a pointer based API for zerocopy, our ByteValued trait only operates on slices (its Bytes that uses pointers, but this trait is about containers that allow byte-level access to its contents, such as guest memory, not about PODs. So the Bytes trait would have its ByteValued bounds updated to whatever the zerocopy equivalent is)

@roypat
Copy link
Collaborator

roypat commented Dec 22, 2023

It seems that rust-vmm/acpi_tables has done the switch from ByteValued to zerocopy a while ago, and it seemed to have gone smoothly: https://github.com/rust-vmm/acpi_tables

I think doing this can be a good first step towards reducing our confusing jungle of traits

@roypat roypat changed the title Introduce custom derive for ByteValued Introduce custom derive for ByteValued/replace ByteValued with zerocopy Dec 22, 2023
@roypat roypat added the good first issue Good for newcomers label Dec 22, 2023
@Ablu
Copy link
Contributor

Ablu commented Dec 22, 2023

Ah. You are right. I misunderstood the API. I thought that it would require an additional copy from the slice to the object. But it just turns a reference to a slice into a reference to a struct instance. So we can just copy to a slice and then use zerocopy.

@DemiMarie
Copy link

I have a crate that solves this without requiring any procedural macros.

roypat added a commit to roypat/vm-memory that referenced this issue Aug 12, 2024
This means we don't have to reason about when it's safe to implement
`ByteValued`, as `zerocopy` gives us derives for `FromBytes` and
`AsBytes` that are safe to use.

Closes rust-vmm#246

Signed-off-by: Patrick Roy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants