Changeset 174359 in webkit

Timestamp:

Oct 6, 2014 12:29:27 PM (10 years ago)

Author:

oliver@apple.com

Message:

REGRESSION(r174226): [JSC] Crash when running the perf test Speedometer/Full.html
​https://bugs.webkit.org/show_bug.cgi?id=137404

Reviewed by Michael Saboff.

Update the Arguments object to recognise that it must always have an
environment record if the referenced callee has one, and if such is not
present it should not try to extract one from the callframe, as that
path leads to madness.

Happily this makes some of the other code more sensible, and removes a
bunch of unnecessary and icky logic.

• interpreter/Interpreter.cpp:

(JSC::unwindCallFrame):

• jit/JITOperations.cpp:
• llint/LLIntSlowPaths.cpp:

(JSC::LLInt::LLINT_SLOW_PATH_DECL):

• runtime/Arguments.cpp:

(JSC::Arguments::tearOff):
(JSC::Arguments::didTearOffActivation): Deleted.

• runtime/Arguments.h:

(JSC::Arguments::argument):
(JSC::Arguments::finishCreation):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• interpreter/Interpreter.cpp (modified) (2 diffs)
• jit/JITOperations.cpp (modified) (1 diff)
• llint/LLIntSlowPaths.cpp (modified) (1 diff)
• runtime/Arguments.cpp (modified) (2 diffs)
• runtime/Arguments.h (modified) (4 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2014-10-06  Oliver Hunt  <oliver@apple.com>
+
+        REGRESSION(r174226): [JSC] Crash when running the perf test Speedometer/Full.html
+        https://bugs.webkit.org/show_bug.cgi?id=137404
+
+        Reviewed by Michael Saboff.
+
+        Update the Arguments object to recognise that it must always have an
+        environment record if the referenced callee has one, and if such is not
+        present it should not try to extract one from the callframe, as that
+        path leads to madness.
+
+        Happily this makes some of the other code more sensible, and removes a
+        bunch of unnecessary and icky logic.
+
+        * interpreter/Interpreter.cpp:
+        (JSC::unwindCallFrame):
+        * jit/JITOperations.cpp:
+        * llint/LLIntSlowPaths.cpp:
+        (JSC::LLInt::LLINT_SLOW_PATH_DECL):
+        * runtime/Arguments.cpp:
+        (JSC::Arguments::tearOff):
+        (JSC::Arguments::didTearOffActivation): Deleted.
+        * runtime/Arguments.h:
+        (JSC::Arguments::argument):
+        (JSC::Arguments::finishCreation):
+
 2014-10-04  Brian J. Burg  <burg@cs.washington.edu>
 

trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp

@@ -449 +449 @@
     }
 
-    JSValue lexicalEnvironment;
     if (codeBlock->codeType() == FunctionCode && codeBlock->needsActivation()) {
 #if ENABLE(DFG_JIT)
@@ -458 +457 @@
     if (codeBlock->codeType() == FunctionCode && codeBlock->usesArguments()) {
         if (Arguments* arguments = visitor->existingArguments()) {
-            if (lexicalEnvironment && lexicalEnvironment.isCell())
-                arguments->didTearOffActivation(callFrame, jsCast<JSLexicalEnvironment*>(lexicalEnvironment));
 #if ENABLE(DFG_JIT)
-            else if (visitor->isInlinedFrame())
+            if (visitor->isInlinedFrame())
                 arguments->tearOff(callFrame, visitor->inlineCallFrame());
 #endif

trunk/Source/JavaScriptCore/jit/JITOperations.cpp

@@ -1591 +1591 @@
 }
 
-void JIT_OPERATION operationTearOffArguments(ExecState* exec, JSCell* argumentsCell, JSCell* activationCell)
+void JIT_OPERATION operationTearOffArguments(ExecState* exec, JSCell* argumentsCell, JSCell*)
 {
     ASSERT(exec->codeBlock()->usesArguments());
-    if (activationCell) {
-        jsCast<Arguments*>(argumentsCell)->didTearOffActivation(exec, jsCast<JSLexicalEnvironment*>(activationCell));
-        return;
-    }
     jsCast<Arguments*>(argumentsCell)->tearOff(exec);
 }

trunk/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp

@@ -1251 +1251 @@
     ASSERT(exec->codeBlock()->usesArguments());
     Arguments* arguments = jsCast<Arguments*>(exec->uncheckedR(VirtualRegister(pc[1].u.operand).offset()).jsValue());
-    if (JSValue activationValue = LLINT_OP_C(2).jsValue())
-        arguments->didTearOffActivation(exec, jsCast<JSLexicalEnvironment*>(activationValue));
-    else
-        arguments->tearOff(exec);
+    arguments->tearOff(exec);
     LLINT_END();
 }

trunk/Source/JavaScriptCore/runtime/Arguments.cpp

@@ -373 +373 @@
 void Arguments::tearOff(CallFrame* callFrame)
 {
+    if (m_callee->jsExecutable()->needsActivation())
+        RELEASE_ASSERT(m_lexicalEnvironment);
     if (isTornOff())
         return;
@@ -392 +394 @@
 }
 
-void Arguments::didTearOffActivation(ExecState* exec, JSLexicalEnvironment* lexicalEnvironment)
-{
-    RELEASE_ASSERT(lexicalEnvironment);
-    if (isTornOff())
-        return;
-
-    if (!m_numArguments)
-        return;
-    
-    m_lexicalEnvironment.set(exec->vm(), this, lexicalEnvironment);
-    tearOff(exec);
-}
-
 void Arguments::tearOff(CallFrame* callFrame, InlineCallFrame* inlineCallFrame)
 {
+    RELEASE_ASSERT(!inlineCallFrame->baselineCodeBlock()->needsActivation());
     if (isTornOff())
         return;

trunk/Source/JavaScriptCore/runtime/Arguments.h

@@ -88 +88 @@
     void tearOff(CallFrame*, InlineCallFrame*);
     bool isTornOff() const { return m_registerArray.get(); }
-    void didTearOffActivation(ExecState*, JSLexicalEnvironment*);
 
     static Structure* createStructure(VM& vm, JSGlobalObject* globalObject, JSValue prototype) 
@@ -274 +273 @@
         return m_registers[index];
 
-    JSLexicalEnvironment* lexicalEnvironment = m_lexicalEnvironment.get();
-    if (!lexicalEnvironment)
-        lexicalEnvironment = CallFrame::create(reinterpret_cast<Register*>(m_registers))->lexicalEnvironment();
-    return lexicalEnvironment->registerAt(index - m_slowArgumentData->bytecodeToMachineCaptureOffset());
+    RELEASE_ASSERT(m_lexicalEnvironment);
+    return m_lexicalEnvironment->registerAt(index - m_slowArgumentData->bytecodeToMachineCaptureOffset());
 }
 
@@ -308 +305 @@
                 codeBlock->framePointerOffsetToGetActivationRegisters());
         }
