Bug 131578 – Change callToJavaScript and callToNativeFunction so their callFrames match the native calling conventions

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

[Michael Saboff]

In callToJavaScript and callToNativeFunction we push the frame pointer on the stack where the calling conventions expect it, but this is really a "dummy".  We save another copy of the frame pointer in the sentinel frame that we actually unwind through.  Some of the unwinding code does a simplistic disassembly.  Our dummy does the trick for some unwinding, e.g. Mac OSX x86-64, but doesn't work on other platforms.

The proposed fix is to have callToJavaScript and callToNativeFunction execute a standard prologue to create a calling convention compliant call frame and then link to that call frame when those functions call out to JavaScript or native code respectively.  Then unwinding should work fine for all platforms.

[Michael Saboff]

Created attachment 231341 [details]
Work in Progress

Runs JSC tests on X86-64.

Will work on other platforms and clean up.

[Michael Saboff]

Created attachment 234021 [details]
Now mostly works on X86 (32 & 64)

Need to fix inspector-protocol/debugger LayoutTests

[Geoffrey Garen]

Comment on attachment 234021 [details]
Now mostly works on X86 (32 & 64)

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

> Source/JavaScriptCore/debugger/Debugger.cpp:600
> +    m_pauseOnCallFrame = m_currentCallFrame ? m_currentCallFrame->callerFrameSkippingTopMostEntryFrame() : 0;

"SkippingTopMostEntryFrame" suggests that we will skip only the top-most entry frame -- not other entry frames, and not other host frames. I don't think that can be the right behavior.

> Source/JavaScriptCore/interpreter/CallFrame.h:270
> +        bool isTopmostEntryFrame() const
>          {
> -            return !!this && codeBlock() == vmEntrySentinelCodeBlock();
> +            return callerFrame() == vm().topJSCallingFrame;
>          }

These names are pretty confusing.

"EntryFrame" doesn't describe what entered what.

"Topmost" is arbitrarily different from "top", used for "topCallFrame", but the difference doesn't mean anything.

"topJSCallingFrame" always refers to a host caller to JavaScript, but the name implies that it could refer to any caller, including a JavaScript caller to JavaScript. Also, the word "calling" is ambiguous: it could either mean "the top frame calling JS" or "the top JS frame calling something else". These meanings are opposites, so we need to clarify.

> Source/JavaScriptCore/interpreter/CallFrame.h:293
> +        CallFrame* callerFrameSkippingTopMostEntryFrame()
>          {
> -            CallFrame* caller = callerFrame();
> -            if (caller->isVMEntrySentinel())
> -                return caller->vmEntrySentinelCallerFrame();
> -            return caller;
> +            if (isTopmostEntryFrame())
> +                return vmPriorTopCallFrame();
> +            return callerFrame();
>          }

How do we skip entry frames that are not the top-most entry frame, along with their host caller frames? Unwind logic updates the JSEntryFrame value, but this logic does not.

> Source/JavaScriptCore/interpreter/CallFrame.h:350
> +        CallFrame* vmPriorTopCallFrame() const
> +        {
> +            return callerFrame()[-EntryFrameStateRegisterCount].entryFrameState().topCallFrame;
> +        }

A constant should not need to be negated in order to be used. The constant we want is the offset to the EntryFrameState structure.

[Michael Saboff]

Created attachment 236236 [details]
Updated patch addressing prior review concerns.

Tested on X86 (32 & 64).  In the process of testing on iOS devices.  Untested on WIN (32 & 64).

[Michael Saboff]

Created attachment 236259 [details]
Updated with fixes for arm on iOS.

Refactored alignment adjustments in callToJavaScript including eliminating callToJavaScriptPrologue/callToJavaScriptEpilogue macros.

Not tested yet on X86 (32 & 64) on windows.

[Michael Saboff]

Created attachment 236301 [details]
Updated with speculative Windows changes and fix for patch to apply cleanly

[Geoffrey Garen]

Comment on attachment 236301 [details]
Updated with speculative Windows changes and fix for patch to apply cleanly

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

> Source/JavaScriptCore/jsc.cpp:690
> -    if (!exec->callerFrame()->isVMEntrySentinel())
> -        exec->vm().interpreter->dumpCallFrame(exec->callerFrame());
> +    exec->vm().interpreter->dumpCallFrame(exec->callerFrame());

Why don't we need this check anymore? That's a good think to explain in per-function ChangeLog comments, which are missing in this patch.

