DFG string concatenation shouldn't be playing fast and loose with effects and OSR exit
https://bugs.webkit.org/show_bug.cgi?id=148338
Reviewed by Michael Saboff and Saam Barati.
Prior to this change, DFG string concatenation appeared to have various different ways of
creating an OSR exit right after a side effect. That's bad, because the exit will cause
us to reexecute the side effect. The code appears to have some hacks for avoiding this,
but some cases are basically unavoidable, like the OOM case of string concatenation: in
trunk that could cause two executions of the toString operation.
This changes the string concatenation code to either be speculative or effectful but
never both. It's already the case that when this code needs to be effectful, it also
needs to be slow (it does int->string conversions, calls JS functions, etc). So, this is
a small price to pay for sanity.
The biggest part of this change is the introduction of StrCat, which is like MakeRope but
does toString conversions on its own instead of relying on separate nodes. StrCat can
take either 2 or 3 children. It's the effectful but not speculative version of MakeRope.
* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGBackwardsPropagationPhase.cpp:
(JSC::DFG::BackwardsPropagationPhase::propagate):
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::parseBlock):
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGDoesGC.cpp:
(JSC::DFG::doesGC):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
(JSC::DFG::FixupPhase::convertStringAddUse):
(JSC::DFG::FixupPhase::fixupToStringOrCallStringConstructor):
(JSC::DFG::FixupPhase::attemptToMakeFastStringAdd):
(JSC::DFG::FixupPhase::canOptimizeStringObjectAccess):
* dfg/DFGNodeType.h:
* dfg/DFGOperations.cpp:
* dfg/DFGOperations.h:
* dfg/DFGPredictionPropagationPhase.cpp:
(JSC::DFG::PredictionPropagationPhase::propagate):
* dfg/DFGSafeToExecute.h:
(JSC::DFG::safeToExecute):
* dfg/DFGSpeculativeJIT.h:
(JSC::DFG::SpeculativeJIT::callOperation):
(JSC::DFG::JSValueOperand::JSValueOperand):
(JSC::DFG::JSValueOperand::~JSValueOperand):
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGValidate.cpp:
(JSC::DFG::Validate::validate):
* ftl/FTLCapabilities.cpp:
(JSC::FTL::canCompile):
* ftl/FTLIntrinsicRepository.h:
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::DFG::LowerDFGToLLVM::compileNode):
(JSC::FTL::DFG::LowerDFGToLLVM::compileValueAdd):
(JSC::FTL::DFG::LowerDFGToLLVM::compileStrCat):
(JSC::FTL::DFG::LowerDFGToLLVM::compileArithAddOrSub):
* jit/JITOperations.h:
* tests/stress/exception-effect-strcat.js: Added. This test previously failed.
* tests/stress/exception-in-strcat-string-overflow.js: Added. An earlier version of this patch made this fail.
* tests/stress/exception-in-strcat.js: Added.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@188825 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
index ff675d9..917e841 100644
--- a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
@@ -151,29 +151,16 @@
break;
}
- // FIXME: Optimize for the case where one of the operands is the
- // empty string. Also consider optimizing for the case where we don't
- // believe either side is the emtpy string. Both of these things should
- // be easy.
-
- if (node->child1()->shouldSpeculateString()
- && attemptToMakeFastStringAdd<StringUse>(node, node->child1(), node->child2()))
+ if (attemptToMakeFastStringAdd(node))
break;
- if (node->child2()->shouldSpeculateString()
- && attemptToMakeFastStringAdd<StringUse>(node, node->child2(), node->child1()))
- break;
- if (node->child1()->shouldSpeculateStringObject()
- && attemptToMakeFastStringAdd<StringObjectUse>(node, node->child1(), node->child2()))
- break;
- if (node->child2()->shouldSpeculateStringObject()
- && attemptToMakeFastStringAdd<StringObjectUse>(node, node->child2(), node->child1()))
- break;
- if (node->child1()->shouldSpeculateStringOrStringObject()
- && attemptToMakeFastStringAdd<StringOrStringObjectUse>(node, node->child1(), node->child2()))
- break;
- if (node->child2()->shouldSpeculateStringOrStringObject()
- && attemptToMakeFastStringAdd<StringOrStringObjectUse>(node, node->child2(), node->child1()))
- break;
+
+ // We could attempt to turn this into a StrCat here. But for now, that wouldn't
+ // significantly reduce the number of branches required.
+ break;
+ }
+
+ case StrCat: {
+ attemptToMakeFastStringAdd(node);
break;
}
@@ -1432,8 +1419,6 @@
return;
}
- // FIXME: We ought to be able to have a ToPrimitiveToString node.
-
observeUseKindOnNode<useKind>(edge.node());
createToString<useKind>(node, edge);
}
@@ -1524,47 +1509,44 @@
return;
}
}
-
- template<UseKind leftUseKind>
- bool attemptToMakeFastStringAdd(Node* node, Edge& left, Edge& right)
+
+ bool attemptToMakeFastStringAdd(Node* node)
{
- ASSERT(leftUseKind == StringUse || leftUseKind == StringObjectUse || leftUseKind == StringOrStringObjectUse);
-
- if (isStringObjectUse<leftUseKind>() && !canOptimizeStringObjectAccess(node->origin.semantic))
+ bool goodToGo = true;
+ m_graph.doToChildren(
+ node,
+ [&] (Edge& edge) {
+ if (edge->shouldSpeculateString())
+ return;
+ if (canOptimizeStringObjectAccess(node->origin.semantic)) {
+ if (edge->shouldSpeculateStringObject())
+ return;
+ if (edge->shouldSpeculateStringOrStringObject())
+ return;
+ }
+ goodToGo = false;
+ });
+ if (!goodToGo)
return false;
-
- convertStringAddUse<leftUseKind>(node, left);
-
- if (right->shouldSpeculateString())
- convertStringAddUse<StringUse>(node, right);
- else if (right->shouldSpeculateStringObject() && canOptimizeStringObjectAccess(node->origin.semantic))
- convertStringAddUse<StringObjectUse>(node, right);
- else if (right->shouldSpeculateStringOrStringObject() && canOptimizeStringObjectAccess(node->origin.semantic))
- convertStringAddUse<StringOrStringObjectUse>(node, right);
- else {
- // At this point we know that the other operand is something weird. The semantically correct
- // way of dealing with this is:
- //
- // MakeRope(@left, ToString(ToPrimitive(@right)))
- //
- // So that's what we emit. NB, we need to do all relevant type checks on @left before we do
- // anything to @right, since ToPrimitive may be effectful.
-
- Node* toPrimitive = m_insertionSet.insertNode(
- m_indexInBlock, resultOfToPrimitive(right->prediction()), ToPrimitive,
- node->origin, Edge(right.node()));
- Node* toString = m_insertionSet.insertNode(
- m_indexInBlock, SpecString, ToString, node->origin, Edge(toPrimitive));
-
- fixupToPrimitive(toPrimitive);
- // Don't fix up ToString. ToString and ToPrimitive are originated from the same bytecode and
- // ToPrimitive may have an observable side effect. ToString should not be converted into Check
- // with speculative type check because OSR exit reproduce an observable side effect done in
- // ToPrimitive.
-
- right.setNode(toString);
- }
+ m_graph.doToChildren(
+ node,
+ [&] (Edge& edge) {
+ if (edge->shouldSpeculateString()) {
+ convertStringAddUse<StringUse>(node, edge);
+ return;
+ }
+ ASSERT(canOptimizeStringObjectAccess(node->origin.semantic));
+ if (edge->shouldSpeculateStringObject()) {
+ convertStringAddUse<StringObjectUse>(node, edge);
+ return;
+ }
+ if (edge->shouldSpeculateStringOrStringObject()) {
+ convertStringAddUse<StringOrStringObjectUse>(node, edge);
+ return;
+ }
+ RELEASE_ASSERT_NOT_REACHED();
+ });
convertToMakeRope(node);
return true;
@@ -1600,11 +1582,15 @@
return false;
Structure* stringObjectStructure = m_graph.globalObjectFor(codeOrigin)->stringObjectStructure();
+ m_graph.registerStructure(stringObjectStructure);
ASSERT(stringObjectStructure->storedPrototype().isObject());
ASSERT(stringObjectStructure->storedPrototype().asCell()->classInfo() == StringPrototype::info());
-
- JSObject* stringPrototypeObject = asObject(stringObjectStructure->storedPrototype());
- Structure* stringPrototypeStructure = stringPrototypeObject->structure();
+
+ FrozenValue* stringPrototypeObjectValue =
+ m_graph.freeze(stringObjectStructure->storedPrototype());
+ StringPrototype* stringPrototypeObject =
+ stringPrototypeObjectValue->dynamicCast<StringPrototype*>();
+ Structure* stringPrototypeStructure = stringPrototypeObjectValue->structure();
if (m_graph.registerStructure(stringPrototypeStructure) != StructureRegisteredAndWatched)
return false;
