Make it unnecessary to null out locals during RetC

We already look up the PC for every frame as we're taking the
backtrace, so we can just skip them when going through a backtrace
frame.  Also, there is currently a guarantee that if an exception
propagates through a frame where pc was pointing at RetC, all the
locals and the $this pointer have already been decref'd, so we don't
need to use a null check to determine this.

hphp/doc/ir.specification

@@ -1077,21 +1077,8 @@ DecRefStack<T,offset> S0:StkPtr
 
 DecRefThis S0:FramePtr
 
-  DecRef the $this pointer in the ActRec S0, checking to see if there
-  is one.  If it needs to be destroyed, zero the this slot before
-  calling the destructor.
-
-  This is done to prevent backtraces from seeing stale this pointers.
-
-DecRefKillThis S0:Obj S1:FramePtr
-
-  Similar to DecRefThis, except for use when we have the $this pointer
-  in S0.  This instruction does the same thing as DecRef, except that
-  in the case it needs to destroy its argument, it first zeros the
-  this pointer on the frame S1.  It is expected that the argument will
-  be the $this pointer.
-
-  This is done to prevent backtraces from seeing stale $this pointers.
+  DecRef the $this pointer in the ActRec S0, if it had one.  Does
+  nothing if there was a class context or null $this on the ActRec.
 
 DecRef S0:Gen
 

hphp/runtime/vm/bytecode.cpp

