Bug 125331 - Split sizing of VarArgs frames from loading arguments for the frame
Summary: Split sizing of VarArgs frames from loading arguments for the frame
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks: 116888
  Show dependency treegraph
 
Reported: 2013-12-05 20:25 PST by Michael Saboff
Modified: 2013-12-06 13:35 PST (History)
1 user (show)

See Also:


Attachments
Patch (21.38 KB, patch)
2013-12-06 11:04 PST, Michael Saboff
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 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.
Comment 1 Michael Saboff 2013-12-06 11:04:45 PST
Created attachment 218606 [details]
Patch
Comment 2 Geoffrey Garen 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.
Comment 3 Mark Lam 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.
Comment 4 Michael Saboff 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.
Comment 5 Michael Saboff 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.
Comment 6 Michael Saboff 2013-12-06 13:35:37 PST
Committed r160244: <http://trac.webkit.org/changeset/160244>