Support macOS ARM64 (Apple silicon) without JIT #6869

rhuanjl · 2023-01-11T17:45:44Z

This is a working port of ChakraCore for Apple Silicon, though jit support requires further work and will be left for a follow up PR.

Work done:

Some updates were brought over from the dotNet GitHub repository to enable ARM64 support in pal, however as CC's PAL is significantly diverged from dotNet PAL this was done on a file by file basis with edits/tweaks to integrate new code with the existing source tree. Some of the new material is licensed by the dotNET foundation rather than microsoft - I've written a new license block for PAL related files to try and cover this and added the dotNET license to the THIRD-PARTY-NOTICES file
Updated build/config files
Worked through platform dependent logic within CC to update as needed, this has particularly included changes/new code around function call logic and argument handling
Made updates to memory management to account for Apple Silicon's use of a 16kb memory page size (rather than 4kb)
Set up ARM64 CI using Cirrus CI

ref: #6860

@ppenzin

rhuanjl · 2023-01-26T18:10:45Z

This may be just about there for --no-jit We still don't have online ARM64 CI but... here's the offline test results:

############# ChakraCore Test Suite #############
Testing Debug build
Using 8 threads

############# Starting disable_jit tests #############
  exclude: exclude_interpreted
  exclude: slow
  exclude: exclude_xplat
  exclude: fails_disable_jit
  exclude: fails_interpreted
  exclude: require_winglob
  exclude: require_simd
  exclude: exclude_x64
  exclude: exclude_debug
  exclude: exclude_disable_jit
  exclude: exclude_amd64
  exclude: fail
  exclude: exclude_mac
  exclude: require_backend

############ Results for disable_jit tests ###########
262: passed 1, failed 0
Array: passed 124, failed 0
Basics: passed 51, failed 0
BigInt: passed 11, failed 0
Boolean: passed 3, failed 0
Bugs: passed 102, failed 0
Closures: passed 29, failed 0
ControlFlow: passed 17, failed 0
Conversions: passed 5, failed 0
Date: passed 10, failed 0
Debugger: passed 19, failed 0
DebuggerCommon: passed 207, failed 0
DynamicCode: passed 1, failed 0
EH: passed 40, failed 0
Error: passed 25, failed 0
Function: passed 77, failed 0
GlobalFunctions: passed 14, failed 0
InlineCaches: passed 24, failed 0
Intl: passed 16, failed 0
JSON: passed 16, failed 0
LetConst: passed 63, failed 0
Lib: passed 11, failed 0
Math: passed 9, failed 0
Miscellaneous: passed 10, failed 0
Number: passed 11, failed 0
Object: passed 65, failed 0
Operators: passed 29, failed 0
Prototypes: passed 9, failed 0
RWC: passed 1, failed 0
RegAlloc: passed 1, failed 0
Regex: passed 41, failed 0
Scanner: passed 3, failed 0
StackTrace: passed 18, failed 0
Strings: passed 43, failed 0
TTBasic: passed 142, failed 0
TTExecuteBasic: passed 28, failed 0
TaggedIntegers: passed 23, failed 0
TryCatch: passed 1, failed 0
UnifiedRegex: passed 22, failed 0
UnitTestFramework: passed 1, failed 0
es5: passed 59, failed 0
es6: passed 244, failed 0
es6module: passed 31, failed 0
es7: passed 24, failed 0
fieldopts: passed 122, failed 0
loop: passed 10, failed 0
strict: passed 107, failed 0
switchStatement: passed 26, failed 0
typedarray: passed 60, failed 0
utf8: passed 9, failed 0
----------------------------
Total: passed 2015, failed 0
Time taken for disable_jit tests: 227.45 seconds

rhuanjl · 2023-02-08T17:32:36Z

Found and enabled working arm CI (Cirrus CI) - will need additional configurations, currently it's just testing a no-jit Test build. Everything is looking promising.

The Jit still needs some work - the calling convention is different so the function prologue and epilogue code is not usable nor is the code for generating a Call. Hopefully total diff to fix this will only be ~200 lines but it's complex stuff.

pleath · 2023-02-08T22:07:19Z

I don't know whether this has come up before, but to run jitted code on a non-Windows target the JIT will also need to generate whatever exception info the ABI requires, i.e., the equivalent of pdata + xdata.

rhuanjl · 2023-02-08T22:27:36Z

