From ab9e26f8af72c48ef5e2ba42f71682aef16016b2 Mon Sep 17 00:00:00 2001 From: mikemag Date: Tue, 2 Apr 2013 11:18:57 -0700 Subject: [PATCH] Avoid stack overflow on super-large expressions of binary operators. A small change to optimize very simple binary operators in the parser. This avoids building very large parse trees for super-large expressions and folds binary operators involving two scalars directly in the parser. I've limited this to simple scalars since it's easy to prove they don't have anything too interesting going on in the other analysis phases leading up to BinaryOpExpression's normal folding. This works for all binary operators. --- .../expression/binary_op_expression.cpp | 28 +++++++++++++++---- .../compiler/expression/scalar_expression.cpp | 13 +++++++++ hphp/compiler/expression/scalar_expression.h | 1 + hphp/compiler/parser/parser.cpp | 4 +++ hphp/test/vm/parser_massive_add_exp.php | 13 +++++++++ hphp/test/vm/parser_massive_add_exp.php.exp | 2 ++ hphp/test/vm/parser_massive_concat_exp.php | 24 ++++++++++++++++ .../test/vm/parser_massive_concat_exp.php.exp | 3 ++ 8 files changed, 82 insertions(+), 6 deletions(-) create mode 100644 hphp/test/vm/parser_massive_add_exp.php create mode 100644 hphp/test/vm/parser_massive_add_exp.php.exp create mode 100644 hphp/test/vm/parser_massive_concat_exp.php create mode 100644 hphp/test/vm/parser_massive_concat_exp.php.exp diff --git a/hphp/compiler/expression/binary_op_expression.cpp b/hphp/compiler/expression/binary_op_expression.cpp index 1845b21c6..a843b2d66 100644 --- a/hphp/compiler/expression/binary_op_expression.cpp +++ b/hphp/compiler/expression/binary_op_expression.cpp @@ -422,13 +422,21 @@ static ExpressionPtr makeIsNull(AnalysisResultConstPtr ar, return result; } +// foldConst() is callable from the parse phase as well as the analysis phase. +// We take advantage of this during the parse phase to reduce very simple +// expressions down to a single scalar and keep the parse tree smaller, +// especially in cases of long chains of binary operators. However, we limit +// the effectivness of this during parse to ensure that we eliminate only +// very simple scalars that don't require analysis in later phases. For now, +// that's just simply scalar values. ExpressionPtr BinaryOpExpression::foldConst(AnalysisResultConstPtr ar) { ExpressionPtr optExp; Variant v1; Variant v2; if (!m_exp2->getScalarValue(v2)) { - if (m_exp1->isScalar() && m_exp1->getScalarValue(v1)) { + if ((ar->getPhase() != AnalysisResult::ParseAllFiles) && + m_exp1->isScalar() && m_exp1->getScalarValue(v1)) { switch (m_op) { case T_IS_IDENTICAL: case T_IS_NOT_IDENTICAL: @@ -486,15 +494,23 @@ ExpressionPtr BinaryOpExpression::foldConst(AnalysisResultConstPtr ar) { if (m_exp1->isScalar()) { if (!m_exp1->getScalarValue(v1)) return ExpressionPtr(); try { + ScalarExpressionPtr scalar1 = + dynamic_pointer_cast(m_exp1); + ScalarExpressionPtr scalar2 = + dynamic_pointer_cast(m_exp2); + // Some data, like the values of __CLASS__ and friends, are not available + // while we're still in the initial parse phase. + if (ar->getPhase() == AnalysisResult::ParseAllFiles) { + if ((scalar1 && scalar1->needsTranslation()) || + (scalar2 && scalar2->needsTranslation())) { + return ExpressionPtr(); + } + } if (!Option::WholeProgram || !Option::ParseTimeOpts) { // In the VM, don't optimize __CLASS__ if within a trait, since // __CLASS__ is not resolved yet. ClassScopeRawPtr clsScope = getOriginalClass(); if (clsScope && clsScope->isTrait()) { - ScalarExpressionPtr scalar1 = - dynamic_pointer_cast(m_exp1); - ScalarExpressionPtr scalar2 = - dynamic_pointer_cast(m_exp2); if ((scalar1 && scalar1->getType() == T_CLASS_C) || (scalar2 && scalar2->getType() == T_CLASS_C)) { return ExpressionPtr(); @@ -565,7 +581,7 @@ ExpressionPtr BinaryOpExpression::foldConst(AnalysisResultConstPtr ar) { return makeScalarExpression(ar, result); } catch (...) { } - } else { + } else if (ar->getPhase() != AnalysisResult::ParseAllFiles) { switch (m_op) { case T_LOGICAL_AND: case T_BOOLEAN_AND: diff --git a/hphp/compiler/expression/scalar_expression.cpp b/hphp/compiler/expression/scalar_expression.cpp index d345ca644..76f29ff20 100644 --- a/hphp/compiler/expression/scalar_expression.cpp +++ b/hphp/compiler/expression/scalar_expression.cpp @@ -108,6 +108,19 @@ void ScalarExpression::toLower(bool funcCall /* = false */) { /////////////////////////////////////////////////////////////////////////////// // static analysis functions +bool ScalarExpression::needsTranslation() const { + switch (m_type) { + case T_LINE: + case T_NS_C: + case T_CLASS_C: + case T_METHOD_C: + case T_FUNC_C: + return true; + default: + return false; + } +} + void ScalarExpression::analyzeProgram(AnalysisResultPtr ar) { if (ar->getPhase() == AnalysisResult::AnalyzeAll) { string id = Util::toLower(getIdentifier()); diff --git a/hphp/compiler/expression/scalar_expression.h b/hphp/compiler/expression/scalar_expression.h index 4cc1e63fd..d7eb90c58 100644 --- a/hphp/compiler/expression/scalar_expression.h +++ b/hphp/compiler/expression/scalar_expression.h @@ -46,6 +46,7 @@ public: virtual std::string getLiteralString() const; std::string getOriginalLiteralString() const; std::string getLiteralStringImpl(bool original) const; + bool needsTranslation() const; TypePtr inferenceImpl(AnalysisResultConstPtr ar, TypePtr type, bool coerce); virtual TypePtr inferAndCheck(AnalysisResultPtr ar, TypePtr type, diff --git a/hphp/compiler/parser/parser.cpp b/hphp/compiler/parser/parser.cpp index d2f488e21..9576dbe0f 100644 --- a/hphp/compiler/parser/parser.cpp +++ b/hphp/compiler/parser/parser.cpp @@ -663,6 +663,10 @@ void Parser::onBinaryOpExp(Token &out, Token &operand1, Token &operand2, } out->exp = bop; + + // If the operands are simple enough we can fold this expression right + // here and keep the parse tree smaller. + if (ExpressionPtr optExp = bop->foldConst(m_ar)) out->exp = optExp; } void Parser::onQOp(Token &out, Token &exprCond, Token *expYes, Token &expNo) { diff --git a/hphp/test/vm/parser_massive_add_exp.php b/hphp/test/vm/parser_massive_add_exp.php new file mode 100644 index 000000000..0a16c7a41 --- /dev/null +++ b/hphp/test/vm/parser_massive_add_exp.php @@ -0,0 +1,13 @@ +