RetC needs to zero ActRec::m_this before destroying it

In the case where m_this was already in a register,
and known to be non-zero, we would fail to zero the ActRec's
m_this field, which could result in a dangling reference
to $this being captured by debug_backtrace.

Put the store on the presumably cold path. Fix DecRefThis to
do the same.

hphp/doc/ir.specification

@@ -977,6 +977,14 @@ DecRefStack<T> S0:StkPtr S1:ConstInt
   DecRef a value of type T at offset S1 on the stack pointed to by S0.
 
 DecRefThis
+  DecRef the this pointer in the ActRec, checking to see if there is
+  one. If it needs to be destroyed, zero ActRec::m_this before calling
+  the destructor.
+
+DecRefKillThis S0:Gen
+  Identical to DecRef, except that in the case it needs to destroy its
+  argument, it first zeros ActRec::m_this. It is expected that the
+  argument will be the $this pointer.
 
 DecRef S0:Gen
 

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

@@ -2517,10 +2517,10 @@ 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, [&] {
-      // Currently we need to store zero back to m_this in case a local
-      // destructor does debug_backtrace.
-      m_as.storeq(0, fpReg[AROFF(m_this)]);
-      cgDecRefStaticType(Type::Obj, scratchReg, exit, true);
+      // 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,
+                         [=]{m_astubs.storeq(0, fpReg[AROFF(m_this)]);});
     });
   };
 
@@ -2533,6 +2533,13 @@ void CodeGenerator::cgDecRefThis(IRInstruction* inst) {
   }
 }
 