I don't know whether this has come up before, but to run jitted code on a non-Windows target the JIT will also need to generate whatever exception info the ABI requires, i.e., the equivalent of pdata + xdata.

I'd seen in the source tree that I'd need to look at this but haven't given it much thought yet - I have been (perhaps naively) hoping that it will not be too hard to figure out once I have the basic functionality of the jit working - the relevant logic is there for apple/linux on amd64 which I hope to be able to model off of.

rhuanjl · 2023-02-10T08:43:02Z

FYI for anyone following this, considering the size/scale of this I'm going to try and clean it up, sort out the license and style issues (and possibly get it to work on linux arm64 too which should be a small step) then land it without JIT support for now, I intent to continue to work on the JIT as a follow up.

- Some material brought in from PAL at github.com/dotnet/ - Various manual changes to fit with existing codebase - Updated licensing in edited files, latest PAL is copyright .dotNet - Updated copyright check scripts to check pal/ files for new text

- Memory page size on apple arm64 is 16kb - This change caused ushort overflows - minor edits to orders of operations to fix - New assembler file for arm64_SAVE_REGISTERS

rhuanjl · 2023-02-14T18:36:00Z

@ppenzin I have had some difficulty getting a functional arm64 linux vm going so leaving this as just macOS arm64 support for now - also as discussed, leaving the JIT for the time being.

Allowing for linux support and Jit support as future work I think this is ready.
Please could you review what's here?

ppenzin

PAL LGTM

Edit: oops, I thought I was just going through PAL change, hold on.

Haven't finished reviewing

ppenzin

Trying new tactic of reviewing commits one by one.

PAL and Memory LGTM overall, but have some questions about the Memory commit.

ppenzin · 2023-03-10T06:15:35Z

lib/Common/Core/SysInfo.h

@@ -62,7 +63,11 @@ class AutoSystemInfo : public SYSTEM_INFO
 #ifdef _WIN32
    static HMODULE GetCRTHandle();
 #endif
+#if defined(__APPLE__) && defined(_M_ARM64)
+    static DWORD const PageSize = 16384;


Nit: are there any consequences to hardcoding the page size? I think base class should contain the page size (this might be out of scope of this review).

I was staying consistent with how it was before, it may not be ideal but it's not a change.

ppenzin · 2023-03-10T06:33:47Z

lib/Common/Memory/CustomHeap.cpp

