Changeset 178692 in webkit for trunk/Source/JavaScriptCore/bytecode/CodeBlock.cpp

Timestamp:

Jan 19, 2015, 8:47:55 PM (10 years ago)

Author:

[email protected]

Message:

Basic block start offsets should never be larger than end offsets in the control flow profiler
​https://bugs.webkit.org/show_bug.cgi?id=140377

Reviewed by Filip Pizlo.

The bytecode generator will emit code more than once for some AST nodes. For instance,
the finally block of TryNode will emit two code paths for its finally block: one for
the normal path, and another for the path where an exception is thrown in the catch block.

This repeated code emission of the same AST node previously broke how the control
flow profiler computed text ranges of basic blocks because when the same AST node
is emitted multiple times, there is a good chance that there are ranges that span
from the end offset of one of these duplicated nodes back to the start offset of
the same duplicated node. This caused a basic block range to report a larger start
offset than end offset. This was incorrect. Now, when this situation is encountered
while linking a CodeBlock, the faulty range in question is ignored.

• bytecode/CodeBlock.cpp:

(JSC::CodeBlock::CodeBlock):
(JSC::CodeBlock::insertBasicBlockBoundariesForControlFlowProfiler):

• bytecode/CodeBlock.h:
• bytecompiler/NodesCodegen.cpp:

(JSC::ForInNode::emitMultiLoopBytecode):
(JSC::ForOfNode::emitBytecode):
(JSC::TryNode::emitBytecode):

• parser/Parser.cpp:

(JSC::Parser<LexerType>::parseConditionalExpression):

• runtime/ControlFlowProfiler.cpp:

(JSC::ControlFlowProfiler::ControlFlowProfiler):

• runtime/ControlFlowProfiler.h:

(JSC::ControlFlowProfiler::dummyBasicBlock):

File:

• trunk/Source/JavaScriptCore/bytecode/CodeBlock.cpp (modified) (2 diffs)

trunk/Source/JavaScriptCore/bytecode/CodeBlock.cpp

@@ -2137 +2137 @@
     }
 
-    if (vm()->controlFlowProfiler()) {
-        const Vector<size_t>& bytecodeOffsets = unlinkedCodeBlock->opProfileControlFlowBytecodeOffsets();
-        for (size_t i = 0, offsetsLength = bytecodeOffsets.size(); i < offsetsLength; i++) {
-            // Because op_profile_control_flow is emitted at the beginning of every basic block, finding 
-            // the next op_profile_control_flow will give us the text range of a single basic block.
-            size_t startIdx = bytecodeOffsets[i];
-            RELEASE_ASSERT(vm()->interpreter->getOpcodeID(instructions[startIdx].u.opcode) == op_profile_control_flow);
-            int basicBlockStartOffset = instructions[startIdx + 1].u.operand;
-            int basicBlockEndOffset;
-            if (i + 1 < offsetsLength) {
-                size_t endIdx = bytecodeOffsets[i + 1];
-                RELEASE_ASSERT(vm()->interpreter->getOpcodeID(instructions[endIdx].u.opcode) == op_profile_control_flow);
-                basicBlockEndOffset = instructions[endIdx + 1].u.operand;
-            } else
-                basicBlockEndOffset = m_sourceOffset + m_ownerExecutable->source().length() - 1;
-
-            BasicBlockLocation* basicBlockLocation = vm()->controlFlowProfiler()->getBasicBlockLocation(m_ownerExecutable->sourceID(), basicBlockStartOffset, basicBlockEndOffset);
-
-            // Find all functions that are enclosed within the range: [basicBlockStartOffset, basicBlockEndOffset]
-            // and insert these functions' start/end offsets as gaps in the current BasicBlockLocation.
-            // This is necessary because in the original source text of a JavaScript program, 
-            // function literals form new basic blocks boundaries, but they aren't represented 
-            // inside the CodeBlock's instruction stream.
-            auto insertFunctionGaps = [basicBlockLocation, basicBlockStartOffset, basicBlockEndOffset] (const WriteBarrier<FunctionExecutable>& functionExecutable) {
-                const UnlinkedFunctionExecutable* executable = functionExecutable->unlinkedExecutable();
-                int functionStart = executable->typeProfilingStartOffset();
-                int functionEnd = executable->typeProfilingEndOffset();
-                if (functionStart >= basicBlockStartOffset && functionEnd <= basicBlockEndOffset)
-                    basicBlockLocation->insertGap(functionStart, functionEnd);
-            };
-
-            for (const WriteBarrier<FunctionExecutable>& executable : m_functionDecls)
-                insertFunctionGaps(executable);
-            for (const WriteBarrier<FunctionExecutable>& executable : m_functionExprs)
-                insertFunctionGaps(executable);
-
-            instructions[startIdx + 1].u.basicBlockLocation = basicBlockLocation;
-        }
-    }
+    if (vm()->controlFlowProfiler())
+        insertBasicBlockBoundariesForControlFlowProfiler(instructions);
 
     m_instructions = WTF::RefCountedArray<Instruction>(instructions);
