- Revision
- 265586
- Author
- alanc...@apple.com
- Date
- 2020-08-12 18:46:10 -0700 (Wed, 12 Aug 2020)
Log Message
Cherry-pick r265405. rdar://problem/66943811
[JSC] Speculate children first in DFG NewArray
https://bugs.webkit.org/show_bug.cgi?id=215308
<rdar://problem/64749263>
Reviewed by Mark Lam.
SpeculativeJIT::emitAllocateRawObject can create uninitialized butterfly since we later fill them.
However, DFG NewArray node has speculation after that. So if speculation failure happens, we release
half-baked butterfly.
Let's see the example.
8459 emitAllocateRawObject(resultGPR, structure, storageGPR, numElements, vectorLengthHint);
...
8482 case ALL_INT32_INDEXING_TYPES:
8483 case ALL_CONTIGUOUS_INDEXING_TYPES: {
8484 JSValueOperand operand(this, use, ManualOperandSpeculation);
8485 JSValueRegs operandRegs = operand.jsValueRegs();
8486 if (hasInt32(node->indexingType())) {
8487 DFG_TYPE_CHECK(
8488 operandRegs, use, SpecInt32Only,
8489 m_jit.branchIfNotInt32(operandRegs));
8490 }
8491 m_jit.storeValue(operandRegs, MacroAssembler::Address(storageGPR, sizeof(JSValue) * operandIdx));
8492 break;
8493 }
L8487-L8489 is doing speculation check. If it failed, the rest of the butterfly can be filled with garbage. This looks OK since
it is Int32 butterfly so GC never scans it. However, if have-a-bad-time happens and the array is reachable from the conservative root,
this half-baked array is converted from Int32 array to ArrayStorage. At that time, since Int32 butterfly should hold JSInt32,
we store this garbage to ArrayStorage. Later, if conservative root still holds this array, and GC scans this garbage as as JSValue,
this value confuses GC.
In this patch, we first perform speculation before creating uninitialized JSArray so that we can ensure that we never exit after
creating this array until we fill it. This strategy is the same to FTL's NewArray implementation.
And we also found that emitAllocateRawObject is allocating an object from JSFinalObject space while we use it for JSArray too.
We should get per-type allocator to ensure JSArray is allocated in its IsoSubspace.
* dfg/DFGOperations.cpp:
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::emitAllocateRawObject):
(JSC::DFG::SpeculativeJIT::compileNewArray):
(JSC::DFG::SpeculativeJIT::compileMaterializeNewObject):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileNewArray):
(JSC::FTL::DFG::LowerDFGToB3::compileMaterializeNewObject):
* runtime/JSObject.h:
(JSC::JSObject::createRawObject): Deleted.
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@265405 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Diff
Modified: branches/safari-610.1-branch/Source/_javascript_Core/ChangeLog (265585 => 265586)
--- branches/safari-610.1-branch/Source/_javascript_Core/ChangeLog 2020-08-13 01:46:07 UTC (rev 265585)
+++ branches/safari-610.1-branch/Source/_javascript_Core/ChangeLog 2020-08-13 01:46:10 UTC (rev 265586)
@@ -1,3 +1,111 @@
+2020-08-12 Alan Coon <alanc...@apple.com>
+
+ Cherry-pick r265405. rdar://problem/66943811
+
+ [JSC] Speculate children first in DFG NewArray
+ https://bugs.webkit.org/show_bug.cgi?id=215308
+ <rdar://problem/64749263>
+
+ Reviewed by Mark Lam.
+
+ SpeculativeJIT::emitAllocateRawObject can create uninitialized butterfly since we later fill them.
+ However, DFG NewArray node has speculation after that. So if speculation failure happens, we release
+ half-baked butterfly.
+
+ Let's see the example.
+
+ 8459 emitAllocateRawObject(resultGPR, structure, storageGPR, numElements, vectorLengthHint);
+ ...
+ 8482 case ALL_INT32_INDEXING_TYPES:
+ 8483 case ALL_CONTIGUOUS_INDEXING_TYPES: {
+ 8484 JSValueOperand operand(this, use, ManualOperandSpeculation);
+ 8485 JSValueRegs operandRegs = operand.jsValueRegs();
+ 8486 if (hasInt32(node->indexingType())) {
+ 8487 DFG_TYPE_CHECK(
+ 8488 operandRegs, use, SpecInt32Only,
+ 8489 m_jit.branchIfNotInt32(operandRegs));
+ 8490 }
+ 8491 m_jit.storeValue(operandRegs, MacroAssembler::Address(storageGPR, sizeof(JSValue) * operandIdx));
+ 8492 break;
+ 8493 }
+
+ L8487-L8489 is doing speculation check. If it failed, the rest of the butterfly can be filled with garbage. This looks OK since
+ it is Int32 butterfly so GC never scans it. However, if have-a-bad-time happens and the array is reachable from the conservative root,
+ this half-baked array is converted from Int32 array to ArrayStorage. At that time, since Int32 butterfly should hold JSInt32,
+ we store this garbage to ArrayStorage. Later, if conservative root still holds this array, and GC scans this garbage as as JSValue,
+ this value confuses GC.
+
+ In this patch, we first perform speculation before creating uninitialized JSArray so that we can ensure that we never exit after
+ creating this array until we fill it. This strategy is the same to FTL's NewArray implementation.
+
+ And we also found that emitAllocateRawObject is allocating an object from JSFinalObject space while we use it for JSArray too.
+ We should get per-type allocator to ensure JSArray is allocated in its IsoSubspace.
+
+ * dfg/DFGOperations.cpp:
+ * dfg/DFGSpeculativeJIT.cpp:
+ (JSC::DFG::SpeculativeJIT::emitAllocateRawObject):
+ (JSC::DFG::SpeculativeJIT::compileNewArray):
+ (JSC::DFG::SpeculativeJIT::compileMaterializeNewObject):
+ * ftl/FTLLowerDFGToB3.cpp:
+ (JSC::FTL::DFG::LowerDFGToB3::compileNewArray):
+ (JSC::FTL::DFG::LowerDFGToB3::compileMaterializeNewObject):
+ * runtime/JSObject.h:
+ (JSC::JSObject::createRawObject): Deleted.
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@265405 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2020-08-08 Yusuke Suzuki <ysuz...@apple.com>
+
+ [JSC] Speculate children first in DFG NewArray
+ https://bugs.webkit.org/show_bug.cgi?id=215308
+ <rdar://problem/64749263>
+
+ Reviewed by Mark Lam.
+
+ SpeculativeJIT::emitAllocateRawObject can create uninitialized butterfly since we later fill them.
+ However, DFG NewArray node has speculation after that. So if speculation failure happens, we release
+ half-baked butterfly.
+
+ Let's see the example.
+
+ 8459 emitAllocateRawObject(resultGPR, structure, storageGPR, numElements, vectorLengthHint);
+ ...
+ 8482 case ALL_INT32_INDEXING_TYPES:
+ 8483 case ALL_CONTIGUOUS_INDEXING_TYPES: {
+ 8484 JSValueOperand operand(this, use, ManualOperandSpeculation);
+ 8485 JSValueRegs operandRegs = operand.jsValueRegs();
+ 8486 if (hasInt32(node->indexingType())) {
+ 8487 DFG_TYPE_CHECK(
+ 8488 operandRegs, use, SpecInt32Only,
+ 8489 m_jit.branchIfNotInt32(operandRegs));
+ 8490 }
+ 8491 m_jit.storeValue(operandRegs, MacroAssembler::Address(storageGPR, sizeof(JSValue) * operandIdx));
+ 8492 break;
+ 8493 }
+
+ L8487-L8489 is doing speculation check. If it failed, the rest of the butterfly can be filled with garbage. This looks OK since
+ it is Int32 butterfly so GC never scans it. However, if have-a-bad-time happens and the array is reachable from the conservative root,
+ this half-baked array is converted from Int32 array to ArrayStorage. At that time, since Int32 butterfly should hold JSInt32,
+ we store this garbage to ArrayStorage. Later, if conservative root still holds this array, and GC scans this garbage as as JSValue,
+ this value confuses GC.
+
+ In this patch, we first perform speculation before creating uninitialized JSArray so that we can ensure that we never exit after
+ creating this array until we fill it. This strategy is the same to FTL's NewArray implementation.
+
+ And we also found that emitAllocateRawObject is allocating an object from JSFinalObject space while we use it for JSArray too.
+ We should get per-type allocator to ensure JSArray is allocated in its IsoSubspace.
+
+ * dfg/DFGOperations.cpp:
+ * dfg/DFGSpeculativeJIT.cpp:
+ (JSC::DFG::SpeculativeJIT::emitAllocateRawObject):
+ (JSC::DFG::SpeculativeJIT::compileNewArray):
+ (JSC::DFG::SpeculativeJIT::compileMaterializeNewObject):
+ * ftl/FTLLowerDFGToB3.cpp:
+ (JSC::FTL::DFG::LowerDFGToB3::compileNewArray):
+ (JSC::FTL::DFG::LowerDFGToB3::compileMaterializeNewObject):
+ * runtime/JSObject.h:
+ (JSC::JSObject::createRawObject): Deleted.
+
2020-08-10 Alan Coon <alanc...@apple.com>
Cherry-pick r265272. rdar://problem/66604070
Modified: branches/safari-610.1-branch/Source/_javascript_Core/dfg/DFGOperations.cpp (265585 => 265586)
--- branches/safari-610.1-branch/Source/_javascript_Core/dfg/DFGOperations.cpp 2020-08-13 01:46:07 UTC (rev 265585)
+++ branches/safari-610.1-branch/Source/_javascript_Core/dfg/DFGOperations.cpp 2020-08-13 01:46:10 UTC (rev 265586)
@@ -2919,8 +2919,9 @@
length * sizeof(EncodedJSValue));
}
- JSObject* result = JSObject::createRawObject(vm, structure, butterfly);
- return bitwise_cast<char*>(result);
+ if (structure->type() == JSType::ArrayType)
+ return bitwise_cast<char*>(JSArray::createWithButterfly(vm, nullptr, structure, butterfly));
+ return bitwise_cast<char*>(JSFinalObject::create(vm, structure, butterfly));
}
JSCell* JIT_OPERATION operationNewObjectWithButterfly(VM* vmPointer, Structure* structure, Butterfly* butterfly)
@@ -2934,8 +2935,9 @@
vm, nullptr, 0, structure->outOfLineCapacity(), false, IndexingHeader(), 0);
}
- JSObject* result = JSObject::createRawObject(vm, structure, butterfly);
- return result;
+ if (structure->type() == JSType::ArrayType)
+ return JSArray::createWithButterfly(vm, nullptr, structure, butterfly);
+ return JSFinalObject::create(vm, structure, butterfly);
}
JSCell* JIT_OPERATION operationNewObjectWithButterflyWithIndexingHeaderAndVectorLength(VM* vmPointer, Structure* structure, unsigned length, Butterfly* butterfly)
@@ -2955,9 +2957,9 @@
sizeof(EncodedJSValue) * length);
}
- // Paradoxically this may allocate a JSArray. That's totally cool.
- JSObject* result = JSObject::createRawObject(vm, structure, butterfly);
- return result;
+ if (structure->type() == JSType::ArrayType)
+ return JSArray::createWithButterfly(vm, nullptr, structure, butterfly);
+ return JSFinalObject::create(vm, structure, butterfly);
}
JSCell* JIT_OPERATION operationNewArrayWithSpreadSlow(JSGlobalObject* globalObject, void* buffer, uint32_t numItems)
Modified: branches/safari-610.1-branch/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp (265585 => 265586)
--- branches/safari-610.1-branch/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2020-08-13 01:46:07 UTC (rev 265585)
+++ branches/safari-610.1-branch/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2020-08-13 01:46:10 UTC (rev 265586)
@@ -130,8 +130,11 @@
slowCases.append(m_jit.jump());
}
- size_t allocationSize = JSFinalObject::allocationSize(inlineCapacity);
- Allocator allocator = allocatorForNonVirtualConcurrently<JSFinalObject>(vm, allocationSize, AllocatorForMode::AllocatorIfExists);
+ Allocator allocator;
+ if (structure->type() == JSType::ArrayType)
+ allocator = allocatorForNonVirtualConcurrently<JSArray>(vm, JSArray::allocationSize(inlineCapacity), AllocatorForMode::AllocatorIfExists);
+ else
+ allocator = allocatorForNonVirtualConcurrently<JSFinalObject>(vm, JSFinalObject::allocationSize(inlineCapacity), AllocatorForMode::AllocatorIfExists);
if (allocator) {
emitAllocateJSObject(resultGPR, JITAllocator::constant(allocator), scratchGPR, TrustedImmPtr(structure), storageGPR, scratch2GPR, slowCases);
m_jit.emitInitializeInlineStorage(resultGPR, structure->inlineCapacity());
@@ -8450,6 +8453,11 @@
unsigned vectorLengthHint = node->vectorLengthHint();
ASSERT(vectorLengthHint >= numElements);
+ // Because we first speculate on all of the children here, we can never exit after creating
+ // uninitialized contiguous JSArray, which ensures that we will never produce a half-baked JSArray.
+ for (unsigned operandIndex = 0; operandIndex < node->numChildren(); ++operandIndex)
+ speculate(node, m_jit.graph().varArgChild(node, operandIndex));
+
GPRTemporary result(this);
GPRTemporary storage(this);
@@ -8463,8 +8471,8 @@
ASSERT(!hasUndecided(structure->indexingType()) || !node->numChildren());
- for (unsigned operandIdx = 0; operandIdx < node->numChildren(); ++operandIdx) {
- Edge use = m_jit.graph().m_varArgChildren[node->firstChild() + operandIdx];
+ for (unsigned operandIndex = 0; operandIndex < node->numChildren(); ++operandIndex) {
+ Edge use = m_jit.graph().varArgChild(node, operandIndex);
switch (node->indexingType()) {
case ALL_BLANK_INDEXING_TYPES:
case ALL_UNDECIDED_INDEXING_TYPES:
@@ -8473,10 +8481,7 @@
case ALL_DOUBLE_INDEXING_TYPES: {
SpeculateDoubleOperand operand(this, use);
FPRReg opFPR = operand.fpr();
- DFG_TYPE_CHECK(
- JSValueRegs(), use, SpecDoubleReal,
- m_jit.branchIfNaN(opFPR));
- m_jit.storeDouble(opFPR, MacroAssembler::Address(storageGPR, sizeof(double) * operandIdx));
+ m_jit.storeDouble(opFPR, MacroAssembler::Address(storageGPR, sizeof(double) * operandIndex));
break;
}
case ALL_INT32_INDEXING_TYPES:
@@ -8483,12 +8488,7 @@
case ALL_CONTIGUOUS_INDEXING_TYPES: {
JSValueOperand operand(this, use, ManualOperandSpeculation);
JSValueRegs operandRegs = operand.jsValueRegs();
- if (hasInt32(node->indexingType())) {
- DFG_TYPE_CHECK(
- operandRegs, use, SpecInt32Only,
- m_jit.branchIfNotInt32(operandRegs));
- }
- m_jit.storeValue(operandRegs, MacroAssembler::Address(storageGPR, sizeof(JSValue) * operandIdx));
+ m_jit.storeValue(operandRegs, MacroAssembler::Address(storageGPR, sizeof(JSValue) * operandIndex));
break;
}
default:
@@ -12233,6 +12233,8 @@
GPRReg storageGPR = storage.gpr();
emitAllocateRawObject(resultGPR, structure, storageGPR, 0, vectorLength);
+
+ // After the allocation, we must not exit until we fill butterfly completely.
m_jit.store32(
JITCompiler::TrustedImm32(publicLength),
Modified: branches/safari-610.1-branch/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp (265585 => 265586)
--- branches/safari-610.1-branch/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2020-08-13 01:46:07 UTC (rev 265585)
+++ branches/safari-610.1-branch/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2020-08-13 01:46:10 UTC (rev 265586)
@@ -6751,6 +6751,8 @@
// that doing the speculations up here might be unprofitable for RA - so we can consider
// sinking this to below the allocation fast path if we find that this has a lot of
// register pressure.
+ // Because we first speculate on all of the children here, we can never exit after creating
+ // uninitialized contiguous JSArray, which ensures that we will never produce a half-baked JSArray.
for (unsigned operandIndex = 0; operandIndex < m_node->numChildren(); ++operandIndex)
speculate(m_graph.varArgChild(m_node, operandIndex));
@@ -12558,8 +12560,11 @@
LValue butterfly;
if (structure->outOfLineCapacity() || hasIndexedProperties(structure->indexingType())) {
- size_t allocationSize = JSFinalObject::allocationSize(structure->inlineCapacity());
- Allocator cellAllocator = allocatorForNonVirtualConcurrently<JSFinalObject>(vm(), allocationSize, AllocatorForMode::AllocatorIfExists);
+ Allocator cellAllocator;
+ if (structure->type() == JSType::ArrayType)
+ cellAllocator = allocatorForNonVirtualConcurrently<JSArray>(vm(), JSArray::allocationSize(structure->inlineCapacity()), AllocatorForMode::AllocatorIfExists);
+ else
+ cellAllocator = allocatorForNonVirtualConcurrently<JSFinalObject>(vm(), JSFinalObject::allocationSize(structure->inlineCapacity()), AllocatorForMode::AllocatorIfExists);
bool hasIndexingHeader = hasIndexedProperties(structure->indexingType());
unsigned indexingHeaderSize = 0;
Modified: branches/safari-610.1-branch/Source/_javascript_Core/runtime/JSObject.h (265585 => 265586)
--- branches/safari-610.1-branch/Source/_javascript_Core/runtime/JSObject.h 2020-08-13 01:46:07 UTC (rev 265585)
+++ branches/safari-610.1-branch/Source/_javascript_Core/runtime/JSObject.h 2020-08-13 01:46:10 UTC (rev 265586)
@@ -114,10 +114,6 @@
public:
using Base = JSCell;
- // This is a super dangerous method for JITs. Sometimes the JITs will want to create either a
- // JSFinalObject or a JSArray. This is the method that will do that.
- static JSObject* createRawObject(VM& vm, Structure* structure, Butterfly* = nullptr);
-
JS_EXPORT_PRIVATE static size_t estimatedSize(JSCell*, VM&);
JS_EXPORT_PRIVATE static void visitChildren(JSCell*, SlotVisitor&);
JS_EXPORT_PRIVATE static void analyzeHeap(JSCell*, HeapAnalyzer&);
@@ -1244,19 +1240,6 @@
JS_EXPORT_PRIVATE EncodedJSValue JSC_HOST_CALL objectPrivateFuncInstanceOf(JSGlobalObject*, CallFrame*);
-inline JSObject* JSObject::createRawObject(VM& vm, Structure* structure, Butterfly* butterfly)
-{
- JSObject* finalObject = new (
- NotNull,
- allocateCell<JSFinalObject>(
- vm.heap,
- JSFinalObject::allocationSize(structure->inlineCapacity())
- )
- ) JSObject(vm, structure, butterfly);
- finalObject->finishCreation(vm);
- return finalObject;
-}
-
inline JSFinalObject* JSFinalObject::create(VM& vm, Structure* structure, Butterfly* butterfly)
{
JSFinalObject* finalObject = new (
