Bug 155178 - Start moving to separated writable and executable mappings in the JIT
Summary: Start moving to separated writable and executable mappings in the JIT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on: 155195
Blocks: 155245 155268
  Show dependency treegraph
 
Reported: 2016-03-08 11:30 PST by Oliver Hunt
Modified: 2016-03-11 10:01 PST (History)
11 users (show)

See Also:


Attachments
Patch (65.15 KB, patch)
2016-03-08 11:44 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (65.58 KB, patch)
2016-03-08 12:00 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
All the build fixes for non-separated heaps (65.65 KB, patch)
2016-03-08 15:45 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
All the build fixes for both at the same time. sigh. (65.64 KB, patch)
2016-03-08 15:52 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2016-03-08 11:30:04 PST
Start moving to separated writable and executable mappings in the JIT
Comment 1 Oliver Hunt 2016-03-08 11:44:08 PST
Created attachment 273309 [details]
Patch
Comment 2 Oliver Hunt 2016-03-08 11:53:16 PST
Waiting for builds
Comment 3 Filip Pizlo 2016-03-08 11:53:50 PST
Comment on attachment 273309 [details]
Patch

looks good to me so far.  lets see what happens with the bots.
Comment 4 Oliver Hunt 2016-03-08 12:00:25 PST
Created attachment 273311 [details]
Patch
Comment 5 Oliver Hunt 2016-03-08 12:10:07 PST
(In reply to comment #4)
> Created attachment 273311 [details]
> Patch

because being able to build is totes overrated :D
Comment 6 Oliver Hunt 2016-03-08 12:53:02 PST
Committed r197793: <http://trac.webkit.org/changeset/197793>
Comment 7 Alex Christensen 2016-03-08 13:36:36 PST
build fix in r197799
Comment 8 Oliver Hunt 2016-03-08 13:44:00 PST
(In reply to comment #7)
> build fix in r197799


...

:-/

I'm going to assume Xcode here. I'm just going to verify that the patch otherwise matched what was reviewed :-/
Comment 9 Oliver Hunt 2016-03-08 13:46:01 PST
(In reply to comment #8)
> (In reply to comment #7)
> > build fix in r197799
> 
> 
> ...
> 
> :-/
> 
> I'm going to assume Xcode here. I'm just going to verify that the patch
> otherwise matched what was reviewed :-/

Seriously confused by what Xcode did to the patch post review. I'm going to rollout this (and your subsequent patch) and get the bot to re-land it, to be safe because the attached patch is the correct one.
Comment 10 Alex Christensen 2016-03-08 13:52:39 PST
Comment on attachment 273311 [details]
Patch

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

if you're going to reland it, please remember these fixes I also needed:

> Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:152
> +            VM_FLAGS_ANYWHERE | VM_FLAGS_RANDOM_ADDR,

VM_FLAGS_RANDOM_ADDR is not defined in my sdk.  does not build.

> Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:164
> +        int result = mprotect(stubBase, stubSize, VM_PROT_EXECUTE_ONLY);

result is not defined when VM_PROT_EXECUTE_ONLY is not defined.  Does not build.
Comment 11 WebKit Commit Bot 2016-03-08 13:55:02 PST
Re-opened since this is blocked by bug 155195
Comment 12 Oliver Hunt 2016-03-08 14:18:51 PST
Comment on attachment 273311 [details]
Patch

Something bizarre happened when this got landed. Happily? it did show another issue i missed
Comment 13 Oliver Hunt 2016-03-08 15:45:30 PST
Created attachment 273355 [details]
All the build fixes for non-separated heaps
Comment 14 Oliver Hunt 2016-03-08 15:52:19 PST
Created attachment 273356 [details]
All the build fixes for both at the same time. sigh.
Comment 15 Oliver Hunt 2016-03-08 16:08:46 PST
Committed r197816: <http://trac.webkit.org/changeset/197816>
Comment 16 Alexey Proskuryakov 2016-03-08 16:24:37 PST
This broke iOS build: 

In file included from /Volumes/Data/slave/ios-9-release/build/Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:57:
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS9.0.sdk/usr/include/mach/mach_vm.h:1:2: error: mach_vm.h unsupported.
#error mach_vm.h unsupported.
 ^
/Volumes/Data/slave/ios-9-release/build/Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:156:33: error: use of undeclared identifier 'VM_FLAGS_RANDOM_ADDR'
            VM_FLAGS_ANYWHERE | VM_FLAGS_RANDOM_ADDR,
Comment 17 Csaba Osztrogonác 2016-03-09 05:10:13 PST
(In reply to comment #15)
> Committed r197816: <http://trac.webkit.org/changeset/197816>

It broke the WinCairo build too:
https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/54752
Comment 18 Benjamin Poulain 2016-03-10 22:22:51 PST
Comment on attachment 273356 [details]
All the build fixes for both at the same time. sigh.

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

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1271
> +        m_assembler.stp<64>(src1, src2, dest, offset.m_value);

Can you please update your storePair/loadPair to use Address instead of base+immediate?

This API is confusing IMHO. It look like you are exposing STP/STNP directly but you are not. The offset is scaled.
With Address it is clear: the offset is always in bytes.
Comment 19 Oliver Hunt 2016-03-11 09:58:22 PST
(In reply to comment #18)
> Comment on attachment 273356 [details]
> All the build fixes for both at the same time. sigh.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273356&action=review
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1271
> > +        m_assembler.stp<64>(src1, src2, dest, offset.m_value);
> 
> Can you please update your storePair/loadPair to use Address instead of
> base+immediate?
> 
> This API is confusing IMHO. It look like you are exposing STP/STNP directly
> but you are not. The offset is scaled.
> With Address it is clear: the offset is always in bytes.

Will do.