Commit 211b3bec authored by oliver@apple.com's avatar oliver@apple.com

fourthTier: DFG shouldn't exit just because a String GetByVal went out-of-bounds

https://bugs.webkit.org/show_bug.cgi?id=117906

Source/JavaScriptCore:

Reviewed by Mark Hahnenberg.

This does the obvious thing, but also makes sure that out-of-bounds accesses
don't fall off into a C call, but try to do the fast thing if the prototype
chain is sane. We ought to probably do this for other array accesses in the
future, as well, since it's so darn easy.

* dfg/DFGAbstractState.cpp:
(JSC::DFG::AbstractState::executeEffects):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGOperations.cpp:
* dfg/DFGOperations.h:
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileGetByValOnString):
* dfg/DFGSpeculativeJIT.h:
(JSC::DFG::SpeculativeJIT::callOperation):
* runtime/JSGlobalObject.cpp:
(JSC::JSGlobalObject::objectPrototypeIsSane):
(JSC):
(JSC::JSGlobalObject::arrayPrototypeChainIsSane):
(JSC::JSGlobalObject::stringPrototypeChainIsSane):
* runtime/JSGlobalObject.h:
(JSGlobalObject):

LayoutTests:

Reviewed by Mark Hahnenberg.

The out-of-bounds benchmark that isn't insane speeds up by 22x in this
patch.

