[AArch64] Fix FMV ifunc resolver usage on old Android APIs. Rename internal compiler-rt FMV functions.

[ilinpv]

Summary

The patch fixes Function Multi Versioning features detection by ifunc resolver on Android API levels < 30.
Ifunc hwcaps parameters are not supported on Android API levels 23-29,
so all CPU features are set unsupported if they were not initialized
before ifunc resolver call.
There is no support for ifunc on Android API levels < 23, so Function
Multi Versioning is disabled in this case.

Also use two underscore prefix for FMV runtime support functions to
avoid conflict with user program ones.

[rprichard]

further confirmation if android_get_device_api_level should work from ifunc_resolver

IIRC an ifunc resolver in Bionic can't generally call any functions in libc, including android_get_device_api_level or __system_property_get, because the ifunc resolver will typically be called before relocations in libc.so have been resolved. I believe an ifunc resolver also can't call __system_property_get because the libc.so system properties are initialized by a constructor function, which isn't called until after relocations are applied (__libc_preinit -> __libc_preinit_impl -> __libc_init_common -> __system_properties_init).

I suspect Bionic ought to apply relocations to libraries in a bottom-up fashion, so that libc.so is relocated before the executable or shared objects, but I _think_ it's currently top-down. Deferring ifunc relocations until after non-ifunc relocations are applied is a separate problem.

[MaskRay]

clang/test/Driver/aarch64-features.c

10

Deleting blank lines for the 3 sets of Android tests (i.e. grouping them together) seems to improve readability.

compiler-rt/lib/builtins/cpu_model.c

1382

I am unfamiliar with how Android ndk builds compiler-rt.

If __ANDROID_API__ >= 30, shall we use the regular Linux code path?

[MaskRay]

In D158641#4611731, @rprichard wrote:

[...]

I suspect Bionic ought to apply relocations to libraries in a bottom-up fashion, so that libc.so is relocated before the executable or shared objects, but I _think_ it's currently top-down. Deferring ifunc relocations until after non-ifunc relocations are applied is a separate problem.

https://maskray.me/blog/2021-01-18-gnu-indirect-function#relocation-resolving-order glibc uses the bottom-up fashion. I agree that adopting the bottom-up fashion for Bionic will be great.

"l_initfini contains main, dep1.so, dep2.so, dep4.so, dep3.so, libc.so.6, ld.so. The relocation resolving order is ld.so (bootstrap), libc.so.6, dep3.so, dep4.so, dep2.so, dep1.so, main, ld.so."

[ilinpv]

compiler-rt/lib/builtins/cpu_model.c

1382

I think that leads to shipping different compile-rt libraries depend on ANDROID_API. If this is an option to consider than runtime check android_get_device_api_level() < 30 can be replaced by __ANDROID_API__ < 30

[enh]

compiler-rt/lib/builtins/cpu_model.c

1382

depends what you mean... in 10 years or so, yes, no-one is likely to still care about the older API levels and we can just delete this. but until then, no, there's _one_ copy of compiler-rt that everyone uses, and although _OS developers_ don't need to support anything more than a couple of years old, most app developers are targeting far lower API levels than that (to maximize the number of possible customers).

TL;DR: "you could add that condition to the #if, but no-one would use it for a decade". (and i think the comment and if below should make it clear enough to future archeologists when this code block can be removed :-) )

[MaskRay]

compiler-rt/lib/builtins/cpu_model.c

1382

My thought was that people build Android with a specific __ANDROID_API__, and only systems >= this level are supported.

#If __ANDROID_API__ < 30
...
#endif

This code has a greater chance to be removed when it becomes obsoleted. The argument is similar to how we find obsoleted GCC workarounds.

[srhines]

compiler-rt/lib/builtins/cpu_model.c

1382

Yes, the NDK currently just ships the oldest supported target API level for compiler-rt, while the Android platform build does have access to both the oldest supported target API level + the most recent target API level, so that we can make better use of features there.

Maybe I'm missing something, but it's feeling like the NDK users won't be able to make great use of FMV without us either bumping the minimum level or shipping multiple runtimes (and then using the #ifdefs properly here).

[MaskRay]

compiler-rt/lib/builtins/cpu_model.c

