[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 
> > and

[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:1379
 
+void init_cpu_features_resolver(unsigned long hwcap, const __ifunc_arg_t *arg) 
{
+  if (__aarch64_cpu_features.features)

ilinpv wrote:
> MaskRay wrote:
> > 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
> 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.
It seems there is no other way than split it in ##_constructor## and 
##_resolver## parts.


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

[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:1379
 
+void init_cpu_features_resolver(unsigned long hwcap, const __ifunc_arg_t *arg) 
{
+  if (__aarch64_cpu_features.features)

MaskRay wrote:
> 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
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.


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

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

[Elliott Hughes via Phabricator via cfe-commits]

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

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

[Ryan Prichard via Phabricator via cfe-commits]

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 e

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

[Elliott Hughes via Phabricator via cfe-commits]

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

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!)


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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: compiler-rt/lib/builtins/cpu_model.c:1379
 
+void init_cpu_features_resolver(unsigned long hwcap, const __ifunc_arg_t *arg) 
{
+  if (__aarch64_cpu_features.features)

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


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

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

[Stephen Hines via Phabricator via cfe-commits]

srhines 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

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).


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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay 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:
> 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.


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

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

[Elliott Hughes via Phabricator via cfe-commits]

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

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 :-) )


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

[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

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`


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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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."


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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/test/Driver/aarch64-features.c:10
 // 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

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



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

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

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


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

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

[Ryan Prichard via Phabricator via cfe-commits]

rprichard added a comment.

> 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.


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

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

[Pavel Iliin via Phabricator via cfe-commits]

ilinpv created this revision.
ilinpv added reviewers: srhines, danielkiss, enh, MaskRay, rprichard.
Herald added subscribers: Enna1, kristof.beyls, krytarowski.
Herald added a project: All.
ilinpv requested review of this revision.
Herald added projects: clang, Sanitizers.
Herald added subscribers: Sanitizers, cfe-commits.

The patch tries to fix Function Multi Versioning features detection in 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.

However, applying this patch to android-ndk-r26-beta2 and build using it a 
simple app with FMV and feature detection lead to crash when FMV ifunc resolver 
called. I suspect issue could be in Android API runtime check called from ifunc 
resolver: ifunc_resolver -> init_cpu_features_resolver -> 
android_get_device_api_level(__system_property_get).
Part of crash stack trace ( thanks to Lingkai.Dong ) :

  08-23 15:37:16.824  4641  4641 F DEBUG   : backtrace:
  08-23 15:37:16.824  4641  4641 F DEBUG   :   #00 pc 1af0  

  08-23 15:37:16.824  4641  4641 F DEBUG   :   #01 pc 15b8  
/data/app/~~dMaXGzJksZog7lGF_CEMHQ==/com.arm.v9.demo-C74mlVTkH7NumAtbKGo36A==/base.apk!libdemo.so
 (offset 0x3e9000) (BuildId: 91106fcefd2e1eb3f7567f0c9b297d48ebfdf452)
  08-23 15:37:16.824  4641  4641 F DEBUG   :   #02 pc 10e8  
/data/app/~~dMaXGzJksZog7lGF_CEMHQ==/com.arm.v9.demo-C74mlVTkH7NumAtbKGo36A==/base.apk!libdemo.so
 (offset 0x3e9000) (BuildId: 91106fcefd2e1eb3f7567f0c9b297d48ebfdf452)
  08-23 15:37:16.824  4641  4641 F DEBUG   :   #03 pc 0004c2f0  
/apex/com.android.runtime/bin/linker64 (__dl__Z19call_ifunc_resolvery+36) 
(BuildId: 87cff915f050a1eab12b7d6dd7c25a63)
  08-23 15:37:16.824  4641  4641 F DEBUG   :   #04 pc 0005dbd8  
/apex/com.android.runtime/bin/linker64 
(__dl__ZL26process_relocation_generalR9RelocatorRK10elf64_rela.__uniq.153370809355997480299804515629147722701+1500)
 (BuildId: 87cff915f050a1eab12b7d6dd7c25a63)

I would appreciate any ideas what could be wrong in the patch, further 
confirmation if android_get_device_api_level should work from ifunc_resolver, 
or tips for ifunc_resolver debugging on Android :)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158641

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/aarch64-features.c
  compiler-rt/lib/builtins/cpu_model.c


Index: compiler-rt/lib/builtins/cpu_model.c
===
--- compiler-rt/lib/builtins/cpu_model.c
+++ compiler-rt/lib/builtins/cpu_model.c
@@ -894,6 +894,7 @@
 #include 
 
 #if defined(__ANDROID__)
+#include 
 #include 
 #include 
 #elif defined(__Fuchsia__)
@@ -1186,7 +1187,8 @@
   // 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)
\
@@ -1374,6 +1376,21 @@
   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;
@@ -1399,7 +1416,7 @@
   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
Index: clang/test/Driver/aarch64-features.c
===
--- clang/test/Driver/aarch64-features.c
+++ clang/test/Driver/aarch64-features.c
@@ -7,12 +7,18 @@
 // CHECK: fno-signed-char
 
 // Check Function Multi Versioning option and rtlib dependency.
-// RUN: %clang --target=aarch64-linux-android -rtlib=compiler-rt \
+// RUN: %clang --target