diff --git a/hphp/doc/ir.specification b/hphp/doc/ir.specification index 0e43313f0..1a2bd3a7d 100644 --- a/hphp/doc/ir.specification +++ b/hphp/doc/ir.specification @@ -1470,23 +1470,20 @@ D:Obj = CreateCl S0:ConstCls S1:ConstInt S2:FramePtr S3:StkPtr object. Saves the $this and Func* from ActRec S2 into the object, and returns the object. -D:Obj = CreateCont S0:TCA S1:FramePtr S2:ConstBool S3:ConstFunc S4:ConstFunc +D:Obj = CreateContFunc S0:ConstFunc S1:ConstFunc - Create a continuation object. Arguments: + Create a continuation object from function. Arguments: - S0 - function pointer to C++ helper - S1 - current frame pointer - S2 - whether the CreateCont hhbc op had getArgs set to true (which means - the function body may call func_get_args, etc). - S3 - the "original" function - S4 - the generator function + S0 - the "original" function + S1 - the generator function -D:Obj = InlineCreateCont S0:{Obj,Null} +D:Obj = CreateContMeth S0:ConstFunc S1:ConstFunc S2:Ctx - Create a zero-argument continuation object. S0 is the this pointer - for the continuation if it was a method, otherwise null. Used to - weaken the use of the frame in CreateCont when inlining an 'outer' - generator function. + Create a continuation object from method. Arguments: + + S0 - the "original" method + S1 - the generator function + S2 - the m_this/m_cls field of the current frame pointer FillContLocals S0:FramePtr S1:ConstFunc S2:ConstFunc S3:Obj diff --git a/hphp/runtime/base/execution_context.h b/hphp/runtime/base/execution_context.h index c563e971a..ba8b4613b 100644 --- a/hphp/runtime/base/execution_context.h +++ b/hphp/runtime/base/execution_context.h @@ -430,16 +430,11 @@ public: void requestExit(); static void getElem(TypedValue* base, TypedValue* key, TypedValue* dest); - template - static c_Continuation* createContinuationHelper( - const Func* origFunc, - const Func* genFunc, - ObjectData* thisPtr, - Class* frameStaticCls); - template - static c_Continuation* createContinuation(ActRec* fp, - const Func* origFunc, - const Func* genFunc); + static c_Continuation* createContFunc(const Func* origFunc, + const Func* genFunc); + static c_Continuation* createContMeth(const Func* origFunc, + const Func* genFunc, + void* objOrCls); static c_Continuation* fillContinuationVars( ActRec* fp, const Func* origFunc, const Func* genFunc, c_Continuation* cont); diff --git a/hphp/runtime/vm/bytecode.cpp b/hphp/runtime/vm/bytecode.cpp index 3f6cd4628..75b4e774b 100644 --- a/hphp/runtime/vm/bytecode.cpp +++ b/hphp/runtime/vm/bytecode.cpp @@ -6981,12 +6981,8 @@ inline void OPTBLD_INLINE VMExecutionContext::iopCreateCl(PC& pc) { m_stack.pushObject(cl2); } -template -c_Continuation* -VMExecutionContext::createContinuationHelper(const Func* origFunc, - const Func* genFunc, - ObjectData* thisPtr, - Class* frameStaticCls) { +static inline c_Continuation* createCont(const Func* origFunc, + const Func* genFunc) { auto const cont = c_Continuation::alloc( origFunc, genFunc->numLocals(), @@ -6999,22 +6995,6 @@ VMExecutionContext::createContinuationHelper(const Func* origFunc, // does. We set it up once, here, and then just change FP to point to it when // we enter the generator body. ActRec* ar = cont->actRec(); - - if (isMethod) { - if (origFunc->isClosureBody()) { - genFunc = genFunc->cloneAndSetClass(origFunc->cls()); - } - - if (thisPtr) { - ar->setThis(thisPtr); - thisPtr->incRefCount(); - } else { - ar->setClass(frameStaticCls); - } - } else { - ar->setThis(nullptr); - } - ar->m_func = genFunc; ar->initNumArgs(1); ar->setVarEnv(nullptr); @@ -7030,19 +7010,29 @@ VMExecutionContext::createContinuationHelper(const Func* origFunc, return cont; } -template c_Continuation* -VMExecutionContext::createContinuation(ActRec* fp, - const Func* origFunc, - const Func* genFunc) { - ObjectData* const thisPtr = fp->hasThis() ? fp->getThis() : nullptr; +VMExecutionContext::createContFunc(const Func* origFunc, + const Func* genFunc) { + auto cont = createCont(origFunc, genFunc); + cont->actRec()->setThis(nullptr); + return cont; +} - return createContinuationHelper( - origFunc, - genFunc, - thisPtr, - frameStaticClass(fp) - ); +c_Continuation* +VMExecutionContext::createContMeth(const Func* origFunc, + const Func* genFunc, + void* objOrCls) { + if (origFunc->isClosureBody()) { + genFunc = genFunc->cloneAndSetClass(origFunc->cls()); + } + + auto cont = createCont(origFunc, genFunc); + auto ar = cont->actRec(); + ar->setThisOrClass(objOrCls); + if (ar->hasThis()) { + ar->getThis()->incRefCount(); + } + return cont; } static inline void setContVar(const Func* genFunc, @@ -7107,16 +7097,6 @@ VMExecutionContext::fillContinuationVars(ActRec* fp, return cont; } -// Explicitly instantiate for hhbctranslator.o and codegen.o -template c_Continuation* VMExecutionContext::createContinuation( - ActRec*, const Func*, const Func*); -template c_Continuation* VMExecutionContext::createContinuation( - ActRec*, const Func*, const Func*); -template c_Continuation* VMExecutionContext::createContinuationHelper( - const Func*, const Func*, ObjectData*, Class*); -template c_Continuation* VMExecutionContext::createContinuationHelper( - const Func*, const Func*, ObjectData*, Class*); - inline void OPTBLD_INLINE VMExecutionContext::iopCreateCont(PC& pc) { NEXT(); DECODE_LITSTR(genName); @@ -7125,10 +7105,9 @@ inline void OPTBLD_INLINE VMExecutionContext::iopCreateCont(PC& pc) { const Func* genFunc = origFunc->getGeneratorBody(genName); assert(genFunc != nullptr); - bool isMethod = origFunc->isMethod(); - c_Continuation* cont = isMethod ? - createContinuation(m_fp, origFunc, genFunc) : - createContinuation(m_fp, origFunc, genFunc); + c_Continuation* cont = origFunc->isMethod() + ? createContMeth(origFunc, genFunc, m_fp->getThisOrClass()) + : createContFunc(origFunc, genFunc); fillContinuationVars(m_fp, origFunc, genFunc, cont); diff --git a/hphp/runtime/vm/bytecode.h b/hphp/runtime/vm/bytecode.h index 4c74d522a..fd927db42 100644 --- a/hphp/runtime/vm/bytecode.h +++ b/hphp/runtime/vm/bytecode.h @@ -315,6 +315,15 @@ struct ActRec { return uintptr_t(p) & 1 ? (Class*)(uintptr_t(p)&~1LL) : nullptr; } + void setThisOrClass(void* objOrCls) { + m_this = (ObjectData*)objOrCls; + assert(hasThis() || hasClass()); + } + + void* getThisOrClass() const { + return m_this; + } + /** * To conserve space, we use unions for pairs of mutually exclusive * fields (fields that are not used at the same time). We use unions diff --git a/hphp/runtime/vm/jit/codegen.cpp b/hphp/runtime/vm/jit/codegen.cpp index 3b472d369..17835e6dc 100644 --- a/hphp/runtime/vm/jit/codegen.cpp +++ b/hphp/runtime/vm/jit/codegen.cpp @@ -395,7 +395,8 @@ CALL_OPCODE(ConvIntToStr); CALL_OPCODE(ConvObjToStr); CALL_OPCODE(ConvCellToStr); -CALL_OPCODE(CreateCont) +CALL_OPCODE(CreateContFunc) +CALL_OPCODE(CreateContMeth) CALL_OPCODE(FillContLocals) CALL_OPCODE(NewArray) CALL_OPCODE(NewTuple) @@ -3485,31 +3486,6 @@ void CodeGenerator::cgAllocObjFast(IRInstruction* inst) { } } -void CodeGenerator::cgInlineCreateCont(IRInstruction* inst) { - auto const& data = *inst->extra(); - auto const helper = data.origFunc->isMethod() - ? &VMExecutionContext::createContinuationHelper - : &VMExecutionContext::createContinuationHelper; - - cgCallHelper( - m_as, - reinterpret_cast(helper), - inst->dst(), - kSyncPoint, - ArgGroup(m_regs) - .immPtr(data.origFunc) - .immPtr(data.genFunc) - .ssa(inst->src(0)) - // Deliberately ignoring frameStaticClass parameter, because - // it's unused if we have a $this pointer, and we don't inline - // functions with a null $this. - ); - if (data.origFunc->isMethod()) { - // We can't support a null $this. - assert(inst->src(0)->isA(Type::Obj)); - } -} - void CodeGenerator::cgCallArray(IRInstruction* inst) { Offset pc = inst->extra()->pc; Offset after = inst->extra()->after; diff --git a/hphp/runtime/vm/jit/dce.cpp b/hphp/runtime/vm/jit/dce.cpp index 6ba8a6c14..c293308bc 100644 --- a/hphp/runtime/vm/jit/dce.cpp +++ b/hphp/runtime/vm/jit/dce.cpp @@ -401,22 +401,6 @@ void optimizeActRecs(IRTrace* trace, DceState& state, IRFactory* factory, forEachInst(trace, [&](IRInstruction* inst) { switch (inst->op()) { - case CreateCont: - { - auto const frameInst = inst->src(1)->inst(); - if (frameInst->op() == DefInlineFP) { - auto const spInst = frameInst->src(0)->inst(); - if (spInst->op() == SpillFrame && - spInst->src(3)->type().subtypeOfAny(Type::Obj, Type::Null)) { - FTRACE(5, "CreateCont ({}): weak use of frame {}\n", - inst->id(), - frameInst->id()); - state[frameInst].incWeakUse(); - } - } - } - break; - // We don't need to generate stores to a frame if it can be // eliminated. case StLoc: @@ -460,28 +444,6 @@ void optimizeActRecs(IRTrace* trace, DceState& state, IRFactory* factory, */ forEachInst(trace, [&](IRInstruction* inst) { switch (inst->op()) { - case CreateCont: - { - auto const fp = inst->src(1); - if (state[fp->inst()].isDead()) { - FTRACE(5, "CreateCont ({}) -> InlineCreateCont\n", inst->id()); - - CreateContData data; - data.origFunc = inst->src(2)->getValFunc(); - data.genFunc = inst->src(3)->getValFunc(); - - assert(fp->inst()->src(0)->inst()->op() == SpillFrame); - auto const thisPtr = fp->inst()->src(0)->inst()->src(3); - factory->replace( - inst, - InlineCreateCont, - data, - thisPtr - ); - } - } - break; - case StLoc: case InlineReturn: { auto const fp = inst->src(0); diff --git a/hphp/runtime/vm/jit/extradata.h b/hphp/runtime/vm/jit/extradata.h index ccb746347..751c5a978 100644 --- a/hphp/runtime/vm/jit/extradata.h +++ b/hphp/runtime/vm/jit/extradata.h @@ -190,11 +190,6 @@ private: uintptr_t m_dataBits; }; -struct CreateContData : IRExtraData { - const Func* origFunc; - const Func* genFunc; -}; - /* * Information for the REQ_BIND_JMPCC stubs we create when a tracelet * ends with conditional jumps. @@ -349,7 +344,6 @@ X(DefInlineFP, DefInlineFPData); X(ReqBindJmp, BCOffset); X(ReqBindJmpNoIR, BCOffset); X(ReqRetranslateNoIR, BCOffset); -X(InlineCreateCont, CreateContData); X(CallArray, CallArrayData); X(LdClsCns, ClsCnsName); X(LookupClsCns, ClsCnsName); diff --git a/hphp/runtime/vm/jit/hhbctranslator.cpp b/hphp/runtime/vm/jit/hhbctranslator.cpp index b7b63d32e..da55c3d00 100644 --- a/hphp/runtime/vm/jit/hhbctranslator.cpp +++ b/hphp/runtime/vm/jit/hhbctranslator.cpp @@ -1004,17 +1004,18 @@ void HhbcTranslator::emitCreateCont(Id funNameStrId) { auto const genFunc = origFunc->getGeneratorBody(genName); auto const origLocals = origFunc->numLocals(); - auto const cont = gen( - CreateCont, - cns( - origFunc->isMethod() ? - (TCA)&VMExecutionContext::createContinuation : - (TCA)&VMExecutionContext::createContinuation - ), - m_tb->fp(), - cns(origFunc), - cns(genFunc) - ); + auto const cont = origFunc->isMethod() + ? gen( + CreateContMeth, + cns(origFunc), + cns(genFunc), + gen(LdCtx, m_tb->fp(), cns(curFunc())) + ) + : gen( + CreateContFunc, + cns(origFunc), + cns(genFunc) + ); ContParamMap params; if (origLocals <= Translator::kMaxInlineContLocals && diff --git a/hphp/runtime/vm/jit/ir.h b/hphp/runtime/vm/jit/ir.h index e1edf7752..054e30975 100644 --- a/hphp/runtime/vm/jit/ir.h +++ b/hphp/runtime/vm/jit/ir.h @@ -434,11 +434,8 @@ O(InterpOneCF, ND, S(FramePtr) S(StkPtr) \ C(Int), T|E|N|Mem|Refs|Er) \ O(Spill, DofS(0), SUnk, Mem) \ O(Reload, DofS(0), SUnk, Mem) \ -O(CreateCont, D(Obj), C(TCA) \ - S(FramePtr) \ - C(Func) \ - C(Func), E|N|Mem|PRc) \ -O(InlineCreateCont, D(Obj), S(Obj,Null), E|N|PRc) \ +O(CreateContFunc, D(Obj), C(Func) C(Func), E|N|PRc) \ +O(CreateContMeth, D(Obj), C(Func) C(Func) S(Ctx), E|N|PRc) \ O(FillContLocals, ND, S(FramePtr) \ C(Func) \ C(Func) \ diff --git a/hphp/runtime/vm/jit/nativecalls.cpp b/hphp/runtime/vm/jit/nativecalls.cpp index 2e69838e8..00acfd9e5 100644 --- a/hphp/runtime/vm/jit/nativecalls.cpp +++ b/hphp/runtime/vm/jit/nativecalls.cpp @@ -175,10 +175,10 @@ static CallMap s_callMap({ {{SSA, 0}, {SSA, 1}, {SSA, 2}}}, /* Continuation support helpers */ - {CreateCont, {FSSA, 0}, DSSA, SNone, - {{SSA, 1}, {SSA, 2}, {SSA, 3}}}, - {InlineCreateCont, nullptr, DSSA, SSync, - {{Immed}, {Immed}, {SSA, 0}}}, + {CreateContFunc, (TCA)&VMExecutionContext::createContFunc, DSSA, SNone, + {{SSA, 0}, {SSA, 1}}}, + {CreateContMeth, (TCA)&VMExecutionContext::createContMeth, DSSA, SNone, + {{SSA, 0}, {SSA, 1}, {SSA, 2}}}, {FillContLocals, (TCA)&VMExecutionContext::fillContinuationVars, DNone, SNone, {{SSA, 0}, {SSA, 1}, {SSA, 2}, {SSA, 3}}}, diff --git a/hphp/runtime/vm/jit/tracebuilder.cpp b/hphp/runtime/vm/jit/tracebuilder.cpp index d73e2f8c4..51582af56 100644 --- a/hphp/runtime/vm/jit/tracebuilder.cpp +++ b/hphp/runtime/vm/jit/tracebuilder.cpp @@ -129,10 +129,12 @@ void TraceBuilder::trackDefInlineFP(IRInstruction* inst) { * * We set m_thisIsAvailable to true on any object method, because we * just don't inline calls to object methods with a null $this. + * + * Static methods probably broken #2490252. */ m_fpValue = calleeFP; m_spValue = calleeSP; - m_thisIsAvailable = target->cls() != nullptr; + m_thisIsAvailable = target->cls() != nullptr && !target->isStatic(); m_curFunc = cns(target); /*