1379

It seems that we don't need the _constructor function. We can just move the

#if defined(__ANDROID__)
  // ifunc resolvers don't have hwcaps in arguments on Android API lower
  // than 30. In this case set detection done and keep all CPU features
  // unsupported (zeros).
  if (android_get_device_api_level() < 30) {
    setCPUFeature(FEAT_MAX);
    return;
  }

logic to init_cpu_features_resolver

[enh]

compiler-rt/lib/builtins/cpu_model.c

1382

Maybe I'm missing something, but it's feeling like the NDK users won't be able to make great use of FMV without us either bumping the minimum level or shipping multiple runtimes (and then using the #ifdefs properly here).

yeah, that's the point of this code --- it's a runtime check so the NDK "just works".

but if people want the __ANDROID_API__ #if too, that's fine. (and, like you say, the platform's two variants mean that we'll be testing both code paths, so i'm not worried about "one of these is the only one that anyone's actually building" problem.)

i have no opinion on whether anyone llvm is more/less likely to understand if/when if (android_get_device_api_level() < 30) versus #if __ANDROID_API__ < 30 can be deleted.

i think the best argument for leaving this change as-is would be "anyone building their own is less likely to screw up" (since developers struggle all the time with the difference between "target" and "min" api, because the ndk terminology is different to the AndroidManifest.xml stuff that developers are more familiar with, which causes confusion). so if this was in libc++ (where i know people do build their own), i'd argue for the code as-is. but since it's compiler-rt (and i'm not aware that anyone's building that themselves) i don't think it matters either way?

to be clear, i'm imagining:

#if __ANDROID_API__ < 30
  if (android_get_device_api_level() < 30) {
    setCPUFeature(FEAT_MAX);
    return;
  }
#endif

