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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
