android: ChakraCore compiles to Android ARMv7 #2224

obastemur · 2016-12-16T15:50:23Z

disclaimer: Android is not officially supported.

obastemur · 2016-12-16T15:52:45Z

Things left to do;

~~fix linker issues and convert couple of MSVC ASM~~ [done]
fix couple of todo items
test on ADB

digitalinfinity

Generally, looks fine to me. Left some comments and didn't review the PAL changes in-depth. Can you also write up something on how to build CC on Android if anyone else wanted to try? Maybe just update our BUILDING file or add it to the wiki?

digitalinfinity · 2016-12-16T19:18:48Z

lib/Common/Core/SysInfo.cpp

@@ -254,7 +254,7 @@ AutoSystemInfo::SSE2Available() const
    return false; // TODO: xplat support
 #endif
 #else
-    #error Unsupported platform.
+    return false;


I'd leave the error here and add an explicit ifdef for ARMv7/Android here so that anyone adding new platform support is aware that this function is something they need to think about

@digitalinfinity if it's not Intel, IMO SSE2Available() should return false. What other possibilities here?

digitalinfinity · 2016-12-16T19:20:15Z

lib/Common/CommonPal.h

@@ -637,9 +648,9 @@ namespace PlatformAgnostic
 #endif
    }

-    __forceinline unsigned char _BitTest(const LONG *_BitBase, int _BitPos)
+    __forceinline unsigned char _BitTest(LONG *_BitBase, int _BitPos)


Why was the const removed here? seems like allowing for const can result in better codegen by the compiler- if it's android specific, I'd prefer doing a const_cast when _bittest is called scoped to Android

Clang intrinsic definition has no const.

digitalinfinity · 2016-12-16T19:22:05Z

lib/Common/CommonPal.h