(which brings us back to the "this is confusing" --- _just_ having the #if would be subtly different, which is why if i'd written this, i'd have written it as-is too!)

[rprichard]

compiler-rt/lib/builtins/cpu_model.c

1382

Unless I'm missing something, calling android_get_device_api_level doesn't work, because (a) the loader hasn't performed the necessary relocations and (b) that API reads system properties, which haven't been initialized yet.

Maybe the device API could/should be exported to a /dev file, which is how we exported the CPU variant to ifunc resolvers.

We could redesign Bionic so that an ifunc resolver can call android_get_device_api_level: e.g:

• Relocate objects in bottom-up order.
• Collect ifunc relocations and defer them until after non-ifunc relocations.
• Have android_get_device_api_level in libc.so call into the loader, which has already initialized its copy of the system properties code.

However, with that approach, we can't call android_get_device_api_level unless __ANDROID_API__ is new enough to have the loader enhancements, in which case, we can just use __ANDROID_API__.

Using the macro isn't great for the NDK because the NDK only distributes the builtins built for the oldest supported API level. I suspect most apps are the same way. Even the platform builtins are built for the oldest API. toolchain/llvm_android/builders.py:

class BuiltinsBuilder(base_builders.LLVMRuntimeBuilder):
    ...
    # Only target the NDK, not the platform. The NDK copy is sufficient for the
    # platform builders, and both NDK+platform builders use the same toolchain,
    # which can only have a single copy installed into its resource directory.

If we really want to make this info available to NDK apps, there's probably some way to detect a new-enough version of libc.so -- e.g. I think a weak symbol reference could detect V and later.

[enh]

compiler-rt/lib/builtins/cpu_model.c

1382

Unless I'm missing something, calling android_get_device_api_level doesn't work, because (a) the loader hasn't performed the necessary relocations and (b) that API reads system properties, which haven't been initialized yet.

i don't think (b) is a problem because this is for apps, so you're being loaded into a zygote clone anyway. but, yes, (a) is the fundamental problem here.

We could redesign Bionic so that an ifunc resolver...

that doesn't solve the problem here, which is backwards compatibility. we can already only target api >= 30 and use what the ifunc resolver passes us. the question is how to fix this for apis 21-29.

If we really want to make this info available to NDK apps, there's probably some way to detect a new-enough version of libc.so -- e.g. I think a weak symbol reference could detect V and later.

well, it's 30 rather than 35, but, yeah --- we have a bunch of stuff that was new in 30 that we could look for:

LIBC_R { # introduced=R
  global:
    __mempcpy_chk;
    __tls_get_addr; # arm64
    call_once;
    cnd_broadcast;
    cnd_destroy;
    cnd_init;
    cnd_signal;
    cnd_timedwait;
    cnd_wait;
    memfd_create;
    mlock2;
    mtx_destroy;
    mtx_init;
    mtx_lock;
    mtx_timedlock;
    mtx_trylock;
    mtx_unlock;
    pthread_cond_clockwait;
    pthread_mutex_clocklock;
    pthread_rwlock_clockrdlock;
    pthread_rwlock_clockwrlock;
    renameat2;
    sem_clockwait;
    statx;
    thrd_create;
    thrd_current;
    thrd_detach;
    thrd_equal;
    thrd_exit;
    thrd_join;
    thrd_sleep;
    thrd_yield;
    tss_create;
    tss_delete;
    tss_get;
    tss_set;

[ilinpv]

compiler-rt/lib/builtins/cpu_model.c

1379

We don't need this "Android API check and return" in init_cpu_features_resolver when it is called from constructor init_cpu_features where hwcaps are obtained through getauxval calls.

[ilinpv]

compiler-rt/lib/builtins/cpu_model.c

1379

It seems there is no other way than split it in _constructor and _resolver parts.

[ilinpv]

compiler-rt/lib/builtins/cpu_model.c

1382

Thanks for the idea! We'll test that and update the patch.

[ilinpv]

Use weak "memfd_create" to check for API 30+, split and rename init cpu features functions ( __init_cpu_features, __init_cpu_features_resolver, __init_cpu_features_constructor )

[ilinpv]

The patch tested on NDK r26 bulding simple Function Multi Versioning project and running on Android API 25,29,30,33 - works fine.

[MaskRay]

The Clang Driver/CodeGen and compiler-rt changes look good to me. Of course please wait for an Android reviewer :)

[enh]

yeah, lgtm from the Android side too, unless rprichard has more comments?

clang/lib/Driver/ToolChains/Clang.cpp

   } else if (Triple.isAArch64() &&
              getToolChain().IsAArch64OutlineAtomicsDefault(Args)) {
     CmdArgs.push_back("-target-feature");
     CmdArgs.push_back("+outline-atomics");
   }
 
   if (Triple.isAArch64() &&
       (Args.hasArg(options::OPT_mno_fmv) ||
+       (Triple.isAndroid() && Triple.isAndroidVersionLT(23)) ||
        getToolChain().GetRuntimeLibType(Args) != ToolChain::RLT_CompilerRT)) {
     // Disable Function Multiversioning on AArch64 target.
     CmdArgs.push_back("-target-feature");
     CmdArgs.push_back("-fmv");
   }
 
   if (Args.hasFlag(options::OPT_faddrsig, options::OPT_fno_addrsig,
                    (TC.getTriple().isOSBinFormatELF() ||

clang/test/Driver/aarch64-features.c

 // RUN: %clang --target=aarch64-none-linux-gnu -### %s -fsyntax-only 2>&1 | FileCheck %s
 // RUN: %clang --target=arm64-none-linux-gnu -### %s -fsyntax-only 2>&1 | FileCheck %s
 
 // CHECK: "-funwind-tables=2"
 
 // The AArch64 PCS states that chars should be unsigned.
 // CHECK: fno-signed-char
 
 // Check Function Multi Versioning option and rtlib dependency.
-// RUN: %clang --target=aarch64-linux-android -rtlib=compiler-rt \
+// RUN: %clang --target=aarch64-linux-android23 -rtlib=compiler-rt \
 // RUN: -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-FMV %s
 
+// RUN: %clang --target=aarch64-linux-android -rtlib=compiler-rt \
+// RUN: -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-FMV-OFF %s
+
 // RUN: %clang --target=aarch64-linux-android -rtlib=compiler-rt -mno-fmv \
 // RUN: -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-FMV-OFF %s
 
+// RUN: %clang --target=aarch64-linux-android22 -rtlib=compiler-rt \
+// RUN: -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-FMV-OFF %s
+
 // RUN: %clang --target=aarch64-linux-gnu -rtlib=libgcc \
 // RUN: -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-FMV-OFF %s
 
 // RUN: %clang --target=arm64-unknown-linux -rtlib=libgcc \
 // RUN: -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-FMV-OFF %s
 
 // CHECK-FMV-OFF: "-target-feature" "-fmv"
 // CHECK-FMV-NOT: "-target-feature" "-fmv"

compiler-rt/lib/builtins/cpu_model.c

typedef struct __ifunc_arg_t {
   unsigned long _hwcap2;
 } __ifunc_arg_t;
 #endif // __has_include(<sys/ifunc.h>)
 
 #if __has_include(<asm/hwcap.h>)
 #include <asm/hwcap.h>
 
 #if defined(__ANDROID__)
+#include <android/api-level.h>
 #include <string.h>
 #include <sys/system_properties.h>
 #elif defined(__Fuchsia__)
 #include <zircon/features.h>
 #include <zircon/syscalls.h>
 #endif
 
 #ifndef _IFUNC_ARG_HWCAP

 
 // Architecture features used
 // in Function Multi Versioning
 struct {
   unsigned long long features;
   // As features grows new fields could be added
 } __aarch64_cpu_features __attribute__((visibility("hidden"), nocommon));
 
-void init_cpu_features_resolver(unsigned long hwcap, const __ifunc_arg_t *arg) {
+static void init_cpu_features_constructor(unsigned long hwcap,
+                                          const __ifunc_arg_t *arg) {
 #define setCPUFeature(F) __aarch64_cpu_features.features |= 1ULL << F
 #define getCPUFeature(id, ftr) __asm__("mrs %0, " #id : "=r"(ftr))
 #define extractBits(val, start, number)                                        \
   (val & ((1ULL << number) - 1ULL) << start) >> start
   if (__aarch64_cpu_features.features)
     return;
   unsigned long hwcap2 = 0;
   if (hwcap & _IFUNC_ARG_HWCAP)
if (hwcap & HWCAP_CPUID) {
     if (hwcap2 & HWCAP2_SVE2 && hwcap & HWCAP_SVE)
       setCPUFeature(FEAT_SVE2);
     if (hwcap & HWCAP_SHA3)
       setCPUFeature(FEAT_SHA3);
   }
   setCPUFeature(FEAT_MAX);
 }
 
+void init_cpu_features_resolver(unsigned long hwcap, const __ifunc_arg_t *arg) {
+  if (__aarch64_cpu_features.features)
+    return;
+#if defined(__ANDROID__)
+  // ifunc resolvers don't have hwcaps in arguments on Android API lower
+  // than 30. In this case set detection done and keep all CPU features
+  // unsupported (zeros).
+  if (android_get_device_api_level() < 30) {
+    setCPUFeature(FEAT_MAX);
+    return;
+  }
+#endif // defined(__ANDROID__)
+  init_cpu_features_constructor(hwcap, arg);
+}
+
 void CONSTRUCTOR_ATTRIBUTE init_cpu_features(void) {
   unsigned long hwcap;
   unsigned long hwcap2;
   // CPU features already initialized.
   if (__aarch64_cpu_features.features)
     return;
 #if defined(__FreeBSD__)
   int res = 0;

 #endif // defined(__ANDROID__)
   hwcap = getauxval(AT_HWCAP);
   hwcap2 = getauxval(AT_HWCAP2);
 #endif // defined(__FreeBSD__)
   __ifunc_arg_t arg;
   arg._size = sizeof(__ifunc_arg_t);
   arg._hwcap = hwcap;
   arg._hwcap2 = hwcap2;
-  init_cpu_features_resolver(hwcap | _IFUNC_ARG_HWCAP, &arg);
+  init_cpu_features_constructor(hwcap | _IFUNC_ARG_HWCAP, &arg);
 #undef extractBits
 #undef getCPUFeature
 #undef setCPUFeature
 #undef IF_EXYNOS9810
 }
 #endif // !defined(DISABLE_AARCH64_FMV)
 #endif // defined(__has_include)
 #endif // __has_include(<sys/auxv.h>)
 #endif // __has_include(<asm/hwcap.h>)
 #endif // defined(__aarch64__)