[webkit-reviews] review requested: [Bug 212825] Make FAST_JIT_PERMISSIONS check in LinkBuffer::copyCompactAndLinkCode a runtime check : [Attachment 401188] Patch with suggested changes

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has asked  for review:
Bug 212825: Make FAST_JIT_PERMISSIONS check in
LinkBuffer::copyCompactAndLinkCode a runtime check
https://bugs.webkit.org/show_bug.cgi?id=212825

Attachment 401188: Patch with suggested changes

https://bugs.webkit.org/attachment.cgi?id=401188&action=review




--- Comment #6 from Michael Saboff <msaboff at apple.com> ---
Created attachment 401188

  --> https://bugs.webkit.org/attachment.cgi?id=401188&action=review

Patch with suggested changes

(In reply to Saam Barati from comment #5)
> Comment on attachment 401176 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401176&action=review
> 
> > Source/JavaScriptCore/assembler/LinkBuffer.cpp:282
> > +	 IF_FAST_JIT_PERMISSIONS(os_thread_self_restrict_rwx_to_rw());
> 
> why not just use a normal if statement here? Doesn't feel like canonical
> webkit style.
> 
> I think 
> if (useFastJITPermissions())
> is much easier to parse and read than macros as control flow.
> 
> > Source/JavaScriptCore/assembler/LinkBuffer.cpp:371
> > +	    
IF_FAST_JIT_PERMISSIONS_ELSE(MacroAssembler::link<memcpy>(jumpsToLink[i],
outData + jumpsToLink[i].from(), location, target), \
> > +		 MacroAssembler::link<performJITMemcpy>(jumpsToLink[i], outData
+ jumpsToLink[i].from(), location, target));
> 
> same comment

Here's a patch with the suggested changes.