Changeset 208326 in webkit

Timestamp:

Nov 3, 2016 7:39:47 AM (7 years ago)

Author:

sbarati@apple.com

Message:

Asking for a value profile prediction should be defensive against not finding a value profile
​https://bugs.webkit.org/show_bug.cgi?id=164306

Reviewed by Mark Lam.

JSTests:

• stress/inlined-tail-call-in-inlined-setter-should-not-crash-when-getting-value-profile.js: Added.

(let.o.set foo):
(bar):

Source/JavaScriptCore:

Currently, the code that calls CodeBlock::valueProfilePredictionForBytecodeOffset
in the DFG assumes it will always be at a value producing node. However, this isn't
true if we tail call from an inlined setter. When we're at a tail call, we try
to find the first caller that isn't a tail call to see what value the
tail_call produces. If we inline a setter, however, we will end up finding
the put_by_id as our first non-tail-called "caller", and that won't have a
value profile associated with it since it's not a value producing node.
CodeBlock::valueProfilePredictionForBytecodeOffset should be defensive
against finding a null value profile.

• bytecode/CodeBlock.h:

(JSC::CodeBlock::valueProfilePredictionForBytecodeOffset):

• dfg/DFGByteCodeParser.cpp:

(JSC::DFG::ByteCodeParser::getPredictionWithoutOSRExit):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/inlined-tail-call-in-inlined-setter-should-not-crash-when-getting-value-profile.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/bytecode/CodeBlock.h (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp (modified) (1 diff)

trunk/JSTests/ChangeLog

@@ +1 @@
+2016-11-03  Saam Barati  <sbarati@apple.com>
+
+        Asking for a value profile prediction should be defensive against not finding a value profile
+        https://bugs.webkit.org/show_bug.cgi?id=164306
+
+        Reviewed by Mark Lam.
+
+        * stress/inlined-tail-call-in-inlined-setter-should-not-crash-when-getting-value-profile.js: Added.
+        (let.o.set foo):
+        (bar):
+
 2016-11-02  Saam Barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2016-11-03  Saam Barati  <sbarati@apple.com>
+
+        Asking for a value profile prediction should be defensive against not finding a value profile
+        https://bugs.webkit.org/show_bug.cgi?id=164306
+
+        Reviewed by Mark Lam.
+
+        Currently, the code that calls CodeBlock::valueProfilePredictionForBytecodeOffset
+        in the DFG assumes it will always be at a value producing node. However, this isn't
+        true if we tail call from an inlined setter. When we're at a tail call, we try
+        to find the first caller that isn't a tail call to see what value the
+        tail_call produces. If we inline a setter, however, we will end up finding
+        the put_by_id as our first non-tail-called "caller", and that won't have a
+        value profile associated with it since it's not a value producing node.
+        CodeBlock::valueProfilePredictionForBytecodeOffset should be defensive
+        against finding a null value profile.
+
+        * bytecode/CodeBlock.h:
+        (JSC::CodeBlock::valueProfilePredictionForBytecodeOffset):
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::getPredictionWithoutOSRExit):
+
 2016-11-02  Yusuke Suzuki  <utatane.tea@gmail.com>
 

trunk/Source/JavaScriptCore/bytecode/CodeBlock.h

@@ -415 +415 @@
     SpeculatedType valueProfilePredictionForBytecodeOffset(const ConcurrentJITLocker& locker, int bytecodeOffset)
     {
-        return valueProfileForBytecodeOffset(bytecodeOffset)->computeUpdatedPrediction(locker);
+        if (ValueProfile* valueProfile = valueProfileForBytecodeOffset(bytecodeOffset))
+            return valueProfile->computeUpdatedPrediction(locker);
+        return SpecNone;
     }
 

trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

