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

Detect CPU features with Linux methods on Android for non-Intel CPUs. #1351

Merged
merged 2 commits into from
Nov 9, 2022

Conversation

gendx
Copy link
Contributor

@gendx gendx commented Nov 9, 2022

After digging into why the is_aarch64_feature_detected! macro didn't work as I expected on my Android device (only asimd, fp, neon were detected by the macro), I came to this line that dispatches the detection depending on the OS.

My understanding is that Android should behave like Linux w.r.t. CPU feature detection on ARM - at least on the devices I've tested with the various methods (getauxval, checking /proc/cpuinfo) worked fine. However, Rust defines a distinct target_os for Android (https://github.com/rust-lang/rust/blob/1.65.0/compiler/rustc_target/src/spec/android_base.rs#L5), so the filtering by target_os = "linux" doesn't work (even though Android is arguably a flavor of Linux).

I've tested this patch manually on a stage 1 compiler and it worked on physical devices (Samsung Galaxy S7 and Samsung Galaxy S20).

I wonder if the feature = "libc" is relevant on Android?

I didn't try the CI script but I guess it's not so relevant at the moment since Android CI was removed (#1346)? I wonder if this PR makes a CI for Android more relevant - although it may be that under the hood qemu doesn't implement as many CPU features as physical devices do?

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) soon.

Please see the contribution instructions for more information.

@Amanieu
Copy link
Member

Amanieu commented Nov 9, 2022

Regarding CI, there are currently some issues on the Rust side that need to be resolved (rust-lang/rust#103673). Unfortunately I currently don't have the time to look into these issues.

Applied suggestion to require `feature = "libc"` on Android targets.

Co-authored-by: Amanieu d'Antras <amanieu@gmail.com>
@Amanieu Amanieu merged commit b168a4c into rust-lang:master Nov 9, 2022
@thomcc
Copy link
Member

thomcc commented Nov 10, 2022

I don't think we use the "libc" feature in the stdlib, do we?

@Amanieu
Copy link
Member

Amanieu commented Nov 10, 2022

We do use it, otherwise feature detection wouldn't work on non-x86 platforms.

@thomcc
Copy link
Member

thomcc commented Nov 10, 2022

We do use it, otherwise feature detection wouldn't work on non-x86 platforms.

I believe it doesn't?

I don't see how the libc feature would be enabled. I thought this was intentional (Last time I looked, some of that code seemed to be a bit sketchy, but looking now it's probably fine?)

@Amanieu
Copy link
Member

Amanieu commented Nov 10, 2022

@gendx gendx deleted the detect-android branch November 23, 2022 22:52
bors bot added a commit to taiki-e/portable-atomic that referenced this pull request Feb 11, 2023
76: detect: Workaround Exynos 9810 bug on aarch64 Android r=taiki-e a=taiki-e

Samsung Exynos 9810 has a bug that big and little cores have different ISAs. And on older Android (pre-9), the kernel incorrectly reports that features available only on some cores are available on all cores.

See https://reviews.llvm.org/D114523 for details.

Our own run-time detection code has not been released yet and is not a problem, but portable-atomic < 1.1 may have been affected by this issue since rustc 1.69-nightly when is_aarch64_feature_detected supported run-time detection on Android. (rust-lang/stdarch#1351, rust-lang/rust#105784)

A patch on stdarch side: rust-lang/stdarch#1378

Co-authored-by: Taiki Endo <te316e89@gmail.com>
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