From b387d70e70cb15dea5555d1ce5e66d16dfa066db Mon Sep 17 00:00:00 2001 From: mwilliams Date: Thu, 21 Mar 2013 19:45:37 -0700 Subject: [PATCH] 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 | 8 ++++ hphp/runtime/vm/translator/hopt/codegen.cpp | 42 +++++++++++++------ hphp/runtime/vm/translator/hopt/codegen.h | 6 ++- hphp/runtime/vm/translator/hopt/ir.cpp | 2 +- hphp/runtime/vm/translator/hopt/ir.h | 2 +- .../vm/translator/hopt/tracebuilder.cpp | 2 +- hphp/test/vm/actrec.php | 42 +++++++++++++++++++ hphp/test/vm/actrec.php.exp | 22 ++++++++++ 8 files changed, 108 insertions(+), 18 deletions(-) create mode 100644 hphp/test/vm/actrec.php create mode 100644 hphp/test/vm/actrec.php.exp diff --git a/hphp/doc/ir.specification b/hphp/doc/ir.specification index 15fb93bcc..e883267b8 100644 --- a/hphp/doc/ir.specification +++ b/hphp/doc/ir.specification @@ -977,6 +977,14 @@ DecRefStack 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 diff --git a/hphp/runtime/vm/translator/hopt/codegen.cpp b/hphp/runtime/vm/translator/hopt/codegen.cpp index b3cf77b71..ee81f05f4 100644 --- a/hphp/runtime/vm/translator/hopt/codegen.cpp +++ b/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 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, diff --git a/hphp/runtime/vm/translator/hopt/codegen.h b/hphp/runtime/vm/translator/hopt/codegen.h index 0f60b580e..8357989f9 100644 --- a/hphp/runtime/vm/translator/hopt/codegen.h +++ b/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 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 slowPathWork = + std::function()); void cgDecRefDynamicType(PhysReg typeReg, PhysReg dataReg, Block* exit, diff --git a/hphp/runtime/vm/translator/hopt/ir.cpp b/hphp/runtime/vm/translator/hopt/ir.cpp index 491b70c1b..ecd84f1db 100644 --- a/hphp/runtime/vm/translator/hopt/ir.cpp +++ b/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; } diff --git a/hphp/runtime/vm/translator/hopt/ir.h b/hphp/runtime/vm/translator/hopt/ir.h index 654456b8a..e37c940dc 100644 --- a/hphp/runtime/vm/translator/hopt/ir.h +++ b/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 ////////////////////////////////////////////////////////////////////// diff --git a/hphp/runtime/vm/translator/hopt/tracebuilder.cpp b/hphp/runtime/vm/translator/hopt/tracebuilder.cpp index 272753e9e..dd1d62529 100644 --- a/hphp/runtime/vm/translator/hopt/tracebuilder.cpp +++ b/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); } diff --git a/hphp/test/vm/actrec.php b/hphp/test/vm/actrec.php new file mode 100644 index 000000000..342144059 --- /dev/null +++ b/hphp/test/vm/actrec.php @@ -0,0 +1,42 @@ +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; diff --git a/hphp/test/vm/actrec.php.exp b/hphp/test/vm/actrec.php.exp new file mode 100644 index 000000000..4401992a1 --- /dev/null +++ b/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"