Bug 123642 – Eliminate HostCall bit from JSC Stack CallerFrame

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

[Michael Saboff]

When we move over to the C stack, the current anding of a 1 to the Caller Frame value stored in the CallFrame will break native stack unwinding.  There needs to be a replacement method for determining the transition from native to VM call frames.

There are two requirements for JSC stack frames that need to be considered for the replacement.  The entire stack with a mix of native and JavaScript frames, needs to be walked. For exception processing, the contiguous set of JavaScript frames need to be walked, stopping at the frame where the native code invoked JS.

[Michael Saboff]

Created attachment 215778 [details]
Patch

The changes to jit/JITStubsARM.h, jit/JITStubsARM64.h, jit/JITStubsMIPS.h, jit/JITStubsMSVC64.asm and jit/JITStubsSH4.h are untested.

[WebKit Commit Bot]

Attachment 215778 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp', u'Source/JavaScriptCore/dfg/DFGJITCompiler.cpp', u'Source/JavaScriptCore/interpreter/CallFrame.h', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.h', u'Source/JavaScriptCore/interpreter/JSStackInlines.h', u'Source/JavaScriptCore/interpreter/Register.h', u'Source/JavaScriptCore/interpreter/StackVisitor.cpp', u'Source/JavaScriptCore/interpreter/VMInspector.cpp', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JITOperations.cpp', u'Source/JavaScriptCore/jit/JITStubsARM.h', u'Source/JavaScriptCore/jit/JITStubsARM64.h', u'Source/JavaScriptCore/jit/JITStubsARMv7.h', u'Source/JavaScriptCore/jit/JITStubsMIPS.h', u'Source/JavaScriptCore/jit/JITStubsMSVC64.asm', u'Source/JavaScriptCore/jit/JITStubsSH4.h', u'Source/JavaScriptCore/jit/JITStubsX86.h', u'Source/JavaScriptCore/jit/JITStubsX86_64.h', u'Source/JavaScriptCore/jsc.cpp', u'Source/JavaScriptCore/llint/LowLevelInterpreter.cpp', u'Source/JavaScriptCore/runtime/VM.cpp', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/ScriptController.cpp', u'Source/WebCore/bindings/js/ScriptDebugServer.cpp']" exit_code: 1
Source/JavaScriptCore/jit/JITStubsX86.h:262:  Extra space before [  [whitespace/braces] [5]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 215778 [details]
Patch

Attachment 215778 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/20328032

New failing tests:
plugins/refcount-leaks.html

[Build Bot]

Created attachment 215792 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Mark Lam]

Comment on attachment 215778 [details]
Patch

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?

[Mark Lam]

Comment on attachment 215778 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215778&action=review

>> 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().

Sorry, I was unclear there and also wrote some of my feedback wrongly.  What I meant to say is:

The hostCallerFrame() method tells me that the callFrame arg in setHostCallFrame() is really a caller frame pointer.  This made me wonder why you didn't just store it in the CallFrameHeader's callerFrame field.  And then I remembered that the callerFrame field is reserved for the previous native C frame which is not necessarily the previous JS frame.  But since what you really want to store there is the previous JS frame (which is the VM.topCallFrame at the moment setHostCallFrame() is called), I suggest calling it the savedTopCallFrame pointer instead.

I had also suggested calling the "host call frame" a "sentinel frame" instead.  I'll apply this change also below to illustrate what it looks like for your consideration.  So, to be precise, this is what I'm suggesting:

1. Change hostCallerFrame() to savedTopCalledFrame():

    CallFrame* savedTopCallFrame() const
    {
        ASSERT(isSentinelFrame());
        return this[JSStack::ScopeChain].callFrame();
    }

2. Change setHostCallFrame() to initializeSentinelFrame() as follows:

    void initializeSentinelFrame(CallFrame* topCallFrame)
    {
        ...
        static_cast<Register*>(this)[JSStack::ScopeChain] = topCallFrame;
        ...
    }

