Skip to content

Make zerocopy-derive an optional dependency#176

Merged
joshlf merged 1 commit intomainfrom
optional-zerocopy-derive
Aug 16, 2023
Merged

Make zerocopy-derive an optional dependency#176
joshlf merged 1 commit intomainfrom
optional-zerocopy-derive

Conversation

@joshlf
Copy link
Member

@joshlf joshlf commented May 23, 2023

This requires a few changes to accomodate:

  • We still want to be able to derive zerocopy traits on our own types so
    that we can rely on it to validate the soundness of those impls for
    us. That means that, when the derive feature is enabled, we still
    use custom derives, but when it's disabled, we implement traits
    manually. However, we also need to ensure that our manual trait impls
    have the correct bounds required in order for those impls to be sound.
    In order to ensure this, we introduce an impl_or_verify! macro
    which, when building with derive enabled, verifies that the manual
    impl matches the one emitted by our custom derive.
  • Since some doc examples use custom derives, we can no longer run doc
    tests unconditionally in CI - when the derive feature is disabled,
    doc tests fail. Instead, we disable doc tests by default (in order to
    run them, cargo test must be run with the --doc flag). We
    introduce a separate "Run doc tests" step in CI - this step only runs
    when the derive feature is enabled.

Closes #169

@joshlf
Copy link
Member Author

joshlf commented May 23, 2023

Some issues to address before it's ready for review/merge, but the numbers look great! In my cloud VM, compiling without zerocopy-derive is ~20x faster:

$ rm -r target/* && time cargo check
   Compiling proc-macro2 v1.0.56
   Compiling unicode-ident v1.0.4
   Compiling quote v1.0.26
    Checking byteorder v1.4.3
   Compiling syn v2.0.15
   Compiling zerocopy-derive v0.7.0-alpha.4 (.../zerocopy/zerocopy-derive)
    Checking zerocopy v0.7.0-alpha.4 (.../zerocopy)
    Finished dev [unoptimized + debuginfo] target(s) in 5.05s

real    0m5.112s
user    0m8.476s
sys     0m1.562s

$ rm -r target/* && time cargo check --no-default-features
    Checking zerocopy v0.7.0-alpha.4 (.../zerocopy)
    Finished dev [unoptimized + debuginfo] target(s) in 0.23s

real    0m0.289s
user    0m0.246s
sys     0m0.045s

cc @djkoloski

@joshlf joshlf force-pushed the optional-zerocopy-derive branch 12 times, most recently from 401d345 to f02025b Compare May 23, 2023 21:17
@joshlf
Copy link
Member Author

joshlf commented May 23, 2023

@djkoloski You might have ideas here. While all of the types with trait impls use derive when cfg(any(feature = "derive", test)), all that tells us is that it's valid to derive the relevant trait. However, it doesn't tell us what bounds are used in the impl that the derive emits. As a result, it's still possible that our manual impls could have looser bounds on type parameters than the impl emitted by the derive. That means that, as written, we can't really rely on the "this impl is sound because the derive succeeds" argument. Any thoughts on how we can do this without just resorting to safety arguments from scratch?

EDIT: Here's one possible idea. We could introduce a macro which spits out code in all cases. When the derive is not used, it emits the impls. When the derive is used, it emits validation code like:

trait IsFromBytes: FromBytes {}
impl<T: SomeBound> IsFromBytes for Foo<T> {}

The idea here is that if the type parameter bounds (in this case, T: SomeBounds) are as restrictive as the ones emitted by the impl, this code will compile. If not, then IsFromBytes: FromBytes won't be satisfied, and the code will not compile.

Here's a prototype that demonstrates the idea working.

@joshlf joshlf force-pushed the optional-zerocopy-derive branch 2 times, most recently from 87a909e to dd7876a Compare May 23, 2023 23:01
@djkoloski
Copy link
Member

Am I understanding correctly that you want to use zerocopy-derive to write your impls when the derive feature is disabled, but want to continue using it when the feature is enabled? It seems easier and more straightforward (to me) to only keep manual impls. Maybe there's a way to:

  1. Move all of the derived impls into a set of files
  2. Mark zerocopy-derive as a dev-dependency
  3. cargo expand (or manually invoke the same derive methods) on that set of files into zerocopy source that gets checked in

That could very easily be overengineering it though.

@joshlf
Copy link
Member Author

joshlf commented May 24, 2023

Yeah specifically I want to make sure that we're able to rely on the derive to do the soundness analysis for us. I'd like to keep the amount of manually verified unsafe code as small as possible.

@joshlf joshlf force-pushed the optional-zerocopy-derive branch from dd7876a to 9ba47b3 Compare May 24, 2023 03:05
@joshlf
Copy link
Member Author

joshlf commented May 24, 2023

Okay, the latest version contains a macro - impl_or_verify! - which does what I was proposing above. There's a small hole - namely, you could write code like this:

#[cfg(not(any(feature = "derive", test)))]
impl_or_verify!(T => FromZeroes for Unalign<T>);

...which would only compile the impls, but would not compile when the derive was used, and thus wouldn't perform its verification function. I'll try to figure out how to plug this hole, but a) I suspect it may be hard to do ergonomically and, b) I consider it a pretty minor risk.

@joshlf joshlf force-pushed the optional-zerocopy-derive branch 8 times, most recently from 11a1e77 to add56b1 Compare May 25, 2023 23:55
@joshlf joshlf added good first issue Good for newcomers and removed good first issue Good for newcomers labels May 28, 2023
@joshlf joshlf force-pushed the optional-zerocopy-derive branch from add56b1 to fca8e50 Compare May 28, 2023 06:23
@joshlf joshlf force-pushed the optional-zerocopy-derive branch 6 times, most recently from 097d542 to cddee17 Compare August 10, 2023 19:40
@joshlf joshlf changed the title [WIP] Make zerocopy-derive an optional dependency Make zerocopy-derive an optional dependency Aug 10, 2023
This requires a few changes to accomodate:
- We still want to be able to derive zerocopy traits on our own types so
  that we can rely on it to validate the soundness of those impls for
  us. That means that, when the `derive` feature is enabled, we still
  use custom derives, but when it's disabled, we implement traits
  manually. However, we also need to ensure that our manual trait impls
  have the correct bounds required in order for those impls to be sound.
  In order to ensure this, we introduce an `impl_or_verify!` macro
  which, when building with `derive` enabled, verifies that the manual
  impl matches the one emitted by our custom derive.
- Since some doc examples use custom derives, we can no longer run doc
  tests unconditionally in CI - when the `derive` feature is disabled,
  doc tests fail. Instead, we disable doc tests by default (in order to
  run them, `cargo test` must be run with the `--doc` flag). We
  introduce a separate "Run doc tests" step in CI - this step only runs
  when the `derive` feature is enabled.

Closes #169
@joshlf joshlf merged commit 11a92e4 into main Aug 16, 2023
@joshlf joshlf deleted the optional-zerocopy-derive branch August 16, 2023 20:04
joshlf added a commit that referenced this pull request Aug 23, 2023
This requires a few changes to accomodate:
- We still want to be able to derive zerocopy traits on our own types so
  that we can rely on it to validate the soundness of those impls for
  us. That means that, when the `derive` feature is enabled, we still
  use custom derives, but when it's disabled, we implement traits
  manually. However, we also need to ensure that our manual trait impls
  have the correct bounds required in order for those impls to be sound.
  In order to ensure this, we introduce an `impl_or_verify!` macro
  which, when building with `derive` enabled, verifies that the manual
  impl matches the one emitted by our custom derive.
- Since some doc examples use custom derives, we can no longer run doc
  tests unconditionally in CI - when the `derive` feature is disabled,
  doc tests fail. Instead, we disable doc tests by default (in order to
  run them, `cargo test` must be run with the `--doc` flag). We
  introduce a separate "Run doc tests" step in CI - this step only runs
  when the `derive` feature is enabled.

Closes #169
joshlf added a commit that referenced this pull request Aug 23, 2023
This requires a few changes to accomodate:
- We still want to be able to derive zerocopy traits on our own types so
  that we can rely on it to validate the soundness of those impls for
  us. That means that, when the `derive` feature is enabled, we still
  use custom derives, but when it's disabled, we implement traits
  manually. However, we also need to ensure that our manual trait impls
  have the correct bounds required in order for those impls to be sound.
  In order to ensure this, we introduce an `impl_or_verify!` macro
  which, when building with `derive` enabled, verifies that the manual
  impl matches the one emitted by our custom derive.
- Since some doc examples use custom derives, we can no longer run doc
  tests unconditionally in CI - when the `derive` feature is disabled,
  doc tests fail. Instead, we disable doc tests by default (in order to
  run them, `cargo test` must be run with the `--doc` flag). We
  introduce a separate "Run doc tests" step in CI - this step only runs
  when the `derive` feature is enabled.

Closes #169
likebreath added a commit to cloud-hypervisor/cloud-hypervisor that referenced this pull request Sep 25, 2023
The 'derive' feature now is optional and requires to be enabled
explicitly [1].

[1] google/zerocopy#176

Signed-off-by: Bo Chen <chen.bo@intel.com>
likebreath added a commit to likebreath/cloud-hypervisor that referenced this pull request Sep 25, 2023
The 'derive' feature of `zerocopy` crate now is optional and requires to
be enabled explicitly [1]. Also, a version bump on `acpi_tables` is
needed to reply on a single version of `zerocopy` to avoid compilation
errors.

[1] google/zerocopy#176

Signed-off-by: Bo Chen <chen.bo@intel.com>
rbradford pushed a commit to cloud-hypervisor/cloud-hypervisor that referenced this pull request Sep 26, 2023
The 'derive' feature of `zerocopy` crate now is optional and requires to
be enabled explicitly [1]. Also, a version bump on `acpi_tables` is
needed to reply on a single version of `zerocopy` to avoid compilation
errors.

[1] google/zerocopy#176

Signed-off-by: Bo Chen <chen.bo@intel.com>
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.

Make zerocopy-derive an optional Cargo dependency

2 participants