Use API 16 for armv7, x86 as current react-native spec #66

Kudo · 2018-11-07T18:02:07Z

One concern for jsc-android-buildscripts is about its minSdkVersion bumped to 21.
This PR follows current React Native Android's spec.

For armv7 and x86, the minimal API is 16.
For arm64 and x86_64, the minimal API is 21. (Android introduce 64 bits support after API 21).

DanielZlotin · 2018-11-07T20:18:58Z

Interesting approach. I'm not sure this is a good idea, implementing C getline yourself differently than what the POSIX api specifies.

Is this the only function missing from ndk < 21?
My guess is users trying to build with min api 16 will crash on x64 due to missing functions

newyankeecodeshop · 2018-11-07T20:53:53Z

How about changing the min API for 32-bit to 18, so that at least getline is available. Then figure out how to patch it to support API 16-17 in a later PR. Maybe it can be enabled via compiler flags, since it's a POSIX API.

Kudo · 2018-11-08T02:14:26Z

@DanielZlotin
As comment, the getline() only used for reading content from /proc/self/smaps.
Even though the implementation is different from POSIX, but it's good enough.
But you reminds me to add a "static" to prevent the symbol pollute out, will revise the PR for this later.

There are some libc implementation for your reference.
I think most of them are both based on complicated getdelim().
(To paste link easier, these might not be official repo but a third party mirror)

[glibc] https://github.com/lattera/glibc/blob/master/stdio-common/getline.c
based on libio https://github.com/lattera/glibc/blob/master/libio/iogetline.c
[eglibc] Pretty much like glibc implementation
[NDK] I don't find the source code from NDK but guess it is also like glibc implementation.
[musl]
https://github.com/ifduyue/musl/blob/master/src/stdio/getline.c
https://github.com/ifduyue/musl/blob/master/src/stdio/getdelim.c
[FreeBSD also provide getline]
https://github.com/freebsd/freebsd/blob/master/lib/libc/stdio/getline.c
https://github.com/freebsd/freebsd/blob/master/lib/libc/stdio/getdelim.c

I do see most of implementation in order to do getdelim() and have a complicated code.
So I turned to use simple fgets() in final and think it's good enough for /proc/self/smaps parsing.

For your further questions:

Is this the only function missing from ndk < 21?
As far as I know, it is the only one.

It is a try-error workflow. After patching this function, I can build on top of API 16 without other problems.

My guess is users trying to build with min api 16 will crash on x64 due to missing functions

Android introduced 64 bits support after API 21, so that x64 min API will be 21.
I think it will not be a problem.

Kudo · 2018-11-08T02:17:21Z

@newyankeecodeshop I just hope if jsc-android-buildscripts could be merged into official RN upstream.
Let's discuss more rounds for getline() concern here. If we don't come out a good solution in the mean time, I will separate API 16 support in another PR.

Kudo · 2018-11-08T02:25:22Z

Some updates:

Update PR to add static symbol protection that may prevent pollute outside.
I tested RNTester with my built JSC without problems from these devices.

Galaxy Nexus: Android 4.3 arm
Pixel: Android 7.1 arm64

newyankeecodeshop · 2018-11-08T11:57:42Z

@Kudo This is unlikely to be merged in to RN upstream because RN is de-coupling the JavaScript runtime via a new "JSI" subsystem. They want to make it easier for people to use their own version of JavaScriptCore, or even use V8.
Is supporting Android 4.1-4.2 that important at this stage? I think having a minimum API of 18 means that there are no edge cases to worry about when parsing /proc/self/smaps on various devices. The Android ecosystem is vast, and it's hard to predict how this code will work on all these different devices. I think supporting API 16-17 should be handled separately, maybe by having people link their own implementation of getline().

kmagiera

Hey @Kudo ! Awesome job on this one. I'm fine with most of the changes and just have a suggestion re missing getline method. There is a chance we can use the builtin one just by providing declaration that's missing from the public headers in android-16 NDK. Can you try that approach and let me know how it goes?

kmagiera · 2018-11-08T14:41:56Z

patches/jsc.patch

