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

zeroize: wasm v128, more aarch64 registers, const Zeroizing::new() #964

Closed

Conversation

brxken128
Copy link
Contributor

@brxken128 brxken128 commented Oct 22, 2023

This includes:

  • Support for wasm's v128 SIMD register
  • Support for Aarch64 int*, float* and poly* registers
  • Removes the aarch64 feature (and the corresponding docs) as the MSRV was above that required by core::arch::aarch64 (breaking change)
  • Ran clippy on the entire crate and added #[allow(...)]s where appropriate (wildcard imports and inline always)
  • Zeroizing::new() is now const (required MSRV bump to 1.61 - breaking change)
  • Cleaned up the docs in general, fixed a typo and removed some irrelevant documentation regarding the lack of support for clearing registers (very outdated)

I attempted to add feature-gated support for __m512* registers but it was only stabilized in 1.73.0, and even with some #[cfg]s, I couldn't get anything working on my 7950x (which has more than enough AVX512 support). It would've been a nice touch considering the rest of the breaking changes.

I could attempt to implement the changes brought up in #841 but maybe by XORing the value with itself within an ASM block or something similar? not really viable due to the generic-ness of the implementation.

I can always revert the two breaking changes - they're not super important but const Zeroizing::new() is quite nice, at not a great cost.

@brxken128 brxken128 marked this pull request as draft October 22, 2023 08:11
@brxken128 brxken128 changed the title zeroize: wasm v128, clippy overhaul, const Zeroizing::new(), doc fixes, more aarch64 registers zeroize(breaking): wasm v128, clippy overhaul, const Zeroizing::new(), doc fixes, more aarch64 registers Oct 22, 2023
@brxken128 brxken128 marked this pull request as ready for review October 22, 2023 08:22
@brxken128 brxken128 changed the title zeroize(breaking): wasm v128, clippy overhaul, const Zeroizing::new(), doc fixes, more aarch64 registers zeroize: wasm v128, more aarch64 registers, const Zeroizing::new() Oct 22, 2023
@tarcieri
Copy link
Member

tarcieri commented Oct 22, 2023

This is a lot of different types of changes in a single PR. I would suggest splitting them up largely around the bullet points you outlined in the original description.

zeroize has been 1.0 for over 4 years. We can't make breaking changes. It uses a heavily trait-based API intended to compose across crates, similar to something like serde, which makes releasing a breaking change nearly impossible (or at best, extremely disruptive). Short of some sort of fundamental unsoundness problem that's unfixable without breaking changes, we aren't going to be doing a 2.0 release.

The aarch64 feature needs to sit around even if it's vestigial.

@brxken128 brxken128 force-pushed the zeroize-aarch64-wasm-simd branch from 7a0d56e to 3c7bebd Compare October 22, 2023 12:55
@brxken128
Copy link
Contributor Author

This is a lot of different types of changes in a single PR. I would suggest splitting them up largely around the bullet points you outlined in the original description.

zeroize has been 1.0 for over 4 years. We can't make breaking changes. It uses a heavily trait-based API intended to compose across crates, similar to something like serde, which makes releasing a breaking change nearly impossible (or at best, extremely disruptive). Short of some sort of fundamental unsoundness problem that's unfixable without breaking changes, we aren't going to be doing a 2.0 release.

The aarch64 feature needs to sit around even if it's vestigial.

That is beyond fair and I honestly kind of expected it: quite a large PR for such a small crate, and the breaking changes were definitely a stretch considering how long it's been at 1.0.

I'll close this PR, split everything down, exclude the breaking changes/MSRV bump and hopefully things will be good to go.

@brxken128 brxken128 closed this Oct 22, 2023
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