RESOLVED FIXED 125331
Split sizing of VarArgs frames from loading arguments for the frame
https://bugs.webkit.org/show_bug.cgi?id=125331
Summary Split sizing of VarArgs frames from loading arguments for the frame
Michael Saboff
Reported 2013-12-05 20:25:33 PST
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.
Attachments
Patch (21.38 KB, patch)
2013-12-06 11:04 PST, Michael Saboff
fpizlo: review+
Michael Saboff
Comment 1 2013-12-06 11:04:45 PST
Geoffrey Garen
Comment 2 2013-12-06 11:23:10 PST
> 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.
Mark Lam
Comment 3 2013-12-06 11:33:14 PST
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.
Michael Saboff
Comment 4 2013-12-06 13:16:34 PST
(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.
Michael Saboff
Comment 5 2013-12-06 13:19:39 PST
(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.
Michael Saboff
Comment 6 2013-12-06 13:35:37 PST
Note You need to log in before you can comment on or make changes to this bug.