Debug-fill popped eval stack slots when running in interpreter

Was hoping to catch a bug from D891329.  Only caught a benign
use-after-pop of an ActRec in the unwinder.  Maybe worth it anyway?

Reviewed By: @edwinsmith

Differential Revision: D907772

hphp/runtime/base/memory-manager.h

@@ -46,8 +46,10 @@ struct SweepNode {
 };
 
 // jemalloc uses 0x5a but we use 0x6a so we can tell the difference
-// when debugging.
+// when debugging.  There's also 0x7a for some cases of ex-TypedValue
+// memory.
 const char kSmartFreeFill = 0x6a;
+const char kTVTrashFill = 0x7a;
 const uintptr_t kSmartFreeWord = 0x6a6a6a6a6a6a6a6aLL;
 const uintptr_t kMallocFreeWord = 0x5a5a5a5a5a5a5a5aLL;
 

hphp/runtime/base/tv-helpers.h

@@ -37,6 +37,14 @@ bool tvIsPlausible(TypedValue);
 bool cellIsPlausible(Cell);
 bool refIsPlausible(Ref);
 
+/*
+ * In a debug build, write garbage into a memory slot for a TypedValue
+ * that should not be used anymore.
+ */
+inline void tvDebugTrash(TypedValue* tv) {
+  if (debug) memset(tv, kTVTrashFill, sizeof *tv);
+}
+
 /*
  * Returns: true if the supplied TypedValue is KindOfDouble or
  * KindOfInt64.  I.e. if it is a TypedNum.

hphp/runtime/vm/bytecode.cpp

@@ -6743,10 +6743,10 @@ OPTBLD_INLINE void VMExecutionContext::iopContSuspendK(PC& pc) {
   c_Continuation* cont = frame_continuation(m_fp);
 
   TypedValue* val = m_stack.topTV();
-  m_stack.popTV();
-  cont->c_Continuation::t_update_key(label, tvAsCVarRef(m_stack.topTV()),
+  cont->c_Continuation::t_update_key(label, tvAsCVarRef(m_stack.indTV(1)),
                                      tvAsCVarRef(val));
   m_stack.popTV();
+  m_stack.popTV();
 
   EventHook::FunctionExit(m_fp);
   ActRec* prevFp = m_fp->arGetSfp();

hphp/runtime/vm/bytecode.h

@@ -569,6 +569,7 @@ class Stack {
   void popX() {
     assert(m_top != m_base);
     assert(!IS_REFCOUNTED_TYPE(m_top->m_type));
+    tvDebugTrash(m_top);
     m_top++;
   }
 
@@ -577,13 +578,15 @@ class Stack {
     assert(m_top != m_base);
     assert(cellIsPlausible(*m_top));
     tvRefcountedDecRefCell(m_top);
+    tvDebugTrash(m_top);
     m_top++;
   }
 
   ALWAYS_INLINE
   void popA() {
     assert(m_top != m_base);
     assert(m_top->m_type == KindOfClass);
+    tvDebugTrash(m_top);
     m_top++;
   }
 
@@ -592,6 +595,7 @@ class Stack {
     assert(m_top != m_base);
     assert(refIsPlausible(*m_top));
     tvDecRefRef(m_top);
+    tvDebugTrash(m_top);
     m_top++;
   }
 
@@ -600,6 +604,7 @@ class Stack {
     assert(m_top != m_base);
     assert(m_top->m_type == KindOfClass || tvIsPlausible(*m_top));
     tvRefcountedDecRef(m_top);
+    tvDebugTrash(m_top);
     m_top++;
   }
 
@@ -616,14 +621,17 @@ class Stack {
     // This should only be used on a pre-live ActRec.
     assert(!ar->hasVarEnv());
     assert(!ar->hasExtraArgs());
-
-    m_top += kNumActRecCells;
-    assert((uintptr_t)m_top <= (uintptr_t)m_base);
+    discardAR();
   }
 
   ALWAYS_INLINE
   void discardAR() {
     assert(m_top != m_base);
+    if (debug) {
+      for (int i = 0; i < kNumActRecCells; ++i) {
+        tvDebugTrash(m_top + i);
+      }
+    }
     m_top += kNumActRecCells;
     assert((uintptr_t)m_top <= (uintptr_t)m_base);
   }
@@ -632,19 +640,30 @@ class Stack {
   void ret() {
     // Leave part of the activation on the stack, since the return value now
     // resides there.
+    if (debug) {
+      for (int i = 0; i < kNumActRecCells - 1; ++i) {
+        tvDebugTrash(m_top + i);
+      }
+    }
     m_top += kNumActRecCells - 1;
     assert((uintptr_t)m_top <= (uintptr_t)m_base);
   }
 
   ALWAYS_INLINE
   void discard() {
     assert(m_top != m_base);
+    tvDebugTrash(m_top);
     m_top++;
   }
 
   ALWAYS_INLINE
   void ndiscard(size_t n) {
     assert((uintptr_t)&m_top[n] <= (uintptr_t)m_base);
+    if (debug) {
+      for (int i = 0; i < n; ++i) {
+        tvDebugTrash(m_top + i);
+      }
+    }
     m_top += n;
   }
 

hphp/runtime/vm/unwind.cpp

@@ -65,10 +65,14 @@ void discardStackTemps(const ActRec* const fp,
         assert(ar->hasThis());
         ar->getThis()->setNoDestruct();
       }
+      FTRACE(2, "  unwind pop AR : {}\n",
+             implicit_cast<void*>(stack.top()));
       stack.popAR();
     },
     [&] (TypedValue* tv) {
       assert(tv == stack.top());
+      FTRACE(2, "  unwind pop TV : {}\n",
+             implicit_cast<void*>(stack.top()));
       stack.popTV();
     }
   );
@@ -150,6 +154,7 @@ void tearDownFrame(ActRec*& fp, Stack& stack, PC& pc, Offset& faultOffset) {
   auto const unwindingGeneratorFrame = func->isGenerator();
   auto const unwindingReturningFrame = curOp == OpRetC || curOp == OpRetV;
   auto const prevFp = fp->arGetSfp();
+  auto const soff = fp->m_soff;
 
   FTRACE(1, "tearDownFrame: {} ({})\n  fp {} prevFp {}\n",
          func->fullName()->data(),
@@ -205,9 +210,16 @@ void tearDownFrame(ActRec*& fp, Stack& stack, PC& pc, Offset& faultOffset) {
     } catch (...) {} // As above, don't let new exceptions out of unwind.
   }
 
+  /*
+   * At the final ActRec in this nesting level.  We don't need to set
+   * pc and fp since we're about to re-throw the exception.  And we
+   * don't want to dereference prefFp since we just popped it.
+   */
+  if (prevFp == fp) return;
+
   assert(stack.isValidAddress(reinterpret_cast<uintptr_t>(prevFp)) ||
          prevFp->m_func->isGenerator());
-  auto const prevOff = fp->m_soff + prevFp->m_func->base();
+  auto const prevOff = soff + prevFp->m_func->base();
   pc = prevFp->m_func->unit()->at(prevOff);
   fp = prevFp;
   faultOffset = prevOff;