Bug 155178 – Start moving to separated writable and executable mappings in the JIT

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Start moving to separated writable and executable mappings in the JIT

[Oliver Hunt]

Created attachment 273309 [details]
Patch

[Oliver Hunt]

Waiting for builds

[Filip Pizlo]

Comment on attachment 273309 [details]
Patch

looks good to me so far.  lets see what happens with the bots.

[Oliver Hunt]

Created attachment 273311 [details]
Patch

[Oliver Hunt]

(In reply to comment #4)
> Created attachment 273311 [details]
> Patch

because being able to build is totes overrated :D

[Oliver Hunt]

Committed r197793: <http://trac.webkit.org/changeset/197793>

[Alex Christensen]

build fix in r197799

[Oliver Hunt]

(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 :-/

[Oliver Hunt]

(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.

[Alex Christensen]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 155195

[Oliver Hunt]

Comment on attachment 273311 [details]
Patch

Something bizarre happened when this got landed. Happily? it did show another issue i missed

[Oliver Hunt]

Created attachment 273355 [details]
All the build fixes for non-separated heaps

[Oliver Hunt]

Created attachment 273356 [details]
All the build fixes for both at the same time. sigh.

[Oliver Hunt]

Committed r197816: <http://trac.webkit.org/changeset/197816>

[Alexey Proskuryakov]

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,

[Csaba Osztrogonác]

(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

[Benjamin Poulain]

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.

[Oliver Hunt]

(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.