Bug 220500 – [JSC] JITCage's Gate mechanism is used in ARM64E even if JITCage is disable

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] JITCage's Gate mechanism is used in ARM64E even if JITCage is used for simplicity

[Yusuke Suzuki]

Created attachment 417346 [details]
Patch

[Yusuke Suzuki]

Created attachment 417347 [details]
Patch

[Mark Lam]

rdar://72965575

[Mark Lam]

Comment on attachment 417347 [details]
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.

[Yusuke Suzuki]

Comment on attachment 417347 [details]
Patch

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

Thanks!

>> 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.

Changed.

>> 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"

Changed.

>> 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.

Changed.

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

Changed.

[Yusuke Suzuki]

Committed r271352: <https://trac.webkit.org/changeset/271352>