[PATCH] D158641: [AArch64][Android][DRAFT] Fix FMV ifunc resolver usage on old Android APIs.

[Pavel Iliin via Phabricator via cfe-commits]

ilinpv added inline comments.


================
Comment at: compiler-rt/lib/builtins/cpu_model.c:1382
+    return;
+#if defined(__ANDROID__)
+  // ifunc resolvers don't have hwcaps in arguments on Android API lower
----------------
enh wrote:
> rprichard wrote:
> > enh wrote:
> > > srhines wrote:
> > > > MaskRay wrote:
> > > > > enh wrote:
> > > > > > ilinpv wrote:
> > > > > > > MaskRay wrote:
> > > > > > > > I am unfamiliar with how Android ndk builds compiler-rt.
> > > > > > > > 
> > > > > > > > If `__ANDROID_API__ >= 30`, shall we use the regular Linux code path?
> > > > > > > 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`
> > > > > > 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 :-) )
> > > > > 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.
> > > > 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).
> > > > 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!)
> > 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.
> > 
> > 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;
> ```
Thanks for the idea! We'll test that and update the patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158641/new/

https://reviews.llvm.org/D158641