[webkit-reviews] review granted: [Bug 122982] Change native function call stubs to use JIT operations instead of ctiVMHandleException : [Attachment 214538] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 122982: Change native function call stubs to use JIT operations instead of
ctiVMHandleException
https://bugs.webkit.org/show_bug.cgi?id=122982

Attachment 214538: Patch
https://bugs.webkit.org/attachment.cgi?id=214538&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=214538&action=review


r=me

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:155
> +    // lookupExceptionHandler leaves the handler CallFrame* in
vm->callFrameForThrow,
> +    // and the address of the handler in vm->targetMachinePCForThrow.
> +    move(TrustedImmPtr(m_vm), GPRInfo::regT0);
> +    loadPtr(Address(GPRInfo::regT0, VM::targetMachinePCForThrowOffset()),
GPRInfo::regT1);
> +    loadPtr(Address(GPRInfo::regT0, VM::callFrameForThrowOffset()),
GPRInfo::regT0);
> +    jump(GPRInfo::regT1);

Can you put this in a helper function?

> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:195
> +    // operationVMHandleException leaves the handler CallFrame* in
vm->callFrameForThrow,
> +    // and the address of the handler in vm->targetMachinePCForThrow.
> +    move(TrustedImmPtr(vm), regT0);
> +    loadPtr(Address(regT0, VM::targetMachinePCForThrowOffset()), regT1);
> +    loadPtr(Address(regT0, VM::callFrameForThrowOffset()), regT0);
>      jump(regT1);

Can you put this in a helper function? (It should be the same on 32-bit:
loadPtr in both cases.)

> Source/JavaScriptCore/jit/JITOperations.cpp:1642
> +void JIT_OPERATION operationVMHandleException(ExecState* exec, VM* vm)

Why do we need the VM* argument? Let's remove it, to simplify callers.

> Source/JavaScriptCore/jit/JITOperations.cpp:1654
> +    vm->topCallFrame = exec;

We don't need this. NativeCallFrameTracer takes care of this for us.

> Source/JavaScriptCore/runtime/VM.h:352
> +	   static ptrdiff_t callFrameForThrowOffset()
> +	   {
> +	       return OBJECT_OFFSETOF(VM, callFrameForThrow);
> +	   }
> +
> +	   static ptrdiff_t targetMachinePCForThrowOffset()
> +	   {
> +	       return OBJECT_OFFSETOF(VM, targetMachinePCForThrow);
> +	   }

Interesting: Looks like we were doing this already -- just inconsistently.