[JSC] JITCage's Gate mechanism is used in ARM64E even if JITCage is used for simplicity
Created attachment 417346 [details] Patch
Created attachment 417347 [details] Patch
rdar://72965575
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 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.
Committed r271352: <https://trac.webkit.org/changeset/271352>