1308780 - Unnecessary version check in ProcessHasSignalHandlers when minimal API level >= 19

Status: NEW

(Core :: JavaScript Engine: JIT, defect, P5)

Description

[Makoto Kato [:m_kato]]

On Andorid 5.0 64-bit (api level >= 21) doesn't have __system_property_get into NDK's library file.  So we has to use it via dynamic link when we support aarch64 binary.

But this check is <= 4.4, so it is unnecessary version check in ProcessHasSignalHandlers when minimal API level >= 19.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Unnecessary version check in ProcessHasSignalHandlers when minimal API level >= 19

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8799246 [details] [diff] [review]
Unnecessary version check in ProcessHasSignalHandlers when minimal API level >= 19

Review of attachment 8799246 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! Although I have a question for you.

::: js/src/asmjs/WasmSignalHandlers.cpp
@@ +1289,5 @@
>      // EINTRquisition.
>      char version_string[PROP_VALUE_MAX];
>      PodArrayZero(version_string);
>      if (__system_property_get("ro.build.version.sdk", version_string) > 0) {
>          if (atol(version_string) < 19)

Double-check me here, but isn't your static #ifdef doing exactly the same as the above dynamic check does? If you worry that this check might be slow, there's no worry (it's done only once per execution of the embedding).

Comment 3

[Makoto Kato [:m_kato]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> Comment on attachment 8799246 [details] [diff] [review]
> Unnecessary version check in ProcessHasSignalHandlers when minimal API level
> >= 19
> 
> Review of attachment 8799246 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch! Although I have a question for you.
> 
> ::: js/src/asmjs/WasmSignalHandlers.cpp
> @@ +1289,5 @@
> >      // EINTRquisition.
> >      char version_string[PROP_VALUE_MAX];
> >      PodArrayZero(version_string);
> >      if (__system_property_get("ro.build.version.sdk", version_string) > 0) {
> >          if (atol(version_string) < 19)
> 
> Double-check me here, but isn't your static #ifdef doing exactly the same as
> the above dynamic check does? If you worry that this check might be slow,
> there's no worry (it's done only once per execution of the embedding).

To use __system_property_get on Android/aarch64, we have to use dlload("libc.so", ...) and dlsym(..., "__system_property_get") such as chromium (https://codereview.chromium.org/393923002).  OK, I will change to it instead.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

I would not be too comfortable doing these reviews as they relate to the code loading / build system. I would recommend asking review to :glandium or :gps for this incoming patch.

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Bug 1308780 - __system_property_get isn't defined in API level 21 of NDK.

From API level 21, __system_property_get isn't defined into NDK's libc.  So if API level 21 of NDK >= 21 like 64-bit Android, we use dlsym.

Review commit: https://reviewboard.mozilla.org/r/109994/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109994/

Comment 6

[Luke Wagner [:luke]]

I'd be curious if, when __ANDROID_API__ >= 21, it's even possible for ro.build.version.sdk to be < 19.

Comment 7

[Mike Hommey [:glandium]]

(In reply to Luke Wagner [:luke] from comment #6)
> I'd be curious if, when __ANDROID_API__ >= 21, it's even possible for
> ro.build.version.sdk to be < 19.

One is build-time, the other is runtime. The main question is whether something built with __ANDROID_API__ >= 21 can run on older systems, and IIRC, they likely don't, although they might, if you're lucky.

But, yeah, I think the code using __system_property_get can just be #if'ed away with newer __ANDROID_API__.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8833884 [details]
Bug 1308780 - __system_property_get isn't defined in API level 21 of NDK.

https://reviewboard.mozilla.org/r/109994/#review111418