Attachment 219832 Details for Bug 126109 – the patch.

[patch] the patch.

bug-126109.patch (text/plain), 12.26 KB, created by Mark Lam on 2013-12-20 19:26:28 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Lam

Created: 2013-12-20 19:26:28 PST

Size: 12.26 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 160956)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,35 @@
>+2013-12-20  Mark Lam  <mark.lam@apple.com>
>+
>+        CStack: Introduce JSStack::ensureCapacityFor().
>+        https://bugs.webkit.org/show_bug.cgi?id=126109.
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Client code should use JSStack::ensureCapacityFor() when checking for
>+        available stack space for pushing JS frames or making arity adjustments.
>+        JSStack::ensureCapacityFor() works for both cases of the JS stack on the
>+        C stack or as a sperate stack.
>+
>+        JSStack::grow() is now private, and is only used by the C Loop LLINT.
>+
>+        Also made some other JSStack methods private as they are not needed
>+        outside of the JSSTack class.
>+
>+        * dfg/DFGOSREntry.cpp:
>+        (JSC::DFG::prepareOSREntry):
>+        * ftl/FTLOSREntry.cpp:
>+        (JSC::FTL::prepareOSREntry):
>+        * interpreter/Interpreter.cpp:
>+        (JSC::sizeFrameForVarargs):
>+        * interpreter/JSStack.h:
>+        * interpreter/JSStackInlines.h:
>+        (JSC::JSStack::ensureCapacityFor):
>+        (JSC::JSStack::topOfStackForCapacityCheck):
>+        * llint/LLIntSlowPaths.cpp:
>+        (JSC::LLInt::LLINT_SLOW_PATH_DECL):
>+        * runtime/CommonSlowPaths.h:
>+        (JSC::CommonSlowPaths::arityCheckFor):
>+
> 2013-12-20  Filip Pizlo  <fpizlo@apple.com>
> 
>         Arity check stack restoration should preserve the ArgumentCount in case there is a register restoration thunk below it
>Index: Source/JavaScriptCore/dfg/DFGOSREntry.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGOSREntry.cpp	(revision 160949)
>+++ Source/JavaScriptCore/dfg/DFGOSREntry.cpp	(working copy)
>@@ -188,7 +188,7 @@ void* prepareOSREntry(ExecState* exec, C
>     //    would have otherwise just kept running albeit less quickly.
>     
>     unsigned frameSize = jitCode->common.requiredRegisterCountForExecutionAndExit();
>-    if (!vm->interpreter->stack().grow(&exec->registers()[virtualRegisterForLocal(frameSize).offset()])) {
>+    if (!vm->interpreter->stack().ensureCapacityFor(&exec->registers()[virtualRegisterForLocal(frameSize - 1).offset()])) {
>         if (Options::verboseOSR())
>             dataLogF("    OSR failed because stack growth failed.\n");
>         return 0;
>Index: Source/JavaScriptCore/ftl/FTLOSREntry.cpp
>===================================================================
>--- Source/JavaScriptCore/ftl/FTLOSREntry.cpp	(revision 160949)
>+++ Source/JavaScriptCore/ftl/FTLOSREntry.cpp	(working copy)
>@@ -80,7 +80,7 @@ void* prepareOSREntry(
>         scratch[local] = JSValue::encode(values.local(local));
>     
>     int stackFrameSize = entryCode->common.requiredRegisterCountForExecutionAndExit();
>-    if (!vm.interpreter->stack().grow(&exec->registers()[virtualRegisterForLocal(stackFrameSize).offset()])) {
>+    if (!vm.interpreter->stack().ensureCapacityFor(&exec->registers()[virtualRegisterForLocal(stackFrameSize - 1).offset()])) {
>         if (Options::verboseOSR())
>             dataLog("    OSR failed because stack growth failed.\n");
>         return 0;
>Index: Source/JavaScriptCore/interpreter/Interpreter.cpp
>===================================================================
>--- Source/JavaScriptCore/interpreter/Interpreter.cpp	(revision 160949)
>+++ Source/JavaScriptCore/interpreter/Interpreter.cpp	(working copy)
>@@ -160,7 +160,7 @@ CallFrame* sizeFrameForVarargs(CallFrame
>         unsigned argumentCountIncludingThis = callFrame->argumentCountIncludingThis();
>         unsigned paddedCalleeFrameOffset = WTF::roundUpToMultipleOf(stackAlignmentRegisters(), -firstFreeRegister + argumentCountIncludingThis + JSStack::CallFrameHeaderSize + 1);
>         CallFrame* newCallFrame = CallFrame::create(callFrame->registers() - paddedCalleeFrameOffset);
>-        if (argumentCountIncludingThis > Arguments::MaxArguments + 1 || !stack->grow(newCallFrame->registers())) {
>+        if (argumentCountIncludingThis > Arguments::MaxArguments + 1 || !stack->ensureCapacityFor(newCallFrame->registers())) {
>             callFrame->vm().throwException(callFrame, createStackOverflowError(callFrame));
>             return 0;
>         }
>@@ -171,7 +171,7 @@ CallFrame* sizeFrameForVarargs(CallFrame
>         unsigned argumentCountIncludingThis = 1;
>         unsigned paddedCalleeFrameOffset = WTF::roundUpToMultipleOf(stackAlignmentRegisters(),  -firstFreeRegister + argumentCountIncludingThis + JSStack::CallFrameHeaderSize + 1);
>         CallFrame* newCallFrame = CallFrame::create(callFrame->registers() - paddedCalleeFrameOffset);
>-        if (!stack->grow(newCallFrame->registers())) {
>+        if (!stack->ensureCapacityFor(newCallFrame->registers())) {
>             callFrame->vm().throwException(callFrame, createStackOverflowError(callFrame));
>             return 0;
>         }
>@@ -188,7 +188,7 @@ CallFrame* sizeFrameForVarargs(CallFrame
>         unsigned argCount = argsObject->length(callFrame);
>         unsigned paddedCalleeFrameOffset = WTF::roundUpToMultipleOf(stackAlignmentRegisters(), -firstFreeRegister + CallFrame::offsetFor(argCount + 1));
>         CallFrame* newCallFrame = CallFrame::create(callFrame->registers() - paddedCalleeFrameOffset);
>-        if (argCount > Arguments::MaxArguments || !stack->grow(newCallFrame->registers())) {
>+        if (argCount > Arguments::MaxArguments || !stack->ensureCapacityFor(newCallFrame->registers())) {
>             callFrame->vm().throwException(callFrame, createStackOverflowError(callFrame));
>             return 0;
>         }
>@@ -200,7 +200,7 @@ CallFrame* sizeFrameForVarargs(CallFrame
>         unsigned argCount = array->length();
>         unsigned paddedCalleeFrameOffset = WTF::roundUpToMultipleOf(stackAlignmentRegisters(), -firstFreeRegister + CallFrame::offsetFor(argCount + 1));
>         CallFrame* newCallFrame = CallFrame::create(callFrame->registers() - paddedCalleeFrameOffset);
>-        if (argCount > Arguments::MaxArguments || !stack->grow(newCallFrame->registers())) {
>+        if (argCount > Arguments::MaxArguments || !stack->ensureCapacityFor(newCallFrame->registers())) {
>             callFrame->vm().throwException(callFrame, createStackOverflowError(callFrame));
>             return 0;
>         }
>@@ -211,7 +211,7 @@ CallFrame* sizeFrameForVarargs(CallFrame
>     unsigned argCount = argObject->get(callFrame, callFrame->propertyNames().length).toUInt32(callFrame);
>     unsigned paddedCalleeFrameOffset = WTF::roundUpToMultipleOf(stackAlignmentRegisters(), -firstFreeRegister + CallFrame::offsetFor(argCount + 1));
>     CallFrame* newCallFrame = CallFrame::create(callFrame->registers() - paddedCalleeFrameOffset);
>-    if (argCount > Arguments::MaxArguments || !stack->grow(newCallFrame->registers())) {
>+    if (argCount > Arguments::MaxArguments || !stack->ensureCapacityFor(newCallFrame->registers())) {
>         callFrame->vm().throwException(callFrame,  createStackOverflowError(callFrame));
>         return 0;
>     }
>Index: Source/JavaScriptCore/interpreter/JSStack.h
>===================================================================
>--- Source/JavaScriptCore/interpreter/JSStack.h	(revision 160949)
>+++ Source/JavaScriptCore/interpreter/JSStack.h	(working copy)
>@@ -81,6 +81,8 @@ namespace JSC {
>         JSStack(VM&, size_t capacity = defaultCapacity);
>         ~JSStack();
>         
>+        bool ensureCapacityFor(Register* newTopOfStack);
>+
>         void gatherConservativeRoots(ConservativeRoots&);
>         void gatherConservativeRoots(ConservativeRoots&, JITStubRoutineSet&, CodeBlockSet&);
>         void sanitizeStack();
>@@ -92,14 +94,10 @@ namespace JSC {
> 
>         size_t size() const { return highAddress() - lowAddress(); }
> 
>-        bool grow(Register* topOfStack);
>-        
>         static size_t committedByteCount();
>         static void initializeThreading();
> 
>-        Register* topOfFrameFor(CallFrame*);
>         Register* startOfFrameFor(CallFrame*);
>-        Register* topOfStack();
> 
>         CallFrame* pushFrame(class CodeBlock*, JSScope*, int argsCount, JSObject* callee);
> 
>@@ -120,6 +118,11 @@ namespace JSC {
> #endif // !ENABLE(DEBUG_JSSTACK)
> 
>     private:
>+
>+        inline Register* topOfFrameFor(CallFrame*);
>+        inline Register* topOfStack();
>+        inline Register* topOfStackForCapacityCheck();
>+
>         Register* lowAddress() const
>         {
>             return m_end;
>@@ -143,6 +146,7 @@ namespace JSC {
>         void installTrapsAfterFrame(CallFrame*) { }
> #endif
> 
>+        bool grow(Register* topOfStack);
>         bool growSlowCase(Register*);
>         void shrink(Register*);
>         void releaseExcessCapacity();
>Index: Source/JavaScriptCore/interpreter/JSStackInlines.h
>===================================================================
>--- Source/JavaScriptCore/interpreter/JSStackInlines.h	(revision 160949)
>+++ Source/JavaScriptCore/interpreter/JSStackInlines.h	(working copy)
>@@ -33,6 +33,36 @@
> 
> namespace JSC {
> 
>+inline bool JSStack::ensureCapacityFor(Register* newTopOfStack)
>+{
>+#if ENABLE(LLINT_C_LOOP)
>+    return grow(newTopOfStack);
>+#else
>+    ASSERT(wtfThreadData().stack().isGrowingDownward());
>+    Register* topOfStack = topOfStackForCapacityCheck();
>+    if (newTopOfStack > topOfStack)
>+        return true;
>+    size_t neededCapacity = (topOfStack - newTopOfStack) * sizeof(Register);
>+    return m_vm.isSafeToRecurse(neededCapacity);
>+#endif
>+}
>+
>+inline Register* JSStack::topOfStackForCapacityCheck()
>+{
>+#if !ENABLE(LLINT_C_LOOP)
>+    // We're trying to get an estimate of the top of the stack for the purpose
>+    // of a capacity check. If m_topCallFrame is 0, then we can't calculate an
>+    // accurate top of stack value. Just use the current stack position as an
>+    // estimate for the top of the stack from where JS code will start executing.
>+    // This is conservative, but it will be safe.
>+    if (!m_topCallFrame) {
>+        Register* p = reinterpret_cast<Register*>(&p);
>+        return p;
>+    }
>+#endif
>+    return topOfStack() + 1;
>+}
>+
> inline Register* JSStack::topOfFrameFor(CallFrame* frame)
> {
>     if (UNLIKELY(!frame))
>Index: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
>===================================================================
>--- Source/JavaScriptCore/llint/LLIntSlowPaths.cpp	(revision 160949)
>+++ Source/JavaScriptCore/llint/LLIntSlowPaths.cpp	(working copy)
>@@ -441,8 +441,15 @@ LLINT_SLOW_PATH_DECL(stack_check)
>     dataLogF("Num vars = %u.\n", exec->codeBlock()->m_numVars);
>     dataLogF("Current end is at %p.\n", exec->vm().interpreter->stack().end());
> #endif
>-    ASSERT(!exec->vm().interpreter->stack().containsAddress(&exec->registers()[virtualRegisterForLocal(exec->codeBlock()->m_numCalleeRegisters).offset()]));
>-    if (UNLIKELY(!vm.interpreter->stack().grow(&exec->registers()[virtualRegisterForLocal(exec->codeBlock()->m_numCalleeRegisters).offset()]))) {
>+
>+    // For JIT enabled builds which uses the C stack, the stack is not growable.
>+    // Hence, if we get here, then we know a stack overflow is imminent. So, just
>+    // throw the StackOverflowError unconditionally.
>+#if ENABLE(LLINT_C_LOOP)
>+    ASSERT(!exec->vm().interpreter->stack().containsAddress(exec->topOfFrame()));
>+    if (UNLIKELY(!vm.interpreter->stack().ensureCapacityFor(exec->topOfFrame())))
>+#endif
>+    {
>         exec = exec->callerFrame();
>         CommonSlowPaths::interpreterThrowInCaller(exec, createStackOverflowError(exec));
>         pc = returnToThrowForThrownException(exec);
>Index: Source/JavaScriptCore/runtime/CommonSlowPaths.h
>===================================================================
>--- Source/JavaScriptCore/runtime/CommonSlowPaths.h	(revision 160949)
>+++ Source/JavaScriptCore/runtime/CommonSlowPaths.h	(working copy)
>@@ -67,15 +67,8 @@ ALWAYS_INLINE int arityCheckFor(ExecStat
>     int neededStackSpace = missingArgumentCount + 1; // Allow space to save the original return PC.
>     int paddedStackSpace = WTF::roundUpToMultipleOf(stackAlignmentRegisters(), neededStackSpace);
> 
>-#if ENABLE(LLINT_CLOOP)
>-    if (!stack->grow(exec->registers() - passedStackSpace))
>+    if (!stack->ensureCapacityFor(exec->registers() - paddedStackSpace))
>         return -1;
>-#else
>-    UNUSED_PARAM(stack);
>-    if (!exec->vm().isSafeToRecurse(paddedStackSpace * sizeof(Register)))
>-        return -1;
>-#endif // ENABLE(LLINT_CLOOP)
>-
>     return paddedStackSpace / stackAlignmentRegisters();
> }
>

Flags:

msaboff: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 126109: 219832