diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog index 394376b00be6be2efcad967d88833f28d88a8592..e23bb2334285109454396d4563307dc3ec516980 100644 --- a/LayoutTests/ChangeLog +++ b/LayoutTests/ChangeLog @@ -1,3 +1,22 @@ +2013-06-21 Filip Pizlo + + 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 fourthTier: DFG should CSE MakeRope diff --git a/LayoutTests/fast/js/regress/script-tests/string-get-by-val-out-of-bounds-insane.js b/LayoutTests/fast/js/regress/script-tests/string-get-by-val-out-of-bounds-insane.js new file mode 100644 index 0000000000000000000000000000000000000000..728de69fdaf56f28f837eb87dd19507e14de98d1 --- /dev/null +++ b/LayoutTests/fast/js/regress/script-tests/string-get-by-val-out-of-bounds-insane.js @@ -0,0 +1,14 @@ +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]; diff --git a/LayoutTests/fast/js/regress/script-tests/string-get-by-val-out-of-bounds.js b/LayoutTests/fast/js/regress/script-tests/string-get-by-val-out-of-bounds.js new file mode 100644 index 0000000000000000000000000000000000000000..218d86f8dc855ded82975873993f88ee78aee536 --- /dev/null +++ b/LayoutTests/fast/js/regress/script-tests/string-get-by-val-out-of-bounds.js @@ -0,0 +1,12 @@ +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]; diff --git a/LayoutTests/fast/js/regress/string-get-by-val-out-of-bounds-expected.txt b/LayoutTests/fast/js/regress/string-get-by-val-out-of-bounds-expected.txt new file mode 100644 index 0000000000000000000000000000000000000000..d3594bf187bbe9bc37cbc87ed3a1fb72a7380c6e --- /dev/null +++ b/LayoutTests/fast/js/regress/string-get-by-val-out-of-bounds-expected.txt @@ -0,0 +1,10 @@ +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 + diff --git a/LayoutTests/fast/js/regress/string-get-by-val-out-of-bounds-insane-expected.txt b/LayoutTests/fast/js/regress/string-get-by-val-out-of-bounds-insane-expected.txt new file mode 100644 index 0000000000000000000000000000000000000000..6cac9e3c12e5f98ea7992fdd53c5316c21157796 --- /dev/null +++ b/LayoutTests/fast/js/regress/string-get-by-val-out-of-bounds-insane-expected.txt @@ -0,0 +1,10 @@ +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 + diff --git a/LayoutTests/fast/js/regress/string-get-by-val-out-of-bounds-insane.html b/LayoutTests/fast/js/regress/string-get-by-val-out-of-bounds-insane.html new file mode 100644 index 0000000000000000000000000000000000000000..a23e36674fb681f8495c312a572a87ee29828a9d --- /dev/null +++ b/LayoutTests/fast/js/regress/string-get-by-val-out-of-bounds-insane.html @@ -0,0 +1,12 @@ + + + + + + + + + + + + diff --git a/LayoutTests/fast/js/regress/string-get-by-val-out-of-bounds.html b/LayoutTests/fast/js/regress/string-get-by-val-out-of-bounds.html new file mode 100644 index 0000000000000000000000000000000000000000..9a30a2ae40a0fd6076db18f81da0f2ef03b8a25b --- /dev/null +++ b/LayoutTests/fast/js/regress/string-get-by-val-out-of-bounds.html @@ -0,0 +1,12 @@ + + + + + + + + + + + + diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog index 734bd866a9e58c9b5f6179d436fc9f64050a6c17..f77ac7d1e828986d8971a6f982cb79a52470b5d3 100644 --- a/Source/JavaScriptCore/ChangeLog +++ b/Source/JavaScriptCore/ChangeLog @@ -1,3 +1,33 @@ +2013-06-22 Filip Pizlo + + 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 fourthTier: GC's put_by_id transition fixpoint should converge more quickly diff --git a/Source/JavaScriptCore/dfg/DFGAbstractState.cpp b/Source/JavaScriptCore/dfg/DFGAbstractState.cpp index 637ac81de97226024c5b7b9f19cb643cd0854709..e9ce574cb86b16d9ee7a2793b94bf86f25572ecd 100644 --- a/Source/JavaScriptCore/dfg/DFGAbstractState.cpp +++ b/Source/JavaScriptCore/dfg/DFGAbstractState.cpp @@ -898,7 +898,19 @@ bool AbstractState::executeEffects(unsigned indexInBlock, Node* node) forNode(node).makeTop(); break; 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; case Array::Arguments: forNode(node).makeTop(); diff --git a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp index e8b0b5bf919680f0bc2f645fec36479011e7421f..a6dbf53857e41a6180bb9ce976577f6c2ac48db8 100644 --- a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp +++ b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp @@ -391,13 +391,25 @@ private: blessArrayOperation(node->child1(), node->child2(), node->child3()); ArrayMode arrayMode = node->arrayMode(); - if (arrayMode.type() == Array::Double - && arrayMode.arrayClass() == Array::OriginalArray - && arrayMode.speculation() == Array::InBounds - && arrayMode.conversion() == Array::AsIs - && m_graph.globalObjectFor(node->codeOrigin)->arrayPrototypeChainIsSane() - && !(node->flags() & NodeUsedAsOther)) - node->setArrayMode(arrayMode.withSpeculation(Array::SaneChain)); + switch (arrayMode.type()) { + case Array::Double: + if (arrayMode.arrayClass() == Array::OriginalArray + && arrayMode.speculation() == Array::InBounds + && arrayMode.conversion() == Array::AsIs + && m_graph.globalObjectFor(node->codeOrigin)->arrayPrototypeChainIsSane() + && !(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()) { case Array::SelectUsingPredictions: diff --git a/Source/JavaScriptCore/dfg/DFGOperations.cpp b/Source/JavaScriptCore/dfg/DFGOperations.cpp index 405ecc99b879a7122d76279145afc9483dadaccf..cd3920fcfc9943e28babcb9b11731ccf3ed23fb3 100644 --- a/Source/JavaScriptCore/dfg/DFGOperations.cpp +++ b/Source/JavaScriptCore/dfg/DFGOperations.cpp @@ -472,7 +472,7 @@ EncodedJSValue DFG_OPERATION operationGetByValCell(ExecState* exec, JSCell* base 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(); NativeCallFrameTracer tracer(vm, exec); @@ -486,6 +486,16 @@ EncodedJSValue DFG_OPERATION operationGetByValArrayInt(ExecState* exec, JSArray* 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) { VM* vm = &exec->vm(); diff --git a/Source/JavaScriptCore/dfg/DFGOperations.h b/Source/JavaScriptCore/dfg/DFGOperations.h index 5d54655a243aa3f513dfc39dab78b7f496c328e5..92a1ecb7988ada1baee77f94dd138848dc6e1ffe 100644 --- a/Source/JavaScriptCore/dfg/DFGOperations.h +++ b/Source/JavaScriptCore/dfg/DFGOperations.h @@ -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_EJI)(ExecState*, EncodedJSValue, StringImpl*); 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_EP)(ExecState*, void*); typedef EncodedJSValue DFG_OPERATION (*J_DFGOperation_EPP)(ExecState*, void*, void*); @@ -136,6 +137,7 @@ EncodedJSValue DFG_OPERATION operationValueAddNotNumber(ExecState*, EncodedJSVal EncodedJSValue DFG_OPERATION operationGetByVal(ExecState*, EncodedJSValue encodedBase, 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 operationGetByValStringInt(ExecState*, JSString*, int32_t) WTF_INTERNAL; EncodedJSValue DFG_OPERATION operationGetById(ExecState*, EncodedJSValue, StringImpl*) WTF_INTERNAL; EncodedJSValue DFG_OPERATION operationGetByIdBuildList(ExecState*, EncodedJSValue, StringImpl*) WTF_INTERNAL; EncodedJSValue DFG_OPERATION operationGetByIdOptimize(ExecState*, EncodedJSValue, StringImpl*) WTF_INTERNAL; diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp index 84d3a464a07778652982704f9660920f3b522200..f8a4544db9586b85bcd4ef00dae3bee57d3cad4f 100644 --- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp +++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp @@ -2075,13 +2075,38 @@ void SpeculativeJIT::compileGetByValOnString(Node* node) GPRReg propertyReg = property.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()))); // 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()))); - - GPRTemporary scratch(this); - GPRReg scratchReg = scratch.gpr(); + JITCompiler::Jump outOfBounds = m_jit.branch32( + MacroAssembler::AboveOrEqual, propertyReg, + MacroAssembler::Address(baseReg, JSString::offsetOfLength())); + if (node->arrayMode().isInBounds()) + speculationCheck(OutOfBounds, JSValueRegs(), 0, outOfBounds); m_jit.loadPtr(MacroAssembler::Address(baseReg, JSString::offsetOfValue()), scratchReg); @@ -2110,7 +2135,46 @@ void SpeculativeJIT::compileGetByValOnString(Node* node) slowPathCall( 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) diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h index 13e5831367b37e8023be0818d05bc1aba4a8fe9f..4d9f67ff47a31f344a63d35b314963bccd25de93 100644 --- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h +++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h @@ -1160,6 +1160,11 @@ public: m_jit.setupArgumentsWithExecState(arg1, arg2); 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) { m_jit.setupArgumentsWithExecState(TrustedImmPtr(pointer), TrustedImmPtr(size)); @@ -1385,6 +1390,11 @@ public: m_jit.setupArgumentsWithExecState(arg1, arg2); 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) { m_jit.setupArgumentsWithExecState(TrustedImmPtr(pointer), TrustedImmPtr(size)); diff --git a/Source/JavaScriptCore/runtime/JSGlobalObject.cpp b/Source/JavaScriptCore/runtime/JSGlobalObject.cpp index c75c71d847fe8c1fc480c8245a364ad806ebd7e4..eb590ce2d750e691ffa1aacaac95ec128616ab3d 100644 --- a/Source/JavaScriptCore/runtime/JSGlobalObject.cpp +++ b/Source/JavaScriptCore/runtime/JSGlobalObject.cpp @@ -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() { return !hasIndexedProperties(m_arrayPrototype->structure()->indexingType()) && m_arrayPrototype->prototype() == m_objectPrototype.get() - && !hasIndexedProperties(m_objectPrototype->structure()->indexingType()) - && m_objectPrototype->prototype().isNull(); + && objectPrototypeIsSane(); +} + +bool JSGlobalObject::stringPrototypeChainIsSane() +{ + return !hasIndexedProperties(m_stringPrototype->structure()->indexingType()) + && m_stringPrototype->prototype() == m_objectPrototype.get() + && objectPrototypeIsSane(); } void JSGlobalObject::createThrowTypeError(ExecState* exec) diff --git a/Source/JavaScriptCore/runtime/JSGlobalObject.h b/Source/JavaScriptCore/runtime/JSGlobalObject.h index 3514baaac7edd3ae9387168f8a4b64dd3d8808fa..0f5a5671ef08f9aa2d9797bdf9fac04cbf4a6bd6 100644 --- a/Source/JavaScriptCore/runtime/JSGlobalObject.h +++ b/Source/JavaScriptCore/runtime/JSGlobalObject.h @@ -348,7 +348,9 @@ public: void haveABadTime(VM&); + bool objectPrototypeIsSane(); bool arrayPrototypeChainIsSane(); + bool stringPrototypeChainIsSane(); void setProfileGroup(unsigned value) { createRareDataIfNeeded(); m_rareData->profileGroup = value; } unsigned profileGroup() const