Bug 220500

Summary:

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

Product:

WebKit

Reporter:

Yusuke Suzuki <ysuzuki>

Component:

New Bugs

Assignee:

Yusuke Suzuki <ysuzuki>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	mark.lam: review+

Description Yusuke Suzuki 2021-01-10 00:37:16 PST

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

Comment 1 Yusuke Suzuki 2021-01-10 00:39:53 PST

Created attachment 417346 [details]
Patch

Comment 2 Yusuke Suzuki 2021-01-10 00:54:22 PST

Created attachment 417347 [details]
Patch

Comment 3 Mark Lam 2021-01-10 01:02:43 PST

rdar://72965575

Comment 4 Mark Lam 2021-01-10 10:13:10 PST

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.

Comment 5 Yusuke Suzuki 2021-01-10 21:39:15 PST

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.

Comment 6 Yusuke Suzuki 2021-01-10 21:40:15 PST

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