3. Change isHostCallFrame() to isSentinelFrame().

4. Change the code at line 430 which restores topCallFrame as follows:

    if (callerFrame->isSentinelFrame()) {
        callFrame->vm().topCallFrame = callerFrame->savedTopCallFrame();
        return false;
    }

5. In JSStack::pushFrame(), change newHostLinkFrame to sentinelFrame:

    CallFrame* sentinelframe = CallFrame::create(newCallFrameSlot + paddedArgsCount + JSStack::CallFrameHeaderSize);
    ...
    sentinelFrame->initSentinelFrame(m_topCallFrame);
    ...
    newCallFrame->init(codeBlock, 0, scope, sentinelFrame, argsCount, callee);

6. In JSStack::popFrame(), change the restoration of topCallFrame to:

    m_topCallFrame = frame->callerFrame()->savedTopCallFrame();
    CallFrame* callerFrame = m_topCallFrame;


Again, my reasoning for the above suggestion is:

1. I find the term "host call frame" confusing as it suggests the native C stack frame that calls into JS (which is how we have always used that term before).  This new sentinel frame is a new construct.  I suggest we make it explicit and call it a SentinelFrame so as to not confuse it with the host call frame which is the native C frame that called into JS.
 
2. I find calling the value stored in the sentinel frame's ScopeChain field the "hostCallerFrame" confusing because:
    a. It is not a native / host caller frame.  It is a JS caller frame of the current JS method. 
    b. It is not store in the callerFrame field of the sentinel frame, but in an overloaded special location i.e. the ScopeChain slot.

    Since that value is also the VM.topCallFrame at that moment the sentinel frame is initialized, why not call it the savedTopCallFrame pointer.  I think framing it this way makes the code clearer.

After thinking this through, I feel more strongly that the code could be made clearer and improved.  So, I'll take back my r+ for now.  What do you think?

[Geoffrey Garen]

> 1. I find the term "host call frame" confusing as it suggests the native C stack frame that calls into JS (which is how we have always used that term before). 

I agree. Good catch.

Michael and I discussed this in person, and came up with these name proposals:

isVMEntrySentinel()
vmEntrySentinelCallerFrame()
callerFrameSkippingVMEntrySentinel()

I also think we should remove setHostCallFrame(), and replace it with CallFrame::createVMEntrySentinel().

I also think that all accessor functions other than vmEntrySentinelCallerFrame() should ASSERT(!isVMEntrySentinel()).

Does that sounds reasonable to you, Mark?

[Mark Lam]

