Bug 155377 – Bring separated jit heap to x86_64

• Component: New Bugs
• Status: NEW

[Oliver Hunt]

Bring separated jit heap to x86_64

[Oliver Hunt]

Created attachment 273756 [details]
Patch

[Oliver Hunt]

Created attachment 273758 [details]
Patch

[Oliver Hunt]

Created attachment 273759 [details]
Patch

[Build Bot]

Comment on attachment 273759 [details]
Patch

Attachment 273759 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/962358

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 273766 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[Oliver Hunt]

Created attachment 273772 [details]
Patch

[Oliver Hunt]

Created attachment 273779 [details]
Patch

[Oliver Hunt]

Created attachment 273782 [details]
Patch

[Darin Adler]

Comment on attachment 273782 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273782&action=review

Someone else will have to review, but I have a few thoughts:

Can we come up with a better name than performJITMemcpy? We should talk about what the function does and try to come up with more normal words for this. Maybe copyIntoExecutableMemory? Or whatever the concept of the function actually is.

> Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:185
> +        intptr_t randomLocation = 0;
> +        randomLocation = arc4random() & ((1 << 25) - 1);
> +        randomLocation += (1 << 24);
> +        randomLocation <<= 21;

Should do the first two operations on the same line; no need to have this “assign 0” that gets overwritten.

Should have a comment explaining where the algorithm comes from.

> Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:186
> +        writableAddr = (mach_vm_address_t)randomLocation;

Normally in WebKit we use C++ casts, so this would be reinterpret_cast. It would be better to do that in the other code below too instead of the C casts.

> Source/WTF/wtf/Platform.h:1160
> +#if defined(ENABLE_SEPARATED_WX_HEAP) && defined(ENABLE_EXECUTABLE_ALLOCATOR_DEMAND) && ENABLE_EXECUTABLE_ALLOCATOR_DEMAND
> +#error Separated WX heap requires a build to use the fixed pool executable allocator. 
> +#endif

Can this check go somewhere else? It doesn’t need to be in Platform.h itself, nor does it have to be compiled over and over again. Just need it one logical place in the project. Presumably the relevant .cpp file or header.

[Oliver Hunt]

Created attachment 274165 [details]
Patch

[Saam Barati]

Comment on attachment 274165 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274165&action=review

Looks good to me. I'll let others also comment

> Source/JavaScriptCore/assembler/X86Assembler.h:2636
> +        auto insn = OP_JMP_rel32;

Why not "instruction" instead of "insn" everywhere?

> Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:182
> +        writableAddr = (mach_vm_address_t)randomLocation;

Style: static_cast

> Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:187
> +            mach_task_self(), (mach_vm_address_t)jitBase,

Ditto

[Oliver Hunt]

Created attachment 274197 [details]
Patch

[Oliver Hunt]

Created attachment 274198 [details]
Patch

[Brady Eidson]

Comment on attachment 274198 [details]
Patch

r-, as this has been pending review for over a year now. It is near-impossible that this patch still applies to trunk and unlikely to still be relevant in its current form.