[Webkit-unassigned] [Bug 238143] [JSC][ARMv7] Support proper near calls and JUMP_ISLANDS

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=238143

Geza Lore <glore at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #455248|commit-queue-               |commit-queue?
              Flags|                            |

--- Comment #4 from Geza Lore <glore at igalia.com> ---
Comment on attachment 455248
  --> https://bugs.webkit.org/attachment.cgi?id=455248
Patch

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

>> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:911
>> +#endif
> 
> We need to avoid these if-defs as much as possible. Why doesn't relinkJump work?
> If we need special handling for tail-call instead, then, we should define relinkTailCall for all assemblers.
> Anyway, let's not add ifdef in AbstractMacroAssembler.h

relinkJump doesn't work  because on ARM jumps (i.e.: jit.jump()) are still far jumps, only jit.nearTailCall() are short branches. Will add relinkTailCall to other assemblers instead of the ifdef.

>> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:-356
>> -#endif // ENABLE(JUMP_ISLANDS)
> 
> Why is it removed? This is necessary to protect it against very small reservation size.

We no longer need this. The jump island in the last region is never used (even if we only have a single region). The allocator now understands this, so no need for this padding. Reservations smaller than (regionSize - islandRegionSize) will just have no islands (as we don't need them). All this is determined in the constructor of FixedVMPoolExecutableAllocator.

>> Source/WTF/wtf/EmbeddedFixedVector.h:52
>> +    }
> 
> We should have much more descriptive name for this create.
> It is hard to know what this create factory function does. For example,
> 
> T::create(3, 1, 2, 3);
> 
> Then, we do not know whether
> 1. we are creating 3 elements array where each element is constructed with 1, 2, 3.
> 2. we are creating 4 elements array, 3, 1, 2, 3

Renamed to createWithSizeAndConstructorArguments

>> Source/WTF/wtf/EmbeddedFixedVector.h:108
>> +
> 
> We should not expose this constructor, and we should define a factory function which has much more descriptive name T::createXXX.
> The rationale is the same to the above one.

This constructor is private (used by createWithSizeAndConstructorArguments above), and I made the constructor of Base protected. This is the cleanest, alternative would be a new constructor of Trailing array that does no initialisation, but then half the constructors initialise inside the class, the other half does not, so it would be a mess and still have additional constructors. Given the constructors of TrailingArray are protected and of EmbeddedFixedVector are private, createWithSizeAndConstructorArguments is the only way of making one.

>> Source/WTF/wtf/FixedVector.h:79
>> +
> 
> Ditto.

Added static createWithSizeAndConstructorArguments factory insted.

>> Source/WTF/wtf/TrailingArray.h:71
>> +
> 
> Ditto.

This is needed by the EmbeddedFixedVector (which is a subclass), so see above. Made all constructors protected however.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20220328/6f6386e5/attachment-0001.htm>