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 @@ +