diff --git a/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp b/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp index f2048a373..255ead254 100644 --- a/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp +++ b/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp @@ -168,7 +168,7 @@ void HhbcTranslator::setBcOff(Offset newOff, bool lastBcOff) { marker.func = getCurFunc(); marker.stackOff = m_tb->getSpOffset() + m_evalStack.numCells() - m_stackDeficit; - m_tb->gen(Marker, &marker); + m_tb->gen(Marker, marker); } m_lastBcOff = lastBcOff; } @@ -641,7 +641,6 @@ void HhbcTranslator::emitTraitExists(const StringData* traitName) { void HhbcTranslator::emitStaticLocInit(uint32_t locId, uint32_t litStrId) { const StringData* name = lookupStringId(litStrId); - LocalId id(locId); SSATmp* value = popC(); SSATmp* box; @@ -668,7 +667,7 @@ void HhbcTranslator::emitStaticLocInit(uint32_t locId, uint32_t litStrId) { } ); } - m_tb->gen(StLoc, &id, m_tb->getFp(), box); + m_tb->gen(StLoc, LocalId(locId), m_tb->getFp(), box); m_tb->gen(DecRef, value); } @@ -1824,7 +1823,7 @@ void HhbcTranslator::emitSwitch(const ImmVector& iv, SSATmp* stack = spillStack(); m_tb->gen(SyncVMRegs, m_tb->getFp(), stack); - m_tb->gen(JmpSwitchDest, &data, index); + m_tb->gen(JmpSwitchDest, data, index); m_hasExit = true; } @@ -1868,7 +1867,7 @@ void HhbcTranslator::emitSSwitch(const ImmVector& iv) { SSATmp* dest = m_tb->gen(fastPath ? LdSSwitchDestFast : LdSSwitchDestSlow, - &data, + data, testVal); m_tb->genDecRef(testVal); SSATmp* stack = spillStack(); diff --git a/hphp/runtime/vm/translator/hopt/ir.h b/hphp/runtime/vm/translator/hopt/ir.h index 0089b80a8..7251331ca 100644 --- a/hphp/runtime/vm/translator/hopt/ir.h +++ b/hphp/runtime/vm/translator/hopt/ir.h @@ -52,6 +52,8 @@ namespace JIT { using namespace HPHP::VM::Transl; +struct IRInstruction; + class FailedIRGen : public std::exception { public: const char* file; @@ -725,8 +727,8 @@ private: * to the list head. */ struct EdgeData : IRExtraData { - struct IRInstruction* jmp; // owner of this edge - EdgeData* next; // next edge to same target + IRInstruction* jmp; // owner of this edge + EdgeData* next; // next edge to same target }; /* @@ -1654,6 +1656,13 @@ struct IRInstruction { */ void setExtra(IRExtraData* data) { assert(!m_extra); m_extra = data; } + /* + * Clear the extra data pointer in a IRInstruction. Used during + * IRFactory::gen to avoid having dangling IRExtraData*'s into stack + * memory. + */ + void clearExtra() { m_extra = nullptr; } + /* * Replace an instruction in place with a Nop. This sometimes may * be a result of a simplification pass. diff --git a/hphp/runtime/vm/translator/hopt/irfactory.h b/hphp/runtime/vm/translator/hopt/irfactory.h index baa3edfd6..dca50adaf 100644 --- a/hphp/runtime/vm/translator/hopt/irfactory.h +++ b/hphp/runtime/vm/translator/hopt/irfactory.h @@ -18,13 +18,13 @@ #define incl_HPHP_VM_IRFACTORY_H_ #include +#include +#include "folly/ScopeGuard.h" #include "util/arena.h" #include "runtime/vm/translator/hopt/ir.h" #include "runtime/vm/translator/hopt/cse.h" -#include - namespace HPHP { namespace VM { namespace JIT { ////////////////////////////////////////////////////////////////////// @@ -37,7 +37,7 @@ namespace HPHP { namespace VM { namespace JIT { * * Normally IRInstruction creation should go through either * IRFactory::gen or TraceBuilder::gen. This utility is used to - * implement those. + * implement those. The lambda must not escape the IRInstruction*. */ namespace detail { @@ -46,12 +46,26 @@ template struct InstructionBuilder { explicit InstructionBuilder(const Func& func) : func(func) {} - // Create an IRInstruction, and then recursively chew on the Args - // list to populate its fields. + /* + * Create an IRInstruction, and then recursively chew on the Args + * list to populate its fields. + * + * The IRInstruction is stack allocated, and should not escape the + * lambda, so we fill it with 0xc0 in debug builds after we're done. + */ template - Ret go(Opcode op, Args... args) { - IRInstruction inst(op); - return go2(&inst, args...); + Ret go(Opcode op, Args&&... args) { + std::aligned_storage< + sizeof(IRInstruction) + >::type buffer; + void* const vpBuffer = &buffer; + SCOPE_EXIT { if (debug) memset(&buffer, 0xc0, sizeof buffer); }; + + new (vpBuffer) IRInstruction(op); + auto const inst = static_cast(vpBuffer); + + SCOPE_EXIT { inst->clearExtra(); }; + return go2(inst, std::forward(args)...); } private: @@ -59,9 +73,9 @@ private: // until there's no overload for the Head type; then it will fall // through to the base case. template - Ret go2(IRInstruction* inst, Head x, Tail... xs) { + Ret go2(IRInstruction* inst, const Head& x, Tail&&... xs) { setter(inst, x); - return go2(inst, xs...); + return go2(inst, std::forward(xs)...); } // Base cases: either there are no SSATmps, or there's a variadic @@ -76,15 +90,13 @@ private: return stop(inst); } - template - Ret go2(IRInstruction* inst, Int num, SSATmp** ssas) { - inst->initializeSrcs(num, ssas); - return stop(inst); - } - // For each of the optional parameters, there's an overloaded // setter: + void setter(IRInstruction* inst, std::pair ssas) { + inst->initializeSrcs(ssas.first, ssas.second); + } + void setter(IRInstruction* inst, Block* target) { inst->setTaken(target); } @@ -97,8 +109,21 @@ private: inst->setTypeParam(t); } - void setter(IRInstruction* inst, IRExtraData* extra) { - inst->setExtra(extra); + void setter(IRInstruction* inst, const IRExtraData& extra) { + /* + * Taking the address of this temporary seems scary, but actually + * it is safe: `extra' was forwarded in all the way from the + * makeInstruction call, but then we bound a const reference to it + * at go2() when it was the head of the varargs list, so it must + * last until the end of the full-expression that called that + * go2(). + * + * This is long enough for it to outlast our call to func, + * although the transient IRInstruction actually will outlive it. + * We null out the extra data in go() before ~IRInstruction runs, + * though. + */ + inst->setExtra(const_cast(&extra)); } // Finally we end up here. @@ -115,7 +140,7 @@ private: template typename std::result_of::type -makeInstruction(Func func, Args... args) { +makeInstruction(Func func, Args&&... args) { typedef typename std::result_of::type Ret; return detail::InstructionBuilder(func).go(args...); } @@ -139,17 +164,17 @@ public: * * Arguments are passed in the following format: * - * gen(Opcode, [IRExtraData*,] [type param,] [exit label,] [srcs ...]); + * gen(Opcode, [IRExtraData&,] [type param,] [exit label,] [srcs ...]); * * All arguments are optional except the opcode. `srcs' may be * specified either as a list of SSATmp*'s, or as a integer size and * a SSATmp**. */ template - IRInstruction* gen(Args... args) { + IRInstruction* gen(Args&&... args) { return makeInstruction( [this] (IRInstruction* inst) { return inst->clone(this); }, - args... + std::forward(args)... ); } diff --git a/hphp/runtime/vm/translator/hopt/linearscan.cpp b/hphp/runtime/vm/translator/hopt/linearscan.cpp index 4b3622fb7..c0c46cef7 100644 --- a/hphp/runtime/vm/translator/hopt/linearscan.cpp +++ b/hphp/runtime/vm/translator/hopt/linearscan.cpp @@ -505,9 +505,8 @@ void LinearScan::insertAllocFreeSpill(Trace* trace, uint32_t numExtraSpillLocs) void LinearScan::insertAllocFreeSpillAux(Trace* trace, uint32_t numExtraSpillLocs) { - ConstData numSpillConst(numExtraSpillLocs); SSATmp* tmp = m_irFactory->gen(DefConst, Type::Int, - &numSpillConst)->getDst(); + ConstData(numExtraSpillLocs))->getDst(); for (Block* block : trace->getBlocks()) { for (auto it = block->begin(); it != block->end(); ) { @@ -1016,8 +1015,12 @@ void LinearScan::rematerializeAux() { // Rematerialize LdLoc. int loc = findLocal(spilledTmp); if (loc != -1) { - LocalId localId(loc); - newInst = m_irFactory->gen(LdLoc, dst->getType(), &localId, curFp); + newInst = m_irFactory->gen( + LdLoc, + dst->getType(), + LocalId(loc), + curFp + ); } } if (newInst) { diff --git a/hphp/runtime/vm/translator/hopt/simplifier.cpp b/hphp/runtime/vm/translator/hopt/simplifier.cpp index 9e04ec424..a76dd857a 100644 --- a/hphp/runtime/vm/translator/hopt/simplifier.cpp +++ b/hphp/runtime/vm/translator/hopt/simplifier.cpp @@ -436,9 +436,11 @@ SSATmp* Simplifier::simplifyNot(SSATmp* src) { case NInstanceOfBitmask: // TODO: combine this with the above check and use isQueryOp or // add an isNegatable. - return m_tb->gen(negateQueryOp(op), - inst->getNumSrcs(), - inst->getSrcs().begin()); + return m_tb->gen( + negateQueryOp(op), + std::make_pair(inst->getNumSrcs(), inst->getSrcs().begin()) + ); + return nullptr; // TODO !(X | non_zero) --> 0 default: (void)op; } @@ -1390,14 +1392,15 @@ SSATmp* Simplifier::simplifyCondJmp(IRInstruction* inst) { // Fuse jumps with query operators. if (isQueryOp(srcOpcode) && !disableBranchFusion(srcOpcode)) { SrcRange ssas = srcInst->getSrcs(); - return m_tb->gen(queryToJmpOp( - inst->getOpcode() == JmpZero - ? negateQueryOp(srcOpcode) - : srcOpcode), - srcInst->getTypeParam(), // if it had a type param - inst->getTaken(), - ssas.size(), - ssas.begin()); + return m_tb->gen( + queryToJmpOp( + inst->getOpcode() == JmpZero + ? negateQueryOp(srcOpcode) + : srcOpcode), + srcInst->getTypeParam(), // if it had a type param + inst->getTaken(), + std::make_pair(ssas.size(), ssas.begin()) + ); } return nullptr; diff --git a/hphp/runtime/vm/translator/hopt/tracebuilder.cpp b/hphp/runtime/vm/translator/hopt/tracebuilder.cpp index cd1708879..878c4c40e 100644 --- a/hphp/runtime/vm/translator/hopt/tracebuilder.cpp +++ b/hphp/runtime/vm/translator/hopt/tracebuilder.cpp @@ -238,7 +238,7 @@ Trace* TraceBuilder::genExitGuardFailure(uint32_t bcOff) { marker.bcOff = bcOff; marker.stackOff = m_spOffset; marker.func = m_curFunc->getValFunc(); - gen(Marker, &marker); // goes on main trace + gen(Marker, marker); // goes on main trace SSATmp* pc = genDefConst((int64_t)bcOff); // TODO change exit trace to a control flow instruction that @@ -305,7 +305,7 @@ Trace* TraceBuilder::genExitTrace(uint32_t bcOff, marker.bcOff = bcOff; marker.stackOff = m_spOffset + numOpnds - stackDeficit; marker.func = m_curFunc->getValFunc(); - exitTrace->back()->push_back(m_irFactory.gen(Marker, &marker)); + exitTrace->back()->push_back(m_irFactory.gen(Marker, marker)); if (beforeExit) { beforeExit(&m_irFactory, exitTrace); @@ -317,7 +317,11 @@ Trace* TraceBuilder::genExitTrace(uint32_t bcOff, srcs[1] = genDefConst(stackDeficit); std::copy(opnds, opnds + numOpnds, srcs + 2); - auto* spillInst = m_irFactory.gen(SpillStack, numOpnds + 2, srcs); + SSATmp** decayedPtr = srcs; + auto* spillInst = m_irFactory.gen( + SpillStack, + std::make_pair(numOpnds + 2, decayedPtr) + ); sp = spillInst->getDst(); exitTrace->back()->push_back(spillInst); } @@ -369,33 +373,27 @@ SSATmp* TraceBuilder::genNot(SSATmp* src) { } SSATmp* TraceBuilder::genDefUninit() { - ConstData cdata(0); - return gen(DefConst, Type::Uninit, &cdata); + return gen(DefConst, Type::Uninit, ConstData(0)); } SSATmp* TraceBuilder::genDefInitNull() { - ConstData cdata(0); - return gen(DefConst, Type::InitNull, &cdata); + return gen(DefConst, Type::InitNull, ConstData(0)); } SSATmp* TraceBuilder::genDefNull() { - ConstData cdata(0); - return gen(DefConst, Type::Null, &cdata); + return gen(DefConst, Type::Null, ConstData(0)); } SSATmp* TraceBuilder::genPtrToInitNull() { - ConstData cdata(&null_variant); - return gen(DefConst, Type::PtrToUninit, &cdata); + return gen(DefConst, Type::PtrToUninit, ConstData(&null_variant)); } SSATmp* TraceBuilder::genPtrToUninit() { - ConstData cdata(&init_null_variant); - return gen(DefConst, Type::PtrToInitNull, &cdata); + return gen(DefConst, Type::PtrToInitNull, ConstData(&init_null_variant)); } SSATmp* TraceBuilder::genDefNone() { - ConstData cdata(0); - return gen(DefConst, Type::None, &cdata); + return gen(DefConst, Type::None, ConstData(0)); } SSATmp* TraceBuilder::genConvToBool(SSATmp* src) { @@ -436,8 +434,7 @@ SSATmp* TraceBuilder::genJmpCond(SSATmp* boolSrc, Trace* target, bool negate) { } void TraceBuilder::genJmp(Block* target, SSATmp* src) { - EdgeData edge; - gen(Jmp_, target, &edge, src); + gen(Jmp_, target, EdgeData(), src); } void TraceBuilder::genExitWhenSurprised(Trace* targetTrace) { @@ -460,8 +457,7 @@ void TraceBuilder::genGuardLoc(uint32_t id, Type type, Trace* exitTrace) { } Type prevType = getLocalType(id); if (prevType == Type::None) { - LocalId local(id); - gen(GuardLoc, type, getFirstBlock(exitTrace), &local, m_fpValue); + gen(GuardLoc, type, getFirstBlock(exitTrace), LocalId(id), m_fpValue); } else { // It doesn't make sense to be guarding on something that's deemed to fail assert(prevType == type); @@ -477,8 +473,7 @@ void TraceBuilder::genAssertLoc(uint32_t id, Type type, bool overrideType /* =false */) { Type prevType = overrideType ? Type::None : getLocalType(id); if (prevType == Type::None || type.strictSubtypeOf(prevType)) { - LocalId local(id); - gen(AssertLoc, type, &local, m_fpValue); + gen(AssertLoc, type, LocalId(id), m_fpValue); } else { assert(prevType == type || prevType.strictSubtypeOf(type)); } @@ -586,8 +581,7 @@ SSATmp* TraceBuilder::genLdLoc(uint32_t id) { if (type.isNull()) { tmp = genDefConst(type); } else { - LocalId loc(id); - tmp = gen(LdLoc, type, &loc, m_fpValue); + tmp = gen(LdLoc, type, LocalId(id), m_fpValue); } return tmp; } @@ -604,14 +598,12 @@ SSATmp* TraceBuilder::genLdLocAsCell(uint32_t id, Trace* exitTrace) { } SSATmp* TraceBuilder::genLdLocAddr(uint32_t id) { - LocalId baseLocalId(id); - return gen(LdLocAddr, getLocalType(id).ptr(), &baseLocalId, getFp()); + return gen(LdLocAddr, getLocalType(id).ptr(), LocalId(id), getFp()); } void TraceBuilder::genStLocAux(uint32_t id, SSATmp* newValue, bool storeType) { - LocalId locId(id); gen(storeType ? StLoc : StLocNT, - &locId, + LocalId(id), m_fpValue, newValue); } @@ -637,8 +629,7 @@ void TraceBuilder::genDecRefLoc(int id) { bool setNull = id < m_curFunc->getValFunc()->numParams(); SSATmp* val = getLocalValue(id); if (val == nullptr && setNull) { - LocalId locId(id); - val = gen(LdLoc, type, &locId, m_fpValue); + val = gen(LdLoc, type, LocalId(id), m_fpValue); } if (val) { if (setNull) { @@ -655,8 +646,7 @@ void TraceBuilder::genDecRefLoc(int id) { type = Type::BoxedCell; } - LocalId local(id); - gen(DecRefLoc, type, &local, m_fpValue); + gen(DecRefLoc, type, LocalId(id), m_fpValue); } /* @@ -665,12 +655,11 @@ void TraceBuilder::genDecRefLoc(int id) { void TraceBuilder::genBindLoc(uint32_t id, SSATmp* newValue, bool doRefCount /* = true */) { - LocalId locId(id); Type trackedType = getLocalType(id); SSATmp* prevValue = 0; if (trackedType == Type::None) { if (doRefCount) { - prevValue = gen(LdLoc, Type::Gen, &locId, m_fpValue); + prevValue = gen(LdLoc, Type::Gen, LocalId(id), m_fpValue); } } else { prevValue = getLocalValue(id); @@ -684,7 +673,7 @@ void TraceBuilder::genBindLoc(uint32_t id, return; } if (trackedType.maybeCounted() && !prevValue && doRefCount) { - prevValue = gen(LdLoc, trackedType, &locId, m_fpValue); + prevValue = gen(LdLoc, trackedType, LocalId(id), m_fpValue); } } bool genStoreType = true; @@ -731,8 +720,7 @@ SSATmp* TraceBuilder::genStLoc(uint32_t id, // prevRef is a ref if (prevRef == nullptr) { // prevRef = ldLoc - LocalId locId(id); - prevRef = gen(LdLoc, trackedType, &locId, m_fpValue); + prevRef = gen(LdLoc, trackedType, LocalId(id), m_fpValue); } SSATmp* prevValue = nullptr; if (doRefCount) { @@ -1003,7 +991,8 @@ SSATmp* TraceBuilder::genCall(SSATmp* actRec, srcs[1] = genDefConst(returnBcOffset); srcs[2] = func; std::copy(params, params + numParams, srcs + 3); - return gen(Call, numParams + 3, srcs); + SSATmp** decayedPtr = srcs; + return gen(Call, std::make_pair(numParams + 3, decayedPtr)); } SSATmp* TraceBuilder::genCallBuiltin(SSATmp* func, @@ -1013,7 +1002,8 @@ SSATmp* TraceBuilder::genCallBuiltin(SSATmp* func, SSATmp* srcs[numArgs + 1]; srcs[0] = func; std::copy(args, args + numArgs, srcs + 1); - return gen(CallBuiltin, type, numArgs + 1, srcs); + SSATmp** decayedPtr = srcs; + return gen(CallBuiltin, type, std::make_pair(numArgs + 1, decayedPtr)); } void TraceBuilder::genRetVal(SSATmp* val) { @@ -1081,7 +1071,8 @@ SSATmp* TraceBuilder::genSpillStack(uint32_t stackAdjustment, srcs[0] = m_spValue; srcs[1] = genDefConst(stackAdjustment); std::copy(spillOpnds, spillOpnds + numOpnds, srcs + 2); - return gen(SpillStack, numOpnds + 2, srcs); + SSATmp** decayedPtr = srcs; + return gen(SpillStack, std::make_pair(numOpnds + 2, decayedPtr)); } SSATmp* TraceBuilder::genLdStack(int32_t stackOff, Type type) { diff --git a/hphp/runtime/vm/translator/hopt/tracebuilder.h b/hphp/runtime/vm/translator/hopt/tracebuilder.h index 88e6e920f..61573a495 100644 --- a/hphp/runtime/vm/translator/hopt/tracebuilder.h +++ b/hphp/runtime/vm/translator/hopt/tracebuilder.h @@ -67,10 +67,10 @@ public: * IRFactory::gen. */ template - SSATmp* gen(Args... args) { + SSATmp* gen(Args&&... args) { return makeInstruction( [this] (IRInstruction* inst) { return optimizeInst(inst); }, - args... + std::forward(args)... ); } @@ -277,20 +277,17 @@ public: template SSATmp* genDefConst(T val) { - ConstData cdata(val); - return gen(DefConst, typeForConst(val), &cdata); + return gen(DefConst, typeForConst(val), ConstData(val)); } template SSATmp* genDefConst(T val, Type type) { - ConstData cdata(val); - return gen(DefConst, type, &cdata); + return gen(DefConst, type, ConstData(val)); } template SSATmp* genLdConst(T val) { - ConstData cdata(val); - return gen(LdConst, typeForConst(val), &cdata); + return gen(LdConst, typeForConst(val), ConstData(val)); } Trace* getTrace() const { return m_trace.get(); } @@ -508,8 +505,7 @@ private: template<> inline SSATmp* TraceBuilder::genDefConst(Type t) { - ConstData cdata(0); - return gen(DefConst, t, &cdata); + return gen(DefConst, t, ConstData(0)); } }}} diff --git a/hphp/runtime/vm/translator/hopt/vectortranslator.cpp b/hphp/runtime/vm/translator/hopt/vectortranslator.cpp index 17bead8e4..91bca81e6 100644 --- a/hphp/runtime/vm/translator/hopt/vectortranslator.cpp +++ b/hphp/runtime/vm/translator/hopt/vectortranslator.cpp @@ -308,8 +308,7 @@ SSATmp* HhbcTranslator::VectorTranslator::genMisPtr() { if (m_needMIS) { return m_tb.genLdAddr(m_misBase, kReservedRSPSpillSpace); } else { - ConstData cdata(nullptr); - return m_tb.gen(DefConst, Type::PtrToCell, &cdata); + return m_tb.gen(DefConst, Type::PtrToCell, ConstData(nullptr)); } } @@ -559,8 +558,7 @@ void HhbcTranslator::VectorTranslator::emitBaseLCR() { if (baseType.subtypeOf(Type::Uninit)) { if (mia & MIA_warn) { m_ht.spillStack(); - LocalId data(base.location.offset); - m_tb.gen(RaiseUninitLoc, &data); + m_tb.gen(RaiseUninitLoc, LocalId(base.location.offset)); } if (mia & MIA_define) { m_tb.genStLoc(base.location.offset, m_tb.genDefInitNull(),