[webkit-reviews] review granted: [Bug 123642] Eliminate HostCall bit from JSC Stack CallerFrame : [Attachment 215778] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Michael Saboff <msaboff at apple.com>'s
request for review:
Bug 123642: Eliminate HostCall bit from JSC Stack CallerFrame
https://bugs.webkit.org/show_bug.cgi?id=123642

Attachment 215778: Patch
https://bugs.webkit.org/attachment.cgi?id=215778&action=review

------- Additional Comments from Mark Lam <mark.lam at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=215778&action=review


r=me.  Please consider my nit-picking suggestion on "savedTopCallFrame()", but
I won't be heart broken if you choose not to adopt it.

> Source/JavaScriptCore/ChangeLog:19
> +	   Replace the HostCallFrame bit anded to the CallerFrame value in a
CallFrame with
> +	   a dummy host CallFrame.  Logically, the host call frame is pushed on
the stack
> +	   before the callee frame when calling from native to JavaScript code.
 The
> +	   callee frame's CallerFrame points at the host call frame and the
host call frame's
> +	   CallerFrame points at the real caller.  The host call frame has a
sentinal (1) in
> +	   the CodeBlock to indicate it is a host call frame.  It's ScopeChain
has
> +	   vm.topCallFrame at the time of the call.  This allows for a complete
stack walk
> +	   as well as walking just the contiguous JSC frames.
> +
> +	   The host call frame and callee frame are currently allocated and
initialized in
> +	   ExecState::init(), but this initialization will be moved to
ctiTrampoline when
> +	   we actually move onto the native stack.

I wonder if it'd be better if we start calling this the "sentinel frame"
instead of the "host call frame".  That's because "host call frame" sounds too
much like the native C frame that preceeds the JS frame (especially when we
start mingling the frames on the native C stack later).  Just a suggestion.

> Source/JavaScriptCore/interpreter/CallFrame.h:286
> +	   void setHostCallFrame(CallFrame* callFrame)
> +	   {
> +	       setCallerFrame(noCaller());
> +	       setReturnPC(0);
> +	       setCodeBlock(hostCodeBlock());
> +	       static_cast<Register*>(this)[JSStack::ScopeChain] = callFrame;

Nit: I'd prefer that the callFrame argument be named topCallFrame since that is
what it is expected to be.

The reason this came to mind was because the first time I read this code, it
made me wonder why you didn't just store the callerFrame in the callerFrame
field.	And then I realized that once we move to using the native C stack, the
set value will not be the true caller frame, but is the saved value of the JS
topCallFrame instead.  This made me feel that it might be better to spell that
out by renaming:

1. setHostCallFrame() ==> setSavedTopCallFrame()
2. the callFrame arg ==> topCallFrame

Similarly, rename the hostCallerFrame() method above to savedTopCallFrame().

> Source/JavaScriptCore/interpreter/Interpreter.cpp:430
> +	   callFrame->vm().topCallFrame = callerFrame->hostCallerFrame();

Nit: I'm a bit uncomfortable with calling this "hostCallerFrame()".  Why don't
we just make it more explicit and call it "cachedTopCallFrame()" or
"savedTopCallFrame()" instead?	I think it'll read better and communicate the
intent more clearly that way.

> Source/JavaScriptCore/interpreter/JSStackInlines.h:95
> +    newHostLinkCallframe->setHostCallFrame(callerFrame);

Ditto nit:  setHostCallFrame(callerFrame) ==>
setSavedTopCallFrame(m_topCallFrame);

> Source/JavaScriptCore/interpreter/JSStackInlines.h:126
> +    // Pop off the callee frame and the host link frame.
> +    CallFrame* callerFrame = frame->callerFrame()->hostCallerFrame();
>  
>      // Pop to the caller:
>      m_topCallFrame = callerFrame;

Ditto nit.

> Source/JavaScriptCore/runtime/VM.cpp:656
>	   CallFrame* callFrame;
> -	   for (callFrame = exec; callFrame && !callFrame->codeBlock();
callFrame = callFrame->callerFrame()->removeHostCallFrameFlag())
> +	   for (callFrame = exec; callFrame && !callFrame->codeBlock(); ) {
>	       stackIndex++;
> +	       callFrame = callFrame->callerFrameOrHostCallerFrame();
> +	   }

why not just change this into a while loop?