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

Remove NoCell bounds emitted by derive(TryFromBytes) on unions #1016

Closed
wants to merge 2 commits into from

Conversation

jswrenn
Copy link
Collaborator

@jswrenn jswrenn commented Mar 4, 2024

This PR applies the approach used to implement is_bit_valid for UnsafeCell to unions. However, it's not entirely satisfactory in this case. Yes, it enables us to remove the NoCell field bounds from derive(TryFromBytes) on unions. Unfortunately, it also makes is_bit_valid uncallable by TryFromBytes::try_from_ref, even when Self: NoCell.

Maybe we can apply an approach similar to the one used to solve #1014 to handle this.

Makes progress towards #5.

Closes #1015.

@jswrenn jswrenn marked this pull request as draft March 4, 2024 22:37
@jswrenn jswrenn changed the base branch from relax-cast_unsized to relax-try_cast_into March 4, 2024 23:14
@jswrenn
Copy link
Collaborator Author

jswrenn commented Mar 5, 2024

This isn't a problem for UnsafeCell, because UnsafeCell is never NoCell. Here, we have some unions that are NoCell, and some unions that aren't.

This isn't a problem for structs, because struct projections always respect that the projection has UnsafeCells at exactly the same ranges the struct they're projected from. We don't need a blanket NoCell bound to ensure the projection is valid.

In the case of unions, projections do not have UnsafeCells at exactly the same ranges as the union they're projected from. If we're projecting through a shared reference, both the union and its field need to be NoCell. If we're projecting through an exclusive reference, they don't need to be NoCell.

@jswrenn
Copy link
Collaborator Author

jswrenn commented Mar 5, 2024

Imagine we had two validity checking methods instead of one: is_bit_valid_shared and is_bit_valid_exclusive, where is_bit_valid_sahred bounds Self: NoCell. TryFromBytes::try_from_ref would call is_bit_valid_shared, and its other methods would call is_bit_valid_exclusive.

The derive implementation of TryFromBytes on unions would look something like this:

fn derive_try_from_bytes_union(ast: &DeriveInput, unn: &DataUnion) -> proc_macro2::TokenStream {
    let extras = Some({
        let fields = unn.fields();
        let field_names = fields.iter().map(|(name, _ty)| name);
        let field_tys = fields.iter().map(|(_name, ty)| ty);
        quote!(
            fn is_bit_valid_shared(
                mut candidate: ::zerocopy::Maybe<Self, ::zerocopy::invariant::Shared>
            ) -> bool
            where
                Self: NoCell,
            {
                false #(|| {
                    // SAFETY: `project` is a field projection of `candidate`,
                    // and `Self` is a union type. The candidate and projection
                    // agree on where their `UnsafeCell`s are, because `Self:
                    // NoCell`, and, by implication, the projected field is
                    // `NoCell`.
                    let field_candidate = unsafe {
                        let project = |slf: *mut Self|
                            ::zerocopy::macro_util::core_reexport::ptr::addr_of_mut!((*slf).#field_names);

                        candidate.reborrow().project(project)
                    };

                    <#field_tys as ::zerocopy::TryFromBytes>::is_bit_valid(field_candidate)
                })*
            }

            fn is_bit_valid_exclusive(
                mut candidate: ::zerocopy::Maybe<Self, ::zerocopy::invariant::Exclusive>
            ) -> bool {
                false #(|| {
                    // SAFETY: `project` is a field projection of `candidate`,
                    // and `Self` is a union type. The candidate and projection
                    // do not need to agree on exactly on where their
                    // `UnsafeCell` ranges are, because `candidate` is
                    // exclusively aliased.
                    let field_candidate = unsafe {
                        let project = |slf: *mut Self|
                            ::zerocopy::macro_util::core_reexport::ptr::addr_of_mut!((*slf).#field_names);

                        candidate.reborrow().project(project)
                    };

                    <#field_tys as ::zerocopy::TryFromBytes>::is_bit_valid(field_candidate)
                })*
            }
        )
    });
    impl_block(ast, unn, Trait::TryFromBytes, FieldBounds::ALL_SELF, SelfBounds::None, None, extras)
}

The bodies of these methods are identical except for their differing SAFETY comments.

@jswrenn jswrenn force-pushed the relax-try_cast_into branch 3 times, most recently from 46674af to 1421250 Compare March 6, 2024 17:25
jswrenn and others added 2 commits March 6, 2024 17:42
Makes progress towards #5.

Closes #1014.

Co-authored-by: Joshua Liebow-Feeser <[email protected]>
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.

None yet

1 participant