Changeset 252160 in webkit

Timestamp:

Nov 6, 2019, 4:29:19 PM (5 years ago)

Author:

mark.lam@apple.com

Message:

JSGlobalObject::fireWatchpointAndMakeAllArrayStructuresSlowPut() should fire its watchpoint as the last step.
​https://bugs.webkit.org/show_bug.cgi?id=203867
<rdar://problem/56813514>

Reviewed by Saam Barati.

JSTests:

• stress/racy-slow-put-cloned-arguments-when-having-a-bad-time.js: Added.

Source/JavaScriptCore:

JSGlobalObject::fireWatchpointAndMakeAllArrayStructuresSlowPut() should make all
the array structures SlowPut before firing the watchpoint. Otherwise, the
concurrent JIT may think it's grabbing the slow put version of the structure, but
is actually grabbing the non-SlowPut version because it happened to beat the
mutator in a race to read the structure before the mutator makes it SlowPut.

Also removed some assertions in DFGSpeculativeJIT.cpp that are vulnerable to races
between when the mutator makes all array structures SlowPut and when it fires the
HavingABadTime watchpoint. The FTL equivalent did not have these assertions.

• dfg/DFGSpeculativeJIT.cpp:

(JSC::DFG::SpeculativeJIT::compileCreateRest):
(JSC::DFG::SpeculativeJIT::compileNewArray):
(JSC::DFG::SpeculativeJIT::compileNewArrayWithSpread):

• runtime/JSGlobalObject.cpp:

(JSC::JSGlobalObject::fireWatchpointAndMakeAllArrayStructuresSlowPut):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/racy-slow-put-cloned-arguments-when-having-a-bad-time.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp (modified) (3 diffs)
• Source/JavaScriptCore/runtime/JSGlobalObject.cpp (modified) (2 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2019-11-06  Mark Lam  <mark.lam@apple.com>
+
+        JSGlobalObject::fireWatchpointAndMakeAllArrayStructuresSlowPut() should fire its watchpoint as the last step.
+        https://bugs.webkit.org/show_bug.cgi?id=203867
+        <rdar://problem/56813514>
+
+        Reviewed by Saam Barati.
+
+        * stress/racy-slow-put-cloned-arguments-when-having-a-bad-time.js: Added.
+
 2019-11-06  Commit Queue  <commit-queue@webkit.org>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-11-06  Mark Lam  <mark.lam@apple.com>
+
+        JSGlobalObject::fireWatchpointAndMakeAllArrayStructuresSlowPut() should fire its watchpoint as the last step.
+        https://bugs.webkit.org/show_bug.cgi?id=203867
+        <rdar://problem/56813514>
+
+        Reviewed by Saam Barati.
+
+        JSGlobalObject::fireWatchpointAndMakeAllArrayStructuresSlowPut() should make all
+        the array structures SlowPut before firing the watchpoint.  Otherwise, the
+        concurrent JIT may think it's grabbing the slow put version of the structure, but
+        is actually grabbing the non-SlowPut version because it happened to beat the
+        mutator in a race to read the structure before the mutator makes it SlowPut.
+
+        Also removed some assertions in DFGSpeculativeJIT.cpp that are vulnerable to races
+        between when the mutator makes all array structures SlowPut and when it fires the
+        HavingABadTime watchpoint.  The FTL equivalent did not have these assertions.
+
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileCreateRest):
+        (JSC::DFG::SpeculativeJIT::compileNewArray):
+        (JSC::DFG::SpeculativeJIT::compileNewArrayWithSpread):
+        * runtime/JSGlobalObject.cpp:
+        (JSC::JSGlobalObject::fireWatchpointAndMakeAllArrayStructuresSlowPut):
+
 2019-11-06  Commit Queue  <commit-queue@webkit.org>
 

trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp

@@ -7816 +7816 @@
         // arguments to have arrayLength exceed MIN_ARRAY_STORAGE_CONSTRUCTION_LENGTH.
         bool shouldAllowForArrayStorageStructureForLargeArrays = false;
-        ASSERT(m_jit.graph().globalObjectFor(node->origin.semantic)->restParameterStructure()->indexingMode() == ArrayWithContiguous || m_jit.graph().globalObjectFor(node->origin.semantic)->isHavingABadTime());
         compileAllocateNewArrayWithSize(m_jit.graph().globalObjectFor(node->origin.semantic), arrayResultGPR, arrayLengthGPR, ArrayWithContiguous, shouldAllowForArrayStorageStructureForLargeArrays);
 
@@ -7980 +7979 @@
     RegisteredStructure structure = m_jit.graph().registerStructure(globalObject->arrayStructureForIndexingTypeDuringAllocation(node->indexingType()));
     if (!globalObject->isHavingABadTime() && !hasAnyArrayStorage(node->indexingType())) {
-        DFG_ASSERT(m_jit.graph(), node, structure->indexingType() == node->indexingType(), structure->indexingType(), node->indexingType());
         ASSERT(
             hasUndecided(structure->indexingType())
@@ -8174 +8172 @@
             // non-ArrayStorage shaped array.
             bool shouldAllowForArrayStorageStructureForLargeArrays = false;
-            ASSERT(m_jit.graph().globalObjectFor(node->origin.semantic)->restParameterStructure()->indexingType() == ArrayWithContiguous || m_jit.graph().globalObjectFor(node->origin.semantic)->isHavingABadTime());
             compileAllocateNewArrayWithSize(m_jit.graph().globalObjectFor(node->origin.semantic), resultGPR, lengthGPR, ArrayWithContiguous, shouldAllowForArrayStorageStructureForLargeArrays);
         }

trunk/Source/JavaScriptCore/runtime/JSGlobalObject.cpp

@@ -1566 +1566 @@
         return;
 
-    // Make sure that all allocations or indexed storage transitions that are inlining
-    // the assumption that it's safe to transition to a non-SlowPut array storage don't
-    // do so anymore.
-    m_havingABadTimeWatchpoint->fireAll(vm, "Having a bad time");
-    ASSERT(isHavingABadTime()); // The watchpoint is what tells us that we're having a bad time.
-    
     // Make sure that all JSArray allocations that load the appropriate structure from
     // this object now load a structure that uses SlowPut.
@@ -1585 +1579 @@
     slowPutStructure = ClonedArguments::createSlowPutStructure(vm, this, m_objectPrototype.get());
     m_clonedArgumentsStructure.set(vm, this, slowPutStructure);
+
+    // Make sure that all allocations or indexed storage transitions that are inlining
+    // the assumption that it's safe to transition to a non-SlowPut array storage don't
+    // do so anymore.
+    // Note: we are deliberately firing the watchpoint here at the end only after
+    // making all the array structures SlowPut. This ensures that the concurrent
+    // JIT threads will always get the SlowPut versions of the structures if
+    // isHavingABadTime() returns true. The concurrent JIT relies on this.
+    m_havingABadTimeWatchpoint->fireAll(vm, "Having a bad time");
+    ASSERT(isHavingABadTime()); // The watchpoint is what tells us that we're having a bad time.
 };