rprichard 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:
> 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D158641/new/
https://reviews.llvm.org/D158641
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