> Source/JavaScriptCore/debugger/Debugger.cpp:610
> +    m_pauseOnCallFrame = m_currentCallFrame ? m_currentCallFrame->callerFrameSkippingTopMostJavaScriptEntryFrame() : 0;

Since this call will only skip the top-most entry frame, it seems like you will not see the whole JavaScript stack, if there are intermediate C++ stack frames. Why is that OK?

> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:99
> +    m_callFrame->vm().topCallFrame->iterate(functor);

Why do we need to iterate from topCallFrame instead of just iterating from m_callFrame, or just returning m_callFrame->callerFrame()?

> Source/JavaScriptCore/interpreter/CallFrame.h:165
> -            if (isVMEntrySentinel() || !codeBlock())
> +            if (!codeBlock())

What guarantees that this will no longer be called on the CallToJavaScriptFrame?

> Source/JavaScriptCore/interpreter/CallFrame.h:252
> +        CallFrame* callerFrameUnwind(CallFrame*& currJSEntryFrame)

You should pick one name for the VM entry frame concept and stick with it. The class name you picked was "CallToJavaScriptFrame", so maybe go with that.

> Source/JavaScriptCore/interpreter/CallFrame.h:267
> +        // This only works for the top most contiguous set of JS frames including the callee
> +        // frame of the top most callToJavaScript/callToNativeFunction. In the latter case,
> +        // it returns the prior JS CallFrame, skipping host frames, or nullptr if there isn't
> +        // a prior JS caller.
> +        // General unwinding must use StackVisitor.
> +        CallFrame* callerFrameSkippingTopMostJavaScriptEntryFrame()

This seems broken, since it won't go all the way up the JS stack.

