MSRV 1.60: cpu: Use std::arch::is_aarch64_feature_detected on AArch64.

[briansmith]

Switch to std::arch::is_aarch64_feature_detected on AArch64 to support more operating systems and environments without having to add new OS- specific code. This requires bumping the MSRV for AAarch64 targets to 1.60.0.

[codecov[bot]]

Codecov Report

Merging #1542 (07b68f3) into main (7bbc307) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1542   +/-   ##
=======================================
  Coverage   92.09%   92.09%           
=======================================
  Files         132      132           
  Lines       18869    18834   -35     
  Branches      196      196           
=======================================
- Hits        17377    17346   -31     
+ Misses       1455     1452    -3     
+ Partials       37       36    -1     

Files	Coverage Δ
src/cpu.rs	100.00% <100.00%> (ø)
src/cpu/arm.rs	91.66% <100.00%> (-0.73%)	:arrow_down:

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[MikaelUrankar]

Thanks. It builds fine on FreeBSD aarch64 with rust 1.63.0. There is a warning though:

warning: function `setup` is never used                                                                                        
   --> src/cpu/arm.rs:71:9                                                                                                     
    |                                                                                                                          
71  |           pub fn setup() {                                                                                               
    |           ^^^^^^^^^^^^^^                                                                                                 
...                                                            
132 | / features! {                                         
133 | |     "neon" then "neon" => NEON {                       
134 | |         mask: 1 << 0,                                                                                                  
135 | |     },                                                                                                                 
...   |                                                                                                                        
153 | |     },                                                 
154 | | }
    | |_- in this macro invocation
    |
    = note: `#[warn(dead_code)]` on by default
    = note: this warning originates in the macro `features` (in Nightly builds, run with -Z macro-backtrace for more info)

Tests are passing.

[briansmith]

Thanks. It builds fine on FreeBSD aarch64 with rust 1.63.0. There is a warning though:

Thanks! That means the new logic isn't being used, and the reason is that in cpu.rs we have these conditionals:

    #[cfg(any(
        target_arch = "x86",
        target_arch = "x86_64",
        all(
            any(target_arch = "aarch64", target_arch = "arm"),
            any(
                target_os = "android",
                target_os = "fuchsia",
                target_os = "linux",
                target_os = "windows"
            )
        )
    ))]

I updated the PR with a new commit that expands this list. PTAL.

[MikaelUrankar]

It builds fine once I added the missing bit in Cargo.toml, tests are also passing:

error[E0433]: failed to resolve: use of undeclared crate or module `spin`
  --> src/cpu.rs:60:22
   |
60 |         static INIT: spin::Once<()> = spin::Once::new();
   |                      ^^^^ use of undeclared crate or module `spin`

error[E0433]: failed to resolve: use of undeclared crate or module `spin`
  --> src/cpu.rs:60:45
   |
60 |         static INIT: spin::Once<()> = spin::Once::new();
   |                                             ^^^^ not found in `spin`
   |
help: consider importing this struct
   |
15 | use core::iter::Once;
   |
help: if you import `Once`, refer to it directly
   |
60 -         static INIT: spin::Once<()> = spin::Once::new();
60 +         static INIT: spin::Once<()> = Once::new();
   |

[briansmith]

I rebased this. Now our MSRV is 1.60 so we can move forward with this.

[Brooooooklyn]

All tests passed on my WOA device with rust 1.72.1

[complexspaces]

I did some testing and seem to have mixed results about this PR.

Inside of an ARM64 Windows 11 VM (running on a M1 host), the tests passed just fine. Additionally, while generally noisy, the benchmarks didn't seem to show a significant difference between this branch and main, indicating that the feature detection for hardware acceleration is still working.

The benchmark results were about the same on the M1 host, strangely.

When I attempted to run the test suite on my M1 host, a large number of them failed:

---- aead::poly1305::tests::test_poly1305 stdout ----
thread 'aead::poly1305::tests::test_poly1305' panicked at 'assertion failed: `(left == right)`
  left: `21`,
 right: `53`', src/cpu/arm.rs:132:1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- arithmetic::bigint::tests::test_elem_reduced_once stdout ----
thread 'arithmetic::bigint::tests::test_elem_reduced_once' panicked at 'Once previously poisoned by a panicked', /Users/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/spin-0.9.2/src/once.rs:296:37

CI also seems to be seeing the same failure cause, although the result is different. It looks like the new detection code isn't picking up on everything from before on Apple's ARM64 systems.

[briansmith]

Thanks @complexspaces. Because so much was refactored in this module, a non-trivial rebase of this on top of main is needed.

I suspect the assertion you are hitting is:

debug_assert_eq!(features & ARMCAP_STATIC, ARMCAP_STATIC);

~Presumably it is supposed to be something like: [wrong idea removed]~

I am not sure we really want that assertion at all, but if we do, it probably belongs here:

 pub unsafe fn initialize_OPENSSL_armcap_P() {
     let detected = detect_features();
+    debug_assert_eq!(detected & ARMCAP_STATIC, ARMCAP_STATIC);

[briansmith]

The reason this PR failed in CI is because CI had a MSRV of 1.60.0 when this was submitted, and there was a bug in 1.60.0 and earlier in the feature detection logic for aarch64-apple-* targets. Were you testing on 1.60.0 and earlier? Other PRs raised the MSRV specifically for this reason and added additional testing in arm.rs.

Regardless, I think I need to rebase this on main before we can investigate further.

[complexspaces]

Were you testing on 1.60.0 and earlier?

I was testing using Rust 1.72.0 on both systems.

Regardless, I think I need to rebase this on main before we can investigate further.

Sounds good 👍. I didn't realize at a glance that there was such a large difference between this branch and main. I should have probably double-checked that before benchmarking for a more accurate result. I can run the benchmarks again once this branch is in a better state CI is happy with.

[briansmith]

Closing this for now. We've implemented some workarounds for rust-lang/rust issues that we can't implement on top of is_aarch64_feature_detected, and I have some vague plans for new functionality that probably can't be built on top of is_aarch64_feature_detected either.