Bug 125331 – Split sizing of VarArgs frames from loading arguments for the frame

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

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.

[Michael Saboff]

Created attachment 218606 [details]
Patch

[Geoffrey Garen]

> 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 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]

(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]

(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]

Committed r160244: <http://trac.webkit.org/changeset/160244>