[webkit-reviews] review granted: [Bug 220500] [JSC] JITCage's Gate mechanism is used in ARM64E even if JITCage is used for simplicity : [Attachment 417347] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 220500: [JSC] JITCage's Gate mechanism is used in ARM64E even if JITCage is
used for simplicity
https://bugs.webkit.org/show_bug.cgi?id=220500

Attachment 417347: Patch

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




--- Comment #4 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 417347
  --> https://bugs.webkit.org/attachment.cgi?id=417347
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:3
> +	   [JSC] JITCage's Gate mechanism is used in ARM64E even if JITCage is
used for simplicity

I think you meant "JITCage is disabled" not "JITCage is used" here.  The "for
simplicity" part could be mistaken to mean that the JITCage is disabled for
simplicity when we meant that the gate mechanism is used for simplicity sake. 
I suggest rephrasing this is as "JITCage's Gate mechanism is used in ARM64E
even if JITCage is disable."  Just drop the "simplicity" part: it's not an
essential detail about this bug and it is more clearly explained in the comment
below.

> Source/JavaScriptCore/ChangeLog:9
> +	   in LLInt we are always using Gate even if ENABLE(JIT_CAGE) is OFF
because it makes LLInt code

I suggest breaking this sentence up as follows: "ENABLE(JIT_CAGE) is OFF
because it makes LLInt" ==> "ENABLE(JIT_CAGE) is OFF.  It makes LLInt"

> Source/JavaScriptCore/llint/LLIntThunks.cpp:348
>  MacroAssemblerCodeRef<NativeToJITGatePtrTag> createJSGateThunk(void*
pointer, PtrTag tag, const char* name)

nit: the .h file does not #if out the prototype for these functions.  Would be
nice to make it consistent.

> Source/JavaScriptCore/llint/LLIntThunks.cpp:494
>  MacroAssemblerCodeRef<NativeToJITGatePtrTag> jitCagePtrThunk()

Ditto nit.