std_detect: Workaround Exynos 9810 bug on aarch64 Android #1378
Conversation
|
r? @Amanieu (rustbot has picked a reviewer for you, use r? to override) |
taiki-e
force-pushed
the
std-detect-aarch64-android
branch
from
a22f460 to
12e7cbc
Compare
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 <[email protected]>
|
An alternative would be to query OS version (ro.build.version.release, which works in the above environment) to exclude problematic versions... |
|
If you can confirm, I'm fine with the approach. |
|
Ah, it appears that ro.arch is actually a field that was removed in Android 12. The above case used Android 12, but Android 10 and Android 11 return a valid value (with both the getprop command and the __system_property_get function).
It is fine that ro.arch is not available for Android 12+ because Android 9+ includes the fix. |
|
Maybe add to the comment that bit of additional information. |
| arch.as_mut_ptr() as *mut libc::c_char, | ||
| ) | ||
| }; | ||
| if len > 0 && arch.starts_with(b"exynos9810\0") { |
There was a problem hiding this comment.
Is this checking that the returned string starts with exynos9810 or that it is exactly equal to exynos9810? In the former case you wouldn't want the \0 in the string.
There was a problem hiding this comment.
I originally meant the latter, but looking at the LLVM patch they seem to be using the former.
So, I removed \0 to match the LLVM patch's behavior.
| ) | ||
| }; | ||
| if len > 0 && arch.starts_with(b"exynos9810\0") { | ||
| return cache::Initializer::default(); |
There was a problem hiding this comment.
We still want to probe for v8.0 features such as aes/sha/crc32.
There was a problem hiding this comment.
IIUC this is a bit tricky because even the armv8.0 optional features may not be consistent between little and big.
So for now, all I can think of is one of the following
- Only check features that are known to be available on exynos-m3 (armv8.0 cores 1).
(According to cmd/compile: go binaries not working on exynos 64 bit CPUs golang/go#28431) The old kernel seems to return auxv/cpuinfo based on features available on cortex-a55 (armv8.2 cores 1), so it can properly handle features even if they are available on exynos-m3 but not on cortex-a55 (though such features probably do not exist 2). - Continue detection on Android 9+ and return default value on pre-9.
The first one seems to make more sense, as it does not require additional __system_property_get to get the version and works on older kernels.
Footnotes
-
At least, all the target features in the output of
rustc --print cfg --target aarch64-linux-android -C target-cpu=exynos-m3are also included in the output ofrustc --print cfg --target aarch64-linux-android -C target-cpu=cortex-a55. ↩
There was a problem hiding this comment.
Implemented the first approach in 9ce03f1.
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. https://reviews.llvm.org/D114523
taiki-e
force-pushed
the
std-detect-aarch64-android
branch
from
12e7cbc to
0adfbec
Compare
Added comment about ro.arch on Android 12+ on Exynos. |
…v8.0 cores if CPU is Exynos 9810
taiki-e
force-pushed
the
std-detect-aarch64-android
branch
from
a873d1a to
9ce03f1
Compare
This module is already `#[cfg(target_arch = "aarch64")]`.
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. That patch is about FEAT_LSE, but IIUC the detection of most features that are available in armv8.2-a but may not be available in armv8.0-a should be affected by this issue.