osx: JIT support #1744

obastemur · 2016-10-14T08:57:26Z

This PR;

Enables JIT on macOS
Fixes / improvements to xplat exception handling

obastemur · 2016-10-15T11:39:33Z

jianchun · 2016-10-17T18:54:12Z

lib/Backend/PDataManager.cpp

-void PDataManager::RegisterPdata(RUNTIME_FUNCTION* pdataStart, _In_ const ULONG_PTR functionStart, _In_ const ULONG_PTR functionEnd, _Out_ PVOID* pdataTable, ULONG entryCount, ULONG maxEntryCount)
+void PDataManager::RegisterPdata(RUNTIME_FUNCTION* pdataStart,
+  _In_ const ULONG_PTR functionStart, _In_ const ULONG_PTR functionEnd,
+  _Out_ PVOID* pdataTable, ULONG entryCount, ULONG maxEntryCount)


nit: Should use 4-space indentation

jianchun · 2016-10-17T18:57:33Z

lib/Runtime/Language/DynamicProfileInfo.cpp

-            }
-        }
-    }
+    DEFINE_DYNAMICPROFILEINFO_RECORDMODULUSOPTYPE_FNC


This approach looks weird. How about removing the "inline" keyword from old implementation instead?

jianchun · 2016-10-17T19:00:07Z

lib/Common/Memory/amd64/XDataAllocator.cpp

@@ -12,6 +12,10 @@ CompileAssert(false)
 #include "XDataAllocator.h"
 #include "Core/DelayLoadLibrary.h"

+#ifndef _WIN32


nit: #ifndef ... not needed.

Indeed not needed. However this is an xplat header file [for now]

I seem to recall you had this same include elsewhere without #ifndef _WIN32.

From PlatformAgnostic maybe ? My approach is to keep enabling anything non-Windows on lib files to prevent Full project breaks.

jianchun · 2016-10-17T19:10:08Z

lib/Runtime/ByteCode/ByteCodeWriter.h

+                }                                                                  \
+                Assert(OpCodeUtil::EncodedSize((Js::OpCode)op, SmallLayout)        \
+                       == (currentOffset - offset));                               \
+            }


Similar to other comment, putting the code to multiple places with a macro looks weird. Please try other cleaner solution e.g. moving the template instantiation to header file.

Indeed weird. Tried other things [i.e. remove inline / use it from header ].. Older Clang behaves strange :/ Actually, we already define those in multiple places (using include )

Tried simply removing inline fixes both cases: #1790

I see that, I do recall it fails on my test machine.. will take CI as the base and remove the inlines

jianchun · 2016-10-17T19:15:33Z

pal/src/exception/machexception.cpp

 --*/

+#include "pal/dbgmsg.h"
+SET_DEFAULT_DEBUG_CHANNEL(EXCEPT); // some headers have code with asserts, so do this first
+


I know nothing about these big changes in PAL. Did you work on these changes, or did you sync them to dotnet coreCLR latest?

Some I had to update + sync to PAL.

jianchun · 2016-10-17T19:25:42Z

lib/Runtime/PlatformAgnostic/Platform/Unix/AssemblyCommon.cpp