-
+        if (codeBlock->needsActivation()) {
+            RELEASE_ASSERT(callFrame->lexicalEnvironment());
+            m_lexicalEnvironment.set(callFrame->vm(), this, callFrame->lexicalEnvironment());
+        }
         // The bytecode generator omits op_tear_off_lexical_environment in cases of no
         // declared parameters, so we need to tear off immediately.
         if (m_isStrictMode || !callee->jsExecutable()->parameterCount())
             tearOff(callFrame);
+        break;
+    }
+        
+    case FakeArgumentValuesCreationMode: {
+        m_numArguments = 0;
+        m_registers = nullptr;
+        tearOff(callFrame);
+        break;
+    }
+    }
+}
+
+inline void Arguments::finishCreation(CallFrame* callFrame, InlineCallFrame* inlineCallFrame, ArgumentsMode mode)
+{
+    Base::finishCreation(callFrame->vm());
+    ASSERT(inherits(info()));
+
+    JSFunction* callee = inlineCallFrame->calleeForCallFrame(callFrame);
+    m_callee.set(callFrame->vm(), this, callee);
+    m_overrodeLength = false;
+    m_overrodeCallee = false;
+    m_overrodeCaller = false;
+    m_isStrictMode = jsCast<FunctionExecutable*>(inlineCallFrame->executable.get())->isStrictMode();
+
+    switch (mode) {
+    case NormalArgumentsCreationMode: {
+        m_numArguments = inlineCallFrame->arguments.size() - 1;
+        
+        if (m_numArguments) {
+            int offsetForArgumentOne = inlineCallFrame->arguments[1].virtualRegister().offset();
+            m_registers = reinterpret_cast<WriteBarrierBase<Unknown>*>(callFrame->registers()) + offsetForArgumentOne - virtualRegisterForArgument(1).offset();
+        } else
+            m_registers = 0;
+        
+        ASSERT(!jsCast<FunctionExecutable*>(inlineCallFrame->executable.get())->symbolTable(inlineCallFrame->specializationKind())->slowArguments());
+        
+        // The bytecode generator omits op_tear_off_lexical_environment in cases of no
+        // declared parameters, so we need to tear off immediately.
+        if (m_isStrictMode || !callee->jsExecutable()->parameterCount())
+            tearOff(callFrame, inlineCallFrame);
         break;
     }
@@ -322 +362 @@
         break;
     } }
-        
-}
-
-inline void Arguments::finishCreation(CallFrame* callFrame, InlineCallFrame* inlineCallFrame, ArgumentsMode mode)
-{
-    Base::finishCreation(callFrame->vm());
-    ASSERT(inherits(info()));
-
-    JSFunction* callee = inlineCallFrame->calleeForCallFrame(callFrame);
-    m_callee.set(callFrame->vm(), this, callee);
-    m_overrodeLength = false;
-    m_overrodeCallee = false;
-    m_overrodeCaller = false;
-    m_isStrictMode = jsCast<FunctionExecutable*>(inlineCallFrame->executable.get())->isStrictMode();
-
-    switch (mode) {
-    case NormalArgumentsCreationMode: {
-        m_numArguments = inlineCallFrame->arguments.size() - 1;
-        
-        if (m_numArguments) {
-            int offsetForArgumentOne = inlineCallFrame->arguments[1].virtualRegister().offset();
-            m_registers = reinterpret_cast<WriteBarrierBase<Unknown>*>(callFrame->registers()) + offsetForArgumentOne - virtualRegisterForArgument(1).offset();
-        } else
-            m_registers = 0;
-        
-        ASSERT(!jsCast<FunctionExecutable*>(inlineCallFrame->executable.get())->symbolTable(inlineCallFrame->specializationKind())->slowArguments());
-        
-        // The bytecode generator omits op_tear_off_lexical_environment in cases of no
-        // declared parameters, so we need to tear off immediately.
-        if (m_isStrictMode || !callee->jsExecutable()->parameterCount())
-            tearOff(callFrame, inlineCallFrame);
-        break;
-    }
-        
-    case FakeArgumentValuesCreationMode: {
-        m_numArguments = 0;
-        m_registers = nullptr;
-        tearOff(callFrame);
-        break;
-    } }
 }