@@ -621,7 +632,7 @@ namespace PlatformAgnostic
 {
    __forceinline unsigned char _BitTestAndSet(LONG *_BitBase, int _BitPos)
    {
-#if defined(__clang__)
+#if defined(__clang__) && !defined(_ARM_)


My ARM fu is weak, so a basic question here- is ARM the same for Windows ARM vs Android ARM v7? Should it be? I know that Windows supports Thumb2, which I don't know if Android supports so the Windows ARM code might end up being slightly different from the Android ARM- I'm not sure what the macro model here should look like. @pleath @satheeshravi thoughts?

+1, also there are ARMv7 and ARMv8, 32-bit and 64-bit, we need to be careful to cover them correctly.

In reply to: 92870165 [](ancestors = 92870165)

@digitalinfinity below code is for x86 (bts is an x86 instruction). and when read the comment, we use intrinsic here for ARM 32 bit instead.

@ThomsonTan intrinsic below should work for ARMv8 too.

digitalinfinity · 2016-12-16T19:23:49Z

lib/Runtime/Language/Arguments.h

 // We use a custom calling convention to invoke JavascriptMethod based on
-// System V AMD64 ABI. At entry of JavascriptMethod the stack layout is:
+// System ABI. At entry of JavascriptMethod the stack layout is:


Interesting- are the calling conventions in the ABIs same for x86/AMD64/ARM on Linux?

AddressOfReturnAddress is intrinsic

digitalinfinity · 2016-12-16T19:24:24Z

lib/Runtime/Language/Arguments.h

@@ -75,6 +70,10 @@ inline int _count_args(const T1&, const T2&, const T3&, const T4&, Js::CallInfo
 #define CALL_ENTRYPOINT(entryPoint, function, callInfo, ...) \
    entryPoint(function, callInfo, nullptr, nullptr, nullptr, nullptr, \
               function, callInfo, ##__VA_ARGS__)
+#elif defined(_ARM_)
+// xplat-todo: fix me


Can you add some more description here? What needs to be fixed?

This part is likely to fail for different targets. I will turn back to here (hence todo note)

updated this to fix me ARM

digitalinfinity · 2016-12-16T19:25:50Z

lib/Runtime/Library/JavascriptError.cpp

@@ -371,7 +371,9 @@ namespace Js

        if (FACILITY_CONTROL == HRESULT_FACILITY(hr) || FACILITY_JSCRIPT == HRESULT_FACILITY(hr))
        {
+#ifndef __ANDROID__


What is this change for? Does this function work xplat? It looks like its mostly getting windows specific errors so perhaps the entire if condition shouldn't disabled for Android, no?

argList != nullptr is wrong. I'm not sure how this works or if it works at all. Clearly it doesn't work with Clang 3.8 abi-arm-android.

entire if condition shouldn't disabled for Android

Sounds Good. Thanks

Actually, shouldn't we consider argList will never be null and keep it as is here?

digitalinfinity · 2016-12-16T19:28:15Z

pal/src/configure.cmake

@@ -1,3 +1,5 @@
+if (NOT CC_TARGET_OS_ANDROID)


On this note- have we cleaned up this file to make sure that it's checking for stuff that ChakraCore actually needs? When I imported the PAL, I didn't bother to clean this up but if our PAL is diverging, can you make a note to clean up this file at some point?

PAL uses this file to feature detect on target. This is not an option when cross-compiling the code.

can you make a note to clean up this file at some point?

Good idea, will do. Thanks

added extra notes

obastemur · 2016-12-17T12:04:48Z

Can you also write up something on how to build CC on Android if anyone else wanted to try? Maybe just update our BUILDING file or add it to the wiki?

@digitalinfinity we need #2224 (comment) steps are done for anyone to experience ChakraCore literally.

obastemur · 2016-12-20T14:58:31Z

Steps to compile:

Download Android NDK and extract
Build/android_toolchain.sh
./build.sh --target=android --arch=arm --no-icu --static --debug --no-jit

After some updates to this PR, currently there are only few linker errors left.

@digitalinfinity do you have any additional comments?

obastemur · 2016-12-21T13:16:05Z

Android ARMv7 linker issues are also fixed (along with the implementation of ChakraCore ASM files)

obastemur · 2016-12-21T16:10:47Z

Also targets RaspberryPI (see #2262 for additional requirements)

digitalinfinity · 2016-12-21T22:14:28Z

No more comments from my side- thanks @obastemur

digitalinfinity · 2016-12-21T22:23:24Z

LGTM

obastemur · 2016-12-21T22:30:48Z

@digitalinfinity @ThomsonTan Thanks for the Reviews!!

Merge pull request #2224 from obastemur:exp disclaimer: Android is not officially supported.

msftclas added the cla-already-signed label Dec 16, 2016

obastemur requested review from digitalinfinity and jianchun December 16, 2016 15:54

digitalinfinity reviewed Dec 16, 2016

View reviewed changes

obastemur force-pushed the exp branch 5 times, most recently from dbb616d to 1aa8826 Compare December 20, 2016 14:55

obastemur force-pushed the exp branch 4 times, most recently from 3d50742 to b95e5d9 Compare December 21, 2016 13:14

obastemur force-pushed the exp branch 3 times, most recently from 7e01db0 to 467795c Compare December 21, 2016 16:09

obastemur force-pushed the exp branch 2 times, most recently from 6a6c50b to 7e65d01 Compare December 21, 2016 16:12

android: ChakraCore compiles to Android ARMv7

6811584

obastemur force-pushed the exp branch from 7e65d01 to 6811584 Compare December 21, 2016 16:21

chakrabot merged commit 6811584 into chakra-core:master Dec 21, 2016

chakrabot pushed a commit that referenced this pull request Dec 21, 2016

[MERGE #2224 @obastemur] android: ChakraCore compiles to Android ARMv7

195604c

Merge pull request #2224 from obastemur:exp disclaimer: Android is not officially supported.

This was referenced Aug 21, 2023

[Snyk] Upgrade yargs from 16.2.0 to 17.7.2 Woodpile37/ChakraCore#1

Merged

[Snyk] Upgrade yargs from 16.2.0 to 17.7.2 Woodpile37/ChakraCore#6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

android: ChakraCore compiles to Android ARMv7 #2224

android: ChakraCore compiles to Android ARMv7 #2224

obastemur commented Dec 16, 2016 •

edited

Loading

obastemur commented Dec 16, 2016 •

edited

Loading

digitalinfinity left a comment

digitalinfinity Dec 16, 2016

obastemur Dec 17, 2016

digitalinfinity Dec 16, 2016

obastemur Dec 17, 2016

digitalinfinity Dec 16, 2016

ThomsonTan Dec 16, 2016

obastemur Dec 17, 2016

digitalinfinity Dec 16, 2016

obastemur Dec 17, 2016

digitalinfinity Dec 16, 2016

obastemur Dec 17, 2016

obastemur Dec 19, 2016

digitalinfinity Dec 16, 2016

obastemur Dec 17, 2016

obastemur Dec 19, 2016

digitalinfinity Dec 16, 2016

obastemur Dec 17, 2016

obastemur Dec 19, 2016

obastemur commented Dec 17, 2016

obastemur commented Dec 20, 2016

obastemur commented Dec 21, 2016

obastemur commented Dec 21, 2016

digitalinfinity commented Dec 21, 2016

digitalinfinity commented Dec 21, 2016

obastemur commented Dec 21, 2016

android: ChakraCore compiles to Android ARMv7 #2224

android: ChakraCore compiles to Android ARMv7 #2224

Conversation

obastemur commented Dec 16, 2016 • edited Loading

obastemur commented Dec 16, 2016 • edited Loading

digitalinfinity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obastemur commented Dec 17, 2016

obastemur commented Dec 20, 2016

obastemur commented Dec 21, 2016

obastemur commented Dec 21, 2016

digitalinfinity commented Dec 21, 2016

digitalinfinity commented Dec 21, 2016

obastemur commented Dec 21, 2016

obastemur commented Dec 16, 2016 •

edited

Loading

obastemur commented Dec 16, 2016 •

edited

Loading