From 51574fb2ec2732373b74f01b71e8b9bd212cfeec Mon Sep 17 00:00:00 2001
From: Jordan DeLong <jdelong@fb.com>
Date: Wed, 5 Jun 2013 16:21:36 -0700
Subject: [PATCH] Fix an assembler bug with FPI region offsets

The assembler recorded the offset to FPI regions based on the
stack depth before the instruction executed.  This is trivially wrong
for instructions like FPushCtorD, but also breaks in nested FPI
regions.  The assembler is tracking fdescDepth independently of the
eval stack, so as long as we continue to require that all FPush* ops
logically push the ActRec *after* all their other pops and pushes,
this fix should work.  It might be nice to some day reword the
bytecode.specification stuff to include ActRecs as part of the eval
stack, though, so it isn't ambigious which order they happen for the
FPIEnt offsets.
---
 hphp/runtime/vm/as.cpp | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hphp/runtime/vm/as.cpp b/hphp/runtime/vm/as.cpp
index f61a1a26e..5cc1c1924 100644
--- a/hphp/runtime/vm/as.cpp
+++ b/hphp/runtime/vm/as.cpp
@@ -541,16 +541,16 @@ struct AsmState : private boost::noncopyable {
     labelMap[label].stackDepth.setBase(*this, 0);
   }
 
-  void beginFpi() {
+  void beginFpi(Offset fpushOff) {
     if (currentStackDepth == nullptr) {
       error("beginFpi called from unreachable instruction");
     }
 
     fpiRegs.push_back(FPIReg());
     FPIReg& fpi = fpiRegs.back();
-    fpi.fpushOff = ue->bcPos();
+    fpi.fpushOff = fpushOff;
     fpi.stackDepth = currentStackDepth;
-    fpi.fpOff = currentStackDepth->currentOffset;
+    fpi.fpOff = currentStackDepth->currentOffset + fdescDepth;
     fdescDepth += kNumActRecCells;
     fdescHighWater = std::max(fdescDepth, fdescHighWater);
   }
@@ -562,7 +562,6 @@ struct AsmState : private boost::noncopyable {
     FPIReg& reg = fpiRegs.back();
     ent.m_fpushOff = reg.fpushOff;
     ent.m_fcallOff = ue->bcPos();
-
     ent.m_fpOff = reg.fpOff;
     if (reg.stackDepth->baseValue) {
       ent.m_fpOff += reg.stackDepth->baseValue.get();
@@ -1041,11 +1040,10 @@ OpcodeParserMap opcode_parsers;
       #name                                                       \
     );                                                            \
                                                                   \
-    if (isFPush(Op##name)) {                                      \
-      as.beginFpi();                                              \
-    } else if (isFCallStar(Op##name)) {                           \
+    if (isFCallStar(Op##name)) {                                  \
       as.endFpi();                                                \
     }                                                             \
+                                                                  \
     as.ue->emitOp(Op##name);                                      \
                                                                   \
     IMM_##imm;                                                    \
@@ -1053,6 +1051,10 @@ OpcodeParserMap opcode_parsers;
     int stackDelta = NUM_PUSH_##push - NUM_POP_##pop;             \
     as.adjustStack(stackDelta);                                   \
                                                                   \
+    if (isFPush(Op##name)) {                                      \
+      as.beginFpi(curOpcodeOff);                                  \
+    }                                                             \
+                                                                  \
     for (auto& kv : labelJumps) {                                 \
       as.addLabelJump(kv.first, kv.second, curOpcodeOff);         \
     }                                                             \