Bug 122982 – Change native function call stubs to use JIT operations instead of ctiVMHandleException

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

CTI Native call is the last user of ctiVMHandleException.  These need to be transitioned to JIT Operations

[Michael Saboff]

Created attachment 214538 [details]
Patch

This patch also fixes https://bugs.webkit.org/show_bug.cgi?id=122980 by eliminating the problem.

[Michael Saboff]

*** Bug 122980 has been marked as a duplicate of this bug. ***

[Geoffrey Garen]

Comment on attachment 214538 [details]
Patch

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.

[Build Bot]

Comment on attachment 214538 [details]
Patch

Attachment 214538 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/4578002

[Alex Christensen]

Delete "EXTERN cti_vm_handle_exception : near" in JITStubsMSVC64.asm and the MSVC ctiVMHandleException implementation in JITStubsX86.h and this compiles successfully.  It runs the tests successfully on Win32 but not on Win64.

[Michael Saboff]

(In reply to comment #3)
> (From update of attachment 214538 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214538&action=review
> 
> r=me
> 
> > 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.

The original code for cti_vm_handle_exception handled the case where callFrame is null.  That version was returning a struct with the catch callFrame/PC.  Given that we are now using the VM to pass back those values, we need a VM even if we don't have a callFrame.  That's why I passed it in as an argument.  Saying that, I have instrumented the code and it doesn't appear that we ever exercise that path.  Unless we can come up with a scenario where we need to unwind and exception and won't have a valid callFrame, I'll remove the VM argument and corresponding code.

> > Source/JavaScriptCore/jit/JITOperations.cpp:1654
> > +    vm->topCallFrame = exec;
> 
> We don't need this. NativeCallFrameTracer takes care of this for us.

I'll remove.
 
> > 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.

It appears we never consumed the values from the VM.

I'll make the helpers requested.

[Michael Saboff]

(In reply to comment #5)
> Delete "EXTERN cti_vm_handle_exception : near" in JITStubsMSVC64.asm and the MSVC ctiVMHandleException implementation in JITStubsX86.h and this compiles successfully.  It runs the tests successfully on Win32 but not on Win64.

Alex, thanks for the help.  I removed the dead references from those two files.  I also found another in jit/JITStubsARM.h.

[Michael Saboff]

Committed r157636: <http://trac.webkit.org/changeset/157636>