Summary: | Start moving to separated writable and executable mappings in the JIT | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Oliver Hunt <oliver> | ||||||||||
Component: | New Bugs | Assignee: | Oliver Hunt <oliver> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, benjamin, cdumez, cmarcelo, commit-queue, keith_miller, mark.lam, msaboff, ossy, ryanhaddad, saam | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 155195 | ||||||||||||
Bug Blocks: | 155245, 155268 | ||||||||||||
Attachments: |
|
Description
Oliver Hunt
2016-03-08 11:30:04 PST
Created attachment 273309 [details]
Patch
Waiting for builds Comment on attachment 273309 [details]
Patch
looks good to me so far. lets see what happens with the bots.
Created attachment 273311 [details]
Patch
(In reply to comment #4) > Created attachment 273311 [details] > Patch because being able to build is totes overrated :D Committed r197793: <http://trac.webkit.org/changeset/197793> (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 :-/ (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 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. Re-opened since this is blocked by bug 155195 Comment on attachment 273311 [details]
Patch
Something bizarre happened when this got landed. Happily? it did show another issue i missed
Created attachment 273355 [details]
All the build fixes for non-separated heaps
Created attachment 273356 [details]
All the build fixes for both at the same time. sigh.
Committed r197816: <http://trac.webkit.org/changeset/197816> 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, (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 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. (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. |