-
Notifications
You must be signed in to change notification settings - Fork 569
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
hw rng attempt to read value from arm64 devices. #3228
base: master
Are you sure you want to change the base?
Conversation
283bb20
to
7712dcf
Compare
Codecov ReportPatch coverage has no change and project coverage change:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #3228 +/- ##
==========================================
- Coverage 88.22% 88.13% -0.09%
==========================================
Files 617 609 -8
Lines 70386 68193 -2193
Branches 6818 6797 -21
==========================================
- Hits 62095 60100 -1995
+ Misses 5431 5268 -163
+ Partials 2860 2825 -35
☔ View full report in Codecov by Sentry. |
@@ -111,6 +130,8 @@ bool Processor_RNG::available() | |||
{ | |||
#if defined(BOTAN_TARGET_CPU_IS_X86_FAMILY) | |||
return CPUID::has_rdrand(); | |||
#elif defined(BOTAN_TARGET_ARCH_IS_ARM64) && !defined(BOTAN_TARGET_OS_IS_MACOS) && defined(BOTAN_TARGET_IS_IOS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any reason to here (or elsewhere in the file) guard against macOS/iOS - if CPUID doesn't indicate support, the code will never be invoked, and the only way CPUID can detect support is with getauxval which doesn't exist on Apple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in fact it s more because the assembly does not compile on these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's unfortunate.
Maybe instead we can use __builtin_aarch64_rndrrs
/ __builtin_arm_rndrrs
builtins? And then only enable support if the builtins exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah good point
You'll also need to update |
d02e0e0
to
7ec19e0
Compare
Seems like this is stalled - should we close? |
It s ready for review. |
Seems OK but needs to be rebased to address the conflict |
72d3b9b
to
b91dc4a
Compare
We may need a newer compiler here? |
9df3b0a
to
32940a0
Compare
32940a0
to
5568ea8
Compare
No description provided.