[webkit-reviews] review denied: [Bug 122289] Transition stack check JITStubs to CCallHelper functions : [Attachment 213430] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Oct 6 13:12:51 PDT 2013
Geoffrey Garen <ggaren at apple.com> has denied Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 122289: Transition stack check JITStubs to CCallHelper functions
https://bugs.webkit.org/show_bug.cgi?id=122289
Attachment 213430: Patch
https://bugs.webkit.org/attachment.cgi?id=213430&action=review
------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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*?
More information about the webkit-reviews
mailing list