From ec4e62d60202cd704bb987f09aa467d807c2aa26 Mon Sep 17 00:00:00 2001 From: mwilliams Date: Fri, 29 Mar 2013 11:36:42 -0700 Subject: [PATCH] Fix closure dispatch when there's more than one Func* We need to make sure that we execute the code corresponding to the actual Func*. This re-uses the prolog array to hold the entry points for the cloned Func*. --- hphp/runtime/vm/func.cpp | 6 +++ hphp/runtime/vm/func.h | 5 +- .../vm/translator/translator-x64-helpers.cpp | 29 ++++++++--- hphp/runtime/vm/translator/translator-x64.cpp | 50 +++++++++++++++---- hphp/runtime/vm/translator/translator-x64.h | 2 +- hphp/runtime/vm/translator/translator.h | 2 +- hphp/test/vm/closure_clone_2.php | 33 ++++++++++++ hphp/test/vm/closure_clone_2.php.exp | 2 + 8 files changed, 109 insertions(+), 20 deletions(-) create mode 100644 hphp/test/vm/closure_clone_2.php create mode 100644 hphp/test/vm/closure_clone_2.php.exp diff --git a/hphp/runtime/vm/func.cpp b/hphp/runtime/vm/func.cpp index 4a0fe25be..fc70bf167 100644 --- a/hphp/runtime/vm/func.cpp +++ b/hphp/runtime/vm/func.cpp @@ -365,6 +365,12 @@ const FPIEnt* Func::findPrecedingFPI(Offset o) const { return fe; } +bool Func::isClonedClosure() const { + if (!isClosureBody()) return false; + if (!cls()) return true; + return cls()->lookupMethod(name()) != this; +} + bool Func::isNameBindingImmutable(const Unit* fromUnit) const { if (RuntimeOption::EvalJitEnableRenameFunction || m_attrs & AttrDynamicInvoke) { diff --git a/hphp/runtime/vm/func.h b/hphp/runtime/vm/func.h index 6ec2cad7b..9ec27f225 100644 --- a/hphp/runtime/vm/func.h +++ b/hphp/runtime/vm/func.h @@ -145,9 +145,9 @@ struct Func { #endif } - FuncId getFuncId() const { + FuncId getFuncId() const { assert(m_funcId != InvalidId); - return m_funcId; + return m_funcId; } void setFuncId(FuncId id); void setNewFuncId(); @@ -321,6 +321,7 @@ struct Func { bool top() const { return shared()->m_top; } const StringData* docComment() const { return shared()->m_docComment; } bool isClosureBody() const { return shared()->m_isClosureBody; } + bool isClonedClosure() const; bool isGenerator() const { return shared()->m_isGenerator; } bool isGeneratorFromClosure() const { return shared()->m_isGeneratorFromClosure; diff --git a/hphp/runtime/vm/translator/translator-x64-helpers.cpp b/hphp/runtime/vm/translator/translator-x64-helpers.cpp index eb1b26ffd..5ce68b7cd 100644 --- a/hphp/runtime/vm/translator/translator-x64-helpers.cpp +++ b/hphp/runtime/vm/translator/translator-x64-helpers.cpp @@ -86,9 +86,18 @@ void TranslatorX64::reqLitHelper(const ReqLitStaticArgs* args) { */ static TCA callAndResume(ActRec *ar) { - VMRegAnchor _(ar, true); - g_vmContext->doFCall(ar, g_vmContext->m_pc); - return Translator::Get()->getResumeHelperRet(); + if (!ar->m_func->isClonedClosure()) { + /* + * If the func is a cloned closure, then the original + * closure has already run the prolog, and the prologs + * array is just being used as entry points for the + * dv funclets. Dont run the prolog again. + */ + VMRegAnchor _(ar, true); + g_vmContext->doFCall(ar, g_vmContext->m_pc); + return Translator::Get()->getResumeHelperRet(); + } + return Translator::Get()->getResumeHelper(); } static_assert(rStashedAR == reg::r15, @@ -99,14 +108,22 @@ asm( ".globl __fcallHelperThunk\n" "__fcallHelperThunk:\n" #if defined(__x86_64__) + // fcallHelper is used for prologs, and (in the case of + // closures) for dispatch to the function body. In the first + // case, there's a call, in the second, there's a jmp. + // We can differentiate by comparing r15 and rVmFp + "mov %r15, %rdi\n" + "cmp %r15, %rbp\n" + "jne 1f\n" + "call fcallHelper\n" + "jmp *%rax\n" // fcallHelper may call doFCall. doFCall changes the return ip // pointed to by r15 so that it points to TranslatorX64::m_retHelper, // which does a REQ_POST_INTERP_RET service request. So we need to // to pop the return address into r15 + m_savedRip before calling // fcallHelper, and then push it back from r15 + m_savedRip after // fcallHelper returns in case it has changed it. - "pop 0x8(%r15)\n" - "mov %r15, %rdi\n" + "1: pop 0x8(%r15)\n" "call fcallHelper\n" "push 0x8(%r15)\n" "jmp *%rax\n" @@ -122,7 +139,7 @@ extern "C" TCA fcallHelper(ActRec* ar) { try { TCA tca = - Translator::Get()->funcPrologue((Func*)ar->m_func, ar->numArgs()); + Translator::Get()->funcPrologue((Func*)ar->m_func, ar->numArgs(), ar); if (tca) { return tca; } diff --git a/hphp/runtime/vm/translator/translator-x64.cpp b/hphp/runtime/vm/translator/translator-x64.cpp index 99d10c8c3..afd938575 100644 --- a/hphp/runtime/vm/translator/translator-x64.cpp +++ b/hphp/runtime/vm/translator/translator-x64.cpp @@ -2039,8 +2039,16 @@ TranslatorX64::emitPopRetIntoActRec(Asm& a) { a. pop (rStashedAR[AROFF(m_savedRip)]); } +static void interp_set_regs(ActRec* ar, Cell* sp, Offset pcOff) { + assert(tl_regState == REGSTATE_DIRTY); + tl_regState = REGSTATE_CLEAN; + vmfp() = (Cell*)ar; + vmsp() = sp; + vmpc() = curUnit()->at(pcOff); +} + TCA -TranslatorX64::funcPrologue(Func* func, int nPassed) { +TranslatorX64::funcPrologue(Func* func, int nPassed, ActRec* ar) { func->validate(); TRACE(1, "funcPrologue %s(%d)\n", func->fullName()->data(), nPassed); int numParams = func->numParams(); @@ -2051,6 +2059,27 @@ TranslatorX64::funcPrologue(Func* func, int nPassed) { // Do a quick test before grabbing the write lease TCA prologue; if (checkCachedPrologue(func, paramIndex, prologue)) return prologue; + if (func->isClonedClosure()) { + assert(ar); + const Func::ParamInfoVec& paramInfo = func->params(); + Offset entry = func->base(); + for (int i = nPassed; i < numParams; ++i) { + const Func::ParamInfo& pi = paramInfo[i]; + if (pi.hasDefaultValue()) { + entry = pi.funcletOff(); + break; + } + } + interp_set_regs(ar, (Cell*)ar - func->numSlotsInFrame(), entry); + SrcKey funcBody(func, entry); + TCA tca = getTranslation(funcBody, false); + tl_regState = REGSTATE_DIRTY; + if (tca) { + // racy, but ok... + func->setPrologue(paramIndex, tca); + } + return tca; + } // If the translator is getting replaced out from under us, refuse to // provide a prologue; we don't know whether this request is running on the @@ -2060,6 +2089,7 @@ TranslatorX64::funcPrologue(Func* func, int nPassed) { // Double check the prologue array now that we have the write lease // in case another thread snuck in and set the prologue already. if (checkCachedPrologue(func, paramIndex, prologue)) return prologue; + SpaceRecorder sr("_FuncPrologue", a); // If we're close to a cache line boundary, just burn some space to // try to keep the func and its body on fewer total lines. @@ -2394,7 +2424,15 @@ TranslatorX64::emitPrologue(Func* func, int nPassed) { // code emitCheckSurpriseFlagsEnter(false, fixup); - emitBindJmp(funcBody); + if (func->isClosureBody() && func->cls()) { + int entry = nPassed <= numParams ? nPassed : numParams + 1; + // Relying on rStashedAR == rVmFp here + a. loadq (rStashedAR[AROFF(m_func)], rax); + a. loadq (rax[Func::prologueTableOff() + sizeof(TCA)*entry], rax); + a. jmp (rax); + } else { + emitBindJmp(funcBody); + } return funcBody; } @@ -4001,14 +4039,6 @@ TranslatorX64::binaryArithLocal(const NormalizedInstruction &i, } } -static void interp_set_regs(ActRec* ar, Cell* sp, Offset pcOff) { - assert(tl_regState == REGSTATE_DIRTY); - tl_regState = REGSTATE_CLEAN; - vmfp() = (Cell*)ar; - vmsp() = sp; - vmpc() = curUnit()->at(pcOff); -} - #define O(opcode, imm, pusph, pop, flags) \ /** * The interpOne methods saves m_pc, m_fp, and m_sp ExecutionContext, diff --git a/hphp/runtime/vm/translator/translator-x64.h b/hphp/runtime/vm/translator/translator-x64.h index d3d08cacb..d467c1563 100644 --- a/hphp/runtime/vm/translator/translator-x64.h +++ b/hphp/runtime/vm/translator/translator-x64.h @@ -939,7 +939,7 @@ private: static int shuffleArgsForMagicCall(ActRec* ar); static void setArgInActRec(ActRec* ar, int argNum, uint64_t datum, DataType t); - TCA funcPrologue(Func* func, int nArgs); + TCA funcPrologue(Func* func, int nArgs, ActRec* ar = nullptr); bool checkCachedPrologue(const Func* func, int param, TCA& plgOut) const; SrcKey emitPrologue(Func* func, int nArgs); int32_t emitNativeImpl(const Func*, bool emitSavedRIPReturn); diff --git a/hphp/runtime/vm/translator/translator.h b/hphp/runtime/vm/translator/translator.h index 1a0a1073a..c16ca49e5 100644 --- a/hphp/runtime/vm/translator/translator.h +++ b/hphp/runtime/vm/translator/translator.h @@ -891,7 +891,7 @@ public: virtual void requestInit() = 0; virtual void requestExit() = 0; virtual void analyzeInstr(Tracelet& t, NormalizedInstruction& i) = 0; - virtual TCA funcPrologue(Func* f, int nArgs) = 0; + virtual TCA funcPrologue(Func* f, int nArgs, ActRec* ar = nullptr) = 0; virtual TCA getCallToExit() = 0; virtual TCA getRetFromInterpretedFrame() = 0; virtual void defineCns(StringData* name) = 0; diff --git a/hphp/test/vm/closure_clone_2.php b/hphp/test/vm/closure_clone_2.php new file mode 100644 index 000000000..0ede5c46d --- /dev/null +++ b/hphp/test/vm/closure_clone_2.php @@ -0,0 +1,33 @@ +foo; + } + }; + } +} + +class X { + private $foo = 1; + use T; +} + +class Y { + private $foo = 2; + use T; +} + +function test() { + $x = new X; + $c = $x->f(); + var_dump($c(true)); + + $y = new Y; + $c = $y->f(); + var_dump($c("foo")); +} + +test(); diff --git a/hphp/test/vm/closure_clone_2.php.exp b/hphp/test/vm/closure_clone_2.php.exp new file mode 100644 index 000000000..420ffd8a5 --- /dev/null +++ b/hphp/test/vm/closure_clone_2.php.exp @@ -0,0 +1,2 @@ +int(1) +int(2)