WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Michael Saboff
Comment 1
2013-12-06 11:04:45 PST
Created
attachment 218606
[details]
Patch
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
Committed
r160244
: <
http://trac.webkit.org/changeset/160244
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug