Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/android #4452

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Feature/android #4452

wants to merge 3 commits into from

Conversation

gashtio
Copy link

@gashtio gashtio commented Dec 20, 2017 •
edited
Loading

Resolves issue #3966

This change is Reviewable

@msftclas
Copy link

msftclas commented Dec 20, 2017 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

obastemur
obastemur reviewed Dec 20, 2017
View reviewed changes
pal/src/sync/cclock.cpp Outdated

CCLock::CCLock(bool shouldTrackThreadId)
{
char *alignedMutexPtr = AlignPointer(this->mutexPtr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the CCLock lands on unaligned memory, it causes SIGBUS on the next mutex manipulation on Android. I see that the OSX tests failed though...maybe I'll guard the alignment for Android only after all tests are finished.

@obastemur
Copy link
Collaborator

@gashtio Thank you for sending this!

@obastemur obastemur self-assigned this Dec 20, 2017
@gashtio gashtio mentioned this pull request Dec 20, 2017
Closed
@gashtio
Copy link
Author

gashtio commented Dec 20, 2017

Okay, all checks pass now.

@gashtio
Copy link
Author

gashtio commented Jan 15, 2018

@obastemur I did some major changes to the PR since I started testing this more thoroughly and it kept breaking whenever JavascriptStackWalker was involved. Indexing into the ArmStackFrame to find the first argument passed to a function was no good, since it's passed in a register rather than on the stack. I had to inspect the assembly to find if the InterpreterStackFrame::InterpreterThunk places arguments passed in registers in some fixed place with respect to the frame pointer and I was in luck. I still have to do some more tests with different version (and flags) of the compiler to check if this is a viable solution or something more involved is needed.

I also dug a bit deeper into my previous issue that the arm_CallFunction ASM was breaking on Android and found multiple problems with that. First, the signature (and invocations from C++ code) assumed 5 args (JavascriptFunction* function, CallInfo info, uint argCount, Var* values, JavascriptMethod entryPoint), but the ASM code actually obtained the arguments count from info.Count, never used argCount and assumed that the $r2 register points to the values, rather than argCount which made it go haywire. I fixed that and now it works in my few tests on Android (also made the function take care of 0 and 1 arguments because I though I needed some ASM to take care of the stack walking logic, which turned out not to be required, but I left that code anyway).

I'm also pretty sure the Windows ARM build isn't working properly because of all this, but I don't have a device to test on.

Also posted this on Reviewable if that's easier to look at.

I've also ported the library for iOS and currently doing some tests, but it relies on code in this PR. If you have any review comments please let me know so we can move this forward :)

obastemur
obastemur reviewed Jan 20, 2018
View reviewed changes
lib/Runtime/Library/JavascriptFunction.cpp Outdated
@@ -1332,23 +1332,10 @@ void __cdecl _alloca_probe_16()
#endif
Js::Var varResult;

//The ARM can pass 4 arguments via registers so handle the cases for 0 or 1 values without resorting to asm code
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we limit these changes to non WIN32? (including the one above?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted those as you requested (updated reviewable), but they're tied to the hand-written assembly function. As I'm not sure if you meant that you're using the same files for another project or something else, I didn't do changes to the assembly - let me know if you want the old version compiled for Win32.

I actually guarded most of the changes so they're compiled for __ANDROID__ only, but the .asm function seemed broken and I modified it for all platforms.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was only referring to this file (JavascriptFunction.cpp)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right, that's done then.

@obastemur
Copy link
Collaborator

@gashtio great effort here! Thank you!

I have a small change request. Rest LGTM.

@obastemur
Copy link
Collaborator

I'm also pretty sure the Windows ARM build isn't working properly because of all this

Right. Not with ChakraCore.

@gashtio
Added alignment for CCLock structure on Android/iOS to prevent SIGBUS…
6293177 
…S/EXC_ARM_DA_ALIGN errors for misaligned access
@gashtio
Compilation fixes for Android
5716723
@gashtio
ARM uses the asm function arm_CallFunction to call into functions now…
647d93d 
…, fixed signature and several issues

Multiple fixes for handling stack unwinding on ARM (with JavascriptStackWalker).
Resolves issue chakra-core#3966

Fixed a problem with DeferredParsingThunk which was referring to the wrong address for the JS function passed to DeferredParse

Changed the scheme for obtaining the JS function called through walking on ARMv7 the stack to a more reliable one. The callee is now expected to put a specific amount of bytes before the frame pointer, rather than put the register arguments somewhere, which is compiler/flags dependent

Fixed possible crash in Release mode where R9 register was reused and and had incorrect value
@kfarnung
Copy link
Contributor

@gashtio I had some luck in the past testing on a Raspberry Pi 2/3 running Windows IoT Core. I don't think our test suite will run, but some basic testing using ch.exe should be possible.

@gashtio
Copy link
Author

gashtio commented Jan 24, 2018

@kfarnung I'll need this on consoles, so having tests is definitely on my list, just not yet. I've only done some small tests with various JS frameworks that seem to run OK. This is more of a initial port to get the basics up and running, but my idea is to add tests for all new platforms.

@rhuanjl rhuanjl mentioned this pull request Jan 25, 2018
Open
@obastemur obastemur mentioned this pull request Apr 25, 2018
Closed
@rhuanjl rhuanjl mentioned this pull request Apr 19, 2020
Closed
@rhuanjl rhuanjl mentioned this pull request Dec 6, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@obastemur obastemur obastemur left review comments

Assignees
No one assigned
Labels
External Contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gashtio @msftclas @obastemur @kfarnung @dilijev