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

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

Closed
wants to merge 1 commit into from

Conversation

briansmith
Copy link
Owner

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.

@briansmith briansmith self-assigned this Jan 16, 2024
Copy link

codecov bot commented Jan 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@briansmith
Copy link
Owner Author

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

build.rs Outdated Show resolved Hide resolved
src/ec/curve25519/x25519.rs Show resolved Hide resolved
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.
@briansmith
Copy link
Owner Author

@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.

Copy link

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me as a first step based on the discussions in #1914 👍

(Although full disclosure: I'm still relatively new to Rust, so learning about things like build.rs as I go).

@briansmith
Copy link
Owner Author

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.

@briansmith briansmith closed this Jan 18, 2024
@BlackHoleFox
Copy link
Contributor

Seems reasonable, thanks for the backnforth.

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.

4 participants