+//     This is only for forEachLine() use.
+//     Just don't want to do strlen() calculating length again.
+//
+static ssize_t getline(char** linep, size_t* linecapp, FILE* stream) {


I checked libc binaries from android-16 NDK and found getline symbols there. So there is a chance that getline definition is only missing from the headers. I suggest (and we used similar trick in the past several times) to try providing only a declaration of getline function which then should be linked with the implementation present in libc:

extern "C" ssize_t getline(char** linep, size_t* linecapp, FILE* stream);

@kmagiera This is awesome that I did not think before.
BTW how do you check getline existed in android-16 NDK?
I cannot find it on my machine 😓

> pwd android-ndk-r17c > toolchains/arm-linux-androideabi-4.9/prebuilt/darwin-x86_64/bin/arm-linux-androideabi-readelf -Ws platforms/android-16/arch-arm/usr/lib/libc.so|grep getline > toolchains/arm-linux-androideabi-4.9/prebuilt/darwin-x86_64/bin/arm-linux-androideabi-readelf -Ws platforms/android-18/arch-arm/usr/lib/libc.so|grep getline 10: 000081b5 2 FUNC GLOBAL DEFAULT 7 getline@@LIBC 1241: 000081b5 2 FUNC GLOBAL DEFAULT 7 getline

I used nm instead of readelf but that shouldn't matter. Also, I only checked static version of libc (that is libc.a) but now I see the dynamic version doesn't have that symbol

So sad. It would be great if getline is just a hidden symbol.

Kudo · 2018-11-08T16:25:30Z

Let me propose one more favorable to getline fallback.
If the content in /proc/self/smaps is unexpected, the parser in JSC will fail IMHO.
https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/linux/MemoryFootprintLinux.cpp#L57-L76

    forEachLine(file, [&] (char* buffer) {
        {
            ...
            int scannedCount = sscanf(buffer, "%lx-%lx %4s %lx %31s %lu %6s", &start, &end, perms, &offset, dev, &inode, path);
            if (scannedCount == 6) {
                isAnonymous = true;
                return;
            }

Kudo · 2018-11-09T00:30:42Z

I finally found the getline implementation in Android.

https://android.googlesource.com/platform/bionic/+/master/libc/stdio/stdio.cpp#870
https://android.googlesource.com/platform/bionic/+/master/libc/upstream-openbsd/lib/libc/stdio/getdelim.c

It's from OpenBSD and the dependency seems less than glibc (glibc based on libio)
If there are concerns about fgets, I could turned to use this one from Android instead.
Any thoughts?

kmagiera · 2018-11-09T10:32:27Z

Thanks for looking more into it @Kudo

Do you know how to trigger that JSC code which uses getline? So that we can test if it works ok?

Another idea that I had is we could try using fgetln to implement getline. fgetln is available on android-16+ (https://android.googlesource.com/platform/bionic/+/android-4.1.2_r2/libc/stdio/fgetln.c#71)

This would look sth like this (I havent compiled this code, just posting to hopefully make it clear):


extern "C" char * fgetln(FILE *fp, size_t *lenp)

ssize_t getline(char** buf, size_t* n, FILE* fp) {
  size_t len = 0;
  char *stdiobuf = fgetln(fp, &len);
  if (stdiobuf == NULL || len == 0) return -1;
  *buf = malloc(len);
  if (*buf == NULL) return -1;
  memcpy(*buf, stdiobuf, len);
  *buf[len - 1] = '\0';
  return len;
}

kmagiera · 2018-11-09T11:56:36Z

It appears there are other issues when running your build on Android 4.1 as some other symbols are missing. Specifically we've run into an issue with missing log2 symbol when tried to run this on Android 4.1 (this problem does not surface when running on 4.3 as different version of libm is available there and it has log2).

See output from nm for android-18 and android-16:

➜  r17b  toolchains/arm-linux-androideabi-4.9/prebuilt/darwin-x86_64/arm-linux-androideabi/bin/nm platforms/android-18/arch-arm/usr/lib/libm.so| grep log2                  (11-09 12:53)
00001740 T log2
00001740 T log2f
00001740 T log2l
➜  r17b  toolchains/arm-linux-androideabi-4.9/prebuilt/darwin-x86_64/arm-linux-androideabi/bin/nm platforms/android-16/arch-arm/usr/lib/libm.so| grep log2                  (11-09 12:53)
[ Exit 1 ]

We addressed that problem in the original version of JSC buildscripts this way: https://github.com/facebook/android-jsc/blob/master/jsc/BUCK#L39

Not sure what other problems we may encounter with this change but we should make sure to test it on 4.1 devices/emulators before we merge your changes in.

newyankeecodeshop · 2018-11-11T22:58:46Z

@Kudo @kmagiera Given the uncertainty around the missing symbols, what about just having this PR enable support for Android 18 and later? This is a great enhancement that is worth merging without being held up by uncertainties around Android 16-17 APIs IMO.

Kudo · 2018-11-12T01:39:45Z

@kmagiera Thanks for your help of the log2 missing symbol.
ff87512 updates for this.
Sorry that I haven't tested on API 16 emulator before.
I was curious as well why I didn't face the log2 compile error as it did while worked on JSC r174650 clang build.
Now I know there are some different compile options between CMake and BUCK.

To address the issue, I first to add -Wl,--no-undefined to ensure there is the log2 undefined symbol error during compile time.
And then add the log2 polyfill, so I could make sure there are no further unresolved symbol.
I've tested on Android 4.1 emulator and it will not crash this time.

Regarding back to the getline issue, I digged the WebKit source code and seems JavaScriptCore does not use it.
It has been used in WebCore and WebKit multi-processes.
I will try to adopt fgetln in later update.

Kudo · 2018-11-12T01:41:21Z

@newyankeecodeshop I've updated the PR and fix the missing log2 issue. It could run on Android 4.1 now.

Kudo · 2018-11-12T08:37:24Z

@kmagiera I wrote a POC code for fgetln backed version.
https://gist.github.com/Kudo/299492cb03e910df8a1201e9b530f4ca#file-getline-cpp-L20
There is also a simple test case inside the gist.

Please take a look first and see if it is okay to do it within JSC.
There are still some differences between the formal OpenBSD implementation of getline.
OpenBSD getline will grow up buffer length by Math.pow(n, 2).
I just realloc() to exact buffer size.

kmagiera · 2018-11-12T09:48:07Z

Hey @Kudo ! Thanks so much for addressing all my comments and sorry for the back and forth on this.

We tested your recent changes and everything appears to be working ok. But one thing we noticed is that MemoryFootprint methods are missing from the binary output. Which suggest (and you mentioned that earlier) that these methods aren't really being used. Given that I'd suggest that we don't add any implementation of getline if it is not being used and instead turn off compiling MemoryFootprintLinux.cpp completely (either by adding IFDEF macros around it or changing sources list). This will make memoryFootprint symbol be missing, but since it is not being used by the JSC library the no-undefined error you added will not raise an error and everything should work well.

patches/jsc.patch

newyankeecodeshop · 2018-11-12T16:01:17Z

scripts/compile/common.sh

@@ -151,7 +153,7 @@ ICU_CXXFLAGS="$COMMON_CXXFLAGS $ICU_CFLAGS -Oz"
 ICU_LDFLAGS="$COMMON_LDFLAGS $PLATFORM_LDFLAGS -s"

 JSC_LDFLAGS="$COMMON_LDFLAGS"
-JSC_CFLAGS="$COMMON_CFLAGS -DU_STATIC_IMPLEMENTATION=1 -DU_SHOW_CPLUSPLUS_API=0"
+JSC_CFLAGS="$COMMON_CFLAGS -DU_STATIC_IMPLEMENTATION=1 -DU_SHOW_CPLUSPLUS_API=0 -Wno-tautological-constant-compare"


What is this additional warning for?

Just want to suppress warning like that:

../../Source/WTF/wtf/text/StringImpl.h:967:18: warning: comparison 'size_t' (aka 'unsigned int') > 4294967295 is always false [-Wtautological-constant-compare] if (size > std::numeric_limits<unsigned>::max()) ~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I check the warning existed in previous build log from Jenkins CI.
Let me remove this and keep changes simple. Thank for your comment.

Kudo · 2018-11-12T16:06:52Z

@kmagiera The idea of eliminating MemoryFootprint makes sense to me.
I've updated the PR, please take a look as well.
Thank you so much to review my code. I learn a lot from the review. 🙇

kmagiera

Great job @Kudo!

I'm accepting this and would love to hear if anyone else has some feedback before I merge this in (not sure if @DanielZlotin has time and availability to take a look)

kmagiera · 2018-11-12T17:10:31Z

Thanks @newyankeecodeshop for your comments 🙌

kmagiera · 2018-11-12T17:17:40Z

Also @DanielZlotin if you get a minute I wanted to give a tldr to respond to your previous comment. I believe this approach is correct cause 64bit builds use different sdk (21) for compiling (with precious sdk versions 64 architecture weren't supported). I think it is actually the best approach if we want to support all cpus with a single set of binaries (an alternative would be to have separate binaries per sdk which would complicated apk splitting, note that cpu splits are now automatic)

As for getline we managed to get rid of it as the code which was using turned out to be unused.

We tested @Kudo patch internally on Android 4.1 and arm64 devices. And will repeat the test once more before merging. I'm pretty confident about this solution and looking forward to it helping with upgrading jsc for all RN users.

Kudo · 2018-11-13T01:21:26Z

@kmagiera Thank you for reviewing my code with much patience.
I really appreciate that you always try to think up better solutions.
The final outcome to remove MemoryFootprint implementation is awesome to me. Less is More 👍

As the changes back and forth few times, please let me know if I should rebase and squash my commits before landed.

DanielZlotin · 2018-11-13T20:02:04Z

@newyankeecodeshop I just hope if jsc-android-buildscripts could be merged into official RN upstream.

working on it ;)

DanielZlotin · 2018-11-13T20:06:22Z

@kmagiera Thank you for reviewing my code with much patience.
I really appreciate that you always try to think up better solutions.
The final outcome to remove MemoryFootprint implementation is awesome to me. Less is More 👍

As the changes back and forth few times, please let me know if I should rebase and squash my commits before landed.

Yes please rebase

Summary: While building arm7, x86, use API 16. While building arm64, x86_64, use API 21. (Android introduced 64 bits support after API 21). There are some patches for missing symbols from NDK API 16. 1. log2() is supported until API 18. Solved this by introduced a log2() polyfill. 2. getline() is supported until API 18. As the WebKit WTF code is not being used in JSC, just comment out it. For discussion details, see the PR commants: react-native-community#66 (comment) 3. Add '-Wl,--no-undefined' ldflags to prevent missing NDK symbols in the future.

Kudo · 2018-11-14T07:41:08Z

@DanielZlotin rebase done, please take a look. Thank you.

DanielZlotin · 2018-11-15T02:00:28Z

Testing...

TareqElMasriDev · 2018-11-15T21:08:26Z

When is this going to see the light?

kmagiera · 2018-11-15T21:44:20Z

I messed up something with version numbers on CI and even so the builds from Circle are Ok (tested them on a number of different devices and emulator + on firebase test lab) I'll have to fix CirecleCI versioning before publishing this to npm.

Merging this now and will publish an update likely tomorrow! Thanks @Kudo for working on this 🙌

hey99xx · 2018-11-16T00:29:25Z

Should minSdkVersion related part of the README be deleted?

kmagiera · 2018-11-16T08:04:28Z

yes, definitely, will do it as soon as new version is published on NPM

kesha-antonov · 2019-04-01T09:01:42Z

Thank you!

kmagiera requested changes Nov 8, 2018

View reviewed changes

kmagiera reviewed Nov 12, 2018

View reviewed changes

patches/jsc.patch Outdated Show resolved Hide resolved

newyankeecodeshop reviewed Nov 12, 2018

View reviewed changes

kmagiera approved these changes Nov 12, 2018

View reviewed changes

DanielZlotin approved these changes Nov 13, 2018

View reviewed changes

hey99xx mentioned this pull request Nov 13, 2018

Update gradle and mk files facebook/react-native#22231

Closed

Kudo force-pushed the api_16_for_32bits branch from d94c6e5 to 28c6384 Compare November 14, 2018 07:39

gengjiawen approved these changes Nov 14, 2018

View reviewed changes

gengjiawen mentioned this pull request Nov 15, 2018

Raise out android-jsc issues in current master facebook/react-native#22274

Closed

3 tasks

DanielZlotin approved these changes Nov 15, 2018

View reviewed changes

Merge branch 'master' into api_16_for_32bits

aa0c18d

Merge branch 'master' into api_16_for_32bits

efbd92a

kmagiera merged commit c73a538 into react-native-community:master Nov 15, 2018

Use API 16 for armv7, x86 as current react-native spec #66

Use API 16 for armv7, x86 as current react-native spec #66

Uh oh!

Conversation

Kudo commented Nov 7, 2018

Uh oh!

DanielZlotin commented Nov 7, 2018

Uh oh!

newyankeecodeshop commented Nov 7, 2018

Uh oh!

Kudo commented Nov 8, 2018

Uh oh!

Kudo commented Nov 8, 2018

Uh oh!

Kudo commented Nov 8, 2018

Uh oh!

newyankeecodeshop commented Nov 8, 2018

Uh oh!

kmagiera left a comment

Choose a reason for hiding this comment

Uh oh!

kmagiera Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

Kudo Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

kmagiera Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

Kudo Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

Kudo commented Nov 8, 2018

Uh oh!

Kudo commented Nov 9, 2018

Uh oh!

kmagiera commented Nov 9, 2018

Uh oh!

kmagiera commented Nov 9, 2018

Uh oh!

newyankeecodeshop commented Nov 11, 2018

Uh oh!

Kudo commented Nov 12, 2018

Uh oh!

Kudo commented Nov 12, 2018

Uh oh!

Kudo commented Nov 12, 2018

Uh oh!

kmagiera commented Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

newyankeecodeshop Nov 12, 2018

Choose a reason for hiding this comment

Uh oh!

Kudo Nov 12, 2018

Choose a reason for hiding this comment

Uh oh!

Kudo commented Nov 12, 2018

Uh oh!

kmagiera left a comment

Choose a reason for hiding this comment

Uh oh!

kmagiera commented Nov 12, 2018

Uh oh!

kmagiera commented Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kudo commented Nov 13, 2018

Uh oh!

DanielZlotin commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanielZlotin commented Nov 13, 2018

Uh oh!

Kudo commented Nov 14, 2018

Uh oh!

DanielZlotin commented Nov 15, 2018

Uh oh!

kmagiera commented Nov 12, 2018 •

edited

Loading

kmagiera commented Nov 12, 2018 •

edited

Loading

DanielZlotin commented Nov 13, 2018 •

edited

Loading

hey99xx commented Nov 16, 2018 •

edited

Loading

kmagiera commented Nov 16, 2018 •

edited

Loading