@@ -920,7 +920,10 @@ UnwindStatus Stack::unwindFrag(ActRec* fp, int offset,
   FTRACE(1, "unwindFrag: func {} ({})\n",
          func->fullName()->data(), func->unit()->filepath()->data());
 
-  bool unwindingGeneratorFrame = func->isGenerator();
+  const bool unwindingGeneratorFrame = func->isGenerator();
+  auto const curOp = *reinterpret_cast<const Opcode*>(pc);
+  using namespace VM;
+  const bool unwindingReturningFrame = curOp == OpRetC || curOp == OpRetV;
   TypedValue* evalTop;
   if (UNLIKELY(unwindingGeneratorFrame)) {
     assert(!isValidAddress((uintptr_t)fp));
@@ -998,12 +1001,34 @@ UnwindStatus Stack::unwindFrag(ActRec* fp, int offset,
     fp->getThis()->setNoDestruct();
   }
 
+  // A generator's locals don't live on this stack.
   if (LIKELY(!unwindingGeneratorFrame)) {
-    // A generator's locals don't live on this stack.
-    // onFunctionExit might throw
-    try {
-      frame_free_locals_inl(fp, func->numLocals());
-    } catch (...) {}
+    /*
+     * If we're unwinding through a frame that's returning, it's only
+     * possible that its locals have already been decref'd.
+     *
+     * Here's why:
+     *
+     *   - If a destructor for any of these things throws a php
+     *     exception, it's swallowed at the dtor boundary and we keep
+     *     running php.
+     *
+     *   - If the destructor for any of these things throws a fatal,
+     *     it's swallowed, and we set surprise flags to throw a fatal
+     *     from now on.
+     *
+     *   - If the second case happened and we have to run another
+     *     destructor, its enter hook will throw, but it will be
+     *     swallowed again.
+     *
+     *   - Finally, the exit hook for the returning function can
+     *     throw, but this happens last so everything is destructed.
+     */
+    if (!unwindingReturningFrame) {
+      try {
+        frame_free_locals_inl(fp, func->numLocals());
+      } catch (...) {}
+    }
     ndiscard(func->numSlotsInFrame());
   }
   FTRACE(1, "unwindFrag: propagate\n");
@@ -2307,62 +2332,75 @@ Array VMExecutionContext::debugBacktrace(bool skip /* = false */,
     frame.set(String(s_line), parserFrame->lineNumber, true);
     bt.append(frame);
   }
+
   Transl::VMRegAnchor _;
   if (!getFP()) {
     // If there are no VM frames, we're done
     return bt;
   }
-  // Get the fp and pc of the top frame (possibly skipping one frame)
-  ActRec* fp;
-  Offset pc = 0;
-  if (skip) {
-    fp = getPrevVMState(getFP(), &pc);
-    if (!fp) {
-      // We skipped over the only VM frame, we're done
-      return bt;
-    }
-  } else {
-    fp = getFP();
-    Unit *unit = getFP()->m_func->unit();
-    assert(unit);
-    pc = unit->offsetOf(m_pc);
-  }
+
   int depth = 0;
-  // Handle the top frame
-  if (withSelf) {
-    // Builtins don't have a file and line number
-    if (!fp->m_func->isBuiltin()) {
-      Unit *unit = fp->m_func->unit();
-      assert(unit);
-      const char* filename = unit->filepath()->data();
-      assert(filename);
-      Offset off = pc;
-      Array frame = Array::Create();
-      frame.set(String(s_file), filename, true);
-      frame.set(String(s_line), unit->getLineNumber(off), true);
-      if (parserFrame) {
-        frame.set(String(s_function), String(s_include), true);
-        frame.set(String(s_args), Array::Create(parserFrame->filename), true);
+  ActRec* fp = nullptr;
+  Offset pc = 0;
+
+  // Get the fp and pc of the top frame (possibly skipping one frame)
+  {
+    if (skip) {
+      fp = getPrevVMState(getFP(), &pc);
+      if (!fp) {
+        // We skipped over the only VM frame, we're done
+        return bt;
+      }
+    } else {
+      fp = getFP();
+      Unit *unit = getFP()->m_func->unit();
+      assert(unit);
+      pc = unit->offsetOf(m_pc);
+    }
+
+    // Handle the top frame
+    if (withSelf) {
+      // Builtins don't have a file and line number
+      if (!fp->m_func->isBuiltin()) {
+        Unit *unit = fp->m_func->unit();
+        assert(unit);
+        const char* filename = unit->filepath()->data();
+        assert(filename);
+        Offset off = pc;
+        Array frame = Array::Create();
+        frame.set(String(s_file), filename, true);
+        frame.set(String(s_line), unit->getLineNumber(off), true);
+        if (parserFrame) {
+          frame.set(String(s_function), String(s_include), true);
+          frame.set(String(s_args), Array::Create(parserFrame->filename), true);
+        }
+        bt.append(frame);
+        depth++;
       }
-      bt.append(frame);
-      depth++;
     }
   }
+
   // Handle the subsequent VM frames
-  for (ActRec* prevFp = getPrevVMState(fp, &pc); fp != nullptr;
-       fp = prevFp, prevFp = getPrevVMState(fp, &pc)) {
+  Offset prevPc = 0;
+  for (ActRec* prevFp = getPrevVMState(fp, &prevPc); fp != nullptr;
+       fp = prevFp, pc = prevPc, prevFp = getPrevVMState(fp, &prevPc)) {
     Array frame = Array::Create();
+
     // do not capture frame for HPHP only functions
     if (fp->m_func->isNoInjection()) {
       continue;
     }
+
+    auto const curUnit = fp->m_func->unit();
+    auto const curOp = *reinterpret_cast<const Opcode*>(curUnit->at(pc));
+    auto const isReturning = curOp == OpRetC || curOp == OpRetV;
+
     // Builtins don't have a file and line number
     if (prevFp && !prevFp->m_func->isBuiltin()) {
-      Unit* unit = prevFp->m_func->unit();
-      assert(unit);
-      const char *filename = unit->filepath()->data();
-      assert(filename);
-      frame.set(String(s_file), filename, true);
+      auto const prevUnit = prevFp->m_func->unit();
+      frame.set(String(s_file),
+                const_cast<StringData*>(prevUnit->filepath()),
+                true);
 
       // In the normal method case, the "saved pc" for line number printing is
       // pointing at the cell conversion (Unbox/Pop) instruction, not the call
@@ -2372,15 +2410,17 @@ Array VMExecutionContext::debugBacktrace(bool skip /* = false */,
       // instruction. Exception handling and the other opcodes (ex. BoxR)
       // already do the right thing. The emitter associates object access with
       // the subsequent expression and this would be difficult to modify.
-      Opcode* opAtSavedPc = (Opcode*)unit->at(pc);
-      assert(opAtSavedPc);
+      auto const opAtPrevPc =
+        *reinterpret_cast<const Opcode*>(prevUnit->at(prevPc));
       Offset pcAdjust = 0;
-      if (*opAtSavedPc == Op::OpPopR || *opAtSavedPc == Op::OpUnboxR) {
+      if (opAtPrevPc == OpPopR || opAtPrevPc == OpUnboxR) {
         pcAdjust = 1;
       }
       frame.set(String(s_line),
-                prevFp->m_func->unit()->getLineNumber(pc - pcAdjust), true);
+                prevFp->m_func->unit()->getLineNumber(prevPc - pcAdjust),
+                true);
     }
+
     // check for include
     String funcname = const_cast<StringData*>(fp->m_func->name());
     if (fp->m_func->isGenerator()) {
@@ -2390,23 +2430,27 @@ Array VMExecutionContext::debugBacktrace(bool skip /* = false */,
       funcname = static_cast<c_Continuation*>(
                        tv->m_data.pobj)->t_getorigfuncname();
     }
+
     if (fp->m_func->isClosureBody()) {
       static StringData* s_closure_label =
           StringData::GetStaticString("{closure}");
       funcname = s_closure_label;
     }
+
     // check for pseudomain
     if (funcname->empty()) {
       if (!prevFp) continue;
       funcname = s_include;
     }
+
     frame.set(String(s_function), funcname, true);
+
     if (!funcname.same(s_include)) {
       // Closures have an m_this but they aren't in object context
       Class* ctx = arGetContextClass(fp);
       if (ctx != nullptr && !fp->m_func->isClosureBody()) {
         frame.set(String(s_class), ctx->name()->data(), true);
-        if (fp->hasThis()) {
+        if (fp->hasThis() && !isReturning) {
           if (withThis) {
             frame.set(String(s_object), Object(fp->getThis()), true);
           }
@@ -2416,14 +2460,14 @@ Array VMExecutionContext::debugBacktrace(bool skip /* = false */,
         }
       }
     }
+
     Array args = Array::Create();
     if (funcname.same(s_include)) {
       if (depth) {
-        args.append(String(const_cast<StringData*>(
-                             fp->m_func->unit()->filepath())));
+        args.append(String(const_cast<StringData*>(curUnit->filepath())));
         frame.set(String(s_args), args, true);
       }
-    } else if (!RuntimeOption::EnableArgsInBacktraces) {
+    } else if (!RuntimeOption::EnableArgsInBacktraces || isReturning) {
       // Provide an empty 'args' array to be consistent with hphpc
       frame.set(String(s_args), args, true);
     } else {
@@ -2448,6 +2492,7 @@ Array VMExecutionContext::debugBacktrace(bool skip /* = false */,
       }
       frame.set(String(s_args), args, true);
     }
+
     bt.append(frame);
     depth++;
   }

hphp/runtime/vm/runtime.h

@@ -106,11 +106,6 @@ frame_free_locals_helper_inl(ActRec* fp, int numLocals) {
     DataType t = loc->m_type;
     if (IS_REFCOUNTED_TYPE(t)) {
       uint64_t datum = loc->m_data.num;
-      // When destroying an array or object we can reenter the VM
-      // to call a __destruct method. Null out the local before
-      // calling the destructor so that stacktrace logic doesn't
-      // choke.
-      tvWriteUninit(loc);
       tvDecRefHelper(t, datum);
     }
   }
@@ -120,10 +115,6 @@ inline void ALWAYS_INLINE
 frame_free_locals_inl(ActRec* fp, int numLocals) {
   if (fp->hasThis()) {
     ObjectData* this_ = fp->getThis();
-    // If a destructor for a local calls debug_backtrace, it can read
-    // the m_this field from the ActRec, so we need to zero it to
-    // ensure they can't access a free'd object.
-    fp->setThis(nullptr);
     decRefObj(this_);
   }
   frame_free_locals_helper_inl(fp, numLocals);
@@ -143,11 +134,6 @@ frame_free_args(TypedValue* args, int count) {
     DataType t = loc->m_type;
     if (IS_REFCOUNTED_TYPE(t)) {
       uint64_t datum = loc->m_data.num;
-      // When destroying an array or object we can reenter the VM
-      // to call a __destruct method. Null out the local before
-      // calling the destructor so that stacktrace logic doesn't
-      // choke.
-      tvWriteUninit(loc);
       tvDecRefHelper(t, datum);
     }
   }

hphp/runtime/vm/translator/hopt/codegen.cpp

@@ -2659,16 +2659,11 @@ void CodeGenerator::cgDecRefThis(IRInstruction* inst) {
     // Check if this is available and we're not in a static context instead
     m_as.testb(1, rbyte(scratchReg));
     ifThen(m_as, CC_Z, [&] {
-      // In the case where the refCount hits zero, we need to store zero
-      // back to m_this in case a local destructor does debug_backtrace.
       cgDecRefStaticType(
         Type::Obj,
         scratchReg,
         exit,
-        true /* genZeroCheck */,
-        [&] (Asm& a) {
-          a.    storeq  (0, fpReg[AROFF(m_this)]);
-        }
+        true /* genZeroCheck */
       );
     });
   };
@@ -2682,23 +2677,6 @@ void CodeGenerator::cgDecRefThis(IRInstruction* inst) {
   }
 }
 
-void CodeGenerator::cgDecRefKillThis(IRInstruction* inst) {
-  auto const src = inst->getSrc(0);
-  auto const fpReg = inst->getSrc(1)->getReg();
-
-  assert(!inst->getTaken());
-
-  cgDecRefStaticType(
-    src->type(),
-    src->getReg(),
-    nullptr,
-    true /* genZeroCheck */,
-    [&] (Asm& a) {
-      a.    storeq  (0, fpReg[AROFF(m_this)]);
-    }
-  );
-}
-
 void CodeGenerator::cgDecRefLoc(IRInstruction* inst) {
   cgDecRefMem(inst->getTypeParam(),
               inst->getSrc(0)->getReg(),
@@ -2963,9 +2941,7 @@ Address CodeGenerator::cgCheckRefCountedType(PhysReg baseReg, int64_t offset) {
 void CodeGenerator::cgDecRefStaticType(Type type,
                                        PhysReg dataReg,
                                        Block* exit,
-                                       bool genZeroCheck,
-                                       std::function<void(Asm&)>
-                                         slowPathWork) {
+                                       bool genZeroCheck) {
   assert(type != Type::Cell && type != Type::Gen);
   assert(type.isKnownDataType());
 
@@ -2987,7 +2963,6 @@ void CodeGenerator::cgDecRefStaticType(Type type,
   if (genZeroCheck && exit == nullptr) {
     // Emit jump to m_astubs (to call release) if count got down to zero
     unlikelyIfBlock(CC_Z, [&] (Asm& a) {
-      if (slowPathWork) slowPathWork(a);
       // Emit the call to release in m_astubs
       cgCallHelper(a, m_tx64->getDtorCall(type.toDataType()),
                    InvalidReg, InvalidReg, kSyncPoint,

hphp/runtime/vm/translator/hopt/codegen.h

@@ -265,9 +265,7 @@ private:
   void cgDecRefStaticType(Type type,
                           PhysReg dataReg,
                           Block* exit,
-                          bool genZeroCheck,
-                          std::function<void(Asm&)> slowPathWork =
-                            std::function<void(Asm&)>());
+                          bool genZeroCheck);
   void cgDecRefDynamicType(PhysReg typeReg,
                            PhysReg dataReg,
                            Block* exit,

hphp/runtime/vm/translator/hopt/dce.cpp

@@ -377,19 +377,6 @@ void optimizeActRecs(Trace* trace, DceState& state, IRFactory* factory,
 
   forEachInst(trace, [&](IRInstruction* inst) {
     switch (inst->op()) {
-    case DecRefKillThis:
-      {
-        auto frame = inst->getSrc(1);
-        auto frameInst = frame->inst();
-        if (frameInst->op() == DefInlineFP) {
-          FTRACE(5, "DecRefKillThis ({}): weak use of frame {}\n",
-                 inst->getIId(),
-                 frameInst->getIId());
-          state[frameInst].incWeakUse();
-        }
-      }
-      break;
-
     case CreateCont:
       {
         auto const frameInst = inst->getSrc(1)->inst();
@@ -438,18 +425,6 @@ void optimizeActRecs(Trace* trace, DceState& state, IRFactory* factory,
    */
   forEachInst(trace, [&](IRInstruction* inst) {
     switch (inst->op()) {
-    case DecRefKillThis:
-      {
-        auto const fp = inst->getSrc(1);
-        if (state[fp->inst()].isDead()) {
-          FTRACE(5, "DecRefKillThis ({}) -> DecRef\n", inst->getIId());
-          inst->setOpcode(DecRef);
-          inst->setSrc(1, nullptr);
-          inst->setNumSrcs(1);
-        }
-      }
-      break;
-
     case CreateCont:
       {
         auto const fp = inst->getSrc(1);

hphp/runtime/vm/translator/hopt/hhbctranslator.cpp

@@ -1933,11 +1933,6 @@ SSATmp* HhbcTranslator::emitDecRefLocalsInline(SSATmp* retVal) {
 
   if (mayHaveThis(getCurFunc())) {
     if (retValSrcLoc && retValSrcOpc == LdThis) {
-      // Note that this doesn't need to be DecRefThis or
-      // DecRefKillThis because we're carefully setting things up to
-      // get turned to DecRefNZ.  This means even if a
-      // debug_backtrace() occurs it can't see a stale $this on the
-      // ActRec.
       gen(DecRef, retVal);
     } else {
       gen(DecRefThis, m_tb->getFp());
@@ -1957,20 +1952,7 @@ SSATmp* HhbcTranslator::emitDecRefLocalsInline(SSATmp* retVal) {
       gen(DecRef, retVal);
       continue;
     }
-
-    // When a parameter goes out of scope during return, we need to
-    // null it out so that debug_backtrace can't capture stale values.
-    // TODO(#2332775): we shouldn't have to do this.
-    const bool needSetNull = id < getCurFunc()->numParams();
-    if (needSetNull) {
-      auto const loc = ldLoc(id);
-      if (needSetNull) {
-        gen(StLoc, LocalId(id), m_tb->getFp(), m_tb->genDefUninit());
-      }
-      gen(DecRef, loc);
-    } else {
-      gen(DecRefLoc, Type::Gen, LocalId(id), m_tb->getFp());
-    }
+    gen(DecRefLoc, Type::Gen, LocalId(id), m_tb->getFp());
   }
 
   return retValSrcLoc ? retValSrcLoc : retVal;

hphp/runtime/vm/translator/hopt/ir.cpp

@@ -479,8 +479,6 @@ bool IRInstruction::killsSource(int idx) const {
     case ArraySet:
     case ArraySetRef:
       return idx == 1;
-    case DecRefKillThis:
-      return idx == 0;
     default:
       not_reached();
       break;

hphp/runtime/vm/translator/hopt/ir.h

@@ -372,8 +372,6 @@ O(IncRef,                      DofS(0), S(Gen),                    Mem|PRc|P) \
 O(DecRefLoc,                        ND, S(FramePtr),            N|E|Mem|Refs) \
 O(DecRefStack,                      ND, S(StkPtr),              N|E|Mem|Refs) \
 O(DecRefThis,                       ND, S(FramePtr),            N|E|Mem|Refs) \
-O(DecRefKillThis,                   ND, S(Obj)                                \
-                                          S(FramePtr),    N|E|Mem|CRc|Refs|K) \
 O(GenericRetDecRefs,         D(StkPtr), S(FramePtr)                           \
                                           S(Gen) C(Int),        E|N|Mem|Refs) \
 O(DecRef,                           ND, S(Gen),           N|E|Mem|CRc|Refs|K) \

hphp/runtime/vm/translator/hopt/tracebuilder.cpp

@@ -730,11 +730,8 @@ SSATmp* TraceBuilder::preOptimizeDecRefThis(IRInstruction* inst) {
       return nullptr;
     }
 
-    // If we're in an inlined callee, it's a shame to keep a reference
-    // to the frame just to kill the $this pointer.  But this is
-    // handled in optimizeActRecs.
     assert(inst->getSrc(0) == m_fpValue);
-    gen(DecRefKillThis, thiss, m_fpValue);
+    gen(DecRef, thiss);
     inst->convertToNop();
     return nullptr;
   }

hphp/runtime/vm/translator/translator-x64.cpp

@@ -11503,9 +11503,9 @@ asm_label(a, release);
   });
   a.    ret    ();
 asm_label(a, doRelease);
-  emitStoreTVType(a, KindOfUninit, rIter[TVOFF(m_type)]);
   jumpDestructor(a, PhysReg(rType), rax);
 
+  moveToAlign(a, kJmpTargetAlign);
   m_freeManyLocalsHelper = a.code.frontier;
   a.    lea    (rVmFp[-cellsToBytes(kNumFreeLocalsHelpers)], rFinished);
 

hphp/test/quick/retc-destruct.php.expect

@@ -1,4 +1,2 @@
-array(1) {
-  [0]=>
-  NULL
+array(0) {
 }

hphp/test/quick/surprise_throw.php.expect

@@ -1,7 +1,6 @@
 hi exit fb_setprofile
 hi enter foo
 yep
-yep
 hi enter Exception::getMessage
 hi exit Exception::getMessage
 yo

hphp/test/slow/exceptions/retc.php

@@ -0,0 +1,27 @@
+<?php
+
+class Obj {
+  public function __destruct() {
+    // Raise a fatal.
+    class Obj {}
+  }
+}
+
+function foo() {
+  $y = new Obj;
+  $x = new Obj;
+  $y = new Obj;
+
+  // Currently our behavior during return when a local dtor throws a
+  // fatal is to swallow it, then keep rethrowing it from the enter
+  // hook for each destructor.  Then we return as normal (and this
+  // test is trying to make sure nothing chokes due to the destroyed
+  // locals), then further out the next enter hook will throw the
+  // fatal.
+}
+
+try {
+  foo();
+} catch (Exception $x) { echo "notreached\n"; }
+foo(); // enter hook throws the fatal
+

hphp/test/slow/exceptions/retc.php.expectf

@@ -0,0 +1 @@
+HipHop Fatal error: Class already declared: Obj in %s on line %d