(In reply to comment #7)
> I also think we should remove setHostCallFrame(), and replace it with CallFrame::createVMEntrySentinel().

This is only used in JSStack::pushFrame() where the memory for the sentinel was allocated before hand.  This function is supposed to initialize that memory, which is why I suggested naming it "initialize...()".  The name "create...()" often suggests to me that it allocates memory as well.  That said, I don't think the difference is such a big deal.  I can see how calling it "create...()" can work.

> I also think that all accessor functions other than vmEntrySentinelCallerFrame() should ASSERT(!isVMEntrySentinel()).

Good idea.
 
> Does that sounds reasonable to you, Mark?

Sounds good to me so far.

[Michael Saboff]

Created attachment 215942 [details]
Updated patch

(In reply to comment #7)
> I also think that all accessor functions other than vmEntrySentinelCallerFrame() should ASSERT(!isVMEntrySentinel()).

I tried adding the ASSERT() to the various accessors and it resulted in many crashes.  I appears that we don't have an order that we initialize fields in the header.  Before the CodeBlock is set up for a normal frame, we could have a random 1 value. 

All other suggested changes were made.

[WebKit Commit Bot]

Attachment 215942 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp', u'Source/JavaScriptCore/dfg/DFGJITCompiler.cpp', u'Source/JavaScriptCore/interpreter/CallFrame.h', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.h', u'Source/JavaScriptCore/interpreter/JSStack.cpp', u'Source/JavaScriptCore/interpreter/JSStackInlines.h', u'Source/JavaScriptCore/interpreter/Register.h', u'Source/JavaScriptCore/interpreter/StackVisitor.cpp', u'Source/JavaScriptCore/interpreter/VMInspector.cpp', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JITOperations.cpp', u'Source/JavaScriptCore/jit/JITStubsARM.h', u'Source/JavaScriptCore/jit/JITStubsARM64.h', u'Source/JavaScriptCore/jit/JITStubsARMv7.h', u'Source/JavaScriptCore/jit/JITStubsMIPS.h', u'Source/JavaScriptCore/jit/JITStubsMSVC64.asm', u'Source/JavaScriptCore/jit/JITStubsSH4.h', u'Source/JavaScriptCore/jit/JITStubsX86.h', u'Source/JavaScriptCore/jit/JITStubsX86_64.h', u'Source/JavaScriptCore/jsc.cpp', u'Source/JavaScriptCore/llint/LowLevelInterpreter.cpp', u'Source/JavaScriptCore/runtime/VM.cpp', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/ScriptController.cpp', u'Source/WebCore/bindings/js/ScriptDebugServer.cpp']" exit_code: 1
Source/JavaScriptCore/jit/JITStubsX86.h:262:  Extra space before [  [whitespace/braces] [5]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 215942 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215942&action=review

r=me

> Source/JavaScriptCore/ChangeLog:14
> +        it is a VM entry sentinal call frame.  It's ScopeChain has vm.topCallFrame at the

Should be "Its".

[Mark Lam]

Comment on attachment 215942 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215942&action=review

I'd prefer if you make these edits before landing.  Thanks.

> Source/JavaScriptCore/ChangeLog:8
> +        Replace the HostCallFrame bit anded to the CallerFrame value in a CallFrame with

"anded to" ==> "or'ed to".

> Source/JavaScriptCore/ChangeLog:9
> +        a VM entry sentinal CallFrame.  Logically, the VM entry sentinal call frame is

"sentinal" ==> "sentinel".  See http://dictionary.reference.com/browse/sentinel.

> Source/JavaScriptCore/ChangeLog:12
> +        code.  The callee frame's CallerFrame points at the VM entry sentinal call frame
> +        and the VM entry sentinal call frame's CallerFrame points at the real caller.

nit: "points at" ==> "points to"?

> Source/JavaScriptCore/ChangeLog:16
> +        the contiguous JSC frames.

nit: "JSC frames" suggests all frames used by the VM.  I think "JS frames" is what you really mean here?

> Source/JavaScriptCore/interpreter/CallFrame.h:282
> +        void initializeVMEntrySentinelCallerFrame(CallFrame* callFrame)

Sorry to be picky about this, but would you mind renaming "initializeVMEntrySentinelCallerFrame" to "initializeVMEntrySentinel"?  My reasoning is that everywhere else you use the term "vmEntrySentinelCallerFrame", you are referring to the frame that precedes the vmEntrySentinel frame.  In this case, you are initializing the vmEntrySentinel which happen to take the caller frame as an argument.

> Source/JavaScriptCore/interpreter/JSStackInlines.h:83
> +    CallFrame* newVMEntrySentinalCallerframe = CallFrame::create(newCallFrameSlot + paddedArgsCount + JSStack::CallFrameHeaderSize);
> +    ASSERT(!!newVMEntrySentinalCallerframe);

"newVMEntrySentinalCallerframe" ==> "newVMEntrySentinel"?

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

==> newVMEntrySentinel->initializeVMEntrySentinel(callerFrame);

> Source/JavaScriptCore/interpreter/JSStackInlines.h:98
> +    newCallFrame->init(codeBlock, 0, scope, newVMEntrySentinalCallerframe, argsCount, callee);

ditto.

[Michael Saboff]

(In reply to comment #12)

I made the edit Geoff suggested.

> (From update of attachment 215942 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215942&action=review
> 
> I'd prefer if you make these edits before landing.  Thanks.
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        Replace the HostCallFrame bit anded to the CallerFrame value in a CallFrame with
> 
> "anded to" ==> "or'ed to".
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        a VM entry sentinal CallFrame.  Logically, the VM entry sentinal call frame is
> 
> "sentinal" ==> "sentinel".  See http://dictionary.reference.com/browse/sentinel.
> 
> > Source/JavaScriptCore/ChangeLog:12
> > +        code.  The callee frame's CallerFrame points at the VM entry sentinal call frame
> > +        and the VM entry sentinal call frame's CallerFrame points at the real caller.
> 
> nit: "points at" ==> "points to"?
> 
> > Source/JavaScriptCore/ChangeLog:16
> > +        the contiguous JSC frames.
> 
> nit: "JSC frames" suggests all frames used by the VM.  I think "JS frames" is what you really mean here?
> 
> > Source/JavaScriptCore/interpreter/CallFrame.h:282
> > +        void initializeVMEntrySentinelCallerFrame(CallFrame* callFrame)
> 
> Sorry to be picky about this, but would you mind renaming "initializeVMEntrySentinelCallerFrame" to "initializeVMEntrySentinel"?  My reasoning is that everywhere else you use the term "vmEntrySentinelCallerFrame", you are referring to the frame that precedes the vmEntrySentinel frame.  In this case, you are initializing the vmEntrySentinel which happen to take the caller frame as an argument.
> 
> > Source/JavaScriptCore/interpreter/JSStackInlines.h:83
> > +    CallFrame* newVMEntrySentinalCallerframe = CallFrame::create(newCallFrameSlot + paddedArgsCount + JSStack::CallFrameHeaderSize);
> > +    ASSERT(!!newVMEntrySentinalCallerframe);
> 
> "newVMEntrySentinalCallerframe" ==> "newVMEntrySentinel"?
> 
> > Source/JavaScriptCore/interpreter/JSStackInlines.h:95
> > +    newVMEntrySentinalCallerframe->initializeVMEntrySentinelCallerFrame(callerFrame);
> 
> ==> newVMEntrySentinel->initializeVMEntrySentinel(callerFrame);
> 
> > Source/JavaScriptCore/interpreter/JSStackInlines.h:98
> > +    newCallFrame->init(codeBlock, 0, scope, newVMEntrySentinalCallerframe, argsCount, callee);
> 
> ditto.

I made the suggested edits, except that I changed initializeVMEntrySentinelCallerFrame() to initializeVMEntrySentinelFrame() and newVMEntrySentinalCallerframe to newVMEntrySentinalFrame.

[Michael Saboff]

Committed r158586: <http://trac.webkit.org/changeset/158586>

[Ryosuke Niwa]

It appears that plugins/refcount-leaks.html started failing after this patch got landed:

http://trac.webkit.org/log/?verbose=on&rev=158586&stop_rev=158585

[Michael Saboff]

(In reply to comment #15)
> It appears that plugins/refcount-leaks.html started failing after this patch got landed:
> 
> http://trac.webkit.org/log/?verbose=on&rev=158586&stop_rev=158585

The Debug version of the test works for me.  I'll check release Release after the build finishes.

[Michael Saboff]

(In reply to comment #16)
> (In reply to comment #15)
> > It appears that plugins/refcount-leaks.html started failing after this patch got landed:
> > 
> > http://trac.webkit.org/log/?verbose=on&rev=158586&stop_rev=158585
> 
> The Debug version of the test works for me.  I'll check release Release after the build finishes.

I have reproduced this running Release version of the tests.  This doesn't happen with WebKitTestRunner, only DumpRenderTree.  Investigating further...

[Ryosuke Niwa]

Filed https://bugs.webkit.org/show_bug.cgi?id=123765 to track the failure.