From effe6d71f3f4c17f4bf80924e70182d2dc1f266c Mon Sep 17 00:00:00 2001 From: Jordan DeLong Date: Mon, 29 Apr 2013 18:38:31 -0700 Subject: [PATCH] Omit stack overflow checks on leaf functions I did an experiment turning off all stack checks, and it looks like it does cost something. This seemed like an easy way to turn it off some of the time, but maybe we should resurrect the SEGV handler. --- hphp/compiler/analysis/emitter.cpp | 14 ++++- hphp/compiler/analysis/emitter.h | 1 + hphp/runtime/vm/bytecode.cpp | 2 +- hphp/runtime/vm/bytecode.h | 12 ++-- hphp/runtime/vm/class.cpp | 3 +- hphp/runtime/vm/core_types.h | 46 ++++++++------ hphp/runtime/vm/func.cpp | 61 ++++++++++++++----- hphp/runtime/vm/func.h | 5 +- .../vm/translator/hopt/irtranslator.cpp | 6 +- hphp/runtime/vm/translator/translator-x64.cpp | 16 ++++- hphp/test/quick/exceptions2.php.expectf | 1 - hphp/test/quick/exceptions3.php.expectf | 2 - hphp/test/quick/exceptions5.php.expectf | 2 +- hphp/test/quick/exceptions6.php.expectf | 2 +- 14 files changed, 118 insertions(+), 55 deletions(-) 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