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 implementation of padding_needed_for #830

Merged
merged 1 commit into from
Feb 3, 2024
Merged

Conversation

ezrosent
Copy link
Contributor

Description says it all. The implementation has detailed comments. Happy to edit commit messages as needed.

@jswrenn
Copy link
Collaborator

jswrenn commented Jan 31, 2024

Re-run the UI tests with:

TRYBUILD=overwrite ./cargo.sh +all test -p zerocopy --features derive
TRYBUILD=overwrite ./cargo.sh +all test -p zerocopy-derive

jswrenn
jswrenn previously requested changes Feb 1, 2024
Copy link
Collaborator

@jswrenn jswrenn left a comment

Choose a reason for hiding this comment

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

Happy to say that the new implementation passes kani, with some slight modifications to the kani harness:

    #[kani::proof]
    fn prove_padding_needed_for() {
        fn model_impl(len: usize, align: NonZeroUsize) -> usize {
            let padded = len.next_multiple_of(align.get());
            let padding = padded - len;
            padding
        }

        // Generate a valid alignment.
        let align: NonZeroUsize = {
            let align: NonZeroUsize = kani::any();
            kani::assume(align.get().is_power_of_two());
            kani::assume(align.get() < 1 << 29);
            align
        };

        // Generate a valid length.
        let len: usize = {
            let len: usize = kani::any();
            kani::assume(len <= isize::MAX as usize);
            len
        };

        // Assert that the optimized and model implementations behave
        // identically.
        let expected = model_impl(len, align);
        let actual = padding_needed_for(len, align);
        assert_eq!(expected, actual, "padding_needed_for({len}, {align})");

        // Basic correctness checks.
        let padded_len = actual + len;
        assert_eq!(padded_len % align, 0);
        assert!(padded_len / align >= len / align);
    }

You can verify this by running:

cargo kani --harness prove_padding_needed_for --output-format terse

The one big thing that needs to be done before merging is to shift over the callsites to padding_needed_for. You ought to be able to do this by deleting the vendored implementation, and replacing all instances of util::core_layout::padding_needed_for with util::padding_needed_for (that should get you 90% of the way there).

@ezrosent
Copy link
Contributor Author

ezrosent commented Feb 3, 2024

Looks like everything passes now; thanks for adding the kani test!

auto-merge was automatically disabled February 3, 2024 00:18

Head branch was pushed to by a user without write access

@joshlf joshlf enabled auto-merge February 3, 2024 00:25
@joshlf joshlf dismissed jswrenn’s stale review February 3, 2024 00:25

Discussed offline

@joshlf joshlf added this pull request to the merge queue Feb 3, 2024
Merged via the queue into google:main with commit 856652a Feb 3, 2024
127 checks passed
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

3 participants