search

android / ndk

The Android Native Development Kit
1.95k stars 254 forks source link

[BUG] android_getCpuFeatures doesn't report idiv on Nexus 7 "flo" device #1605

Closed rprichard closed 5 days ago

rprichard commented 2 years ago 
$ run_tests.py --filter test-cpufeatures.test_arm_idiv_support
Finished pushing tests
FAIL test-cpufeatures.test_arm_idiv_support [armeabi-v7a-19-new]: adb -s 04c90017 shell 'cd /data/local/tmp/tests/armeabi-v7a-19-new/ndk-build/test-cpufeatures/armeabi-v7a && LD_LIBRARY_PATH=/data/local/tmp/tests/armeabi-v7a-19-new/ndk-build/test-cpufeatures/armeabi-v7a ./test_arm_idiv_support 2>&1'
android-19 razor 04c90017 KTU84P
cpu-features reports the following CPUID: 0x511006f0
status of arm idiv instruction:
  reported      : unsupported
  runtime check : supported

The udiv and sdiv instructions seem to work on this device, in both ARM and Thumb modes. android_getCpuFeatures() isn't reporting either of ANDROID_CPU_ARM_FEATURE_IDIV_ARM or ANDROID_CPU_ARM_FEATURE_IDIV_THUMB2.

/proc/cpuinfo doesn't report any of idiv, idiva, or idivt.

The build fingerprint is google/razor/flo:4.4.4/KTU84P/1227136:user/release-keys.

/proc/cpuinfo:

Processor   : ARMv7 Processor rev 0 (v7l)
processor   : 0
BogoMIPS    : 13.53

processor   : 1
BogoMIPS    : 13.53

processor   : 2
BogoMIPS    : 13.53

processor   : 3
BogoMIPS    : 13.53

Features    : swp half thumb fastmult vfp edsp neon vfpv3 tls vfpv4 
CPU implementer : 0x51
CPU architecture: 7
CPU variant : 0x1
CPU part    : 0x06f
CPU revision    : 0

Hardware    : QCT APQ8064 FLO
Revision    : 0000
Serial      : 0000000000000000

fastboot reports:

PRODUCT NAME - flo
VARIANT - flo 32G
HW VERSION - rev_e
BOOTLOADER VERSION - FLO-04.02
...

cpu-features.c already has code to work around a lack of idiv reporting. Maybe this device is similar?

        // Handle kernel configuration bugs that prevent the correct
        // reporting of CPU features.
        static const struct CpuFix {
            uint32_t  cpuid;
            uint64_t  or_flags;
        } cpu_fixes[] = {
            /* The Nexus 4 (Qualcomm Krait) kernel configuration
             * forgets to report IDIV support. */
            { 0x510006f2, ANDROID_CPU_ARM_FEATURE_IDIV_ARM |
                          ANDROID_CPU_ARM_FEATURE_IDIV_THUMB2 },
            { 0x510006f3, ANDROID_CPU_ARM_FEATURE_IDIV_ARM |
                          ANDROID_CPU_ARM_FEATURE_IDIV_THUMB2 },
        };

The chipsets for the Nexus 4 and the 2013 Nexus 7 devices look similar:

rprichard commented 2 years ago

The https://github.com/pytorch/cpuinfo project also detects the Krait idiv non-kernel-reporting bug, but it matches any CPU where the implementer is 'Q'/0x51 and the CPU part is 0x04d or 0x06f. It ignores the other MIDR fields (variant, architecture, and revision). The Android cpu-features.c code only ignores the "architecture" field, because it ignores the CPU architecture field in /proc/cpuinfo.

If Android cpu-features's Krait workaround also ignored variant and revision, then it too would recognize my Nexus 7 device as supporting idiv.

pytorch cpuinfo references:

enh-google commented 2 years ago

lol, i'm really torn on this ... on the one hand "deprecated; we're actively telling people to move to the google cpufeatures", but on the other "breaks our own tests on our own devices" ...

rprichard commented 2 years ago

Ah, I didn't realize it was deprecated. From https://developer.android.com/ndk/guides/cpu-features:

The NDK still provides a deprecated library named cpufeatures for source compatibility with apps that already use it. Unlike the newer and more complete cpu_features library, this historical library does not have workarounds for about specific SoCs.

I don't think I understand that last sentence, "does not have workarounds for about specific SoCs". Should it be "for as many specific SoCs"?

It looks like google/cpu_features uses the same logic as the NDK's cpufeatures, so it would similarly report the 2013 Nexus 7 CPU as lacking idiv: https://github.com/google/cpu_features/blob/ebcdfcaeffe67607d112ce019a12ad1c73e0475e/src/impl_arm_linux_or_android.c#L155-L160

Both google/cpu_features and cpufeatures have code using dlopen/dlsym to get getauxval. e.g. In cpu_features:

#if defined(HAVE_STRONG_GETAUXVAL)
#include <sys/auxv.h>
static unsigned long GetElfHwcapFromGetauxval(uint32_t hwcap_type) {
  return getauxval(hwcap_type);
}
#elif defined(HAVE_DLFCN_H)
// On Android we probe the system's C library for a 'getauxval' function and
// call it if it exits, or return 0 for failure. This function is available
// since API level 20.
//
// This code does *NOT* check for '__ANDROID_API__ >= 20' to support the edge
// case where some NDK developers use headers for a platform that is newer than
// the one really targetted by their application. This is typically done to use
// newer native APIs only when running on more recent Android versions, and
// requires careful symbol management.
//
// Note that getauxval() can't really be re-implemented here, because its
// implementation does not parse /proc/self/auxv. Instead it depends on values
// that are passed by the kernel at process-init time to the C runtime
// initialization layer.
... use dlopen and dlsym ...

The NDK stubs and sys/auxv.h report that getauxval is available starting with API 18. Also, I think the CMake file will respect __ANDROID_API__:

check_symbol_exists(getauxval "sys/auxv.h" HAVE_STRONG_GETAUXVAL)

(I'm not sure exactly what kind of check that check_symbol_exists will do, but I think it will report getauxval as available/unavailable depending on __ANDROID_API__.)

The upstream kernel also has a version of this Krait idiv workaround:

The kernel patches come from CodeAurora.

It looks like it recognizes a large range of part numbers as Krait -- [0x040..0x07f] inclusive.

It looks like the MIDR architecture field for Krait is 0xf (not, say, 7). I think that matches the scheme documented here: https://developer.arm.com/documentation/ddi0406/b/System-Level-Architecture/Virtual-Memory-System-Architecture--VMSA-/CP15-registers-for-a-VMSA-implementation/c0--Main-ID-Register--MIDR-

enh-google commented 2 years ago

I don't think I understand that last sentence, "does not have workarounds for about specific SoCs". Should it be "for as many specific SoCs"?

yeah, fixed: https://critique-ng.corp.google.com/cl/410265000

anyway, yeah, please send a fix/file a bug with the "real" cpu_features, and, yeah, even if this is deprecated, it makes sense to add this historical workaround since it's affecting our testing.

i also wonder whether we should actually add the deprecated annotations to the header? is the "real" cpu_features in prefab yet? (we should probably make it easy to do the new right thing before we make it more annoying to keep using the easy but deprecated thing. https://goomics.net/50/ )

jfgoog commented 2 years ago

This is now causing CI test failures with armeabi-v7a-19-new.

Example: https://android-build.googleplex.com/builds/tests/view?invocationId=I48700010065660625&testResultId=TR82628113328604546

DanAlbert commented 5 days ago

Closing since this library is no longer maintained.

enh-google commented 5 days ago

(and i just checked that we already fixed the part of the documentation that caused confusion above.)