ArraySlice needs to keep the source array alive.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 24 Jun 2019 17:38:12 +0000 (17:38 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 24 Jun 2019 17:38:12 +0000 (17:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=197374
<rdar://problem/50304429>

Reviewed by Michael Saboff and Filip Pizlo.

JSTests:

* stress/array-slice-must-keep-source-array-alive.js: Added.

Source/JavaScriptCore:

The implementation of the FTL ArraySlice intrinsics may GC while allocating the
result array and its butterfly.  Previously, ArraySlice already keeps the source
butterfly alive in order to copy from it to the new butterfly after the allocation.
Unfortunately, this is not enough.  We also need to keep the source array alive
so that GC will scan the values in the butterfly as well.  Note: the butterfly
does not have a visitChildren() method to do this scan.  It's the parent object's
responsibility to do the scanning.

This patch fixes this by introducing a keepAlive() utility method, and we use it
to keep the source array alive while allocating the result array and butterfly.

keepAlive() works by using a patchpoint to communicate to B3 that a value (the
source array in this case) is still in use.  It also uses a fence to keep B3 from
relocating the patchpoint, which may defeat the fix.

For the DFG's SpeculativeJIT::compileArraySlice(), we may have lucked out and the
source array cell is kept alive.  This patch makes it explicit that we should
keep its cell alive till after the result array has been allocated.

For the Baseline JIT and LLInt, we use the arrayProtoFuncSlice() runtime function
and there is no issue because the source array (in "thisObj") is in the element
copying loop that follows the allocation of the result array.  However, for
documentation purposes, this patch adds a call to HeapCell::use() to indicate that
the source array need to kept alive at least until after the allocation of the
result array.

* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileArraySlice):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileArraySlice):
(JSC::FTL::DFG::LowerDFGToB3::allocateJSArray):
(JSC::FTL::DFG::LowerDFGToB3::keepAlive):
* runtime/ArrayPrototype.cpp:
(JSC::arrayProtoFuncSlice):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@246740 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/array-slice-must-keep-source-array-alive.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
Source/JavaScriptCore/runtime/ArrayPrototype.cpp

index 24e3156..1e5eee5 100644 (file)
@@ -1,3 +1,13 @@
+2019-06-21  Mark Lam  <mark.lam@apple.com>
+
+        ArraySlice needs to keep the source array alive.
+        https://bugs.webkit.org/show_bug.cgi?id=197374
+        <rdar://problem/50304429>
+
+        Reviewed by Michael Saboff and Filip Pizlo.
+
+        * stress/array-slice-must-keep-source-array-alive.js: Added.
+
 2019-06-22  Robin Morisset  <rmorisset@apple.com> and Yusuke Suzuki  <ysuzuki@apple.com>
 
         All prototypes should call didBecomePrototype()
diff --git a/JSTests/stress/array-slice-must-keep-source-array-alive.js b/JSTests/stress/array-slice-must-keep-source-array-alive.js
new file mode 100644 (file)
index 0000000..27514db
--- /dev/null
@@ -0,0 +1,19 @@
+//@ runDefault("--useConcurrentGC=false", "--useConcurrentJIT=false", "--thresholdForFTLOptimizeAfterWarmUp=1000")
+
+var functions = [];
+
+function boo() {
+    functions.push(new Function("a", "return a"));
+    return functions.splice(0);
+}
+
+function test() {
+    functions = boo().slice();
+}
+
+noDFG(boo);
+noInline(boo);
+noInline(test);
+
+for (var i = 0; i < 10000; i++)
+    test();
index 617d9e5..74bd2ff 100644 (file)
@@ -1,3 +1,46 @@
+2019-06-24  Mark Lam  <mark.lam@apple.com>
+
+        ArraySlice needs to keep the source array alive.
+        https://bugs.webkit.org/show_bug.cgi?id=197374
+        <rdar://problem/50304429>
+
+        Reviewed by Michael Saboff and Filip Pizlo.
+
+        The implementation of the FTL ArraySlice intrinsics may GC while allocating the
+        result array and its butterfly.  Previously, ArraySlice already keeps the source
+        butterfly alive in order to copy from it to the new butterfly after the allocation.
+        Unfortunately, this is not enough.  We also need to keep the source array alive
+        so that GC will scan the values in the butterfly as well.  Note: the butterfly
+        does not have a visitChildren() method to do this scan.  It's the parent object's
+        responsibility to do the scanning.
+
+        This patch fixes this by introducing a keepAlive() utility method, and we use it
+        to keep the source array alive while allocating the result array and butterfly.
+
+        keepAlive() works by using a patchpoint to communicate to B3 that a value (the
+        source array in this case) is still in use.  It also uses a fence to keep B3 from
+        relocating the patchpoint, which may defeat the fix.
+
+        For the DFG's SpeculativeJIT::compileArraySlice(), we may have lucked out and the
+        source array cell is kept alive.  This patch makes it explicit that we should
+        keep its cell alive till after the result array has been allocated.
+
+        For the Baseline JIT and LLInt, we use the arrayProtoFuncSlice() runtime function
+        and there is no issue because the source array (in "thisObj") is in the element
+        copying loop that follows the allocation of the result array.  However, for
+        documentation purposes, this patch adds a call to HeapCell::use() to indicate that
+        the source array need to kept alive at least until after the allocation of the
+        result array.
+
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileArraySlice):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileArraySlice):
+        (JSC::FTL::DFG::LowerDFGToB3::allocateJSArray):
+        (JSC::FTL::DFG::LowerDFGToB3::keepAlive):
+        * runtime/ArrayPrototype.cpp:
+        (JSC::arrayProtoFuncSlice):
+
 2019-06-22  Robin Morisset  <rmorisset@apple.com> and Yusuke Suzuki  <ysuzuki@apple.com>
 
         All prototypes should call didBecomePrototype()
index 1f3638d..fcd5cb9 100644 (file)
@@ -8353,17 +8353,18 @@ void SpeculativeJIT::compileArraySlice(Node* node)
         }
     }
 
-
     GPRTemporary temp3(this);
     GPRReg tempValue = temp3.gpr();
+
     {
+        // We need to keep the source array alive at least until after we're done
+        // with anything that can GC (e.g. allocating the result array below).
         SpeculateCellOperand cell(this, m_jit.graph().varArgChild(node, 0));
+
         m_jit.load8(MacroAssembler::Address(cell.gpr(), JSCell::indexingTypeAndMiscOffset()), tempValue);
         // We can ignore the writability of the cell since we won't write to the source.
         m_jit.and32(TrustedImm32(AllWritableArrayTypesAndHistory), tempValue);
-    }
 
-    {
         JSValueRegsTemporary emptyValue(this);
         JSValueRegs emptyValueRegs = emptyValue.regs();
 
index c4a6705..c89010a 100644 (file)
@@ -5116,6 +5116,7 @@ private:
     {
         JSGlobalObject* globalObject = m_graph.globalObjectFor(m_node->origin.semantic);
 
+        LValue sourceArray = lowCell(m_graph.varArgChild(m_node, 0));
         LValue sourceStorage = lowStorage(m_graph.varArgChild(m_node, m_node->numChildren() - 1));
         LValue inputLength = m_out.load32(sourceStorage, m_heaps.Butterfly_publicLength);
 
@@ -5141,7 +5142,7 @@ private:
 
         ArrayValues arrayResult;
         {
-            LValue indexingType = m_out.load8ZeroExt32(lowCell(m_graph.varArgChild(m_node, 0)), m_heaps.JSCell_indexingTypeAndMisc);
+            LValue indexingType = m_out.load8ZeroExt32(sourceArray, m_heaps.JSCell_indexingTypeAndMisc);
             // We can ignore the writability of the cell since we won't write to the source.
             indexingType = m_out.bitAnd(indexingType, m_out.constInt32(AllWritableArrayTypesAndHistory));
             // When we emit an ArraySlice, we dominate the use of the array by a CheckStructure
@@ -5156,6 +5157,9 @@ private:
             arrayResult = allocateJSArray(resultLength, resultLength, structure, indexingType, false, false);
         }
 
+        // Keep the sourceArray alive at least until after anything that can GC.
+        keepAlive(sourceArray);
+
         LBasicBlock loop = m_out.newBlock();
         LBasicBlock continuation = m_out.newBlock();
 
@@ -13711,7 +13715,7 @@ private:
         }
         
         ValueFromBlock noButterfly = m_out.anchor(m_out.intPtrZero);
-        
+
         LValue predicate;
         if (shouldLargeArraySizeCreateArrayStorage)
             predicate = m_out.aboveOrEqual(publicLength, m_out.constInt32(MIN_ARRAY_STORAGE_CONSTRUCTION_LENGTH));
@@ -13721,16 +13725,16 @@ private:
         m_out.branch(predicate, rarely(largeCase), usually(fastCase));
         
         m_out.appendTo(fastCase, largeCase);
-            
+
         LValue payloadSize =
             m_out.shl(m_out.zeroExt(vectorLength, pointerType()), m_out.constIntPtr(3));
-            
+
         LValue butterflySize = m_out.add(
             payloadSize, m_out.constIntPtr(sizeof(IndexingHeader)));
-            
+
         LValue allocator = allocatorForSize(vm().jsValueGigacageAuxiliarySpace, butterflySize, failCase);
         LValue startOfStorage = allocateHeapCell(allocator, failCase);
-            
+
         LValue butterfly = m_out.add(startOfStorage, m_out.constIntPtr(sizeof(IndexingHeader)));
         
         m_out.store32(publicLength, butterfly, m_heaps.Butterfly_publicLength);
@@ -17274,7 +17278,16 @@ private:
             return false;
         return true;
     }
-    
+
+    void keepAlive(LValue value)
+    {
+        PatchpointValue* patchpoint = m_out.patchpoint(Void);
+        patchpoint->effects = Effects::none();
+        patchpoint->effects.writesLocalState = true;
+        patchpoint->append(value, ValueRep::ColdAny);
+        patchpoint->setGenerator([=] (CCallHelpers&, const StackmapGenerationParams&) { });
+    }
+
     void addWeakReference(JSCell* target)
     {
         m_graph.m_plan.weakReferences().addLazily(target);
index 57a8695..b9a4e75 100644 (file)
@@ -1003,6 +1003,10 @@ EncodedJSValue JSC_HOST_CALL arrayProtoFuncSlice(ExecState* exec)
         RETURN_IF_EXCEPTION(scope, { });
     }
 
+    // Document that we need to keep the source array alive until after anything
+    // that can GC (e.g. allocating the result array).
+    thisObj->use();
+
     unsigned n = 0;
     for (unsigned k = begin; k < end; k++, n++) {
         JSValue v = getProperty(exec, thisObj, k);