> Source/JavaScriptCore/interpreter/CallToJavaScriptFrame.h:34
> +struct CallToJavaScriptFrame {

Since this isn't the whole stack frame, and is rather a data structure that happens to be located on the stack, we should name it more like the data it represents.

Some options:

VMEntryRecord
CallToJavaScriptRecord

> Source/JavaScriptCore/interpreter/Interpreter.cpp:-693
> -    if (callFrame->isVMEntrySentinel()) {
> -        // This happens when we throw stack overflow in a function that is called
> -        // directly from callToJavaScript. Stack overflow throws the exception in the
> -        // context of the caller. In that case the caller is the sentinel frame. The
> -        // right thing to do is to pretend that the exception is uncaught so that we
> -        // go to the uncaught exception handler, which returns through callToJavaScript.
> -        return 0;
> -    }

Why don't we need this check anymore?

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:206
> +    CallToJavaScriptFrameFromFramePointer(cfr, sp)

Here, we can just subtract sizeof(CallToJavaScriptFrame) from sp instead of using the brittle constant.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:71
> +if C_LOOP
> +    const CalleeSaveRegisterCount = 0
> +elsif X86 or X86_WIN
> +    const CalleeSaveRegisterCount = 3
> +elsif X86_64 or SH4
> +    const CalleeSaveRegisterCount = 5
> +elsif ARM or ARMv7 or ARMv7_TRADITIONAL
> +    const CalleeSaveRegisterCount = 7
> +elsif ARM64 or MIPS
> +    const CalleeSaveRegisterCount = 10
> +end
> +
> +const CalleeRegisterSaveSize = CalleeSaveRegisterCount * PtrSize

Let's remove these brittle constants, and instead of subtracting CalleeRegisterSaveSize from the base pointer, let's use the stack pointer.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:669
> +# CallToJavaScriptFrame* callToJavaScriptFrameFromFramePointer(const CallFrame* callFrame)
> +global _callToJavaScriptFrameFromFramePointer
> +_callToJavaScriptFrameFromFramePointer:
> +    if X86_64
> +        const callFrame = t4
> +        const result = t0
> +    elsif X86 or X86_WIN
> +        const callFrame = t2
> +        const result = t0
> +    else
> +        const callFrame = a0
> +        const result = t0
> +    end
> +
> +    if X86 or X86_WIN
> +        loadp 4[sp], callFrame
> +    end
> +
> +    CallToJavaScriptFrameFromFramePointer(callFrame, result)
> +    ret

It's not such a good interface to use CallFrame* to mean "a pointer to a stack frame that includes the CallToJavaScript structure and some callee-saved registers, which has none of the data members that a CallFrame has". It's very easy to use this API wrong, and try to pass it a CallFrame*.

One way to fix this is to pass in the real CallFrame*, whose parent frame is known to be the callToJavaScript frame, and simply to add the CallFrame header size to it in order to produce a CallToJavaScriptFrame*. Then, you don't even need this global function implemented in assembly.

> Source/JavaScriptCore/runtime/VM.cpp:673
> +        FindFirstCallerFrameWithCodeblockFunctor functor(exec);
> +        topCallFrame->iterate(functor);

Why do we iterate the whole stack, instead of starting from exec?

[Michael Saboff]

(In reply to comment #7)

Discussed in person with Geoff the design and feedback.

Some overall agreements.
 - Eliminate callerFrameSkippingTopMostJavaScriptEntryFrame()
 - Change name of callerFrameUnwind(CallFrame*&) to callerFrame(CallFrame*&).  This is the preferred iterative method for unwinding.
 - Changed CallToJavaScriptFrame name to VMEntryRecord
 - After some discussion, agreed to use platform specific offset to get the VMEntryRecord from a callToJavaScript frame pointer. Given that callToJavaScript needs to find the VMEntryRecord after returning from a  call to JavaScript code or when a stack overflow is detected, argument count rounding logic in C++ code would need to be duplicated in the assembly code which would be as brittle or more than having callee save register count constants in the assembly code.  The brittleness has been somewhat mitigated given that after change set r172429, the constants can and will be located near the actual callee save and restore macros.
 - Agreed that we store the CallFrame* of the first JavaScript callee instead of the pointer to the callToJavaScript frame.  After coding this up, discovered that this won't work due to register preservation stubs and arity fix up code that move the callee's CallFrame* after the call is made.

Response to individual comments inline.

> (From update of attachment 236301 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236301&action=review
> 
> > Source/JavaScriptCore/jsc.cpp:690
> > -    if (!exec->callerFrame()->isVMEntrySentinel())
> > -        exec->vm().interpreter->dumpCallFrame(exec->callerFrame());
> > +    exec->vm().interpreter->dumpCallFrame(exec->callerFrame());
> 
> Why don't we need this check anymore? That's a good think to explain in per-function ChangeLog comments, which are missing in this patch.

Will use the safe callerFrame(CallFrame*&) variant instead of callerFrame().
 
> > Source/JavaScriptCore/debugger/Debugger.cpp:610
> > +    m_pauseOnCallFrame = m_currentCallFrame ? m_currentCallFrame->callerFrameSkippingTopMostJavaScriptEntryFrame() : 0;
> 
> Since this call will only skip the top-most entry frame, it seems like you will not see the whole JavaScript stack, if there are intermediate C++ stack frames. Why is that OK?

Replaced with callerFrame(CallFrame*&).

> > Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:99
> > +    m_callFrame->vm().topCallFrame->iterate(functor);
> 
> Why do we need to iterate from topCallFrame instead of just iterating from m_callFrame, or just returning m_callFrame->callerFrame()?

Because m_callFrame can be some arbitrary frame in the stack and contiguous JavaScript stack frames are linked across intermeaning C++ frame as a singly linked list.

> > Source/JavaScriptCore/interpreter/CallFrame.h:165
> > -            if (isVMEntrySentinel() || !codeBlock())
> > +            if (!codeBlock())
> 
> What guarantees that this will no longer be called on the CallToJavaScriptFrame?

In all current cases, it is called in slow path code from a passed in CallFrame object to find the extent of a stack, either for stacks bound checking or for setting the stack when OSR from the llint to the Baseline JIT.  We could add a debug only check to walk the stack to make sure that the current object doesn't point to a CallToJavaScriptFrame.

> > Source/JavaScriptCore/interpreter/CallFrame.h:252
> > +        CallFrame* callerFrameUnwind(CallFrame*& currJSEntryFrame)
> 
> You should pick one name for the VM entry frame concept and stick with it. The class name you picked was "CallToJavaScriptFrame", so maybe go with that.

Now using VM entry.  This method now has the signature: CallFrame* callerFrame(CallFrame*& currVMEntryFrame)

> > Source/JavaScriptCore/interpreter/CallFrame.h:267
> > +        // This only works for the top most contiguous set of JS frames including the callee
> > +        // frame of the top most callToJavaScript/callToNativeFunction. In the latter case,
> > +        // it returns the prior JS CallFrame, skipping host frames, or nullptr if there isn't
> > +        // a prior JS caller.
> > +        // General unwinding must use StackVisitor.
> > +        CallFrame* callerFrameSkippingTopMostJavaScriptEntryFrame()
> 
> This seems broken, since it won't go all the way up the JS stack.

This has been eliminated.  It's uses have been replaced with callerFrame(CallFrame*&)

> > Source/JavaScriptCore/interpreter/CallToJavaScriptFrame.h:34
> > +struct CallToJavaScriptFrame {
> 
> Since this isn't the whole stack frame, and is rather a data structure that happens to be located on the stack, we should name it more like the data it represents.
> 
> Some options:
> 
> VMEntryRecord
> CallToJavaScriptRecord

Changed to VMEntryRecord

> > Source/JavaScriptCore/interpreter/Interpreter.cpp:-693
> > -    if (callFrame->isVMEntrySentinel()) {
> > -        // This happens when we throw stack overflow in a function that is called
> > -        // directly from callToJavaScript. Stack overflow throws the exception in the
> > -        // context of the caller. In that case the caller is the sentinel frame. The
> > -        // right thing to do is to pretend that the exception is uncaught so that we
> > -        // go to the uncaught exception handler, which returns through callToJavaScript.
> > -        return 0;
> > -    }
> 
> Why don't we need this check anymore?

We should add a nullptr check.  In all cases we'll have a valid CallFrame* or a nullptr.  When a stack overflow is detected in callToJavaScript itself,  we use VM::topCallFrame (which could be a nullptr).  If the entry JS callee detects a stack overflow when it goes to make an outgoing call, the callee's CallFrame* is used for the exception.

> > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:206
> > +    CallToJavaScriptFrameFromFramePointer(cfr, sp)
> 
> Here, we can just subtract sizeof(CallToJavaScriptFrame) from sp instead of using the brittle constant.
> 
> > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:71
> > +if C_LOOP
> > +    const CalleeSaveRegisterCount = 0
> > +elsif X86 or X86_WIN
> > +    const CalleeSaveRegisterCount = 3
> > +elsif X86_64 or SH4
> > +    const CalleeSaveRegisterCount = 5
> > +elsif ARM or ARMv7 or ARMv7_TRADITIONAL
> > +    const CalleeSaveRegisterCount = 7
> > +elsif ARM64 or MIPS
> > +    const CalleeSaveRegisterCount = 10
> > +end
> > +
> > +const CalleeRegisterSaveSize = CalleeSaveRegisterCount * PtrSize
> 
> Let's remove these brittle constants, and instead of subtracting CalleeRegisterSaveSize from the base pointer, let's use the stack pointer.
> 
> > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:669
> > +# CallToJavaScriptFrame* callToJavaScriptFrameFromFramePointer(const CallFrame* callFrame)
> > +global _callToJavaScriptFrameFromFramePointer
> > +_callToJavaScriptFrameFromFramePointer:
> > +    if X86_64
> > +        const callFrame = t4
> > +        const result = t0
> > +    elsif X86 or X86_WIN
> > +        const callFrame = t2
> > +        const result = t0
> > +    else
> > +        const callFrame = a0
> > +        const result = t0
> > +    end
> > +
> > +    if X86 or X86_WIN
> > +        loadp 4[sp], callFrame
> > +    end
> > +
> > +    CallToJavaScriptFrameFromFramePointer(callFrame, result)
> > +    ret

Agreed that this is a better option than the alternatives as discussed above.

> It's not such a good interface to use CallFrame* to mean "a pointer to a stack frame that includes the CallToJavaScript structure and some callee-saved registers, which has none of the data members that a CallFrame has". It's very easy to use this API wrong, and try to pass it a CallFrame*.
> 
> One way to fix this is to pass in the real CallFrame*, whose parent frame is known to be the callToJavaScript frame, and simply to add the CallFrame header size to it in order to produce a CallToJavaScriptFrame*. Then, you don't even need this global function implemented in assembly.

We agree that using CallFrame* to mean "a pointer to a stack frame that includes the CallToJavaScript structure and some callee-saved registers, which has none of the data members that a CallFrame has" is not a good thing.

Given that using a "real CallFrame*, whose parent frame is known to be the callToJavaScript frame" won't work due to callee's changing their CallFrame* after the call, I will come up with more type safe method of addressing the VMEntry frame pointer.

> > Source/JavaScriptCore/runtime/VM.cpp:673
> > +        FindFirstCallerFrameWithCodeblockFunctor functor(exec);
> > +        topCallFrame->iterate(functor);
> 
> Why do we iterate the whole stack, instead of starting from exec?

As described above, the contiguous sets of JavaSCript frames are a singly linked list that needs to be iterated from the top most frame on down.  The CallFrame* for the exception could be the prior topCallFrame that made a C++ call that subsequently reentered JavaScript.

[Michael Saboff]

Created attachment 236667 [details]
Updated patch addressing prior concerns and with other changes from discussion with ggaren

[Geoffrey Garen]

Comment on attachment 236667 [details]
Updated patch addressing prior concerns and with other changes from discussion with ggaren

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

r=me with some naming fixes.

> Source/JavaScriptCore/jsc.cpp:696
>  EncodedJSValue JSC_HOST_CALL functionDumpCallFrame(ExecState* exec)
>  {
> -    if (!exec->callerFrame()->isVMEntrySentinel())
> -        exec->vm().interpreter->dumpCallFrame(exec->callerFrame());
> +    VMEntryFrame* topVMEntryCallFrame = exec->vm().topVMEntryCallFrame;
> +    ExecState* callerFrame = exec->callerFrame(topVMEntryCallFrame);
> +    if (callerFrame)
> +        exec->vm().interpreter->dumpCallFrame(callerFrame);
>      return JSValue::encode(jsUndefined());
>  }
>  #endif

It's weird that this function dumps exec's caller and not exec. That seems like a bug. Oh well. Not new, I guess.

> Source/JavaScriptCore/debugger/Debugger.cpp:610
> +    VMEntryFrame* topVMEntryCallFrame = m_vm->topVMEntryCallFrame;

You should either name the class VMEntryCallFrame or name the variable and the data member vmEntryFrame. It's not so good to name one "EntryFrame" and the other "EntryCallFrame". That makes it sounds like they're subtly different things.

I'd suggest the "EntryFrame" variant, since it is shorter, and we don't really want this class to seem similar to the CallFrame class, since they are different data structures.

> Source/JavaScriptCore/interpreter/StackVisitor.cpp:45
> +    m_frame.m_callerIsVMEntry = false;

Should be "m_callerIsVMEntryFrame" to match the type name.

> Source/JavaScriptCore/llint/LLIntThunks.cpp:109
> +extern "C" VMEntryRecord* vmEntryRecordFromVMEntryFrame(VMEntryFrame* entryFrame)

No need to put "FromVMEntryFrame" in the name here, since it's part of the signature of the function.

[Michael Saboff]

Committed r172665: <http://trac.webkit.org/changeset/172665>

[Csaba Osztrogonác]

(In reply to comment #11)
> Committed r172665: <http://trac.webkit.org/changeset/172665>

It broke the Thumb2 Linux build:

[ 13%] Building CXX object Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/llint/LowLevelInterpreter.cpp.o
{standard input}: Assembler messages:
{standard input}:29: Error: r13 not allowed here -- `subs sp,r7,#48'
{standard input}:41: Error: r13 not allowed here -- `subs sp,r4,#8'
{standard input}:54: Error: r13 not allowed here -- `subs sp,r7,#48'
{standard input}:64: Error: r13 not allowed here -- `subs sp,r7,#28'
{standard input}:139: Error: r13 not allowed here -- `subs sp,r7,#48'
{standard input}:149: Error: r13 not allowed here -- `subs sp,r7,#28'
{standard input}:165: Error: r13 not allowed here -- `subs sp,r7,#48'
{standard input}:177: Error: r13 not allowed here -- `subs sp,r4,#8'
{standard input}:190: Error: r13 not allowed here -- `subs sp,r7,#48'
{standard input}:200: Error: r13 not allowed here -- `subs sp,r7,#28'
{standard input}:261: Error: r13 not allowed here -- `subs sp,r7,#48'
{standard input}:271: Error: r13 not allowed here -- `subs sp,r7,#28'
{standard input}:1502: Error: r13 not allowed here -- `subs sp,r7,#48'
{standard input}:1512: Error: r13 not allowed here -- `subs sp,r7,#28'

[Csaba Osztrogonác]

The problem is that this change reverted the previous fix
for the same issue - https://trac.webkit.org/changeset/163179. :-/

[Csaba Osztrogonác]

Created attachment 236733 [details]
preprocessed LowLevelInterpreter.cpp

[Csaba Osztrogonác]

Created attachment 236734 [details]
assembly

[Csaba Osztrogonác]

new bug report for fixing the build issue - https://bugs.webkit.org/show_bug.cgi?id=136028