search

briansmith / ring

Safe, fast, small crypto using Rust
Other
3.69k stars 697 forks source link

Build: DRY conditions for allowing use of x25519_neon assembly. #1919

Closed briansmith closed 7 months ago

briansmith commented 8 months ago

Expand the conditions in which the x25519_neon function will be avoided. This is a temporary measure until we can properly condition it on the target ABI.

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (73fb637) 91.58% compared to head (ec16ff2) 96.02%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1919 +/- ## ========================================== + Coverage 91.58% 96.02% +4.44% ========================================== Files 134 136 +2 Lines 19684 20776 +1092 Branches 226 226 ========================================== + Hits 18027 19950 +1923 + Misses 1623 792 -831 Partials 34 34 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

briansmith commented 8 months ago

@pixlwave @lcruz99 @BlackHoleFox PTAL at this, since you've been interested in this topic previously.

briansmith commented 7 months ago

@BlackHoleFox Thanks for taking a look. I updated the PR to address the build.rs nit you mentioned, and to clarify the issue with x25519_NEON. PTAL.

briansmith commented 7 months ago

I'm going to close this without merging it. I don't like the denylist-like approach. I think we only need to support x25519_neon for, at most, Linux and Android, so we should switch to an allowlist-based approach that lists them specifically. Further, we should change build.rs so that x25519_neon is not compiled at all on non-Linux/Android targets.

BlackHoleFox commented 7 months ago

Seems reasonable, thanks for the backnforth.