The C helper loadVarargs() in Interpreter.cpp is currently responsible for sizing and creating the calleeFrame for a variable argument call. When we transition to the C stack, we can't allocate space on the stack possibly in use by loadVarargs() and its callers operationLoadVarargs() and llint_slow_path_call_varargs(). loadVarargs() can be split into two phases, the first function to compute size of the new frame and the second function to fill in the argument values. The first function would return the size to the JIT or LLint engine to allocate the frame. After allocating the frame, the second function can freely populate the new frame. In preparation, this patch can have the first function size and allocate the new frame. When the transition is made to the C stack, the first function will be changed as described.
Created attachment 218606 [details] Patch
> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1118 > + vm.newCallFrameReturnValue = execCallee; This is pretty weird. Why can't we just return execCallee as a void*? > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:559 > +macro branchIfException(label) > + loadp ScopeChain[cfr], t3 > + andp MarkedBlockMask, t3 > + loadp MarkedBlock::m_weakSet + WeakSet::m_vm[t3], t3 > + bineq VM::m_exception + TagOffset[t3], EmptyValueTag, label > +end Can we just have _llint_slow_path_size_varargs return 0 in the failure case? > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:727 > + # Check for exception from _llint_slow_path_size_varargs This comment doesn't add anything.
Comment on attachment 218606 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218606&action=review > Source/JavaScriptCore/ChangeLog:9 > + prepartion for moving onto the C stack. sizeAndAllocFrameForVarargs() will prepartion ==> preparation. > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1108 > +LLINT_SLOW_PATH_DECL(slow_path_size_varargs) Why not call this “slow_path_size_and_alloc_frame_for_varargs” (similar to what you did for JIT) to be more clear of its purpose? > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1114 > + ExecState* execCallee = sizeAndAllocFrameForVarargs(exec, &vm.interpreter->stack(), I prefer “calleeCallFrame” (or “newCallFrame”) to “execCallee”. “execCallee” sounds too much like “the ExecState’s callee field” which is misleading.
(In reply to comment #2) > > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1118 > > + vm.newCallFrameReturnValue = execCallee; > > This is pretty weird. Why can't we just return execCallee as a void*? The llint slow paths always return a PC, ExecState* pair. Any slow path that wants to return a value writes into the virtual register array based on opcode args. I'm reluctant to return the new CallFrame* as the ExecState* return value as that breaks the convention. > > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:559 > > +macro branchIfException(label) > > + loadp ScopeChain[cfr], t3 > > + andp MarkedBlockMask, t3 > > + loadp MarkedBlock::m_weakSet + WeakSet::m_vm[t3], t3 > > + bineq VM::m_exception + TagOffset[t3], EmptyValueTag, label > > +end > > Can we just have _llint_slow_path_size_varargs return 0 in the failure case? > > > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:727 > > + # Check for exception from _llint_slow_path_size_varargs > > This comment doesn't add anything. Removed.
(In reply to comment #3) > (From update of attachment 218606 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=218606&action=review > > > Source/JavaScriptCore/ChangeLog:9 > > + prepartion for moving onto the C stack. sizeAndAllocFrameForVarargs() will > > prepartion ==> preparation. Fixed. > > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1108 > > +LLINT_SLOW_PATH_DECL(slow_path_size_varargs) > > Why not call this “slow_path_size_and_alloc_frame_for_varargs” (similar to what you did for JIT) to be more clear of its purpose? Done. > > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1114 > > + ExecState* execCallee = sizeAndAllocFrameForVarargs(exec, &vm.interpreter->stack(), > > I prefer “calleeCallFrame” (or “newCallFrame”) to “execCallee”. “execCallee” sounds too much like “the ExecState’s callee field” which is misleading. execCallee is the current convention in the file the callee's ExecState.
Committed r160244: <http://trac.webkit.org/changeset/160244>