Changeset 249319 in webkit

Timestamp:

Aug 30, 2019 3:00:31 AM (5 years ago)

Author:

ysuzuki@apple.com

Message:

[JSC] DFG ByteCodeParser should not copy JIT-related part of SimpleJumpTable
​https://bugs.webkit.org/show_bug.cgi?id=201331

Reviewed by Mark Lam.

JSTests:

• stress/simple-jump-table-copy.js: Added.

(let.code):
(g2):

Source/JavaScriptCore:

SimpleJumpTable's non-JIT part is not changed after CodeBlock is finalized well. On the other hand, JIT related part is allocated on-demand.
For example, ctiOffsets can be grown by Baseline JIT compiler. There is race condition as follows.

1. DFG ByteCodeParser is inlining and copying SimpleJumpTable
2. Baseline JIT compiler is expanding JIT-related part of SimpleJumpTable

Then, (1) reads the broken Vector, and crashes. Since JIT-related part is unnecessary in (1), we should not clone that.
This patch adds CodeBlock::addSwitchJumpTableFromProfiledCodeBlock, which only copies non JIT-related part of the given SimpleJumpTable offered
by profiled CodeBlock.

• bytecode/CodeBlock.h:

(JSC::CodeBlock::addSwitchJumpTableFromProfiledCodeBlock):

• bytecode/JumpTable.h:

(JSC::SimpleJumpTable::cloneNonJITPart const):
(JSC::SimpleJumpTable::clear):

• dfg/DFGByteCodeParser.cpp:

(JSC::DFG::ByteCodeParser::InlineStackEntry::InlineStackEntry):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/simple-jump-table-copy.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/bytecode/CodeBlock.h (modified) (1 diff)
• Source/JavaScriptCore/bytecode/JumpTable.h (modified) (2 diffs)
• Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp (modified) (1 diff)

trunk/JSTests/ChangeLog

@@ +1 @@
+2019-08-30  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] DFG ByteCodeParser should not copy JIT-related part of SimpleJumpTable
+        https://bugs.webkit.org/show_bug.cgi?id=201331
+
+        Reviewed by Mark Lam.
+
+        * stress/simple-jump-table-copy.js: Added.
+        (let.code):
+        (g2):
+
 2019-08-30  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-08-30  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] DFG ByteCodeParser should not copy JIT-related part of SimpleJumpTable
+        https://bugs.webkit.org/show_bug.cgi?id=201331
+
+        Reviewed by Mark Lam.
+
+        SimpleJumpTable's non-JIT part is not changed after CodeBlock is finalized well. On the other hand, JIT related part is allocated on-demand.
+        For example, ctiOffsets can be grown by Baseline JIT compiler. There is race condition as follows.
+
+            1. DFG ByteCodeParser is inlining and copying SimpleJumpTable
+            2. Baseline JIT compiler is expanding JIT-related part of SimpleJumpTable
+
+        Then, (1) reads the broken Vector, and crashes. Since JIT-related part is unnecessary in (1), we should not clone that.
+        This patch adds CodeBlock::addSwitchJumpTableFromProfiledCodeBlock, which only copies non JIT-related part of the given SimpleJumpTable offered
+        by profiled CodeBlock.
+
+        * bytecode/CodeBlock.h:
+        (JSC::CodeBlock::addSwitchJumpTableFromProfiledCodeBlock):
+        * bytecode/JumpTable.h:
+        (JSC::SimpleJumpTable::cloneNonJITPart const):
+        (JSC::SimpleJumpTable::clear):
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::InlineStackEntry::InlineStackEntry):
+
 2019-08-30  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/Source/JavaScriptCore/bytecode/CodeBlock.h

@@ -599 +599 @@
         m_rareData->m_switchJumpTables.clear();
     }
+#if ENABLE(DFG_JIT)
+    void addSwitchJumpTableFromProfiledCodeBlock(SimpleJumpTable& profiled)
+    {
+        createRareDataIfNecessary();
+        m_rareData->m_switchJumpTables.append(profiled.cloneNonJITPart());
+    }
+#endif
 
     size_t numberOfStringSwitchJumpTables() const { return m_rareData ? m_rareData->m_stringSwitchJumpTables.size() : 0; }

trunk/Source/JavaScriptCore/bytecode/JumpTable.h

@@ -78 +78 @@
 
     struct SimpleJumpTable {
-        // FIXME: The two Vectors can be combind into one Vector<OffsetLocation>
+        // FIXME: The two Vectors can be combined into one Vector<OffsetLocation>
         Vector<int32_t> branchOffsets;
-        int32_t min;
+        int32_t min { INT32_MIN };
 #if ENABLE(JIT)
         Vector<CodeLocationLabel<JSSwitchPtrTag>> ctiOffsets;
         CodeLocationLabel<JSSwitchPtrTag> ctiDefault;
+#endif
+
+#if ENABLE(DFG_JIT)
+        // JIT part can be later expanded without taking a lock while non-JIT part is stable after CodeBlock is finalized.
+        SimpleJumpTable cloneNonJITPart() const
+        {
+            SimpleJumpTable result;
+            result.branchOffsets = branchOffsets;
+            result.min = min;
+            return result;
+        }
 #endif
 
@@ -107 +118 @@
         }
 #endif
-        
+
+#if ENABLE(DFG_JIT)
         void clear()
         {
             branchOffsets.clear();
-#if ENABLE(JIT)
             ctiOffsets.clear();
+        }
 #endif
-        }
     };
 

trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

@@ -7117 +7117 @@
         for (unsigned i = 0; i < codeBlock->numberOfSwitchJumpTables(); ++i) {
             m_switchRemap[i] = byteCodeParser->m_codeBlock->numberOfSwitchJumpTables();
-            byteCodeParser->m_codeBlock->addSwitchJumpTable() = codeBlock->switchJumpTable(i);
+            byteCodeParser->m_codeBlock->addSwitchJumpTableFromProfiledCodeBlock(codeBlock->switchJumpTable(i));
         }
     } else {