From 9690cc4f525438bd305fa1508e6f7ca38bfa4bd5 Mon Sep 17 00:00:00 2001 From: alia Date: Mon, 18 Mar 2013 18:27:19 -0700 Subject: [PATCH] Fixed problems with HHIR's LdObjMethod instruction This diff fixes problems with FPushObjMethodD stack unwinding, to HHIR. This diff simply moves the try-catch into the methodCacheSlowPath. This should also improve the code generated by gcc for the fast path. --- hphp/doc/ir.specification | 8 +- hphp/runtime/vm/translator/hopt/codegen.cpp | 4 +- hphp/runtime/vm/translator/hopt/ir.h | 2 +- .../vm/translator/hopt/nativecalls.cpp | 2 - hphp/runtime/vm/translator/targetcache.cpp | 114 ++++++++++-------- 5 files changed, 77 insertions(+), 53 deletions(-) diff --git a/hphp/doc/ir.specification b/hphp/doc/ir.specification index c5c51cea6..039685216 100644 --- a/hphp/doc/ir.specification +++ b/hphp/doc/ir.specification @@ -689,7 +689,13 @@ D:PtrToGen = LdClsPropAddrCached S0:Cls S1:ConstStr S2:ConstStr throw a fatal error if the optional label L is not present, or (2) jump to L if it is present. -D:Func = LdObjMethod S0:ConstInt S1:ConstStr S2:StkPtr +LdObjMethod S0:Cls S1:ConstStr S2:StkPtr + + Stores a pointer to an object's method into an activation record. S0 + points to the object's class, S1 is the method name, and S3 points + to the activation record. Caches the mapping in the target + cache. Fatals if the class does not have an accessible method with + the given name and does not have a __call method. D:Cls = LdObjClass S0:Obj diff --git a/hphp/runtime/vm/translator/hopt/codegen.cpp b/hphp/runtime/vm/translator/hopt/codegen.cpp index 8d0d2ad24..fadd316ac 100644 --- a/hphp/runtime/vm/translator/hopt/codegen.cpp +++ b/hphp/runtime/vm/translator/hopt/codegen.cpp @@ -1834,7 +1834,6 @@ void CodeGenerator::cgLdObjClass(IRInstruction* inst) { } void CodeGenerator::cgLdObjMethod(IRInstruction *inst) { - auto dstReg = inst->getDst()->getReg(); auto cls = inst->getSrc(0); auto clsReg = cls->getReg(); auto name = inst->getSrc(1); @@ -1860,7 +1859,8 @@ void CodeGenerator::cgLdObjMethod(IRInstruction *inst) { m_as.storeq(rScratch, actRecReg[AROFF(m_func)]); }, [&] { // else call slow path helper - cgCallHelper(m_as, (TCA)methodCacheSlowPath, dstReg, kSyncPoint, + cgCallHelper(m_as, (TCA)methodCacheSlowPath, InvalidReg, + kSyncPoint, ArgGroup().addr(rVmTl, handle) .ssa(actRec) .ssa(name) diff --git a/hphp/runtime/vm/translator/hopt/ir.h b/hphp/runtime/vm/translator/hopt/ir.h index 36aafca69..c4447984a 100644 --- a/hphp/runtime/vm/translator/hopt/ir.h +++ b/hphp/runtime/vm/translator/hopt/ir.h @@ -233,7 +233,7 @@ O(LdClsMethod, D(Func), S(Cls) C(Int), C) \ O(LdPropAddr, D(PtrToGen), S(Obj) C(Int), C) \ O(LdClsPropAddr, D(PtrToGen), S(Cls) S(Str) C(Cls), C|E|N|Er) \ O(LdClsPropAddrCached, D(PtrToGen), S(Cls) CStr CStr C(Cls), C|E|N|Er) \ -O(LdObjMethod, D(Func), S(Cls) CStr S(StkPtr), C|E|N|Refs|Er) \ +O(LdObjMethod, ND, S(Cls) CStr S(StkPtr), E|N|Refs|Er) \ O(LdGblAddrDef, D(PtrToGen), S(Str), E|N|CRc) \ O(LdGblAddr, D(PtrToGen), S(Str), N ) \ O(LdObjClass, D(Cls), S(Obj), C) \ diff --git a/hphp/runtime/vm/translator/hopt/nativecalls.cpp b/hphp/runtime/vm/translator/hopt/nativecalls.cpp index f753b0489..deb3f1a19 100644 --- a/hphp/runtime/vm/translator/hopt/nativecalls.cpp +++ b/hphp/runtime/vm/translator/hopt/nativecalls.cpp @@ -70,8 +70,6 @@ static CallMap s_callMap({ {{SSA, 0}, {TV, 1}}}, {ArrayAdd, (TCA)array_add, DSSA, SNone, {{SSA, 0}, {SSA, 1}}}, {Box, (TCA)box_value, DSSA, SNone, {{TV, 0}}}, - {LdObjMethod, (TCA)TargetCache::MethodCache::lookup, - DSSA, SSync, {{SSA, 0}, {SSA, 2}, {SSA, 1}}}, {NewArray, (TCA)new_array, DSSA, SNone, {{SSA, 0}}}, {NewTuple, (TCA)new_tuple, DSSA, SNone, {{SSA, 0}, {SSA, 1}}}, diff --git a/hphp/runtime/vm/translator/targetcache.cpp b/hphp/runtime/vm/translator/targetcache.cpp index 9111a8965..ff0489b01 100644 --- a/hphp/runtime/vm/translator/targetcache.cpp +++ b/hphp/runtime/vm/translator/targetcache.cpp @@ -461,61 +461,81 @@ void methodCacheSlowPath(MethodCache::Pair* mce, assert(ar->getThis()->getVMClass() == cls); assert(IMPLIES(mce->m_key, mce->m_value)); - bool isMagicCall = mce->m_key & 0x1u; - bool isStatic; - const Func* func; + try { + bool isMagicCall = mce->m_key & 0x1u; + bool isStatic; + const Func* func; - auto* storedClass = reinterpret_cast(mce->m_key & ~0x3u); - if (storedClass == cls) { - isStatic = mce->m_key & 0x2u; - func = mce->m_value; - } else { - if (LIKELY(storedClass != nullptr && - ((func = cls->wouldCall(mce->m_value)) != nullptr) && - !isMagicCall)) { - Stats::inc(Stats::TgtCache_MethodHit, func != nullptr); - isMagicCall = false; + auto* storedClass = reinterpret_cast(mce->m_key & ~0x3u); + if (storedClass == cls) { + isStatic = mce->m_key & 0x2u; + func = mce->m_value; } else { - Class* ctx = arGetContextClass((ActRec*)ar->m_savedRbp); - Stats::inc(Stats::TgtCache_MethodMiss); - TRACE(2, "MethodCache: miss class %p name %s!\n", cls, name->data()); - func = g_vmContext->lookupMethodCtx(cls, name, ctx, - MethodLookup::ObjMethod, false); - if (UNLIKELY(!func)) { - isMagicCall = true; - func = cls->lookupMethod(s___call.get()); - if (UNLIKELY(!func)) { - // Do it again, but raise the error this time. - (void) g_vmContext->lookupMethodCtx(cls, name, ctx, - MethodLookup::ObjMethod, true); - NOT_REACHED(); - } - } else { + if (LIKELY(storedClass != nullptr && + ((func = cls->wouldCall(mce->m_value)) != nullptr) && + !isMagicCall)) { + Stats::inc(Stats::TgtCache_MethodHit, func != nullptr); isMagicCall = false; + } else { + Class* ctx = arGetContextClass((ActRec*)ar->m_savedRbp); + Stats::inc(Stats::TgtCache_MethodMiss); + TRACE(2, "MethodCache: miss class %p name %s!\n", cls, name->data()); + func = g_vmContext->lookupMethodCtx(cls, name, ctx, + MethodLookup::ObjMethod, false); + if (UNLIKELY(!func)) { + isMagicCall = true; + func = cls->lookupMethod(s___call.get()); + if (UNLIKELY(!func)) { + // Do it again, but raise the error this time. + (void) g_vmContext->lookupMethodCtx(cls, name, ctx, + MethodLookup::ObjMethod, true); + NOT_REACHED(); + } + } else { + isMagicCall = false; + } } + + isStatic = func->attrs() & AttrStatic; + + mce->m_key = uintptr_t(cls) | (uintptr_t(isStatic) << 1) | + uintptr_t(isMagicCall); + mce->m_value = func; } - isStatic = func->attrs() & AttrStatic; + assert(func); + func->validate(); + ar->m_func = func; - mce->m_key = uintptr_t(cls) | (uintptr_t(isStatic) << 1) | - uintptr_t(isMagicCall); - mce->m_value = func; - } + if (UNLIKELY(isStatic)) { + decRefObj(ar->getThis()); + if (debug) ar->setThis(nullptr); // suppress assert in setClass + ar->setClass(cls); + } - assert(func); - func->validate(); - ar->m_func = func; - - if (UNLIKELY(isStatic)) { - decRefObj(ar->getThis()); - if (debug) ar->setThis(nullptr); // suppress assert in setClass - ar->setClass(cls); - } - - assert(!ar->hasVarEnv() && !ar->hasInvName()); - if (UNLIKELY(isMagicCall)) { - ar->setInvName(name); - assert(name->isStatic()); // No incRef needed. + assert(!ar->hasVarEnv() && !ar->hasInvName()); + if (UNLIKELY(isMagicCall)) { + ar->setInvName(name); + assert(name->isStatic()); // No incRef needed. + } + } catch (...) { + /* + * Barf. + * + * If the slow lookup fails, we're going to rewind to the state + * before the FPushObjMethodD that dumped us here. In this state, + * the object is still on the stack, but for efficiency reasons, + * we've smashed this TypedValue* with the ActRec we were trying + * to push. + * + * Reconstitute the virtual object before rethrowing. + */ + TypedValue* shouldBeObj = reinterpret_cast(ar) + + kNumActRecCells - 1; + ObjectData* arThis = ar->getThis(); + shouldBeObj->m_type = KindOfObject; + shouldBeObj->m_data.pobj = arThis; + throw; } }