Skip to content

Commit

Permalink
Fixup uses KnownInt32 incorrectly in some nodes
Browse files Browse the repository at this point in the history 
https://bugs.webkit.org/show_bug.cgi?id=195279
<rdar://problem/47915654>

Reviewed by Yusuke Suzuki.

JSTests:

* stress/known-int32-cant-be-used-across-bytecode-boundary.js: Added.
(foo):

Source/JavaScriptCore:

Fixup was sometimes using KnownInt32 edges when it knew some
incoming value is an Int32 based on what the bytecode would return.
However, because bytecode may result in Int32 for some node does
not mean we'll pick Int32 as the value format for that local. For example,
we may choose for a value to be represented as a double. This patch
corrects such uses of KnownInt32.

* dfg/DFGArgumentsEliminationPhase.cpp:
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileArrayPush):
(JSC::DFG::SpeculativeJIT::compileGetDirectPname):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileArrayPush):


Canonical link: https://commits.webkit.org/210044@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242954 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
Saam Barati committed Mar 14, 2019
1 parent b9f3fa6 commit 7eaa565
Show file tree
Hide file tree
Showing 7 changed files with 107 additions and 33 deletions.
11 changes: 11 additions & 0 deletions JSTests/ChangeLog
View file
@@ -1,3 +1,14 @@
2019-03-14 Saam barati <sbarati@apple.com>

Fixup uses KnownInt32 incorrectly in some nodes
https://bugs.webkit.org/show_bug.cgi?id=195279
<rdar://problem/47915654>

Reviewed by Yusuke Suzuki.

* stress/known-int32-cant-be-used-across-bytecode-boundary.js: Added.
(foo):

2019-03-14 Keith Miller <keith_miller@apple.com>

DFG liveness can't skip tail caller inline frames
Expand Down
26 changes: 26 additions & 0 deletions JSTests/stress/known-int32-cant-be-used-across-bytecode-boundary.js
View file
@@ -0,0 +1,26 @@
//@ runDefault("--useConcurrentJIT=0", "--useMaximalFlushInsertionPhase=1")

function foo() {
var x0, x1, x2, x3, x4, x5, x6, x7, x8, x9, x10, x11, x12, x13;
var copy = [];
var value = this[1];

for (var p in this)
copy[copy.length] = value;

for (var i = 0; i < 1000; i++) {
for (var j = 0; j < 1; j++) {
}
Math.min(0 ** []);
}
};

noInline(foo);

let array0 = new Array(3).fill(1);
delete array0[0];

([])[1000] = 0xFFFFF;

for (var i = 0; i < 100; i++)
foo.call(array0);
24 changes: 24 additions & 0 deletions Source/JavaScriptCore/ChangeLog
View file
@@ -1,3 +1,27 @@
2019-03-14 Saam barati <sbarati@apple.com>

Fixup uses KnownInt32 incorrectly in some nodes
https://bugs.webkit.org/show_bug.cgi?id=195279
<rdar://problem/47915654>

Reviewed by Yusuke Suzuki.

Fixup was sometimes using KnownInt32 edges when it knew some
incoming value is an Int32 based on what the bytecode would return.
However, because bytecode may result in Int32 for some node does
not mean we'll pick Int32 as the value format for that local. For example,
we may choose for a value to be represented as a double. This patch
corrects such uses of KnownInt32.

* dfg/DFGArgumentsEliminationPhase.cpp:
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileArrayPush):
(JSC::DFG::SpeculativeJIT::compileGetDirectPname):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileArrayPush):

2019-03-14 Keith Miller <keith_miller@apple.com>