@@ -4067 +4030 @@
 #endif
 
+void CodeBlock::insertBasicBlockBoundariesForControlFlowProfiler(Vector<Instruction, 0, UnsafeVectorOverflow>& instructions)
+{
+    const Vector<size_t>& bytecodeOffsets = unlinkedCodeBlock()->opProfileControlFlowBytecodeOffsets();
+    for (size_t i = 0, offsetsLength = bytecodeOffsets.size(); i < offsetsLength; i++) {
+        // Because op_profile_control_flow is emitted at the beginning of every basic block, finding 
+        // the next op_profile_control_flow will give us the text range of a single basic block.
+        size_t startIdx = bytecodeOffsets[i];
+        RELEASE_ASSERT(vm()->interpreter->getOpcodeID(instructions[startIdx].u.opcode) == op_profile_control_flow);
+        int basicBlockStartOffset = instructions[startIdx + 1].u.operand;
+        int basicBlockEndOffset;
+        if (i + 1 < offsetsLength) {
+            size_t endIdx = bytecodeOffsets[i + 1];
+            RELEASE_ASSERT(vm()->interpreter->getOpcodeID(instructions[endIdx].u.opcode) == op_profile_control_flow);
+            basicBlockEndOffset = instructions[endIdx + 1].u.operand;
+        } else {
+            basicBlockEndOffset = m_sourceOffset + m_ownerExecutable->source().length() - 1; // Offset before the closing brace.
+            basicBlockStartOffset = std::min(basicBlockStartOffset, basicBlockEndOffset); // Some start offsets may be at the closing brace, ensure it is the offset before.
+        }
+
+        // The following check allows for the same textual JavaScript basic block to have its bytecode emitted more
+        // than once and still play nice with the control flow profiler. When basicBlockStartOffset is larger than 
+        // basicBlockEndOffset, it indicates that the bytecode generator has emitted code for the same AST node 
+        // more than once (for example: ForInNode, Finally blocks in TryNode, etc). Though these are different 
+        // basic blocks at the bytecode level, they are generated from the same textual basic block in the JavaScript 
+        // program. The condition: 
+        // (basicBlockEndOffset < basicBlockStartOffset) 
+        // is encountered when op_profile_control_flow lies across the boundary of these duplicated bytecode basic 
+        // blocks and the textual offset goes from the end of the duplicated block back to the beginning. These 
+        // ranges are dummy ranges and are ignored. The duplicated bytecode basic blocks point to the same 
+        // internal data structure, so if any of them execute, it will record the same textual basic block in the 
+        // JavaScript program as executing.
+        // At the bytecode level, this situation looks like:
+        // j: op_profile_control_flow (from j->k, we have basicBlockEndOffset < basicBlockStartOffset)
+        // ...
+        // k: op_profile_control_flow (we want to skip over the j->k block and start fresh at offset k as the start of a new basic block k->m).
+        // ...
+        // m: op_profile_control_flow
+        if (basicBlockEndOffset < basicBlockStartOffset) {
+            RELEASE_ASSERT(i + 1 < offsetsLength); // We should never encounter dummy blocks at the end of a CodeBlock.
+            instructions[startIdx + 1].u.basicBlockLocation = vm()->controlFlowProfiler()->dummyBasicBlock();
+            continue;
+        }
+
+        BasicBlockLocation* basicBlockLocation = vm()->controlFlowProfiler()->getBasicBlockLocation(m_ownerExecutable->sourceID(), basicBlockStartOffset, basicBlockEndOffset);
+
+        // Find all functions that are enclosed within the range: [basicBlockStartOffset, basicBlockEndOffset]
+        // and insert these functions' start/end offsets as gaps in the current BasicBlockLocation.
+        // This is necessary because in the original source text of a JavaScript program, 
+        // function literals form new basic blocks boundaries, but they aren't represented 
+        // inside the CodeBlock's instruction stream.
+        auto insertFunctionGaps = [basicBlockLocation, basicBlockStartOffset, basicBlockEndOffset] (const WriteBarrier<FunctionExecutable>& functionExecutable) {
+            const UnlinkedFunctionExecutable* executable = functionExecutable->unlinkedExecutable();
+            int functionStart = executable->typeProfilingStartOffset();
+            int functionEnd = executable->typeProfilingEndOffset();
+            if (functionStart >= basicBlockStartOffset && functionEnd <= basicBlockEndOffset)
+                basicBlockLocation->insertGap(functionStart, functionEnd);
+        };
+
+        for (const WriteBarrier<FunctionExecutable>& executable : m_functionDecls)
+            insertFunctionGaps(executable);
+        for (const WriteBarrier<FunctionExecutable>& executable : m_functionExprs)
+            insertFunctionGaps(executable);
+
+        instructions[startIdx + 1].u.basicBlockLocation = basicBlockLocation;
+    }
+}
+
 } // namespace JSC