@@ -294,7 +295,11 @@ BOOL Heap<TAlloc, TPreReservedAlloc>::ProtectAllocationWithExecuteReadWrite(Allo
    }
    else
    {
+        #if defined(__APPLE__) && defined(_M_ARM64)
+        protectFlags = PAGE_READWRITE; // PAGE_EXECUTE_READWRITE banned on Apple Silicon


Side note: this would be trouble for JIT in the future

I was worried about it but I'm not sure if it will be a problem, I was able to get some run time produced code to run by setting a page to READWRITE, writing it and then switching to EXECUTE_READ before execution.

Ah, that's great, and it is actually a safer way to do this. Write+execute permissions provide an easier entry for JOP/ROP exploits (not that EXECUTE_READ is completely bulletproof, but it slows potential attacks somewhat).

Maybe we should explore using that on other platforms as well.

ppenzin · 2023-03-10T06:37:35Z

lib/Common/Core/SysInfo.h

@@ -62,7 +63,11 @@ class AutoSystemInfo : public SYSTEM_INFO
 #ifdef _WIN32
    static HMODULE GetCRTHandle();
 #endif
+#if defined(__APPLE__) && defined(_M_ARM64)


"Apple" + "ARM64" can mean both iPhone and Arm-based Mac, I wonder if it is necessary to distinguish the two (I don't think we are trying to support iPhone).

This is a good point, some of my #ifdefs may not be the most consistent, I don't think the codebase as a whole will run on an iPhone at the moment but I haven't tried.

AFAIK M1/M2 are essentially just souped up A-series processors and ARM macOS can even run iOS apps natively so there's probably no need to distinguish.

I suspect it won't run for one reason or the other, but this is probably low priority at the moment

ppenzin

Runtime commit looks good. Very impressive actually!

ppenzin · 2023-03-10T06:43:47Z

lib/Runtime/Debug/TTEventLog.cpp

@@ -2818,6 +2819,8 @@ namespace TTD
        TTDAssert(wcscmp(_u("x64"), archString.Contents) == 0, "Mismatch in arch between record and replay!!!");
 #elif defined(_M_ARM)
        TTDAssert(wcscmp(_u("arm64"), archString.Contents) == 0, "Mismatch in arch between record and replay!!!");
+#elif defined(_M_ARM64)
+        TTDAssert(wcscmp(_u("arm64"), archString.Contents) == 0, "Mismatch in arch between record and replay!!!");


Curious why _M_ARM and _M_ARM64 both test for "arm64"

The M_ARM path may be wrong but I don't have testing set up for ARM or a device to check it on, so was nervous about changing it - it's a very old line of code and before microsoft stopped supporting CC it used to pass arm tests on windows, implying perhaps that both sides of the condition were being set incorrectly.

Can you add a comment to look at it later?

Will do (let me know when you've finished reviewing and I'll address all points together if that's ok)

ppenzin · 2023-03-16T17:05:27Z

lib/Common/PlatformAgnostic/AssemblyCommon.h

@@ -24,6 +25,9 @@ void mac_fde_wrapper(const char *dataStart, mac_fde_reg_op reg_op);
 #define __REGISTER_FRAME(addr) __register_frame(addr)
 #define __DEREGISTER_FRAME(addr) __deregister_frame(addr)
 #endif // __APPLE__
+#elif defined(_M_ARM64)


Maybe add a comment what #if this corresponds to, would be easier to follow.

ppenzin · 2023-03-16T17:07:34Z

test/CMakeLists.txt

Huge thanks for doing this!

ppenzin · 2023-03-16T17:10:21Z

One more suggestion, but overall looks good.

- Update entry point call for Apple arm64 function calling convention - New Assembler routines for function calls

- Enables dynamic interpretor thunks on Apple Silicon - Remainder of the JIT will compile with these edits but not yet work

- small changes e.g. alternatives for windows intrinsics - build file updates

- update DateTimeFormat.js which required ICU_Version<72 - update ICU path for native tests (different on Apple Silicon) - use find_package for Python

- Build and test Debug, Test and Release Builds - Cirrus CI supports Apple Silicon - Not intended to replace Azure for other builds due to usage limits

rhuanjl · 2023-03-21T21:31:20Z

test/Number/rlexe.xml

+      <!--TODO Investigate and re-enable this test on ARM64 macOS-->
+      <tags>exclude_mac</tags> 


This test began failing in the online apple silicon CI but not offline - Number.toString on a double was outputting a slightly different answer to the baseline. Ok to merge anyway and investigate later? @ppenzin

I believe this was due to an update to the VM image used by Cirrus CI as I'd not made any relevant change to the source and the test fail doesn't occur offline.

NOTE, double to string conversion is implementation defined and has long been a point where CC was out of line with other implementations, so I don't think this really counts as a bug - though it would be nice to work out why it's different on a particularly version of macOS (in the online CI)

We have #149 filed for this issue in general, maybe add a comment there so that we don't forget. I think it is OK to merge in current state.

rhuanjl marked this pull request as draft January 11, 2023 17:45

rhuanjl force-pushed the apple-silicon branch 9 times, most recently from bd5aac7 to 8733d46 Compare January 12, 2023 19:43

rhuanjl force-pushed the apple-silicon branch 3 times, most recently from 9db2135 to 8adf180 Compare January 21, 2023 15:26

rhuanjl mentioned this pull request Jan 21, 2023

Support lacking for Apple Silicon (ARM) #6860

Open

rhuanjl force-pushed the apple-silicon branch from 8adf180 to 9d2bc10 Compare January 26, 2023 18:09

rhuanjl force-pushed the apple-silicon branch 4 times, most recently from 6396881 to 5ab9610 Compare February 8, 2023 23:50

rhuanjl force-pushed the apple-silicon branch 3 times, most recently from bb4a158 to b86980e Compare February 14, 2023 18:18

rhuanjl added 2 commits February 14, 2023 18:21

Update PAL to support Apple Silicon

91399ac

- Some material brought in from PAL at github.com/dotnet/ - Various manual changes to fit with existing codebase - Updated licensing in edited files, latest PAL is copyright .dotNet - Updated copyright check scripts to check pal/ files for new text

Update Memory handling to support Apple Silicon

0213e77

- Memory page size on apple arm64 is 16kb - This change caused ushort overflows - minor edits to orders of operations to fix - New assembler file for arm64_SAVE_REGISTERS

rhuanjl force-pushed the apple-silicon branch from b86980e to d5ed057 Compare February 14, 2023 18:21

rhuanjl marked this pull request as ready for review February 14, 2023 18:34

rhuanjl changed the title ~~WIP: Support macOS ARM64 (Apple silicon)~~ Support macOS ARM64 (Apple silicon) without JIT Feb 14, 2023

rhuanjl requested a review from ppenzin February 15, 2023 22:54

ppenzin previously approved these changes Mar 10, 2023

View reviewed changes

ppenzin reviewed Mar 10, 2023

View reviewed changes

ppenzin approved these changes Mar 16, 2023

View reviewed changes

tcare approved these changes Mar 21, 2023

View reviewed changes

rhuanjl added 5 commits March 21, 2023 18:00

Update Runtime to support Apple Silicon

755968e

- Update entry point call for Apple arm64 function calling convention - New Assembler routines for function calls

Incomplete Backend support for Apple Silicon

41e7cf8

- Enables dynamic interpretor thunks on Apple Silicon - Remainder of the JIT will compile with these edits but not yet work

Apple Silicon support, miscellaneous

01e3003

- small changes e.g. alternatives for windows intrinsics - build file updates

Apple Silicon Test Suite updates

ab8da0b

- update DateTimeFormat.js which required ICU_Version<72 - update ICU path for native tests (different on Apple Silicon) - use find_package for Python

Cirrus CI for Apple Silicon noJit

f4cd0ac

- Build and test Debug, Test and Release Builds - Cirrus CI supports Apple Silicon - Not intended to replace Azure for other builds due to usage limits

rhuanjl force-pushed the apple-silicon branch from d5ed057 to f4cd0ac Compare March 21, 2023 18:00

Temporarily disable Number.toString test

ce86e5e

rhuanjl force-pushed the apple-silicon branch from feaa67f to ce86e5e Compare March 21, 2023 21:30

rhuanjl commented Mar 21, 2023

View reviewed changes

rhuanjl merged commit c3ead3f into chakra-core:master Mar 22, 2023

rhuanjl deleted the apple-silicon branch March 27, 2023 08:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support macOS ARM64 (Apple silicon) without JIT #6869

Support macOS ARM64 (Apple silicon) without JIT #6869

rhuanjl commented Jan 11, 2023 •

edited

Loading

rhuanjl commented Jan 26, 2023

rhuanjl commented Feb 8, 2023

pleath commented Feb 8, 2023

rhuanjl commented Feb 8, 2023

rhuanjl commented Feb 10, 2023

rhuanjl commented Feb 14, 2023

ppenzin left a comment •

edited

Loading

ppenzin left a comment

ppenzin Mar 10, 2023

rhuanjl Mar 10, 2023

ppenzin Mar 10, 2023

rhuanjl Mar 10, 2023

ppenzin Mar 14, 2023

ppenzin Mar 16, 2023

ppenzin Mar 10, 2023

rhuanjl Mar 10, 2023

fatcerberus Mar 13, 2023

ppenzin Mar 14, 2023

ppenzin left a comment

ppenzin Mar 10, 2023

rhuanjl Mar 10, 2023

ppenzin Mar 14, 2023

rhuanjl Mar 16, 2023

ppenzin Mar 16, 2023

ppenzin Mar 16, 2023

ppenzin commented Mar 16, 2023

rhuanjl Mar 21, 2023

ppenzin Mar 22, 2023 •

edited

Loading

		<!--TODO Investigate and re-enable this test on ARM64 macOS-->
		<tags>exclude_mac</tags>

Support macOS ARM64 (Apple silicon) without JIT #6869

Support macOS ARM64 (Apple silicon) without JIT #6869

Conversation

rhuanjl commented Jan 11, 2023 • edited Loading

rhuanjl commented Jan 26, 2023

rhuanjl commented Feb 8, 2023

pleath commented Feb 8, 2023

rhuanjl commented Feb 8, 2023

rhuanjl commented Feb 10, 2023

rhuanjl commented Feb 14, 2023

ppenzin left a comment • edited Loading

Choose a reason for hiding this comment

ppenzin 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

ppenzin 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

ppenzin commented Mar 16, 2023

Choose a reason for hiding this comment

ppenzin Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

rhuanjl commented Jan 11, 2023 •

edited

Loading

ppenzin left a comment •

edited

Loading

ppenzin Mar 22, 2023 •

edited

Loading