Bug 41641 - Update compile flags to allow use of ExecutableAllocatorFixedVMPool on platforms other than x86-64
Summary: Update compile flags to allow use of ExecutableAllocatorFixedVMPool on platfo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
: 41679 (view as bug list)
Depends on: 41679 41686
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-05 15:00 PDT by Gavin Barraclough
Modified: 2010-07-08 11:58 PDT (History)
6 users (show)

See Also:


Attachments
The patch (8.96 KB, patch)
2010-07-05 15:06 PDT, Gavin Barraclough
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2010-07-05 15:00:19 PDT
This may be useful on 32-bit platforms, too.
Comment 1 Gavin Barraclough 2010-07-05 15:06:55 PDT
Created attachment 60570 [details]
The patch

The constants VM_POOL_SIZE & COALESCE_LIMIT will require some tuning, have put in some hopefully sensible starting points.

This patch is a cleanup that should not change the behaviour on any platform.
Comment 2 WebKit Review Bot 2010-07-05 18:21:51 PDT
http://trac.webkit.org/changeset/62511 might have broken SnowLeopard Intel Release (Tests)
Comment 3 Gavin Barraclough 2010-07-05 22:55:07 PDT
fixed in r62511
Comment 4 Xan Lopez 2010-07-06 04:36:21 PDT
I rolled this out because it breaks Linux/64bit. I'm unsure of what we are supposed to do, since ExecutableAllocatorPosix.cpp is now defined only for 32bit, and ExecutableAllocatorFixedVMPool.cpp is Darwin-only atm.
Comment 5 Mark Rowe (bdash) 2010-07-06 04:43:00 PDT
Xan, if this was rolled out you need to do two things:
1) Mention the revision number in which it was rolled out.
2) Reopen the bug.  If the change is no longer in the tree then the bug clearly isn’t “fixed”.
Comment 6 Xan Lopez 2010-07-06 05:10:35 PDT
Sorry, for some reason thought the bot had reopened it. The bug is linked in the 'Depends on' list though, revision where this was rolled out is r62544.
Comment 7 Gavin Barraclough 2010-07-07 11:11:26 PDT
I hadn't realized the JIT had been enabled on x86-64 Linux using the posix allocator - this is terribly unsafe, we should only be using the fixed pool allocator on 64bit (due to a branch range issue). However this is also implemented in terms of posix mmap/munmap, so should work on Linux too.  Do you know what the problem is here? - is it just missing arc4random? - if so, we can just disable the ASLR on !OS(DARWIN) platforms.
Comment 8 Xan Lopez 2010-07-07 16:45:40 PDT
(In reply to comment #7)
> I hadn't realized the JIT had been enabled on x86-64 Linux using the posix allocator - this is terribly unsafe, we should only be using the fixed pool allocator on 64bit (due to a branch range issue). However this is also implemented in terms of posix mmap/munmap, so should work on Linux too.  Do you know what the problem is here? - is it just missing arc4random? - if so, we can just disable the ASLR on !OS(DARWIN) platforms.

Oh, I haven't tried to actually compile the file. Just saw that it used to be guarded by OS(DARWIN) and has mach headers, so it won't compile on Linux out of the box. If the right thing to do is use this I can try to get it into shape.
Comment 9 Zoltan Herczeg 2010-07-07 23:17:51 PDT
Exactly, arc4random is missing. In Qt-Linux 64, mmap with 0 address works fine, and not sure what is the advantage of forcing a custom address space (anyway an idea: since the random address space may collide with something else, you could encolse it in a loop, and could try 3 or 4 times with different random values before crash with out of memory)

Anyway, that patch was rolled out, so you can incorporate this fix into that patch before landing again.
Comment 10 Gavin Barraclough 2010-07-08 11:55:22 PDT
*** Bug 41679 has been marked as a duplicate of this bug. ***
Comment 11 Gavin Barraclough 2010-07-08 11:58:53 PDT
Relanded in 62799, GTK fixes in 62806, 62811.