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

Optionally implement Zeroable and Pod from bytemuck #12

Merged
merged 3 commits into from
Oct 10, 2024

Conversation

tecc
Copy link
Contributor

@tecc tecc commented Sep 5, 2024

This PR adds implementations for Zeroable and Pod to some types in rend where they are applicable.
These traits do not get implemented unless the bytemuck feature is enabled.

Specifically, at the time of writing:

  • Two trait impl macros have been added to traits.rs: unsafe_impl_zeroable and unsafe_impl_pod.
  • Zeroable and Pod impls have been added for the unsigned integers, signed integers, and floats.
  • Zeroable has been implemented for char.

I refrained from implementing these values for atomics and other variations of the Zeroable/Pod types. Those require more features to be enabled on the bytemuck crate.

@djkoloski
Copy link
Collaborator

Is this for anything in particular? I'd prefer to keep rend as slim as possible, and there are some alternatives which make the choice of support unclear.

@tecc
Copy link
Contributor Author

tecc commented Sep 8, 2024

I'm not exactly sure what you mean by "for anything in particular".

I've simply added a feature that exists in version 0.4.1 and 0.4.2 (via #10) that does not yet exist in 0.5.
I did neglect to write that it is feature gated behind the bytemuck feature, which I probably should've explicitly stated.

Note: You did express the intent of support for bytemuck in version 0.5 as well:
#10 (comment)

@tecc tecc changed the title Implement Zeroable and Pod from bytemuck Optionally implement Zeroable and Pod from bytemuck Sep 8, 2024
@djkoloski
Copy link
Collaborator

I'm not exactly sure what you mean by "for anything in particular".

rend is explicitly by and for rkyv, and so caters pretty directly to rkyv's needs. That's why rend supports bytecheck, for example. So if more people/projects are interested in using rend, I'd like to know what use cases need bytemuck support (as an example) before adding it. Adding support is committing to the integration after all, and I'd like to have some idea of what the future use is before adding it.

Note: You did express the intent of support for bytemuck in version 0.5 as well

I did, and my stance has shifted since then having maintained a bunch of crate integrations for rkyv. At the time, I also saw potential in using bytemuck in rkyv, but that didn't pan out and I'm happy in my little rkyv ecosystem I've built up.

@tecc
Copy link
Contributor Author

tecc commented Sep 9, 2024

That's fair enough I suppose. I will preface this by saying that I don't use rkyv.

My use case is a short experiment I'm doing in filesystems and partitioning.
I require rend to support bytemuck's traits because most of the data is represented as it is stored and I prefer having the endianness in the type (so I don't screw up the endianness later on).

As a short example, consider the following data structure, the GPT Partition Table Header:

use bytemuck::{Zeroable, Pod};
use rend::{u32_le, u64_le};

#[derive(Zeroable, Pod)]
#[repr(C)]
pub struct PartitionTableHeader {
    pub signature: [u8; 8],
    pub revision: u32_le,
    pub header_size: u32_le,
    pub crc32_checksum: u32_le,
    pub _reserved: [u8; 4],
    pub lba_of_this_header: u64_le,
    pub lba_of_alternate_header: u64_le,
    pub first_usable_block: u64_le,
    pub last_usable_block: u64_le,
    pub disk_guid: [u8; 16],
    pub lba_of_entry_array: u64_le,
    pub partition_entry_count: u32_le,
    pub size_of_partition_entry: u32_le,
    pub partition_entry_array_crc32: u32_le,
    // Required so that there is no padding, otherwise `Pod`'s constraints are unfulfilled.
    // This could alternatively be solved using rend's unaligned types and `#[repr(C, packed)]`.
    pub _reserved2: [u8; 36],
}

In this case, because the in-memory representation is exactly what should be read from and written to disk (including endianness), I can just cast the struct using bytemuck and move on to do more important things.
If the endian-aware integer types (here u32_le and u64_le) do not implement the Zeroable and Pod traits, it immediately becomes difficult (alternatives 1, 2, 3, and 4 would no longer need rend, for the record):

  1. Using byte arrays ([u8; 4] and [u8; 8]) makes it unclear what the values of the fields are supposed to be, and I would probably have to create helper functions for every field so that all code can set fields and get the value of fields correctly.
  2. Using native integers (u32, u64) is not very preferable either. Whilst it solves the problem of unclear representation, it also becomes exponentially easier to screw up the endianness (since all of the fields are public, everyone can modify them - and I'd rather keep the fields public).
  3. Using native integers but creating a copy of the data with the correct endianness when writing would remove the point of the representation being #[repr(C)], which although not necessarily a bad thing feels very sluggish as compared to "declare the field, and it will be written". In fact, it'd probably remove the point of using rend altogether.
  4. Using a different endian-aware integer library (such as simple-endian) and trying to get a similar feature included there isn't an altogether bad idea, although it'd mean I'd have to change every single usage to use that of simple_endian. Even then, I prefer rend's design to simple-endian's.
  5. Finally, just doing unsafe impl Zeroable for PartitionTableHeader and unsafe impl Pod for PartitionTableHeader is an alternative, but a comparatively ugly one. You lose the safety that the derive macros provide for you. Granted, you can add the constraint checks yourself, but having to do that for every type that uses bytemuck's traits is unreasonable.

The 5 alternatives I listed are mostly hypotheticals (apart from 1, which I actually did try before scrapping it because as one might expect, it was not a pleasant experience).
I could also just use rend 0.4.1 or 0.4.2 but that's about on par with using simple-endian.

@joshlf
Copy link
Contributor

joshlf commented Sep 24, 2024

Hey @tecc, I came across this because I'm in the process of adding zerocopy support to rend. It sounds like zerocopy's byteorder module might already do what you want? They already implement the zerocopy traits, which provide the behavior you're looking for.

tecc and others added 3 commits October 10, 2024 16:24
`Zeroable` and `Pod` has been implemented for all unsigned integers, signed integers and floats.
Additionally, `Zeroable` has been implemented for `char` types.
@djkoloski djkoloski merged commit bf38c53 into rkyv:main Oct 10, 2024
20 checks passed
@djkoloski
Copy link
Collaborator

Sorry for the delay. After taking some more time to think about it and receiving a few more requests for this feature, I'm okay with adding support for bytemuck. Thanks for the PR, I appreciate your hard work!

@tecc
Copy link
Contributor Author

tecc commented Oct 12, 2024

(Apologies for the late reply, I've been busy so I forgot)

Hey @tecc, I came across this because I'm in the process of adding zerocopy support to rend. It sounds like zerocopy's byteorder module might already do what you want? They already implement the zerocopy traits, which provide the behavior you're looking for.

That's not a particularly bad idea either, though it'd mean I'd have to strip out my use of bytemuck and replace it with zerocopy.
Zerocopy also does not have an equivalent of bytemuck::zeroed (the const fn) which slightly irks me. Not that that's the worst thing in the world.

Sorry for the delay. After taking some more time to think about it and receiving a few more requests for this feature, I'm okay with adding support for bytemuck. Thanks for the PR, I appreciate your hard work!

Thank you for merging it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants