- Revision
- 246883
- Author
- [email protected]
- Date
- 2019-06-27 09:35:18 -0700 (Thu, 27 Jun 2019)
Log Message
Cherry-pick r246740. rdar://problem/52054323
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.
JSTests:
* stress/array-slice-must-keep-source-array-alive.js: Added.
Source/_javascript_Core:
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
Modified Paths
Added Paths
Diff
Modified: branches/safari-607-branch/JSTests/ChangeLog (246882 => 246883)
--- branches/safari-607-branch/JSTests/ChangeLog 2019-06-27 16:10:01 UTC (rev 246882)
+++ branches/safari-607-branch/JSTests/ChangeLog 2019-06-27 16:35:18 UTC (rev 246883)
@@ -1,3 +1,68 @@
+2019-06-27 Kocsen Chung <[email protected]>
+
+ Cherry-pick r246740. rdar://problem/52054323
+
+ 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.
+
+ JSTests:
+
+ * stress/array-slice-must-keep-source-array-alive.js: Added.
+
+ Source/_javascript_Core:
+
+ 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
+
+ 2019-06-21 Mark Lam <[email protected]>
+
+ 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-24 Kocsen Chung <[email protected]>
Cherry-pick r246372. rdar://problem/51927634
Added: branches/safari-607-branch/JSTests/stress/array-slice-must-keep-source-array-alive.js (0 => 246883)
--- branches/safari-607-branch/JSTests/stress/array-slice-must-keep-source-array-alive.js (rev 0)
+++ branches/safari-607-branch/JSTests/stress/array-slice-must-keep-source-array-alive.js 2019-06-27 16:35:18 UTC (rev 246883)
@@ -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();
Modified: branches/safari-607-branch/Source/_javascript_Core/ChangeLog (246882 => 246883)
--- branches/safari-607-branch/Source/_javascript_Core/ChangeLog 2019-06-27 16:10:01 UTC (rev 246882)
+++ branches/safari-607-branch/Source/_javascript_Core/ChangeLog 2019-06-27 16:35:18 UTC (rev 246883)
@@ -1,3 +1,101 @@
+2019-06-27 Kocsen Chung <[email protected]>
+
+ Cherry-pick r246740. rdar://problem/52054323
+
+ 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.
+
+ JSTests:
+
+ * stress/array-slice-must-keep-source-array-alive.js: Added.
+
+ Source/_javascript_Core:
+
+ 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
+
+ 2019-06-24 Mark Lam <[email protected]>
+
+ 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-24 Kocsen Chung <[email protected]>
Cherry-pick r246372. rdar://problem/51927634
Modified: branches/safari-607-branch/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp (246882 => 246883)
--- branches/safari-607-branch/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2019-06-27 16:10:01 UTC (rev 246882)
+++ branches/safari-607-branch/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2019-06-27 16:35:18 UTC (rev 246883)
@@ -8242,17 +8242,18 @@
}
}
-
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();
Modified: branches/safari-607-branch/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp (246882 => 246883)
--- branches/safari-607-branch/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2019-06-27 16:10:01 UTC (rev 246882)
+++ branches/safari-607-branch/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2019-06-27 16:35:18 UTC (rev 246883)
@@ -4887,6 +4887,7 @@
{
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);
@@ -4912,7 +4913,7 @@
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
@@ -4927,6 +4928,9 @@
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();
@@ -13393,7 +13397,7 @@
}
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));
@@ -13403,16 +13407,16 @@
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);
@@ -16920,7 +16924,16 @@
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);
Modified: branches/safari-607-branch/Source/_javascript_Core/runtime/ArrayPrototype.cpp (246882 => 246883)
--- branches/safari-607-branch/Source/_javascript_Core/runtime/ArrayPrototype.cpp 2019-06-27 16:10:01 UTC (rev 246882)
+++ branches/safari-607-branch/Source/_javascript_Core/runtime/ArrayPrototype.cpp 2019-06-27 16:35:18 UTC (rev 246883)
@@ -988,6 +988,10 @@
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);
