diff --git a/hphp/compiler/analysis/emitter.cpp b/hphp/compiler/analysis/emitter.cpp index dd0b0809d..fabaea425 100644 --- a/hphp/compiler/analysis/emitter.cpp +++ b/hphp/compiler/analysis/emitter.cpp @@ -275,6 +275,7 @@ static int32_t countStackValues(const std::vector& immVec) { getUnitEmitter().recordSourceLocation(m_tempLoc ? m_tempLoc.get() : \ m_node->getLocation().get(), curPos); \ if (flags & TF) getEmitterVisitor().restoreJumpTargetEvalStack(); \ + if (isFCallStar(opcode)) getEmitterVisitor().recordCall(); \ getEmitterVisitor().setPrevOpcode(opcode); \ } @@ -1398,6 +1399,10 @@ void EmitterVisitor::restoreJumpTargetEvalStack() { m_evalStack = it->second; } +void EmitterVisitor::recordCall() { + m_curFunc->setContainsCalls(); +} + bool EmitterVisitor::isJumpTarget(Offset target) { // Returns true iff one of the following conditions is true: // 1) We have seen an instruction that jumps to the specified offset @@ -6666,8 +6671,13 @@ void EmitterVisitor::copyOverFPIRegions(FuncEmitter* fe) { } void EmitterVisitor::saveMaxStackCells(FuncEmitter* fe) { - fe->setMaxStackCells(m_actualStackHighWater + kNumActRecCells - + m_fdescHighWater); + // Max stack cells is used for stack overflow checks. We need to + // count all the locals, and all cells due to ActRecs. We don't + // need to count this function's own ActRec because whoever called + // it already included it in its "max stack cells". + fe->setMaxStackCells(m_actualStackHighWater + + fe->numLocals() + + m_fdescHighWater); m_actualStackHighWater = 0; m_fdescHighWater = 0; } diff --git a/hphp/compiler/analysis/emitter.h b/hphp/compiler/analysis/emitter.h index 555937655..8af293f4d 100644 --- a/hphp/compiler/analysis/emitter.h +++ b/hphp/compiler/analysis/emitter.h @@ -359,6 +359,7 @@ public: recordJumpTarget(target, m_evalStack); } void restoreJumpTargetEvalStack(); + void recordCall(); bool isJumpTarget(Offset target); void setPrevOpcode(Opcode op) { m_prevOpcode = op; } Opcode getPrevOpcode() const { return m_prevOpcode; } diff --git a/hphp/runtime/vm/bytecode.cpp b/hphp/runtime/vm/bytecode.cpp index 57b5f4584..e56fbaf4c 100644 --- a/hphp/runtime/vm/bytecode.cpp +++ b/hphp/runtime/vm/bytecode.cpp @@ -1652,7 +1652,7 @@ static inline void checkStack(Stack& stk, const Func* f) { // Check whether func's maximum stack usage would overflow the stack. // Both native and VM stack overflows are independently possible. if (!stack_in_bounds(info) || - stk.wouldOverflow(f->maxStackCells() + kMaxJITInlineStackCells)) { + stk.wouldOverflow(f->maxStackCells() + kStackCheckPadding)) { TRACE(1, "Maximum VM stack depth exceeded.\n"); raise_error("Stack overflow"); } diff --git a/hphp/runtime/vm/bytecode.h b/hphp/runtime/vm/bytecode.h index 35ecc19a8..2dc2094e6 100644 --- a/hphp/runtime/vm/bytecode.h +++ b/hphp/runtime/vm/bytecode.h @@ -419,11 +419,15 @@ constexpr size_t kNumIterCells = sizeof(Iter) / sizeof(Cell); constexpr size_t kNumActRecCells = sizeof(ActRec) / sizeof(Cell); /* - * We pad all stack overflow checks by a small amount to allow for - * inlining functions without having to either do another stack check - * or chase down prologues to smash. + * We pad all stack overflow checks by a small amount to allow for two + * things: + * + * - inlining functions without having to either do another stack + * check (or chase down prologues to smash checks to be bigger). + * + * - omitting stack overflow checks on leaf functions */ -constexpr int kMaxJITInlineStackCells = 4 + kNumActRecCells; +constexpr int kStackCheckPadding = 20; struct Fault { enum Type : int16_t { diff --git a/hphp/runtime/vm/class.cpp b/hphp/runtime/vm/class.cpp index f380e9f35..f14639682 100644 --- a/hphp/runtime/vm/class.cpp +++ b/hphp/runtime/vm/class.cpp @@ -13,6 +13,7 @@ | license@php.net so we can mail you a copy immediately. | +----------------------------------------------------------------------+ */ +#include "runtime/vm/class.h" #include "runtime/vm/class.h" #include "runtime/base/base_includes.h" @@ -1305,7 +1306,7 @@ void Class::setSpecial() { // Use 86ctor(), since no program-supplied constructor exists m_ctor = findSpecialMethod(this, sd86ctor); assert(m_ctor && "class had no user-defined constructor or 86ctor"); - assert(m_ctor->attrs() == (AttrPublic|AttrNoInjection)); + assert(m_ctor->attrs() == (AttrPublic|AttrNoInjection|AttrPhpLeafFn)); } void Class::applyTraitPrecRule(const PreClass::TraitPrecRule& rule) { diff --git a/hphp/runtime/vm/core_types.h b/hphp/runtime/vm/core_types.h index e3d958f10..ed62d93d2 100644 --- a/hphp/runtime/vm/core_types.h +++ b/hphp/runtime/vm/core_types.h @@ -72,25 +72,32 @@ constexpr Offset kInvalidOffset = std::numeric_limits::max(); typedef uint32_t Slot; const Slot kInvalidSlot = Slot(-1); -// Special types that are not relevant to the runtime as a whole. -// The order for public/protected/private matters in numerous places. -// -// Attr unions are directly stored as integers in .hhbc repositories, so -// incompatible changes here require a schema version bump. -// -// AttrTrait on a method means that the method is NOT a constructor, -// even though it may look like one -// -// AttrNoOverride (WholeProgram only) on a class means its not extended -// and on a method means that no extending class defines the method. -// -// AttrVariadicByRef indicates a function is a builtin that takes -// variadic arguments, where the arguments are either by ref or -// optionally by ref. (It is equivalent to having ClassInfo's -// (RefVariableArguments | MixedVariableArguments).) -// -// AttrMayUseVV indicates that a function may need to use a VarEnv or -// varargs (aka extraArgs) at run time. +/* + * Special types that are not relevant to the runtime as a whole. + * The order for public/protected/private matters in numerous places. + * + * Attr unions are directly stored as integers in .hhbc repositories, so + * incompatible changes here require a schema version bump. + * + * AttrTrait on a method means that the method is NOT a constructor, + * even though it may look like one + * + * AttrNoOverride (WholeProgram only) on a class means its not extended + * and on a method means that no extending class defines the method. + * + * AttrVariadicByRef indicates a function is a builtin that takes + * variadic arguments, where the arguments are either by ref or + * optionally by ref. (It is equivalent to having ClassInfo's + * (RefVariableArguments | MixedVariableArguments).) + * + * AttrMayUseVV indicates that a function may need to use a VarEnv or + * varargs (aka extraArgs) at run time. + * + * AttrPhpLeafFn indicates a function does not make any explicit calls + * to other php functions. It may still call other user-level + * functions via re-entry (e.g. for destructors or autoload), and it + * may make calls to builtins using FCallBuiltin. + */ enum Attr { AttrNone = 0, // class property method // AttrReference = (1 << 0), // X // @@ -101,6 +108,7 @@ enum Attr { AttrAbstract = (1 << 5), // X X // AttrFinal = (1 << 6), // X X // AttrInterface = (1 << 7), // X // + AttrPhpLeafFn = (1 << 7), // X // AttrTrait = (1 << 8), // X X // AttrNoInjection = (1 << 9), // X // AttrUnique = (1 << 10), // X X // diff --git a/hphp/runtime/vm/func.cpp b/hphp/runtime/vm/func.cpp index d07ba2890..97ce7af56 100644 --- a/hphp/runtime/vm/func.cpp +++ b/hphp/runtime/vm/func.cpp @@ -13,6 +13,7 @@ | license@php.net so we can mail you a copy immediately. | +----------------------------------------------------------------------+ */ +#include "runtime/vm/func.h" #include #include @@ -23,7 +24,6 @@ #include "util/debug.h" #include "runtime/base/strings.h" #include "runtime/vm/core_types.h" -#include "runtime/vm/func.h" #include "runtime/vm/runtime.h" #include "runtime/vm/repo.h" #include "runtime/vm/translator/targetcache.h" @@ -459,6 +459,7 @@ void Func::prettyPrint(std::ostream& out) const { if (m_attrs & AttrPrivate) { out << "private "; } if (m_attrs & AttrAbstract) { out << "abstract "; } if (m_attrs & AttrFinal) { out << "final "; } + if (m_attrs & AttrPhpLeafFn) { out << "(leaf) "; } out << preClass()->name()->data() << "::" << m_name->data(); } else { out << "Function " << m_name->data(); @@ -674,23 +675,50 @@ const Func* Func::getGeneratorBody(const StringData* name) const { // FuncEmitter. FuncEmitter::FuncEmitter(UnitEmitter& ue, int sn, Id id, const StringData* n) - : m_ue(ue), m_pce(nullptr), m_sn(sn), m_id(id), m_name(n), m_numLocals(0), - m_numUnnamedLocals(0), m_activeUnnamedLocals(0), m_numIterators(0), - m_nextFreeIterator(0), m_retTypeConstraint(nullptr), - m_returnType(KindOfInvalid), m_top(false), m_isClosureBody(false), - m_isGenerator(false), m_isGeneratorFromClosure(false), - m_hasGeneratorAsBody(false), m_info(nullptr), m_builtinFuncPtr(nullptr) { -} + : m_ue(ue) + , m_pce(nullptr) + , m_sn(sn) + , m_id(id) + , m_name(n) + , m_numLocals(0) + , m_numUnnamedLocals(0) + , m_activeUnnamedLocals(0) + , m_numIterators(0) + , m_nextFreeIterator(0) + , m_retTypeConstraint(nullptr) + , m_returnType(KindOfInvalid) + , m_top(false) + , m_isClosureBody(false) + , m_isGenerator(false) + , m_isGeneratorFromClosure(false) + , m_hasGeneratorAsBody(false) + , m_containsCalls(false) + , m_info(nullptr) + , m_builtinFuncPtr(nullptr) +{} FuncEmitter::FuncEmitter(UnitEmitter& ue, int sn, const StringData* n, PreClassEmitter* pce) - : m_ue(ue), m_pce(pce), m_sn(sn), m_name(n), m_numLocals(0), - m_numUnnamedLocals(0), m_activeUnnamedLocals(0), m_numIterators(0), - m_nextFreeIterator(0), m_retTypeConstraint(nullptr), - m_returnType(KindOfInvalid), m_top(false), m_isClosureBody(false), - m_isGenerator(false), m_isGeneratorFromClosure(false), - m_hasGeneratorAsBody(false), m_info(nullptr), m_builtinFuncPtr(nullptr) { -} + : m_ue(ue) + , m_pce(pce) + , m_sn(sn) + , m_name(n) + , m_numLocals(0) + , m_numUnnamedLocals(0) + , m_activeUnnamedLocals(0) + , m_numIterators(0) + , m_nextFreeIterator(0) + , m_retTypeConstraint(nullptr) + , m_returnType(KindOfInvalid) + , m_top(false) + , m_isClosureBody(false) + , m_isGenerator(false) + , m_isGeneratorFromClosure(false) + , m_hasGeneratorAsBody(false) + , m_containsCalls(false) + , m_info(nullptr) + , m_builtinFuncPtr(nullptr) +{} FuncEmitter::~FuncEmitter() { } @@ -880,6 +908,8 @@ Func* FuncEmitter::create(Unit& unit, PreClass* preClass /* = NULL */) const { attrs = Attr(attrs & ~AttrPersistent); } + if (!m_containsCalls) attrs = Attr(attrs | AttrPhpLeafFn); + Func* f = (m_pce == nullptr) ? m_ue.newFunc(this, unit, m_id, m_line1, m_line2, m_base, m_past, m_name, attrs, m_top, m_docComment, @@ -997,6 +1027,7 @@ void FuncEmitter::serdeMetaData(SerDe& sd) { (m_isGenerator) (m_isGeneratorFromClosure) (m_hasGeneratorAsBody) + (m_containsCalls) (m_params) (m_localNames) diff --git a/hphp/runtime/vm/func.h b/hphp/runtime/vm/func.h index 6addef705..0556c6879 100644 --- a/hphp/runtime/vm/func.h +++ b/hphp/runtime/vm/func.h @@ -638,6 +638,8 @@ public: void setHasGeneratorAsBody(bool b) { m_hasGeneratorAsBody = b; } bool hasGeneratorAsBody() const { return m_hasGeneratorAsBody; } + void setContainsCalls() { m_containsCalls = true; } + void addUserAttribute(const StringData* name, TypedValue tv); void commit(RepoTxn& txn) const; @@ -683,6 +685,7 @@ private: bool m_isGenerator; bool m_isGeneratorFromClosure; bool m_hasGeneratorAsBody; + bool m_containsCalls; Func::UserAttributeMap m_userAttributes; @@ -695,7 +698,7 @@ class FuncRepoProxy : public RepoProxy { friend class Func; friend class FuncEmitter; public: - FuncRepoProxy(Repo& repo); + explicit FuncRepoProxy(Repo& repo); ~FuncRepoProxy(); void createSchema(int repoId, RepoTxn& txn); diff --git a/hphp/runtime/vm/translator/hopt/irtranslator.cpp b/hphp/runtime/vm/translator/hopt/irtranslator.cpp index 6cf3114a6..eed691900 100644 --- a/hphp/runtime/vm/translator/hopt/irtranslator.cpp +++ b/hphp/runtime/vm/translator/hopt/irtranslator.cpp @@ -1132,10 +1132,8 @@ static bool shouldIRInline(const Func* curFunc, if (func->numIterators() != 0) { return refuse("iterators"); } - if (func->maxStackCells() >= kMaxJITInlineStackCells) { - FTRACE(1, "{} >= {}\n", - func->maxStackCells(), - kMaxJITInlineStackCells); + if (func->maxStackCells() >= kStackCheckPadding) { + FTRACE(1, "{} >= {}\n", func->maxStackCells(), kStackCheckPadding); return refuse("too many stack cells"); } diff --git a/hphp/runtime/vm/translator/translator-x64.cpp b/hphp/runtime/vm/translator/translator-x64.cpp index 8515b9e95..6f620eca3 100644 --- a/hphp/runtime/vm/translator/translator-x64.cpp +++ b/hphp/runtime/vm/translator/translator-x64.cpp @@ -1555,7 +1555,7 @@ void TranslatorX64::unprotectCode() { void TranslatorX64::emitStackCheck(int funcDepth, Offset pc) { - funcDepth += kMaxJITInlineStackCells * sizeof(Cell); + funcDepth += kStackCheckPadding * sizeof(Cell); uint64_t stackMask = cellsToBytes(RuntimeOption::EvalVMStackElms) - 1; a. mov_reg64_reg64(rVmSp, rScratch); // copy to destroy @@ -2127,8 +2127,18 @@ TranslatorX64::funcPrologue(Func* func, int nPassed, ActRec* ar) { emitPopRetIntoActRec(a); // entry point for magic methods comes later emitRB(a, RBTypeFuncEntry, func->fullName()->data()); - // Guard: we have stack enough stack space to complete this function. - emitStackCheck(cellsToBytes(func->maxStackCells()), func->base()); + + /* + * Guard: we have stack enough stack space to complete this + * function. We omit overflow checks if it is a leaf function + * that can't use more than kStackCheckPadding cells. + */ + auto const needStackCheck = + !(func->attrs() & AttrPhpLeafFn) || + func->maxStackCells() >= kStackCheckPadding; + if (needStackCheck) { + emitStackCheck(cellsToBytes(func->maxStackCells()), func->base()); + } } SrcKey skFuncBody = emitPrologue(func, nPassed); diff --git a/hphp/test/quick/exceptions2.php.expectf b/hphp/test/quick/exceptions2.php.expectf index e3373172f..4f16cd2c5 100644 --- a/hphp/test/quick/exceptions2.php.expectf +++ b/hphp/test/quick/exceptions2.php.expectf @@ -1235,4 +1235,3 @@ C::__destruct C::__destruct C::__destruct C::__destruct -C::__destruct diff --git a/hphp/test/quick/exceptions3.php.expectf b/hphp/test/quick/exceptions3.php.expectf index a735ba0f9..1cfdc5d41 100644 --- a/hphp/test/quick/exceptions3.php.expectf +++ b/hphp/test/quick/exceptions3.php.expectf @@ -1460,5 +1460,3 @@ C::__destruct C::__destruct C::__destruct C::__destruct -C::__destruct -C::__destruct diff --git a/hphp/test/quick/exceptions5.php.expectf b/hphp/test/quick/exceptions5.php.expectf index a275b605e..c20b53189 100644 --- a/hphp/test/quick/exceptions5.php.expectf +++ b/hphp/test/quick/exceptions5.php.expectf @@ -1 +1 @@ -HipHop Fatal error: Stack overflow in %s on line 21 +HipHop Fatal error: Stack overflow in %s on line 18 diff --git a/hphp/test/quick/exceptions6.php.expectf b/hphp/test/quick/exceptions6.php.expectf index bff5ad9cf..a275b605e 100644 --- a/hphp/test/quick/exceptions6.php.expectf +++ b/hphp/test/quick/exceptions6.php.expectf @@ -1 +1 @@ -HipHop Fatal error: Stack overflow in %s on line 26 +HipHop Fatal error: Stack overflow in %s on line 21