* fast/js/regress/script-tests/string-get-by-val-out-of-bounds-insane.js: Added.
(foo):
* fast/js/regress/script-tests/string-get-by-val-out-of-bounds.js: Added.
(foo):
* fast/js/regress/string-get-by-val-out-of-bounds-expected.txt: Added.
* fast/js/regress/string-get-by-val-out-of-bounds-insane-expected.txt: Added.
* fast/js/regress/string-get-by-val-out-of-bounds-insane.html: Added.
* fast/js/regress/string-get-by-val-out-of-bounds.html: Added.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@153244 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent c0a31d7e
2013-06-21 Filip Pizlo <fpizlo@apple.com>
fourthTier: DFG shouldn't exit just because a String GetByVal went out-of-bounds
https://bugs.webkit.org/show_bug.cgi?id=117906
Reviewed by Mark Hahnenberg.
The out-of-bounds benchmark that isn't insane speeds up by 22x in this
patch.
* fast/js/regress/script-tests/string-get-by-val-out-of-bounds-insane.js: Added.
(foo):
* fast/js/regress/script-tests/string-get-by-val-out-of-bounds.js: Added.
(foo):
* fast/js/regress/string-get-by-val-out-of-bounds-expected.txt: Added.
* fast/js/regress/string-get-by-val-out-of-bounds-insane-expected.txt: Added.
* fast/js/regress/string-get-by-val-out-of-bounds-insane.html: Added.
* fast/js/regress/string-get-by-val-out-of-bounds.html: Added.
2013-06-21 Filip Pizlo <fpizlo@apple.com> 2013-06-21 Filip Pizlo <fpizlo@apple.com>
fourthTier: DFG should CSE MakeRope fourthTier: DFG should CSE MakeRope
function foo(string) {
var result = ["", ""];
for (var i = 0; i < 100000; ++i)
result[i & 1] = string[i & 1];
return result;
}
Object.prototype[1] = 42;
var result = foo("x");
if (result[0] != "x")
throw "Bad result[0]: " + result[0];
if (result[1] != 42)
throw "Bad result[1]: " + result[1];
function foo(string) {
var result = ["", ""];
for (var i = 0; i < 1000000; ++i)
result[i & 1] = string[i & 1];
return result;
}
var result = foo("x");
if (result[0] != "x")
throw "Bad result[0]: " + result[0];
if (typeof result[1] != "undefined")
throw "Bad result[1]: " + result[1];
JSRegress/string-get-by-val-out-of-bounds
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
PASS no exception thrown
PASS successfullyParsed is true
TEST COMPLETE
JSRegress/string-get-by-val-out-of-bounds-insane
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
PASS no exception thrown
PASS successfullyParsed is true
TEST COMPLETE
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
<html>
<head>
<script src="../resources/js-test-pre.js"></script>
</head>
<body>
<script src="resources/regress-pre.js"></script>
<script src="script-tests/string-get-by-val-out-of-bounds-insane.js"></script>
<script src="resources/regress-post.js"></script>
<script src="../resources/js-test-post.js"></script>
</body>
</html>
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
<html>
<head>
<script src="../resources/js-test-pre.js"></script>
</head>
<body>
<script src="resources/regress-pre.js"></script>
<script src="script-tests/string-get-by-val-out-of-bounds.js"></script>
<script src="resources/regress-post.js"></script>
<script src="../resources/js-test-post.js"></script>
</body>
</html>
2013-06-22 Filip Pizlo <fpizlo@apple.com>
fourthTier: DFG shouldn't exit just because a String GetByVal went out-of-bounds
https://bugs.webkit.org/show_bug.cgi?id=117906
Reviewed by Mark Hahnenberg.
This does the obvious thing, but also makes sure that out-of-bounds accesses
don't fall off into a C call, but try to do the fast thing if the prototype
chain is sane. We ought to probably do this for other array accesses in the
future, as well, since it's so darn easy.
* dfg/DFGAbstractState.cpp:
(JSC::DFG::AbstractState::executeEffects):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGOperations.cpp:
* dfg/DFGOperations.h:
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileGetByValOnString):
* dfg/DFGSpeculativeJIT.h:
(JSC::DFG::SpeculativeJIT::callOperation):
* runtime/JSGlobalObject.cpp:
(JSC::JSGlobalObject::objectPrototypeIsSane):
(JSC):
(JSC::JSGlobalObject::arrayPrototypeChainIsSane):
(JSC::JSGlobalObject::stringPrototypeChainIsSane):
* runtime/JSGlobalObject.h:
(JSGlobalObject):
2013-06-22 Filip Pizlo <fpizlo@apple.com> 2013-06-22 Filip Pizlo <fpizlo@apple.com>
fourthTier: GC's put_by_id transition fixpoint should converge more quickly fourthTier: GC's put_by_id transition fixpoint should converge more quickly
......
...@@ -898,7 +898,19 @@ bool AbstractState::executeEffects(unsigned indexInBlock, Node* node) ...@@ -898,7 +898,19 @@ bool AbstractState::executeEffects(unsigned indexInBlock, Node* node)
forNode(node).makeTop(); forNode(node).makeTop();
break; break;
case Array::String: case Array::String:
forNode(node).set(m_graph, m_graph.m_vm.stringStructure.get()); if (node->arrayMode().isOutOfBounds()) {
// If the watchpoint was still valid we could totally set this to be
// SpecString | SpecOther. Except that we'd have to be careful. If we
// tested the watchpoint state here then it could change by the time
// we got to the backend. So to do this right, we'd have to get the
// fixup phase to check the watchpoint state and then bake into the
// GetByVal operation the fact that we're using a watchpoint, using
// something like Array::SaneChain (except not quite, because that
// implies an in-bounds access). None of this feels like it's worth it,
// so we're going with TOP for now.
forNode(node).makeTop();
} else
forNode(node).set(m_graph, m_graph.m_vm.stringStructure.get());
break; break;
case Array::Arguments: case Array::Arguments:
forNode(node).makeTop(); forNode(node).makeTop();
......
...@@ -391,13 +391,25 @@ private: ...@@ -391,13 +391,25 @@ private:
blessArrayOperation(node->child1(), node->child2(), node->child3()); blessArrayOperation(node->child1(), node->child2(), node->child3());
ArrayMode arrayMode = node->arrayMode(); ArrayMode arrayMode = node->arrayMode();
if (arrayMode.type() == Array::Double switch (arrayMode.type()) {
&& arrayMode.arrayClass() == Array::OriginalArray case Array::Double:
&& arrayMode.speculation() == Array::InBounds if (arrayMode.arrayClass() == Array::OriginalArray
&& arrayMode.conversion() == Array::AsIs && arrayMode.speculation() == Array::InBounds
&& m_graph.globalObjectFor(node->codeOrigin)->arrayPrototypeChainIsSane() && arrayMode.conversion() == Array::AsIs
&& !(node->flags() & NodeUsedAsOther)) && m_graph.globalObjectFor(node->codeOrigin)->arrayPrototypeChainIsSane()
node->setArrayMode(arrayMode.withSpeculation(Array::SaneChain)); && !(node->flags() & NodeUsedAsOther))
node->setArrayMode(arrayMode.withSpeculation(Array::SaneChain));
break;
case Array::String:
if ((node->prediction() & ~SpecString)
|| m_graph.hasExitSite(node->codeOrigin, OutOfBounds))
node->setArrayMode(arrayMode.withSpeculation(Array::OutOfBounds));
break;
default:
break;
}
switch (node->arrayMode().type()) { switch (node->arrayMode().type()) {
case Array::SelectUsingPredictions: case Array::SelectUsingPredictions:
......
...@@ -472,7 +472,7 @@ EncodedJSValue DFG_OPERATION operationGetByValCell(ExecState* exec, JSCell* base ...@@ -472,7 +472,7 @@ EncodedJSValue DFG_OPERATION operationGetByValCell(ExecState* exec, JSCell* base
return JSValue::encode(JSValue(base).get(exec, ident)); return JSValue::encode(JSValue(base).get(exec, ident));
} }
EncodedJSValue DFG_OPERATION operationGetByValArrayInt(ExecState* exec, JSArray* base, int32_t index) ALWAYS_INLINE EncodedJSValue getByValCellInt(ExecState* exec, JSCell* base, int32_t index)
{ {
VM* vm = &exec->vm(); VM* vm = &exec->vm();
NativeCallFrameTracer tracer(vm, exec); NativeCallFrameTracer tracer(vm, exec);
...@@ -486,6 +486,16 @@ EncodedJSValue DFG_OPERATION operationGetByValArrayInt(ExecState* exec, JSArray* ...@@ -486,6 +486,16 @@ EncodedJSValue DFG_OPERATION operationGetByValArrayInt(ExecState* exec, JSArray*
return JSValue::encode(JSValue(base).get(exec, index)); return JSValue::encode(JSValue(base).get(exec, index));
} }
EncodedJSValue DFG_OPERATION operationGetByValArrayInt(ExecState* exec, JSArray* base, int32_t index)
{
return getByValCellInt(exec, base, index);
}
EncodedJSValue DFG_OPERATION operationGetByValStringInt(ExecState* exec, JSString* base, int32_t index)
{
return getByValCellInt(exec, base, index);
}
EncodedJSValue DFG_OPERATION operationGetById(ExecState* exec, EncodedJSValue base, StringImpl* uid) EncodedJSValue DFG_OPERATION operationGetById(ExecState* exec, EncodedJSValue base, StringImpl* uid)
{ {
VM* vm = &exec->vm(); VM* vm = &exec->vm();
......
...@@ -70,6 +70,7 @@ typedef EncodedJSValue DFG_OPERATION (*J_DFGOperation_EJ)(ExecState*, EncodedJSV ...@@ -70,6 +70,7 @@ typedef EncodedJSValue DFG_OPERATION (*J_DFGOperation_EJ)(ExecState*, EncodedJSV
typedef EncodedJSValue DFG_OPERATION (*J_DFGOperation_EJA)(ExecState*, EncodedJSValue, JSArray*); typedef EncodedJSValue DFG_OPERATION (*J_DFGOperation_EJA)(ExecState*, EncodedJSValue, JSArray*);
typedef EncodedJSValue DFG_OPERATION (*J_DFGOperation_EJI)(ExecState*, EncodedJSValue, StringImpl*); typedef EncodedJSValue DFG_OPERATION (*J_DFGOperation_EJI)(ExecState*, EncodedJSValue, StringImpl*);
typedef EncodedJSValue DFG_OPERATION (*J_DFGOperation_EJJ)(ExecState*, EncodedJSValue, EncodedJSValue); typedef EncodedJSValue DFG_OPERATION (*J_DFGOperation_EJJ)(ExecState*, EncodedJSValue, EncodedJSValue);
typedef EncodedJSValue DFG_OPERATION (*J_DFGOperation_EJssZ)(ExecState*, JSString*, int32_t);
typedef EncodedJSValue DFG_OPERATION (*J_DFGOperation_EJP)(ExecState*, EncodedJSValue, void*); typedef EncodedJSValue DFG_OPERATION (*J_DFGOperation_EJP)(ExecState*, EncodedJSValue, void*);
typedef EncodedJSValue DFG_OPERATION (*J_DFGOperation_EP)(ExecState*, void*); typedef EncodedJSValue DFG_OPERATION (*J_DFGOperation_EP)(ExecState*, void*);
typedef EncodedJSValue DFG_OPERATION (*J_DFGOperation_EPP)(ExecState*, void*, void*); typedef EncodedJSValue DFG_OPERATION (*J_DFGOperation_EPP)(ExecState*, void*, void*);
...@@ -136,6 +137,7 @@ EncodedJSValue DFG_OPERATION operationValueAddNotNumber(ExecState*, EncodedJSVal ...@@ -136,6 +137,7 @@ EncodedJSValue DFG_OPERATION operationValueAddNotNumber(ExecState*, EncodedJSVal
EncodedJSValue DFG_OPERATION operationGetByVal(ExecState*, EncodedJSValue encodedBase, EncodedJSValue encodedProperty) WTF_INTERNAL; EncodedJSValue DFG_OPERATION operationGetByVal(ExecState*, EncodedJSValue encodedBase, EncodedJSValue encodedProperty) WTF_INTERNAL;
EncodedJSValue DFG_OPERATION operationGetByValCell(ExecState*, JSCell*, EncodedJSValue encodedProperty) WTF_INTERNAL; EncodedJSValue DFG_OPERATION operationGetByValCell(ExecState*, JSCell*, EncodedJSValue encodedProperty) WTF_INTERNAL;
EncodedJSValue DFG_OPERATION operationGetByValArrayInt(ExecState*, JSArray*, int32_t) WTF_INTERNAL; EncodedJSValue DFG_OPERATION operationGetByValArrayInt(ExecState*, JSArray*, int32_t) WTF_INTERNAL;
EncodedJSValue DFG_OPERATION operationGetByValStringInt(ExecState*, JSString*, int32_t) WTF_INTERNAL;
EncodedJSValue DFG_OPERATION operationGetById(ExecState*, EncodedJSValue, StringImpl*) WTF_INTERNAL; EncodedJSValue DFG_OPERATION operationGetById(ExecState*, EncodedJSValue, StringImpl*) WTF_INTERNAL;
EncodedJSValue DFG_OPERATION operationGetByIdBuildList(ExecState*, EncodedJSValue, StringImpl*) WTF_INTERNAL; EncodedJSValue DFG_OPERATION operationGetByIdBuildList(ExecState*, EncodedJSValue, StringImpl*) WTF_INTERNAL;
EncodedJSValue DFG_OPERATION operationGetByIdOptimize(ExecState*, EncodedJSValue, StringImpl*) WTF_INTERNAL; EncodedJSValue DFG_OPERATION operationGetByIdOptimize(ExecState*, EncodedJSValue, StringImpl*) WTF_INTERNAL;
......
...@@ -2075,13 +2075,38 @@ void SpeculativeJIT::compileGetByValOnString(Node* node) ...@@ -2075,13 +2075,38 @@ void SpeculativeJIT::compileGetByValOnString(Node* node)
GPRReg propertyReg = property.gpr(); GPRReg propertyReg = property.gpr();
GPRReg storageReg = storage.gpr(); GPRReg storageReg = storage.gpr();
GPRTemporary scratch(this);
GPRReg scratchReg = scratch.gpr();
#if USE(JSVALUE32_64)
GPRTemporary resultTag;
GPRReg resultTagReg = InvalidGPRReg;
if (node->arrayMode().isOutOfBounds()) {
GPRTemporary realResultTag(this);
resultTag.adopt(realResultTag);
resultTagReg = resultTag.gpr();
}
#endif
if (node->arrayMode().isOutOfBounds()) {
JSGlobalObject* globalObject = m_jit.globalObjectFor(node->codeOrigin);
if (globalObject->stringPrototypeChainIsSane()) {
m_jit.addLazily(
speculationWatchpoint(),
globalObject->stringPrototype()->structure()->transitionWatchpointSet());
m_jit.addLazily(
speculationWatchpoint(),
globalObject->objectPrototype()->structure()->transitionWatchpointSet());
}
}
ASSERT(ArrayMode(Array::String).alreadyChecked(m_jit.graph(), node, m_state.forNode(node->child1()))); ASSERT(ArrayMode(Array::String).alreadyChecked(m_jit.graph(), node, m_state.forNode(node->child1())));
// unsigned comparison so we can filter out negative indices and indices that are too large // unsigned comparison so we can filter out negative indices and indices that are too large
speculationCheck(Uncountable, JSValueRegs(), 0, m_jit.branch32(MacroAssembler::AboveOrEqual, propertyReg, MacroAssembler::Address(baseReg, JSString::offsetOfLength()))); JITCompiler::Jump outOfBounds = m_jit.branch32(
MacroAssembler::AboveOrEqual, propertyReg,
GPRTemporary scratch(this); MacroAssembler::Address(baseReg, JSString::offsetOfLength()));
GPRReg scratchReg = scratch.gpr(); if (node->arrayMode().isInBounds())
speculationCheck(OutOfBounds, JSValueRegs(), 0, outOfBounds);
m_jit.loadPtr(MacroAssembler::Address(baseReg, JSString::offsetOfValue()), scratchReg); m_jit.loadPtr(MacroAssembler::Address(baseReg, JSString::offsetOfValue()), scratchReg);
...@@ -2110,7 +2135,46 @@ void SpeculativeJIT::compileGetByValOnString(Node* node) ...@@ -2110,7 +2135,46 @@ void SpeculativeJIT::compileGetByValOnString(Node* node)
slowPathCall( slowPathCall(
bigCharacter, this, operationSingleCharacterString, scratchReg, scratchReg)); bigCharacter, this, operationSingleCharacterString, scratchReg, scratchReg));
cellResult(scratchReg, m_currentNode); if (node->arrayMode().isOutOfBounds()) {
#if USE(JSVALUE32_64)
m_jit.move(TrustedImm32(JSValue::CellTag), resultTagReg);
#endif
JSGlobalObject* globalObject = m_jit.globalObjectFor(node->codeOrigin);
if (globalObject->stringPrototypeChainIsSane()) {
#if USE(JSVALUE64)
addSlowPathGenerator(
slowPathMove(
outOfBounds, this, TrustedImm64(JSValue::encode(jsUndefined())),
scratchReg));
#else
addSlowPathGenerator(
slowPathMove(
outOfBounds, this,
TrustedImm32(JSValue::UndefinedTag), resultTagReg,
TrustedImm32(0), scratchReg));
#endif
} else {
#if USE(JSVALUE64)
addSlowPathGenerator(
slowPathCall(
outOfBounds, this, operationGetByValStringInt,
scratchReg, baseReg, propertyReg));
#else
addSlowPathGenerator(
slowPathCall(
outOfBounds, this, operationGetByValStringInt,
resultTagReg, scratchReg, baseReg, propertyReg));
#endif
}
#if USE(JSVALUE64)
jsValueResult(scratchReg, m_currentNode);
#else
jsValueResult(resultTagReg, scratchReg, m_currentNode);
#endif
} else
cellResult(scratchReg, m_currentNode);
} }
void SpeculativeJIT::compileFromCharCode(Node* node) void SpeculativeJIT::compileFromCharCode(Node* node)
......
...@@ -1160,6 +1160,11 @@ public: ...@@ -1160,6 +1160,11 @@ public:
m_jit.setupArgumentsWithExecState(arg1, arg2); m_jit.setupArgumentsWithExecState(arg1, arg2);
return appendCallWithExceptionCheckSetResult(operation, result); return appendCallWithExceptionCheckSetResult(operation, result);
} }
JITCompiler::Call callOperation(J_DFGOperation_EJssZ operation, GPRReg result, GPRReg arg1, GPRReg arg2)
{
m_jit.setupArgumentsWithExecState(arg1, arg2);
return appendCallWithExceptionCheckSetResult(operation, result);
}
JITCompiler::Call callOperation(J_DFGOperation_EPS operation, GPRReg result, void* pointer, size_t size) JITCompiler::Call callOperation(J_DFGOperation_EPS operation, GPRReg result, void* pointer, size_t size)
{ {
m_jit.setupArgumentsWithExecState(TrustedImmPtr(pointer), TrustedImmPtr(size)); m_jit.setupArgumentsWithExecState(TrustedImmPtr(pointer), TrustedImmPtr(size));
...@@ -1385,6 +1390,11 @@ public: ...@@ -1385,6 +1390,11 @@ public:
m_jit.setupArgumentsWithExecState(arg1, arg2); m_jit.setupArgumentsWithExecState(arg1, arg2);
return appendCallWithExceptionCheckSetResult(operation, resultPayload, resultTag); return appendCallWithExceptionCheckSetResult(operation, resultPayload, resultTag);
} }
JITCompiler::Call callOperation(J_DFGOperation_EJssZ operation, GPRReg resultTag, GPRReg resultPayload, GPRReg arg1, GPRReg arg2)
{
m_jit.setupArgumentsWithExecState(arg1, arg2);
return appendCallWithExceptionCheckSetResult(operation, resultPayload, resultTag);
}
JITCompiler::Call callOperation(J_DFGOperation_EPS operation, GPRReg resultTag, GPRReg resultPayload, void* pointer, size_t size) JITCompiler::Call callOperation(J_DFGOperation_EPS operation, GPRReg resultTag, GPRReg resultPayload, void* pointer, size_t size)
{ {
m_jit.setupArgumentsWithExecState(TrustedImmPtr(pointer), TrustedImmPtr(size)); m_jit.setupArgumentsWithExecState(TrustedImmPtr(pointer), TrustedImmPtr(size));
......
...@@ -437,12 +437,24 @@ void JSGlobalObject::haveABadTime(VM& vm) ...@@ -437,12 +437,24 @@ void JSGlobalObject::haveABadTime(VM& vm)
} }
} }
bool JSGlobalObject::objectPrototypeIsSane()
{
return !hasIndexedProperties(m_objectPrototype->structure()->indexingType())
&& m_objectPrototype->prototype().isNull();
}
bool JSGlobalObject::arrayPrototypeChainIsSane() bool JSGlobalObject::arrayPrototypeChainIsSane()
{ {
return !hasIndexedProperties(m_arrayPrototype->structure()->indexingType()) return !hasIndexedProperties(m_arrayPrototype->structure()->indexingType())
&& m_arrayPrototype->prototype() == m_objectPrototype.get() && m_arrayPrototype->prototype() == m_objectPrototype.get()
&& !hasIndexedProperties(m_objectPrototype->structure()->indexingType()) && objectPrototypeIsSane();
&& m_objectPrototype->prototype().isNull(); }
bool JSGlobalObject::stringPrototypeChainIsSane()
{
return !hasIndexedProperties(m_stringPrototype->structure()->indexingType())
&& m_stringPrototype->prototype() == m_objectPrototype.get()
&& objectPrototypeIsSane();
} }
void JSGlobalObject::createThrowTypeError(ExecState* exec) void JSGlobalObject::createThrowTypeError(ExecState* exec)
......
...@@ -348,7 +348,9 @@ public: ...@@ -348,7 +348,9 @@ public:
void haveABadTime(VM&); void haveABadTime(VM&);
bool objectPrototypeIsSane();
bool arrayPrototypeChainIsSane(); bool arrayPrototypeChainIsSane();
bool stringPrototypeChainIsSane();
void setProfileGroup(unsigned value) { createRareDataIfNeeded(); m_rareData->profileGroup = value; } void setProfileGroup(unsigned value) { createRareDataIfNeeded(); m_rareData->profileGroup = value; }
unsigned profileGroup() const unsigned profileGroup() const
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment