From 6b8e5611d66646c78c99d7af5787deb18fbf1cb1 Mon Sep 17 00:00:00 2001 From: Jordan DeLong Date: Wed, 1 May 2013 16:46:34 -0700 Subject: [PATCH] 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 | 17 +- hphp/runtime/vm/bytecode.cpp | 151 ++++++++++++------ hphp/runtime/vm/runtime.h | 14 -- hphp/runtime/vm/translator/hopt/codegen.cpp | 29 +--- hphp/runtime/vm/translator/hopt/codegen.h | 4 +- hphp/runtime/vm/translator/hopt/dce.cpp | 25 --- .../vm/translator/hopt/hhbctranslator.cpp | 20 +-- hphp/runtime/vm/translator/hopt/ir.cpp | 2 - hphp/runtime/vm/translator/hopt/ir.h | 2 - .../vm/translator/hopt/tracebuilder.cpp | 5 +- hphp/runtime/vm/translator/translator-x64.cpp | 2 +- hphp/test/quick/retc-destruct.php.expect | 4 +- hphp/test/quick/surprise_throw.php.expect | 1 - hphp/test/slow/exceptions/retc.php | 27 ++++ hphp/test/slow/exceptions/retc.php.expectf | 1 + 15 files changed, 135 insertions(+), 169 deletions(-) create mode 100644 hphp/test/slow/exceptions/retc.php create mode 100644 hphp/test/slow/exceptions/retc.php.expectf diff --git a/hphp/doc/ir.specification b/hphp/doc/ir.specification index d271098f7..2740f0a0f 100644 --- a/hphp/doc/ir.specification +++ b/hphp/doc/ir.specification @@ -1077,21 +1077,8 @@ DecRefStack 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 diff --git a/hphp/runtime/vm/bytecode.cpp b/hphp/runtime/vm/bytecode.cpp index 18718655d..c60524d16 100644 --- a/hphp/runtime/vm/bytecode.cpp +++ b/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(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(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(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(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(fp->m_func->name()); if (fp->m_func->isGenerator()) { @@ -2390,23 +2430,27 @@ Array VMExecutionContext::debugBacktrace(bool skip /* = false */, funcname = static_cast( 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( - fp->m_func->unit()->filepath()))); + args.append(String(const_cast(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++; } diff --git a/hphp/runtime/vm/runtime.h b/hphp/runtime/vm/runtime.h index 99832b1c3..9538190c4 100644 --- a/hphp/runtime/vm/runtime.h +++ b/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); } } diff --git a/hphp/runtime/vm/translator/hopt/codegen.cpp b/hphp/runtime/vm/translator/hopt/codegen.cpp index 4d62c7780..fc3aa3a2b 100644 --- a/hphp/runtime/vm/translator/hopt/codegen.cpp +++ b/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 - 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, diff --git a/hphp/runtime/vm/translator/hopt/codegen.h b/hphp/runtime/vm/translator/hopt/codegen.h index 3667c92cf..56e1f0665 100644 --- a/hphp/runtime/vm/translator/hopt/codegen.h +++ b/hphp/runtime/vm/translator/hopt/codegen.h @@ -265,9 +265,7 @@ private: void cgDecRefStaticType(Type type, PhysReg dataReg, Block* exit, - bool genZeroCheck, - std::function slowPathWork = - std::function()); + bool genZeroCheck); void cgDecRefDynamicType(PhysReg typeReg, PhysReg dataReg, Block* exit, diff --git a/hphp/runtime/vm/translator/hopt/dce.cpp b/hphp/runtime/vm/translator/hopt/dce.cpp index 325fa141c..4071a4533 100644 --- a/hphp/runtime/vm/translator/hopt/dce.cpp +++ b/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); diff --git a/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp b/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp index 5cbb63713..7ac8f696a 100644 --- a/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp +++ b/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; diff --git a/hphp/runtime/vm/translator/hopt/ir.cpp b/hphp/runtime/vm/translator/hopt/ir.cpp index 41d1f48ae..7eac1ab56 100644 --- a/hphp/runtime/vm/translator/hopt/ir.cpp +++ b/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; diff --git a/hphp/runtime/vm/translator/hopt/ir.h b/hphp/runtime/vm/translator/hopt/ir.h index fb0e0d4be..924c42c43 100644 --- a/hphp/runtime/vm/translator/hopt/ir.h +++ b/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) \ diff --git a/hphp/runtime/vm/translator/hopt/tracebuilder.cpp b/hphp/runtime/vm/translator/hopt/tracebuilder.cpp index 47359bc13..8af81298a 100644 --- a/hphp/runtime/vm/translator/hopt/tracebuilder.cpp +++ b/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; } diff --git a/hphp/runtime/vm/translator/translator-x64.cpp b/hphp/runtime/vm/translator/translator-x64.cpp index b1a20bc99..73c488872 100644 --- a/hphp/runtime/vm/translator/translator-x64.cpp +++ b/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); diff --git a/hphp/test/quick/retc-destruct.php.expect b/hphp/test/quick/retc-destruct.php.expect index ce56a7f7c..63a30c0d8 100644 --- a/hphp/test/quick/retc-destruct.php.expect +++ b/hphp/test/quick/retc-destruct.php.expect @@ -1,4 +1,2 @@ -array(1) { - [0]=> - NULL +array(0) { } diff --git a/hphp/test/quick/surprise_throw.php.expect b/hphp/test/quick/surprise_throw.php.expect index 3b325a68a..4b04251c6 100644 --- a/hphp/test/quick/surprise_throw.php.expect +++ b/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 diff --git a/hphp/test/slow/exceptions/retc.php b/hphp/test/slow/exceptions/retc.php new file mode 100644 index 000000000..bd179170d --- /dev/null +++ b/hphp/test/slow/exceptions/retc.php @@ -0,0 +1,27 @@ +