+extern void mac_fde_wrapper(const char *dataStart, mac_fde_reg_op reg_op)
+{
+    for(const char *head = dataStart, *last = dataStart + CC_FDE_SIZE;
+        head != last;


I suggest avoid the last check. FDE is not of fixed size. The check may mislead some to think it is. The CIE/FDE data is guaranteed to terminate from the break in the loop body below. If for any reason the data is wrong and does not, it makes no difference whether it hangs/crashes here or crashes later on. Hanging/crashing here is better in my opinion because it is closer to the source of problem.

There is no guarantee here for a meaningful hang or crash. Indeed FDE is not a fixed size and shouldn't be bigger than the defined size above. + if it is bigger, see assert below

My biggest complain is about the misleading for loop. It reads as if it would loop some fixed size and end at last. Actually no, it ends at a terminator and break from inside the loop body.

Indeed it does and I see your complain. I would rather add a comment there instead of removing it.

I prefer self-explanatory code instead of comment. head != last is meaningless. It is almost always true.

jianchun · 2016-10-17T19:26:09Z

lib/Runtime/PlatformAgnostic/Platform/Unix/AssemblyCommon.cpp

+    {
+        const char *op = head;
+        // Get Length of record [ Read 4 bytes ]
+        const uint32_t* op_data = (const uint32_t*)op;


const uint32_t* op_data [](start = 8, length = 23)

Why not const uint32_t length = ...?

could be... op -> cast to -> op_data?

I meant the pointer op_data was not as straightforward. Just do const uint32_t length = *(const uint32_t*)op; and use length.

jianchun · 2016-10-17T19:29:28Z

lib/Runtime/PlatformAgnostic/Platform/Unix/AssemblyCommon.cpp

+            break;
+        }
+        // if it's not 0xffffffff,
+        // there we have the length of the CIE or FDE record.


nit: Comment is not very clear (as code not checking 0xFFFFFFFF). We actually ignored 0xFFFFFFFF which identifies extended length. Our records are small so we never need extended length.

jianchun · 2016-10-17T19:31:23Z

lib/Runtime/PlatformAgnostic/Platform/Unix/AssemblyCommon.cpp

+        }
+        // if it's not 0xffffffff,
+        // there we have the length of the CIE or FDE record.
+        head += 4; // get next


head += 4; // get next [](start = 8, length = 22)

nit: Please comment it is to read next 4 bytes and non zero value identifies FDE (zero is CIE).

jianchun · 2016-10-17T19:36:21Z

lib/Common/PlatformAgnostic/AssemblyCommon.h

+extern "C" void __register_frame(const void* ehframe);
+extern "C" void __deregister_frame(const void* ehframe);
+
+#if defined(_AMD64_) && !defined(DISABLE_JIT)


!defined(DISABLE_JIT) [](start = 24, length = 21)

Please use ENABLE_NATIVE_CODEGEN instead of !defined(DISABLE_JIT). The former is used more.

Is there a reason we define DISABLE_JIT in that case?

ENABLE_NATIVE_CODEGEN existed from the beginning of this project and used everywhere. DISABLE_JIT used rarely (haven't checked so not sure about history, might be added recently, maybe we should not have added it). I prefer the former for overall consistency.

I recall DISABLE_JIT is used while enabling JIT on linux. Since we have it and naming explains the intention, I prefer keeping it. If we want to remove them all, that's something else.

I'm ok if you insist. Just want to note that another reason I prefer ENABLE_NATIVE_CODEGEN is it is positive. Easier to read and understand than double negative !defined(DISABLE_JIT).

Just noticed this part. ENABLE_NATIVE_CODEGEN is a subset of DISABLE_JIT. ENABLE_NATIVE_CODEGEN is generally meant for JIT-related code directly. DISABLE_JIT was added as part of doing the Interpreter-only version of ChakraCore last year- it implies !ENABLE_NATIVE_CODEGEN but also disables other things like dynamic interpreter thunks, interpreter profiling etc.

Thanks @digitalinfinity for explanation, now I understand. !defined(DISABLE_JIT) here is fine to me.

jianchun · 2016-10-17T19:39:02Z

lib/Common/PlatformAgnostic/AssemblyCommon.h

+typedef void(*mac_fde_reg_op)(const void*addr);
+void mac_fde_wrapper(const char *dataStart, mac_fde_reg_op reg_op);
+#define __REGISTER_FRAME(addr) mac_fde_wrapper((char*)addr, __register_frame)
+#define __DEREGISTER_FRAME(addr) mac_fde_wrapper((char*)addr, __deregister_frame)


nit: Use (const char*)addr to be consistent with signature.

obastemur · 2016-10-18T12:23:08Z

@dotnet-bot test Ubuntu ubuntu_linux_debug_static

obastemur · 2016-10-18T12:51:17Z

@dotnet-bot test Ubuntu ubuntu_linux_test_static

obastemur · 2016-10-18T13:09:32Z

@dotnet-bot test Ubuntu ubuntu_linux_release

This PR; - Enables JIT on macOS - Fixes / improvements to xplat exception handling

jianchun · 2016-10-27T23:01:13Z

lib/Runtime/Language/DynamicProfileInfo.cpp

            }
            else
            {
-                divideTypeInfo[profileId] = divideTypeInfo[profileId].Merge(ValueType::Float);
+                divideTypeInfo[profileId] = divideTypeInfo[profileId]
+                                           .Merge(ValueType::Float);
            }


nit: DynamicProfileInfo.cpp contains some formatting changes that are unnecessary (changes when you originally put them into a macro?).

80 chars. since it is updated, left it.

jianchun · 2016-10-27T23:02:50Z

lib/Runtime/Library/amd64/JavascriptFunctionA.S

-//    jmp _ZN2Js18JavascriptFunction17CallAsmJsFunctionIiEET_PNS_16RecyclableObjectEPFPvS4_NS_8CallInfoEzEjPS5_
-//??$CallAsmJsFunction@T__m128@@@JavascriptFunction@Js@@SA?AT__m128@@PEAVRecyclableObject@1@PEAXIPEAPEAX@Z ENDP
-
-


nit: Why moving this section... here they are closer to next implementation.
Also you deleted __m128 version. Commented out currently, maybe needed in future.

osx linker. ordering issues. we add that easily when we need it. it is keep showing up on my auto searches as if I forgot that..

jianchun

@obastemur Thanks for making this change! Left 2 nit comments (can ignore).

obastemur · 2016-10-27T23:24:36Z

@jianchun Thanks for the review

Merge pull request #1744 from obastemur:xoj This PR; - Enables JIT on macOS - Fixes / improvements to xplat exception handling

msftclas added the cla-already-signed label Oct 14, 2016

obastemur force-pushed the xoj branch 10 times, most recently from c4a9979 to 2733317 Compare October 15, 2016 11:27

jianchun reviewed Oct 17, 2016

View reviewed changes

obastemur force-pushed the xoj branch 3 times, most recently from e00b064 to 4816a60 Compare October 18, 2016 12:12

obastemur force-pushed the xoj branch from 4816a60 to 62ba238 Compare October 27, 2016 21:54

obastemur force-pushed the xoj branch 2 times, most recently from d13c2a2 to 3350b7c Compare October 27, 2016 22:44

osx: JIT support

4c410fd

This PR; - Enables JIT on macOS - Fixes / improvements to xplat exception handling

obastemur force-pushed the xoj branch from 3350b7c to 4c410fd Compare October 27, 2016 22:46

jianchun reviewed Oct 27, 2016

View reviewed changes

jianchun approved these changes Oct 27, 2016

View reviewed changes

chakrabot merged commit 4c410fd into chakra-core:master Oct 27, 2016

chakrabot pushed a commit that referenced this pull request Oct 27, 2016

[MERGE #1744 @obastemur] osx: JIT support

aee5cdb

Merge pull request #1744 from obastemur:xoj This PR; - Enables JIT on macOS - Fixes / improvements to xplat exception handling

obastemur deleted the xoj branch November 15, 2016 13:18

		// jmp _ZN2Js18JavascriptFunction17CallAsmJsFunctionIiEET_PNS_16RecyclableObjectEPFPvS4_NS_8CallInfoEzEjPS5_
		//??$CallAsmJsFunction@T__m128@@@JavascriptFunction@Js@@SA?AT__m128@@PEAVRecyclableObject@1@PEAXIPEAPEAX@Z ENDP

osx: JIT support #1744

osx: JIT support #1744

Conversation

obastemur commented Oct 14, 2016

obastemur commented Oct 15, 2016

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

jianchun Oct 17, 2016 • edited Loading

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 Oct 18, 2016

obastemur commented Oct 18, 2016

obastemur commented Oct 18, 2016

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

jianchun left a comment

Choose a reason for hiding this comment

obastemur commented Oct 27, 2016

jianchun Oct 17, 2016 •

edited

Loading