Bug 122289 – Transition stack check JITStubs to CCallHelper functions

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

[Michael Saboff]

The JITStubs cti_stack_check, cti_op_call_arityCheck & cti_op_construct_arityCheck need to be transitioned to CCallHelper functions and likely moved to JITOperations.

[Michael Saboff]

Created attachment 213417 [details]
Works for X64-64 baseline JIT

Next DFG & FTL

[Michael Saboff]

Created attachment 213419 [details]
Now DFG 64 bit is working

[Michael Saboff]

Created attachment 213430 [details]
Patch

[Geoffrey Garen]

Comment on attachment 213430 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:128
> +        // Remove hostCallFlag from caller

Should be "hostCallFrameFlag". Please add a period to make this a sentence.

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:131
> +        andPtr(TrustedImmPtr(reinterpret_cast<void *>(~CallFrame::hostCallFrameFlag())), GPRInfo::argumentGPR0);

Should be "void*".

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:386
> +    m_speculative->callOperationCheckCallerException(operationStackCheck, m_codeBlock);

This is an awkward read. It sounds like you're saying "check for an exception thrown by my caller". How about "callOperationDuringCallFrameInitialization" or "callOperationWithCallFrameRollbackOnException"?

> Source/JavaScriptCore/dfg/DFGJITCompiler.h:263
> -    
> +

Please revert.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1727
>          prepareForExternalCall();
>          m_jit.emitStoreCodeOrigin(m_currentNode->codeOrigin);
>          JITCompiler::Call call = m_jit.appendCall(function);
> +        m_jit.exceptionCheckForCaller();
> +        return call;
> +    }
> +    JITCompiler::Call appendCallWithCallerExceptionCheck(const FunctionPtr& function)
> +    {
> +        prepareForExternalCall();
> +        m_jit.emitStoreCodeOrigin(m_currentNode->codeOrigin);
> +        JITCompiler::Call call = m_jit.appendCall(function);
>          m_jit.exceptionCheck();
>          return call;
>      }

Is this backwards? "appendCallWithCallerExceptionCheck" seems to call "exceptionCheck", while "appendCallWithExceptionCheck" seems to call "exceptionCheckForCaller".

> Source/JavaScriptCore/ftl/FTLLink.cpp:110
> +        // Until then, use a JIT ASSERT

Please add a period, to make this a complete sentence.

> Source/JavaScriptCore/ftl/FTLLink.cpp:136
> +        jit.load64(state.graph.m_vm.addressOfException(), GPRInfo::regT1);

Ditto.

> Source/JavaScriptCore/jit/JITOperations.cpp:44
> +    // We pass in our own code block, because the callframe hasn't been populated.
> +    CodeBlock* codeBlock = static_cast<CodeBlock*>(codeBlockPtr);

Why isn't the function's argument type CodeBlock*?

[Michael Saboff]

Created attachment 213538 [details]
Updated patch addressing review comments

[Filip Pizlo]

Comment on attachment 213538 [details]
Updated patch addressing review comments

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

r=me, but don't use "B" for CodeBlock*.

> Source/JavaScriptCore/jit/JITOperations.h:54
> +    B: CodeBlock*

I've been moving towards having new "types" use multi-character names.  The convention is that it's always first letter capitalized, all other letters lower case.  I think that "Cb" would be intuitive enough.

[Michael Saboff]

(In reply to comment #6)
> (From update of attachment 213538 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213538&action=review
> 
> r=me, but don't use "B" for CodeBlock*.
> 
> > Source/JavaScriptCore/jit/JITOperations.h:54
> > +    B: CodeBlock*
> 
> I've been moving towards having new "types" use multi-character names.  The convention is that it's always first letter capitalized, all other letters lower case.  I think that "Cb" would be intuitive enough.

I'll use Cb.  I didn't notice the multi-letter names in the comment, but now see them used in the declarations.

[Geoffrey Garen]

Comment on attachment 213538 [details]
Updated patch addressing review comments

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1102
> +    JITCompiler::Call callOperationWithCallFrameRollbackOnException(V_JITOperation_EB operation, void* pointer)

Let's make all the names consistent with this name:

    appendCallWithCallerExceptionCheck => appendCallWithCallFrameRollbackOnException
    appendCallWithCallerExceptionCheckSetResult => appendCallWithCallFrameRollbackOnExceptionSetResult
    exceptionCheckForCaller => exceptionCheckWithCallFrameRollback
    m_exceptionChecksForCaller => m_exceptionChecksWithCallFrameRollback
    exceptionChecksForCaller => exceptionChecksWithCallFrameRollback

[Michael Saboff]

(In reply to comment #8)
> (From update of attachment 213538 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213538&action=review
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1102
> > +    JITCompiler::Call callOperationWithCallFrameRollbackOnException(V_JITOperation_EB operation, void* pointer)
> 
> Let's make all the names consistent with this name:
> 
>     appendCallWithCallerExceptionCheck => appendCallWithCallFrameRollbackOnException
>     appendCallWithCallerExceptionCheckSetResult => appendCallWithCallFrameRollbackOnExceptionSetResult
>     exceptionCheckForCaller => exceptionCheckWithCallFrameRollback
>     m_exceptionChecksForCaller => m_exceptionChecksWithCallFrameRollback
>     exceptionChecksForCaller => exceptionChecksWithCallFrameRollback

I made these changes.

[Michael Saboff]

Committed r157050: <http://trac.webkit.org/changeset/157050>