DFG liveness can't skip tail caller inline frames
Expand Down
8 changes: 7 additions & 1 deletion Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp
View file
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2015-2018 Apple Inc. All rights reserved.
* Copyright (C) 2015-2019 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
Expand Down Expand Up @@ -661,6 +661,12 @@ class ArgumentsEliminationPhase : public Phase {
if (!m_candidates.contains(node))
break;

ASSERT(node->origin.exitOK);
ASSERT(node->child1().useKind() == Int32Use);
insertionSet.insertNode(
nodeIndex, SpecNone, Check, node->origin,
node->child1());

node->setOpAndDefaultFlags(PhantomCreateRest);
// We don't need this parameter for OSR exit, we can find out all the information
// we need via the static parameter count and the dynamic argument count.
Expand Down
25 changes: 11 additions & 14 deletions Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
View file
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2012-2018 Apple Inc. All rights reserved.
* Copyright (C) 2012-2019 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
Expand Down Expand Up @@ -1149,12 +1149,10 @@ class FixupPhase : public Phase {
Edge& element = m_graph.varArgChild(node, i + elementOffset);
switch (node->arrayMode().type()) {
case Array::Int32:
insertCheck<Int32Use>(element.node());
fixEdge<KnownInt32Use>(element);
fixEdge<Int32Use>(element);
break;
case Array::Double:
insertCheck<DoubleRepRealUse>(element.node());
fixEdge<DoubleRepUse>(element);
fixEdge<DoubleRepRealUse>(element);
break;
case Array::Contiguous:
case Array::ArrayStorage:
Expand All @@ -1163,7 +1161,6 @@ class FixupPhase : public Phase {
default:
break;
}
ASSERT(shouldNotHaveTypeCheck(element.useKind()));
}
break;
}
Expand Down Expand Up @@ -1868,7 +1865,7 @@ class FixupPhase : public Phase {

blessArrayOperation(m_graph.varArgChild(node, 0), m_graph.varArgChild(node, 1), m_graph.varArgChild(node, 2));
fixEdge<CellUse>(m_graph.varArgChild(node, 0));
fixEdge<KnownInt32Use>(m_graph.varArgChild(node, 1));
fixEdge<Int32Use>(m_graph.varArgChild(node, 1));
break;
}
case GetDirectPname: {
Expand All @@ -1878,7 +1875,7 @@ class FixupPhase : public Phase {
Edge& enumerator = m_graph.varArgChild(node, 3);
fixEdge<CellUse>(base);
fixEdge<KnownCellUse>(property);
fixEdge<KnownInt32Use>(index);
fixEdge<Int32Use>(index);
fixEdge<KnownCellUse>(enumerator);
break;
}
Expand All @@ -1889,16 +1886,16 @@ class FixupPhase : public Phase {
}
case GetEnumeratorStructurePname: {
fixEdge<KnownCellUse>(node->child1());
fixEdge<KnownInt32Use>(node->child2());
fixEdge<Int32Use>(node->child2());
break;
}
case GetEnumeratorGenericPname: {
fixEdge<KnownCellUse>(node->child1());
fixEdge<KnownInt32Use>(node->child2());
fixEdge<Int32Use>(node->child2());
break;
}
case ToIndexString: {
fixEdge<KnownInt32Use>(node->child1());
fixEdge<Int32Use>(node->child1());
break;
}
case ProfileType: {
Expand Down Expand Up @@ -1990,7 +1987,7 @@ class FixupPhase : public Phase {

case CreateRest: {
watchHavingABadTime(node);
fixEdge<KnownInt32Use>(node->child1());
fixEdge<Int32Use>(node->child1());
break;
}

Expand Down Expand Up @@ -2154,7 +2151,7 @@ class FixupPhase : public Phase {
else
fixEdge<UntypedUse>(propertyEdge);
fixEdge<UntypedUse>(m_graph.varArgChild(node, 2));
fixEdge<KnownInt32Use>(m_graph.varArgChild(node, 3));
fixEdge<Int32Use>(m_graph.varArgChild(node, 3));
break;
}

Expand Down Expand Up @@ -2204,7 +2201,7 @@ class FixupPhase : public Phase {
fixEdge<UntypedUse>(propertyEdge);
fixEdge<CellUse>(m_graph.varArgChild(node, 2));
fixEdge<CellUse>(m_graph.varArgChild(node, 3));
fixEdge<KnownInt32Use>(m_graph.varArgChild(node, 4));
fixEdge<Int32Use>(m_graph.varArgChild(node, 4));
break;
}

Expand Down
34 changes: 22 additions & 12 deletions Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
View file
Expand Up @@ -8570,12 +8570,13 @@ void SpeculativeJIT::compileArrayPush(Node* node)
case Array::Contiguous: {
if (elementCount == 1) {
Edge& element = m_jit.graph().varArgChild(node, elementOffset);
if (node->arrayMode().type() == Array::Int32) {
ASSERT(element.useKind() == Int32Use);
speculateInt32(element);
}
JSValueOperand value(this, element, ManualOperandSpeculation);
JSValueRegs valueRegs = value.jsValueRegs();

if (node->arrayMode().type() == Array::Int32)
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()));
m_jit.storeValue(valueRegs, MacroAssembler::BaseIndex(storageGPR, storageLengthGPR, MacroAssembler::TimesEight));
Expand All @@ -8590,6 +8591,14 @@ void SpeculativeJIT::compileArrayPush(Node* node)
return;
}

if (node->arrayMode().type() == Array::Int32) {
for (unsigned elementIndex = 0; elementIndex < elementCount; ++elementIndex) {
Edge element = m_jit.graph().varArgChild(node, elementIndex + elementOffset);
ASSERT(element.useKind() == Int32Use);
speculateInt32(element);
}
}

GPRTemporary buffer(this);
GPRReg bufferGPR = buffer.gpr();

Expand All @@ -8615,12 +8624,9 @@ void SpeculativeJIT::compileArrayPush(Node* node)
storageDone.link(&m_jit);
for (unsigned elementIndex = 0; elementIndex < elementCount; ++elementIndex) {
Edge& element = m_jit.graph().varArgChild(node, elementIndex + elementOffset);
JSValueOperand value(this, element, ManualOperandSpeculation);
JSValueOperand value(this, element, ManualOperandSpeculation); // We did type checks above.
JSValueRegs valueRegs = value.jsValueRegs();

if (node->arrayMode().type() == Array::Int32)
DFG_ASSERT(m_jit.graph(), node, !needsTypeCheck(element, SpecInt32Only));

m_jit.storeValue(valueRegs, MacroAssembler::Address(bufferGPR, sizeof(EncodedJSValue) * elementIndex));
value.use();
}
Expand All @@ -8643,11 +8649,10 @@ void SpeculativeJIT::compileArrayPush(Node* node)
case Array::Double: {
if (elementCount == 1) {
Edge& element = m_jit.graph().varArgChild(node, elementOffset);
speculate(node, element);
SpeculateDoubleOperand value(this, element);
FPRReg valueFPR = value.fpr();

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()));
m_jit.storeDouble(valueFPR, MacroAssembler::BaseIndex(storageGPR, storageLengthGPR, MacroAssembler::TimesEight));
Expand All @@ -8662,6 +8667,12 @@ void SpeculativeJIT::compileArrayPush(Node* node)
return;
}

for (unsigned elementIndex = 0; elementIndex < elementCount; ++elementIndex) {
Edge element = m_jit.graph().varArgChild(node, elementIndex + elementOffset);
ASSERT(element.useKind() == DoubleRepRealUse);
speculate(node, element);
}

GPRTemporary buffer(this);
GPRReg bufferGPR = buffer.gpr();

Expand Down Expand Up @@ -8690,8 +8701,6 @@ void SpeculativeJIT::compileArrayPush(Node* node)
SpeculateDoubleOperand value(this, element);
FPRReg valueFPR = value.fpr();

DFG_ASSERT(m_jit.graph(), node, !needsTypeCheck(element, SpecDoubleReal));

m_jit.storeDouble(valueFPR, MacroAssembler::Address(bufferGPR, sizeof(double) * elementIndex));
value.use();
}
Expand Down Expand Up @@ -13086,6 +13095,7 @@ void SpeculativeJIT::compileGetDirectPname(Node* node)
{
Edge& baseEdge = m_jit.graph().varArgChild(node, 0);
Edge& propertyEdge = m_jit.graph().varArgChild(node, 1);
Edge& indexEdge = m_jit.graph().varArgChild(node, 2);

SpeculateCellOperand base(this, baseEdge);
SpeculateCellOperand property(this, propertyEdge);
Expand All @@ -13094,14 +13104,14 @@ void SpeculativeJIT::compileGetDirectPname(Node* node)

#if CPU(X86)
// Not enough registers on X86 for this code, so always use the slow path.
speculate(node, indexEdge);
flushRegisters();
JSValueRegsFlushedCallResult result(this);
JSValueRegs resultRegs = result.regs();
callOperation(operationGetByValCell, resultRegs, baseGPR, CCallHelpers::CellValue(propertyGPR));
m_jit.exceptionCheck();
jsValueResult(resultRegs, node);
#else
Edge& indexEdge = m_jit.graph().varArgChild(node, 2);
Edge& enumeratorEdge = m_jit.graph().varArgChild(node, 3);
SpeculateStrictInt32Operand index(this, indexEdge);
SpeculateCellOperand enumerator(this, enumeratorEdge);
Expand Down
12 changes: 6 additions & 6 deletions Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
View file
Expand Up @@ -4674,14 +4674,12 @@ class LowerDFGToB3 {
Output::StoreType storeType;

Edge& element = m_graph.varArgChild(m_node, elementOffset);
speculate(element);
if (m_node->arrayMode().type() != Array::Double) {
value = lowJSValue(element, ManualOperandSpeculation);
if (m_node->arrayMode().type() == Array::Int32)
DFG_ASSERT(m_graph, m_node, !m_interpreter.needsTypeCheck(element, SpecInt32Only));
storeType = Output::Store64;
} else {
value = lowDouble(element);
DFG_ASSERT(m_graph, m_node, !m_interpreter.needsTypeCheck(element, SpecDoubleReal));
storeType = Output::StoreDouble;
}

Expand Down Expand Up @@ -4720,6 +4718,11 @@ class LowerDFGToB3 {
return;
}

for (unsigned elementIndex = 0; elementIndex < elementCount; ++elementIndex) {
Edge element = m_graph.varArgChild(m_node, elementIndex + elementOffset);
speculate(element);
}

LValue prevLength = m_out.load32(storage, m_heaps.Butterfly_publicLength);
LValue newLength = m_out.add(prevLength, m_out.constInt32(elementCount));

Expand Down Expand Up @@ -4756,12 +4759,9 @@ class LowerDFGToB3 {
Output::StoreType storeType;
if (m_node->arrayMode().type() != Array::Double) {
value = lowJSValue(element, ManualOperandSpeculation);
if (m_node->arrayMode().type() == Array::Int32)
DFG_ASSERT(m_graph, m_node, !m_interpreter.needsTypeCheck(element, SpecInt32Only));
storeType = Output::Store64;
} else {
value = lowDouble(element);
DFG_ASSERT(m_graph, m_node, !m_interpreter.needsTypeCheck(element, SpecDoubleReal));
storeType = Output::StoreDouble;
}

Expand Down

0 comments on commit 7eaa565

Please sign in to comment.