@@ -852 +852 @@
     {
         SpeculatedType prediction;
-        CodeBlock* profiledBlock = nullptr;
-
         {
             ConcurrentJITLocker locker(m_inlineStackTop->m_profiledBlock->m_lock);
             prediction = m_inlineStackTop->m_profiledBlock->valueProfilePredictionForBytecodeOffset(locker, bytecodeIndex);
-
-            if (prediction == SpecNone) {
-                // If we have no information about the values this
-                // node generates, we check if by any chance it is
-                // a tail call opcode. In that case, we walk up the
-                // inline frames to find a call higher in the call
-                // chain and use its prediction. If we only have
-                // inlined tail call frames, we use SpecFullTop
-                // to avoid a spurious OSR exit.
-                Instruction* instruction = m_inlineStackTop->m_profiledBlock->instructions().begin() + bytecodeIndex;
-                OpcodeID opcodeID = m_vm->interpreter->getOpcodeID(instruction->u.opcode);
-
-                switch (opcodeID) {
-                case op_tail_call:
-                case op_tail_call_varargs:
-                case op_tail_call_forward_arguments: {
-                    if (!inlineCallFrame()) {
-                        prediction = SpecFullTop;
-                        break;
-                    }
-                    CodeOrigin* codeOrigin = inlineCallFrame()->getCallerSkippingTailCalls();
-                    if (!codeOrigin) {
-                        prediction = SpecFullTop;
-                        break;
-                    }
-                    InlineStackEntry* stack = m_inlineStackTop;
-                    while (stack->m_inlineCallFrame != codeOrigin->inlineCallFrame)
-                        stack = stack->m_caller;
-                    bytecodeIndex = codeOrigin->bytecodeIndex;
-                    profiledBlock = stack->m_profiledBlock;
-                    break;
-                }
-
-                default:
-                    break;
-                }
-            }
-        }
-
-        if (profiledBlock) {
+        }
+
+        if (prediction != SpecNone)
+            return prediction;
+
+        // If we have no information about the values this
+        // node generates, we check if by any chance it is
+        // a tail call opcode. In that case, we walk up the
+        // inline frames to find a call higher in the call
+        // chain and use its prediction. If we only have
+        // inlined tail call frames, we use SpecFullTop
+        // to avoid a spurious OSR exit.
+        Instruction* instruction = m_inlineStackTop->m_profiledBlock->instructions().begin() + bytecodeIndex;
+        OpcodeID opcodeID = m_vm->interpreter->getOpcodeID(instruction->u.opcode);
+
+        switch (opcodeID) {
+        case op_tail_call:
+        case op_tail_call_varargs:
+        case op_tail_call_forward_arguments: {
+            // Things should be more permissive to us returning BOTTOM instead of TOP here.
+            // Currently, this will cause us to Force OSR exit. This is bad because returning
+            // TOP will cause anything that transitively touches this speculated type to
+            // also become TOP during prediction propagation.
+            // https://bugs.webkit.org/show_bug.cgi?id=164337
+            if (!inlineCallFrame())
+                return SpecFullTop;
+
+            CodeOrigin* codeOrigin = inlineCallFrame()->getCallerSkippingTailCalls();
+            if (!codeOrigin)
+                return SpecFullTop;
+
+            InlineStackEntry* stack = m_inlineStackTop;
+            while (stack->m_inlineCallFrame != codeOrigin->inlineCallFrame)
+                stack = stack->m_caller;
+
+            bytecodeIndex = codeOrigin->bytecodeIndex;
+            CodeBlock* profiledBlock = stack->m_profiledBlock;
             ConcurrentJITLocker locker(profiledBlock->m_lock);
-            prediction = profiledBlock->valueProfilePredictionForBytecodeOffset(locker, bytecodeIndex);
-        }
-
-        return prediction;
+            return profiledBlock->valueProfilePredictionForBytecodeOffset(locker, bytecodeIndex);
+        }
+
+        default:
+            return SpecNone;
+        }
+
+        RELEASE_ASSERT_NOT_REACHED();
+        return SpecNone;
     }