enh 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
----------------
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;
```
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
