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

Add DstLayout::extend #633

Merged
merged 1 commit into from
Nov 24, 2023
Merged

Add DstLayout::extend #633

merged 1 commit into from
Nov 24, 2023

Conversation

jswrenn
Copy link
Collaborator

@jswrenn jswrenn commented Nov 21, 2023

DstLayout::extend is comparable to alloc::Layout, except that it additionally handles DSTs. This is the first of two steps needed for DstLayout to support an API in the form of:

// For illustrative purposes; `align` and `packed`
// can't actually be combined like this.
#[repr(C, align(2), packed(4))]
struct ReprCStruct<T, U, V> {
    foo: Foo<T>,
    bar: Bar<U>,
    baz: Baz<V>,
}

impl<T, U, V> KnownLayout for ReprCStruct<T, U, V> {
    const LAYOUT: DstLayout = {
        use ::zerocopy::macro_utils::core_reexport::num::NonZeroUsize;
        use ::zerocopy::DstLayout;

        let min_align = NonZeroUsize::new(2); // or `None`, w/o `align`
        let max_align = NonZeroUsize::new(4); // or `None`, w/o `packed`

        DstLayout::for_type::<()>()
            .extend(<Foo<T> as KnownLayout>::LAYOUT, max_align)
            .extend(<Bar<T> as KnownLayout>::LAYOUT, max_align)
            .extend(<Baz<T> as KnownLayout>::LAYOUT, max_align)
            .pad_to_align(min_align, max_align)
    };
}

(The next step is implementing DstLayout::pad_to_align).

Despite the similarities between DstLayout::extend and Layout::extend, the former cannot be implemented in terms of the latter because Layout::extend is not const. This introduces a risk that our computations here diverge from those in Layout::extend, but this PR also includes Kani proofs that DstLayout::extend behaves comparably to Layout::extend.

Makes progress towards #29.

@jswrenn jswrenn force-pushed the layout-extend branch 3 times, most recently from 56e41cc to 439b94b Compare November 22, 2023 17:19
@jswrenn jswrenn changed the title [wip] Add DstLayout::extend Add DstLayout::extend Nov 22, 2023
@jswrenn jswrenn force-pushed the layout-extend branch 3 times, most recently from 0b10804 to 96f3ff3 Compare November 22, 2023 17:32
@jswrenn jswrenn requested a review from joshlf November 22, 2023 17:32
@jswrenn jswrenn marked this pull request as ready for review November 22, 2023 17:32
@jswrenn jswrenn force-pushed the layout-extend branch 4 times, most recently from 301a488 to 17af0f8 Compare November 22, 2023 17:52
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated
Comment on lines 478 to 479
debug_assert!(max_align.is_power_of_two());
debug_assert!(self._align.get() <= max_align.get());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also assert that field._align.get() <= Self::MAX_ALIGN?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't mean to assert debug_assert!(self._align.get() <= max_align.get());, and revised the PR accordingly. I don't think we need to peek into self and field and make sure they're valid — that's the job of DstLayout's constructors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't currently document anywhere that DstLayout's align field is guaranteed to have a particular maximum value - that's just a consequence of Rust's type rules. IMO we should either explicitly document that as an invariant and add comments at construction sites proving that it's upheld, or we should assert it here out of an abundance of caution. As this code is currently written, it could break in the future if Rust raises the maximum alignment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made three changes:

  1. I've verified, with Kani, that this method is robust to future increases of Rust's maximum-allowed alignment.
  2. I've added debug assertions that fire if we see an alignment exceeding Rust's current maximum-allowed alignment.
  3. I've replaced the + operations with explicit checked adds, and panic messages explaining what went wrong.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@jswrenn jswrenn force-pushed the layout-extend branch 4 times, most recently from 354f4df to 37d1bff Compare November 22, 2023 19:37
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@jswrenn jswrenn force-pushed the layout-extend branch 2 times, most recently from 23c9256 to 7b3af4a Compare November 23, 2023 15:15
@jswrenn jswrenn requested a review from joshlf November 24, 2023 13:58
src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
This method is comparable to `Layout::extend`, but also handles
dynamically sized types.

Makes progress towards #29.
@jswrenn jswrenn added this pull request to the merge queue Nov 24, 2023
Merged via the queue into main with commit 3fdad7d Nov 24, 2023
126 checks passed
@jswrenn jswrenn deleted the layout-extend branch November 24, 2023 19:26
@joshlf joshlf mentioned this pull request Dec 5, 2023
34 tasks
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.

2 participants