+void CodeGenerator::cgDecRefKillThis(IRInstruction* inst) {
+  // DecRef may bring the count to zero, and run the destructor.
+  // Generate code for this.
+  assert(!inst->getTaken());
+  cgDecRefWork(inst, true, true);
+}
+
 void CodeGenerator::cgDecRefLoc(IRInstruction* inst) {
   cgDecRefMem(inst->getTypeParam(),
               inst->getSrc(0)->getReg(),
@@ -2802,7 +2809,8 @@ Address CodeGenerator::cgCheckRefCountedType(PhysReg baseReg,
 void CodeGenerator::cgDecRefStaticType(Type type,
                                        PhysReg dataReg,
                                        Block* exit,
-                                       bool genZeroCheck) {
+                                       bool genZeroCheck,
+                                       std::function<void()> slowPathWork) {
   assert(type != Type::Cell && type != Type::Gen);
   assert(type.isKnownDataType());
 
@@ -2824,9 +2832,11 @@ 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, [&] {
-      // Emit the call to release in m_astubs
-      cgCallHelper(m_astubs, m_tx64->getDtorCall(type.toDataType()),
-                   InvalidReg, InvalidReg, kSyncPoint, ArgGroup().reg(dataReg));
+        if (slowPathWork) slowPathWork();
+        // Emit the call to release in m_astubs
+        cgCallHelper(m_astubs, m_tx64->getDtorCall(type.toDataType()),
+                     InvalidReg, InvalidReg, kSyncPoint,
+                     ArgGroup().reg(dataReg));
     });
   }
   if (patchStaticCheck) {
@@ -2961,13 +2971,19 @@ void CodeGenerator::cgDecRefMem(IRInstruction* inst) {
               inst->getTaken());
 }
 
-void CodeGenerator::cgDecRefWork(IRInstruction* inst, bool genZeroCheck) {
+void CodeGenerator::cgDecRefWork(IRInstruction* inst,
+                                 bool genZeroCheck, bool killThis) {
   SSATmp* src   = inst->getSrc(0);
   if (!isRefCounted(src)) return;
   Block* exit = inst->getTaken();
   Type type = src->getType();
   if (type.isKnownDataType()) {
-    cgDecRefStaticType(type, src->getReg(), exit, genZeroCheck);
+    if (killThis) {
+      cgDecRefStaticType(type, src->getReg(), exit, genZeroCheck,
+                         [=] { m_astubs.storeq(0, rVmFp[AROFF(m_this)]);});
+    } else {
+      cgDecRefStaticType(type, src->getReg(), exit, genZeroCheck);
+    }
   } else {
     cgDecRefDynamicType(src->getReg(1),
                         src->getReg(0),
@@ -2980,19 +2996,19 @@ void CodeGenerator::cgDecRef(IRInstruction *inst) {
   // DecRef may bring the count to zero, and run the destructor.
   // Generate code for this.
   assert(!inst->getTaken());
-  cgDecRefWork(inst, true);
+  cgDecRefWork(inst, true, false);
 }
 
 void CodeGenerator::cgDecRefNZ(IRInstruction* inst) {
   // DecRefNZ cannot bring the count to zero.
   // Therefore, we don't generate zero-checking code.
   assert(!inst->getTaken());
-  cgDecRefWork(inst, false);
+  cgDecRefWork(inst, false, false);
 }
 
 void CodeGenerator::cgDecRefNZOrBranch(IRInstruction* inst) {
   assert(inst->getTaken());
-  cgDecRefWork(inst, true);
+  cgDecRefWork(inst, true, false);
 }
 
 void CodeGenerator::emitSpillActRec(SSATmp* sp,

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

@@ -174,7 +174,7 @@ private:
   void cgStRefWork(IRInstruction* inst, bool genStoreType);
   void cgStPropWork(IRInstruction* inst, bool genStoreType);
   void cgIncRefWork(Type type, SSATmp* src);
-  void cgDecRefWork(IRInstruction* inst, bool genZeroCheck);
+  void cgDecRefWork(IRInstruction* inst, bool genZeroCheck, bool killThis);
 
   template<class OpInstr, class Oper>
   void cgUnaryIntOp(SSATmp* dst, SSATmp* src, OpInstr, Oper);
@@ -246,7 +246,9 @@ private:
   void cgDecRefStaticType(Type type,
                           PhysReg dataReg,
                           Block* exit,
-                          bool genZeroCheck);
+                          bool genZeroCheck,
+                          std::function<void()> slowPathWork =
+                          std::function<void()>());
   void cgDecRefDynamicType(PhysReg typeReg,
                            PhysReg dataReg,
                            Block* exit,

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

@@ -451,7 +451,7 @@ bool IRInstruction::killsSources() const {
 bool IRInstruction::killsSource(int idx) const {
   if (!killsSources()) return false;
 
-  if (m_op == DecRef) {
+  if (m_op == DecRef || m_op == DecRefKillThis) {
     assert(idx == 0);
     return true;
   }

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

@@ -315,6 +315,7 @@ O(IncRef,                      DofS(0), S(Gen),                    Mem|PRc|P) \
 O(DecRefLoc,                        ND, S(StkPtr),              N|E|Mem|Refs) \
 O(DecRefStack,                      ND, S(StkPtr) C(Int),       N|E|Mem|Refs) \
 O(DecRefThis,                       ND, SUnk,                   N|E|Mem|Refs) \
+O(DecRefKillThis,                   ND, S(Gen),           N|E|Mem|CRc|Refs|K) \
 O(GenericRetDecRefs,         D(StkPtr), S(StkPtr)                             \
                                           S(Gen) C(Int),        E|N|Mem|Refs) \
 O(DecRef,                           ND, S(Gen),           N|E|Mem|CRc|Refs|K) \
@@ -676,7 +677,6 @@ X(LdConst,            ConstData);
 X(Jmp_,               EdgeData);
 X(ExitTrace,          ExitData);
 X(ExitTraceCc,        ExitData);
-
 #undef X
 
 //////////////////////////////////////////////////////////////////////

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

@@ -1014,7 +1014,7 @@ void TraceBuilder::genDecRefStack(Type type, uint32_t stackOff) {
 void TraceBuilder::genDecRefThis() {
   if (isThisAvailable()) {
     SSATmp* thiss = genLdThis(nullptr);
-    genDecRef(thiss);
+    gen(DecRefKillThis, thiss);
   } else {
     gen(DecRefThis, m_fpValue);
   }

hphp/test/vm/actrec.php

@@ -0,0 +1,42 @@
+<?php
+
+class X {
+  private $bar=1;
+  function __destruct() {
+    var_dump(__METHOD__);
+    global $e;
+    $e = debug_backtrace(true);
+  }
+  function foo($ids) {
+    return array($this->bar,
+                 $ids,
+                 $this->bar,
+                 $this->bar,
+                 $this->bar,
+                 $this->bar);
+  }
+}
+
+function test() {
+  $a = new X;
+  yield 1;
+  yield $a;
+  global $g;
+  $g = null;
+  yield 2;
+}
+
+function main() {
+  global $g;
+  $g = test();
+  for ($g->next(); $g && $g->valid(); $g->next())
+    var_dump($g->current());
+  var_dump($g);
+  global $e;
+  $e = null;
+}
+
+main();
+$a = new X;
+var_dump($a->foo(1));
+$a = null;

hphp/test/vm/actrec.php.exp

@@ -0,0 +1,22 @@
+int(1)
+object(X)#2 (1) {
+  ["bar":"X":private]=>
+  int(1)
+}
+string(13) "X::__destruct"
+NULL
+array(6) {
+  [0]=>
+  int(1)
+  [1]=>
+  int(1)
+  [2]=>
+  int(1)
+  [3]=>
+  int(1)
+  [4]=>
+  int(1)
+  [5]=>
+  int(1)
+}
+string(13) "X::__destruct"