Bug 25246 – Jumps may fail to be linked correctly on x86_64.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Gavin Barraclough]

A problem could occur where two pieces of JIT code which we intend to link together fall more than 2Gb apart.  The solution here is to allocate a single 2gb region of VM from which all JIT code will be allocated.

This places an artificial 2Gb cap on JIT code.  If we wish to do so in future we will need make it possible for the JIT to handle linking jumps between JIT code regions further than 2Gb apart (we currently just assert that we don't hit this case) – however even if we do so, it will likely still make sense to allocate out of (multiple instances of) the heap implemented here (to ensure the maximum amount of code possible is allocated within range of x86_64 relative branches).

[Gavin Barraclough]

Created attachment 29551 [details]
The patch

[Geoffrey Garen]

Comment on attachment 29551 [details]
The patch

> +    // All entries of the same size share a single entry
> +    // in the AVLTree, and are linked together in a linked
> +    // list, using equalSize.
> +    void* pointer;
> +    size_t size;
> +    FreeListEntry* equalSize;

Should equalSize just be called "next" or "nextEntry", since it's a link in a list?

+// Used to reverse sort an array of FreeListEntry pointers.
+static int sortFreeListEntriesByPointer(const void* leftPtr, const void* rightPtr)

Maybe call this "reverseSortFreeListEntriesByPointer" or "sortFreeListEntriesByPointerAscending" (Descending?).

+// Used to reverse sort an array of pointers.
+static int sortCommonSizedAllocations(const void* leftPtr, const void* rightPtr)

Same here.

+        if (FreeListEntry* entryInFreeList = freeList.search(entry->size, freeList.EQUAL)) {
+            // Find the last entry in the chain of entries already in the freeList.
+            while (FreeListEntry* next = entryInFreeList->equalSize)
+                entryInFreeList = next;

Would it be better to add to the head of the list instead of the tail, to avoid having to walk the whole list?

+    // instead we we periodically perform a sweep to try to coalesce neigboring

Typo: "we we"

+    size_t countFreedSiceLastCoalesce;

Typo: Should be "Since".

+            // Remove the entry from the freeList.  But! -
+            // Each entry in the tree may represent a chain of multiple chunks of the
+            // same size, and we only want to remove one on them.  So, if this entry
+            // does have a chain, just snip the last one off the end.

How about popping from the head instead of the tail? (Same comment as above.)

+                // There is memory left over, and it is large than the common size.

Typo: Should be "larger".

+    // All allocations must be a multiple of the commonSize
+    // specified to the constructor.

I guess this comment is not true?

+                // There is memory left over, and it is large than the common size.
+                // We can reuse the existing FreeListEntry node to add this back

I think this case is hit if the memory left over is smaller than the common size, too, right?

r=me

[Gavin Barraclough]

> +    // All allocations must be a multiple of the commonSize
> +    // specified to the constructor.
> 
> I guess this comment is not true?
> 
> +                // There is memory left over, and it is large than the common
> size.
> +                // We can reuse the existing FreeListEntry node to add this
> back
> 
> I think this case is hit if the memory left over is smaller than the common
> size, too, right?
> 

Errk, yes, these are complete lies, based on an early misunderstanding.

[Gavin Barraclough]

Created attachment 29563 [details]
Patch with review fixes.

[Gavin Barraclough]

Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj
Adding         JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp
Sending        JavaScriptCore/jit/ExecutableAllocatorPosix.cpp
Sending        JavaScriptCore/wtf/AVLTree.h
Transmitting file data .....
Committed revision 42638.