Diff
Modified: branches/safari-605-branch/JSTests/ChangeLog (230301 => 230302)
--- branches/safari-605-branch/JSTests/ChangeLog 2018-04-05 17:05:08 UTC (rev 230301)
+++ branches/safari-605-branch/JSTests/ChangeLog 2018-04-05 17:05:12 UTC (rev 230302)
@@ -1,3 +1,63 @@
+2018-04-05 Jason Marcell <jmarc...@apple.com>
+
+ Cherry-pick r230287. rdar://problem/39208588
+
+ REGRESSION(r222563): removed DoubleReal type check causes tons of crashes because CSE has never known how to handle SaneChain
+ https://bugs.webkit.org/show_bug.cgi?id=184319
+
+ Reviewed by Saam Barati.
+
+ JSTests:
+
+ * stress/array-push-nan-to-double-array-cse-sane-and-insane-chain.js: Added.
+ (foo):
+ (bar):
+ * stress/array-push-nan-to-double-array.js: Added.
+ (foo):
+ (bar):
+
+ Source/_javascript_Core:
+
+ In r222581, we replaced type checks about DoubleReal in ArrayPush in the DFG/FTL backends with
+ assertions. That's correct because FixupPhase was emitting those checks as Check(DoubleRealRep:) before
+ the ArrayPush.
+
+ But this revealed a longstanding CSE bug: CSE will happily match a SaneChain GetByVal with a InBounds
+ GetByVal. SaneChain can return NaN while InBounds cannot. This means that if we first use AI to
+ eliminate the Check(DoubleRealRep:) based on the input being a GetByVal(InBounds) but then replace that
+ with a GetByVal(SaneChain), then we will hit the assertion.
+
+ This teaches CSE to not replace GetByVal(InBounds) with GetByVal(SaneChain) and vice versa. That gets
+ tricky because PutByVal can match either. So, we use the fact that it's legal for a store to def() more
+ than once: PutByVal now defs() a HeapLocation for InBounds and a HeapLocation for SaneChain.
+
+ * dfg/DFGCSEPhase.cpp:
+ * dfg/DFGClobberize.h:
+ (JSC::DFG::clobberize):
+ * dfg/DFGHeapLocation.cpp:
+ (WTF::printInternal):
+ * dfg/DFGHeapLocation.h:
+ * dfg/DFGSpeculativeJIT.cpp:
+ (JSC::DFG::SpeculativeJIT::compileArrayPush):
+
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@230287 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2018-04-04 Filip Pizlo <fpi...@apple.com>
+
+ REGRESSION(r222563): removed DoubleReal type check causes tons of crashes because CSE has never known how to handle SaneChain
+ https://bugs.webkit.org/show_bug.cgi?id=184319
+
+ Reviewed by Saam Barati.
+
+ * stress/array-push-nan-to-double-array-cse-sane-and-insane-chain.js: Added.
+ (foo):
+ (bar):
+ * stress/array-push-nan-to-double-array.js: Added.
+ (foo):
+ (bar):
+
2018-04-03 Jason Marcell <jmarc...@apple.com>
Cherry-pick r229987. rdar://problem/39155464
Added: branches/safari-605-branch/JSTests/stress/array-push-nan-to-double-array-cse-sane-and-insane-chain.js (0 => 230302)
--- branches/safari-605-branch/JSTests/stress/array-push-nan-to-double-array-cse-sane-and-insane-chain.js (rev 0)
+++ branches/safari-605-branch/JSTests/stress/array-push-nan-to-double-array-cse-sane-and-insane-chain.js 2018-04-05 17:05:12 UTC (rev 230302)
@@ -0,0 +1,23 @@
+function foo(array, otherArray, i)
+{
+ var x = otherArray[i];
+ var y = otherArray[i];
+ array.push(y);
+ return x / 42;
+}
+
+function bar()
+{
+ return [];
+}
+
+noInline(foo);
+noInline(bar);
+
+for (var i = 0; i < 10000; ++i)
+ foo(bar(), [42.5], 0);
+
+var result = bar();
+foo(result, [,42.5], 0);
+if (result[0] !== void 0)
+ throw "Bad result: " + result;
Added: branches/safari-605-branch/JSTests/stress/array-push-nan-to-double-array.js (0 => 230302)
--- branches/safari-605-branch/JSTests/stress/array-push-nan-to-double-array.js (rev 0)
+++ branches/safari-605-branch/JSTests/stress/array-push-nan-to-double-array.js 2018-04-05 17:05:12 UTC (rev 230302)
@@ -0,0 +1,20 @@
+function foo(array, num, denum)
+{
+ array.push(num/denum);
+}
+
+function bar()
+{
+ return [];
+}
+
+noInline(foo);
+noInline(bar);
+
+for (var i = 0; i < 10000; ++i)
+ foo(bar(), 42.5, 3.1);
+
+var result = bar();
+foo(result, 0, 0);
+if ("" + result[0] !== "NaN")
+ throw "Bad result: " + result;
Modified: branches/safari-605-branch/Source/_javascript_Core/ChangeLog (230301 => 230302)
--- branches/safari-605-branch/Source/_javascript_Core/ChangeLog 2018-04-05 17:05:08 UTC (rev 230301)
+++ branches/safari-605-branch/Source/_javascript_Core/ChangeLog 2018-04-05 17:05:12 UTC (rev 230302)
@@ -1,3 +1,78 @@
+2018-04-05 Jason Marcell <jmarc...@apple.com>
+
+ Cherry-pick r230287. rdar://problem/39208588
+
+ REGRESSION(r222563): removed DoubleReal type check causes tons of crashes because CSE has never known how to handle SaneChain
+ https://bugs.webkit.org/show_bug.cgi?id=184319
+
+ Reviewed by Saam Barati.
+
+ JSTests:
+
+ * stress/array-push-nan-to-double-array-cse-sane-and-insane-chain.js: Added.
+ (foo):
+ (bar):
+ * stress/array-push-nan-to-double-array.js: Added.
+ (foo):
+ (bar):
+
+ Source/_javascript_Core:
+
+ In r222581, we replaced type checks about DoubleReal in ArrayPush in the DFG/FTL backends with
+ assertions. That's correct because FixupPhase was emitting those checks as Check(DoubleRealRep:) before
+ the ArrayPush.
+
+ But this revealed a longstanding CSE bug: CSE will happily match a SaneChain GetByVal with a InBounds
+ GetByVal. SaneChain can return NaN while InBounds cannot. This means that if we first use AI to
+ eliminate the Check(DoubleRealRep:) based on the input being a GetByVal(InBounds) but then replace that
+ with a GetByVal(SaneChain), then we will hit the assertion.
+
+ This teaches CSE to not replace GetByVal(InBounds) with GetByVal(SaneChain) and vice versa. That gets
+ tricky because PutByVal can match either. So, we use the fact that it's legal for a store to def() more
+ than once: PutByVal now defs() a HeapLocation for InBounds and a HeapLocation for SaneChain.
+
+ * dfg/DFGCSEPhase.cpp:
+ * dfg/DFGClobberize.h:
+ (JSC::DFG::clobberize):
+ * dfg/DFGHeapLocation.cpp:
+ (WTF::printInternal):
+ * dfg/DFGHeapLocation.h:
+ * dfg/DFGSpeculativeJIT.cpp:
+ (JSC::DFG::SpeculativeJIT::compileArrayPush):
+
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@230287 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2018-04-04 Filip Pizlo <fpi...@apple.com>
+
+ REGRESSION(r222563): removed DoubleReal type check causes tons of crashes because CSE has never known how to handle SaneChain
+ https://bugs.webkit.org/show_bug.cgi?id=184319
+
+ Reviewed by Saam Barati.
+
+ In r222581, we replaced type checks about DoubleReal in ArrayPush in the DFG/FTL backends with
+ assertions. That's correct because FixupPhase was emitting those checks as Check(DoubleRealRep:) before
+ the ArrayPush.
+
+ But this revealed a longstanding CSE bug: CSE will happily match a SaneChain GetByVal with a InBounds
+ GetByVal. SaneChain can return NaN while InBounds cannot. This means that if we first use AI to
+ eliminate the Check(DoubleRealRep:) based on the input being a GetByVal(InBounds) but then replace that
+ with a GetByVal(SaneChain), then we will hit the assertion.
+
+ This teaches CSE to not replace GetByVal(InBounds) with GetByVal(SaneChain) and vice versa. That gets
+ tricky because PutByVal can match either. So, we use the fact that it's legal for a store to def() more
+ than once: PutByVal now defs() a HeapLocation for InBounds and a HeapLocation for SaneChain.
+
+ * dfg/DFGCSEPhase.cpp:
+ * dfg/DFGClobberize.h:
+ (JSC::DFG::clobberize):
+ * dfg/DFGHeapLocation.cpp:
+ (WTF::printInternal):
+ * dfg/DFGHeapLocation.h:
+ * dfg/DFGSpeculativeJIT.cpp:
+ (JSC::DFG::SpeculativeJIT::compileArrayPush):
+
2018-04-03 Jason Marcell <jmarc...@apple.com>
Cherry-pick r229987. rdar://problem/39155464
Modified: branches/safari-605-branch/Source/_javascript_Core/dfg/DFGCSEPhase.cpp (230301 => 230302)
--- branches/safari-605-branch/Source/_javascript_Core/dfg/DFGCSEPhase.cpp 2018-04-05 17:05:08 UTC (rev 230301)
+++ branches/safari-605-branch/Source/_javascript_Core/dfg/DFGCSEPhase.cpp 2018-04-05 17:05:12 UTC (rev 230302)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2011-2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2011-2018 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -469,22 +469,21 @@
case Array::Int32:
if (!mode.isInBounds())
break;
- heap = HeapLocation(
- indexedPropertyLoc, IndexedInt32Properties, base, index);
+ heap = HeapLocation(indexedPropertyLoc, IndexedInt32Properties, base, index);
break;
- case Array::Double:
+ case Array::Double: {
if (!mode.isInBounds())
break;
- heap = HeapLocation(
- indexedPropertyLoc, IndexedDoubleProperties, base, index);
+ LocationKind kind = mode.isSaneChain() ? IndexedPropertyDoubleSaneChainLoc : IndexedPropertyDoubleLoc;
+ heap = HeapLocation(kind, IndexedDoubleProperties, base, index);
break;
+ }
case Array::Contiguous:
if (!mode.isInBounds())
break;
- heap = HeapLocation(
- indexedPropertyLoc, IndexedContiguousProperties, base, index);
+ heap = HeapLocation(indexedPropertyLoc, IndexedContiguousProperties, base, index);
break;
case Array::Int8Array:
Modified: branches/safari-605-branch/Source/_javascript_Core/dfg/DFGClobberize.h (230301 => 230302)
--- branches/safari-605-branch/Source/_javascript_Core/dfg/DFGClobberize.h 2018-04-05 17:05:08 UTC (rev 230301)
+++ branches/safari-605-branch/Source/_javascript_Core/dfg/DFGClobberize.h 2018-04-05 17:05:12 UTC (rev 230302)
@@ -811,7 +811,8 @@
if (mode.isInBounds()) {
read(Butterfly_publicLength);
read(IndexedDoubleProperties);
- def(HeapLocation(indexedPropertyLoc, IndexedDoubleProperties, graph.varArgChild(node, 0), graph.varArgChild(node, 1)), LazyNode(node));
+ LocationKind kind = mode.isSaneChain() ? IndexedPropertyDoubleSaneChainLoc : IndexedPropertyDoubleLoc;
+ def(HeapLocation(kind, IndexedDoubleProperties, graph.varArgChild(node, 0), graph.varArgChild(node, 1)), LazyNode(node));
return;
}
read(World);
@@ -930,7 +931,8 @@
write(IndexedDoubleProperties);
if (node->arrayMode().mayStoreToHole())
write(Butterfly_publicLength);
- def(HeapLocation(indexedPropertyLoc, IndexedDoubleProperties, base, index), LazyNode(value));
+ def(HeapLocation(IndexedPropertyDoubleLoc, IndexedDoubleProperties, base, index), LazyNode(value));
+ def(HeapLocation(IndexedPropertyDoubleSaneChainLoc, IndexedDoubleProperties, base, index), LazyNode(value));
return;
case Array::Contiguous:
Modified: branches/safari-605-branch/Source/_javascript_Core/dfg/DFGHeapLocation.cpp (230301 => 230302)
--- branches/safari-605-branch/Source/_javascript_Core/dfg/DFGHeapLocation.cpp 2018-04-05 17:05:08 UTC (rev 230301)
+++ branches/safari-605-branch/Source/_javascript_Core/dfg/DFGHeapLocation.cpp 2018-04-05 17:05:12 UTC (rev 230302)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2014-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2014-2018 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -128,6 +128,10 @@
out.print("IndexedPropertyDoubleLoc");
return;
+ case IndexedPropertyDoubleSaneChainLoc:
+ out.print("IndexedPropertyDoubleSaneChainLoc");
+ return;
+
case IndexedPropertyInt52Loc:
out.print("IndexedPropertyInt52Loc");
return;
Modified: branches/safari-605-branch/Source/_javascript_Core/dfg/DFGHeapLocation.h (230301 => 230302)
--- branches/safari-605-branch/Source/_javascript_Core/dfg/DFGHeapLocation.h 2018-04-05 17:05:08 UTC (rev 230301)
+++ branches/safari-605-branch/Source/_javascript_Core/dfg/DFGHeapLocation.h 2018-04-05 17:05:12 UTC (rev 230302)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2014-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2014-2018 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -48,6 +48,7 @@
GlobalVariableLoc,
HasIndexedPropertyLoc,
IndexedPropertyDoubleLoc,
+ IndexedPropertyDoubleSaneChainLoc,
IndexedPropertyInt52Loc,
IndexedPropertyJSLoc,
IndexedPropertyStorageLoc,
Modified: branches/safari-605-branch/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp (230301 => 230302)
--- branches/safari-605-branch/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2018-04-05 17:05:08 UTC (rev 230301)
+++ branches/safari-605-branch/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2018-04-05 17:05:12 UTC (rev 230302)
@@ -7983,7 +7983,7 @@
JSValueRegs valueRegs = value.jsValueRegs();
if (node->arrayMode().type() == Array::Int32)
- RELEASE_ASSERT(!needsTypeCheck(element, SpecInt32Only));
+ DFG_ASSERT(m_jit.graph(), node, !needsTypeCheck(element, SpecInt32Only));
m_jit.load32(MacroAssembler::Address(storageGPR, Butterfly::offsetOfPublicLength()), storageLengthGPR);
MacroAssembler::Jump slowPath = m_jit.branch32(MacroAssembler::AboveOrEqual, storageLengthGPR, MacroAssembler::Address(storageGPR, Butterfly::offsetOfVectorLength()));
@@ -8028,7 +8028,7 @@
JSValueRegs valueRegs = value.jsValueRegs();
if (node->arrayMode().type() == Array::Int32)
- RELEASE_ASSERT(!needsTypeCheck(element, SpecInt32Only));
+ DFG_ASSERT(m_jit.graph(), node, !needsTypeCheck(element, SpecInt32Only));
m_jit.storeValue(valueRegs, MacroAssembler::Address(bufferGPR, sizeof(EncodedJSValue) * elementIndex));
value.use();
@@ -8055,7 +8055,7 @@
SpeculateDoubleOperand value(this, element);
FPRReg valueFPR = value.fpr();
- RELEASE_ASSERT(!needsTypeCheck(element, SpecDoubleReal));
+ DFG_ASSERT(m_jit.graph(), node, !needsTypeCheck(element, SpecDoubleReal));
m_jit.load32(MacroAssembler::Address(storageGPR, Butterfly::offsetOfPublicLength()), storageLengthGPR);
MacroAssembler::Jump slowPath = m_jit.branch32(MacroAssembler::AboveOrEqual, storageLengthGPR, MacroAssembler::Address(storageGPR, Butterfly::offsetOfVectorLength()));
@@ -8099,7 +8099,7 @@
SpeculateDoubleOperand value(this, element);
FPRReg valueFPR = value.fpr();
- RELEASE_ASSERT(!needsTypeCheck(element, SpecDoubleReal));
+ DFG_ASSERT(m_jit.graph(), node, !needsTypeCheck(element, SpecDoubleReal));
m_jit.storeDouble(valueFPR, MacroAssembler::Address(bufferGPR, sizeof(double) * elementIndex));
value.use();
Modified: branches/safari-605-branch/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp (230301 => 230302)
--- branches/safari-605-branch/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2018-04-05 17:05:08 UTC (rev 230301)
+++ branches/safari-605-branch/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2018-04-05 17:05:12 UTC (rev 230302)
@@ -4345,11 +4345,11 @@
if (m_node->arrayMode().type() != Array::Double) {
value = lowJSValue(element, ManualOperandSpeculation);
if (m_node->arrayMode().type() == Array::Int32)
- RELEASE_ASSERT(!m_interpreter.needsTypeCheck(element, SpecInt32Only));
+ DFG_ASSERT(m_graph, m_node, !m_interpreter.needsTypeCheck(element, SpecInt32Only));
storeType = Output::Store64;
} else {
value = lowDouble(element);
- RELEASE_ASSERT(!m_interpreter.needsTypeCheck(element, SpecDoubleReal));
+ DFG_ASSERT(m_graph, m_node, !m_interpreter.needsTypeCheck(element, SpecDoubleReal));
storeType = Output::StoreDouble;
}
@@ -4425,11 +4425,11 @@
if (m_node->arrayMode().type() != Array::Double) {
value = lowJSValue(element, ManualOperandSpeculation);
if (m_node->arrayMode().type() == Array::Int32)
- RELEASE_ASSERT(!m_interpreter.needsTypeCheck(element, SpecInt32Only));
+ DFG_ASSERT(m_graph, m_node, !m_interpreter.needsTypeCheck(element, SpecInt32Only));
storeType = Output::Store64;
} else {
value = lowDouble(element);
- RELEASE_ASSERT(!m_interpreter.needsTypeCheck(element, SpecDoubleReal));
+ DFG_ASSERT(m_graph, m_node, !m_interpreter.needsTypeCheck(element, SpecDoubleReal));
storeType = Output::StoreDouble;
}
