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

@@ -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
 

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)

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) \

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}}},

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<Class*>(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<Class*>(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<TypedValue*>(ar) +
+      kNumActRecCells - 1;
+    ObjectData* arThis = ar->getThis();
+    shouldBeObj->m_type = KindOfObject;
+    shouldBeObj->m_data.pobj = arThis;
+    throw;
   }
 }