From c16d3b3536d988c13f3f4c67f85374ebae550d03 Mon Sep 17 00:00:00 2001 From: bsimmers Date: Thu, 25 Jul 2013 11:49:00 -0700 Subject: [PATCH] Rename curUnit() to liveUnit() curUnit and friends are evil and have been the source of many bugs I've had to track down in the region translator. Time to force an audit of all callsites. Most of the uses of curUnit() could be changed to grab the unit from somewhere else, and the few where that wasn't reasonable use liveUnit to make the intent more clear. --- hphp/runtime/vm/bytecode.cpp | 16 ++++---- hphp/runtime/vm/bytecode.h | 5 +++ hphp/runtime/vm/jit/code-gen.h | 1 + hphp/runtime/vm/jit/ir-translator.cpp | 6 +-- hphp/runtime/vm/jit/translator-inline.h | 2 +- .../runtime/vm/jit/translator-x64-helpers.cpp | 4 +- hphp/runtime/vm/jit/translator-x64.cpp | 35 +++++++++-------- hphp/runtime/vm/jit/translator.cpp | 39 +++++++++---------- hphp/runtime/vm/jit/translator.h | 6 +-- 9 files changed, 61 insertions(+), 53 deletions(-) diff --git a/hphp/runtime/vm/bytecode.cpp b/hphp/runtime/vm/bytecode.cpp index 382b9e9a7..391e75139 100644 --- a/hphp/runtime/vm/bytecode.cpp +++ b/hphp/runtime/vm/bytecode.cpp @@ -833,9 +833,11 @@ string Stack::toString(const ActRec* fp, int offset, // Use depth-first recursion to get the output order correct. std::ostringstream os; - os << prefix << "=== Stack at " << curUnit()->filepath()->data() << ":" << - curUnit()->getLineNumber(curUnit()->offsetOf(vmpc())) << " func " << - curFunc()->fullName()->data() << " ===\n"; + auto unit = fp->unit(); + os << prefix << "=== Stack at " + << unit->filepath()->data() << ":" + << unit->getLineNumber(unit->offsetOf(vmpc())) << " func " + << curFunc()->fullName()->data() << " ===\n"; toStringFrame(os, fp, offset, m_top, prefix); @@ -1520,7 +1522,7 @@ void VMExecutionContext::enterVMWork(ActRec* enterFnAr) { } Stats::inc(Stats::VMEnter); if (ThreadInfo::s_threadInfo->m_reqInjectionData.getJit()) { - (void) curUnit()->offsetOf(m_pc); /* assert */ + (void) m_fp->unit()->offsetOf(m_pc); /* assert */ if (enterFnAr) { assert(start); tx()->enterTCAfterProlog(start); @@ -4354,7 +4356,7 @@ inline void OPTBLD_INLINE VMExecutionContext::iopCGetM(PC& pc) { const ImmVector& immVec = ImmVector::createFromStream(oldPC + 1); StringData* name; MemberCode mc; - if (immVec.decodeLastMember(curUnit(), name, mc)) { + if (immVec.decodeLastMember(m_fp->unit(), name, mc)) { recordType(TypeProfileKey(mc, name), m_stack.top()->m_type); } } @@ -5966,9 +5968,9 @@ bool VMExecutionContext::doFCallArray(PC& pc) { reinterpret_cast(tx()->getRetFromInterpretedFrame()); assert(isReturnHelper(ar->m_savedRip)); TRACE(3, "FCallArray: pc %p func %p base %d\n", m_pc, - m_fp->m_func->unit()->entry(), + m_fp->unit()->entry(), int(m_fp->m_func->base())); - ar->m_soff = m_fp->m_func->unit()->offsetOf(pc) + ar->m_soff = m_fp->unit()->offsetOf(pc) - (uintptr_t)m_fp->m_func->base(); assert(pcOff() > m_fp->m_func->base()); diff --git a/hphp/runtime/vm/bytecode.h b/hphp/runtime/vm/bytecode.h index b27a942df..eb3d00ca4 100644 --- a/hphp/runtime/vm/bytecode.h +++ b/hphp/runtime/vm/bytecode.h @@ -319,6 +319,11 @@ struct ActRec { return m_this; } + const Unit* unit() const { + m_func->validate(); + return m_func->unit(); + } + /** * To conserve space, we use unions for pairs of mutually exclusive * fields (fields that are not used at the same time). We use unions diff --git a/hphp/runtime/vm/jit/code-gen.h b/hphp/runtime/vm/jit/code-gen.h index 5720dae22..b08c739d8 100644 --- a/hphp/runtime/vm/jit/code-gen.h +++ b/hphp/runtime/vm/jit/code-gen.h @@ -320,6 +320,7 @@ private: void emitContVarEnvHelperCall(SSATmp* fp, TCA helper); const Func* curFunc() const; Class* curClass() const { return curFunc()->cls(); } + const Unit* curUnit() const { return curFunc()->unit(); } void recordSyncPoint(Asm& as, SyncOptions sync = SyncOptions::kSyncPoint); Address getDtorGeneric(); Address getDtorTyped(); diff --git a/hphp/runtime/vm/jit/ir-translator.cpp b/hphp/runtime/vm/jit/ir-translator.cpp index 9c42f1393..23e1ad84a 100644 --- a/hphp/runtime/vm/jit/ir-translator.cpp +++ b/hphp/runtime/vm/jit/ir-translator.cpp @@ -625,7 +625,7 @@ void IRTranslator::translateCreateCont(const NormalizedInstruction& i) { } void IRTranslator::translateContEnter(const NormalizedInstruction& i) { - auto after = nextSrcKey(i).offset(); + auto after = i.nextSk().offset(); // ContEnter can't exist in an inlined function right now. (If it // ever can, this curFunc() needs to change.) @@ -1327,7 +1327,7 @@ IRTranslator::translateFCall(const NormalizedInstruction& i) { auto const numArgs = i.imm[0].u_IVA; always_assert(!m_hhbcTrans.isInlining() && "curUnit and curFunc calls"); - const PC after = curUnit()->at(nextSrcKey(i).offset()); + const PC after = i.m_unit->at(i.nextSk().offset()); const Func* srcFunc = curFunc(); Offset returnBcOffset = srcFunc->unit()->offsetOf(after - srcFunc->base()); @@ -1370,7 +1370,7 @@ IRTranslator::translateFCall(const NormalizedInstruction& i) { void IRTranslator::translateFCallArray(const NormalizedInstruction& i) { const Offset pcOffset = i.offset(); - SrcKey next = nextSrcKey(i); + SrcKey next = i.nextSk(); const Offset after = next.offset(); HHIR_EMIT(FCallArray, pcOffset, after); diff --git a/hphp/runtime/vm/jit/translator-inline.h b/hphp/runtime/vm/jit/translator-inline.h index 65a7187bc..13ebde774 100644 --- a/hphp/runtime/vm/jit/translator-inline.h +++ b/hphp/runtime/vm/jit/translator-inline.h @@ -44,7 +44,7 @@ inline ActRec*& vmFirstAR() { return g_vmContext->m_firstAR; } inline ActRec* curFrame() { return (ActRec*)vmfp(); } inline const Func* curFunc() { return curFrame()->m_func; } -inline const Unit* curUnit() { return curFunc()->unit(); } +inline const Unit* liveUnit() { return curFunc()->unit(); } inline Class* curClass() { const auto* func = curFunc(); auto* cls = func->cls(); diff --git a/hphp/runtime/vm/jit/translator-x64-helpers.cpp b/hphp/runtime/vm/jit/translator-x64-helpers.cpp index 132957b51..908037315 100644 --- a/hphp/runtime/vm/jit/translator-x64-helpers.cpp +++ b/hphp/runtime/vm/jit/translator-x64-helpers.cpp @@ -225,8 +225,8 @@ void TranslatorX64::fCallArrayHelper(const Offset pcOff, const Offset pcNext) { VMExecutionContext *ec = g_vmContext; ec->m_fp = fp; ec->m_stack.top() = sp; - ec->m_pc = curUnit()->at(pcOff); - PC pc = curUnit()->at(pcNext); + ec->m_pc = fp->unit()->at(pcOff); + PC pc = fp->unit()->at(pcNext); tl_regState = VMRegState::CLEAN; bool runFunc = ec->doFCallArray(pc); diff --git a/hphp/runtime/vm/jit/translator-x64.cpp b/hphp/runtime/vm/jit/translator-x64.cpp index 2ff457b86..fb3372252 100644 --- a/hphp/runtime/vm/jit/translator-x64.cpp +++ b/hphp/runtime/vm/jit/translator-x64.cpp @@ -597,7 +597,7 @@ bool TranslatorX64::profileSrcKey(const SrcKey& sk) const { } TCA TranslatorX64::retranslate(const TranslArgs& args) { - if (isDebuggerAttachedProcess() && isSrcKeyInBL(curUnit(), args.m_sk)) { + if (isDebuggerAttachedProcess() && isSrcKeyInBL(args.m_sk)) { // We are about to translate something known to be blacklisted by // debugger, exit early SKTRACE(1, args.m_sk, "retranslate abort due to debugger\n"); @@ -616,7 +616,7 @@ TCA TranslatorX64::retranslate(const TranslArgs& args) { TCA TranslatorX64::retranslateAndPatchNoIR(SrcKey sk, bool align, TCA toSmash) { - if (isDebuggerAttachedProcess() && isSrcKeyInBL(curUnit(), sk)) { + if (isDebuggerAttachedProcess() && isSrcKeyInBL(sk)) { // We are about to translate something known to be blacklisted by // debugger, exit early SKTRACE(1, sk, "retranslateAndPatchNoIR abort due to debugger\n"); @@ -752,7 +752,7 @@ TranslatorX64::getTranslation(const TranslArgs& args) { curFunc()->validate(); SKTRACE(2, sk, "getTranslation: curUnit %s funcId %x offset %d\n", - curUnit()->filepath()->data(), + sk.unit()->filepath()->data(), sk.getFuncId(), sk.offset()); SKTRACE(2, sk, " funcId: %x \n", @@ -859,7 +859,7 @@ TranslatorX64::createTranslation(const TranslArgs& args) { TCA stubstart = astubs.frontier(); TCA req = emitServiceReq(REQ_RETRANSLATE, sk.offset()); SKTRACE(1, sk, "inserting anchor translation for (%p,%d) at %p\n", - curUnit(), sk.offset(), req); + sk.unit(), sk.offset(), req); SrcRec* sr = m_srcDB.insert(sk); sr->setFuncInfo(curFunc()); sr->setAnchorTranslation(req); @@ -868,7 +868,7 @@ TranslatorX64::createTranslation(const TranslArgs& args) { size_t stubsize = astubs.frontier() - stubstart; assert(asize == 0); if (stubsize && RuntimeOption::EvalDumpTCAnchors) { - addTranslation(TransRec(sk, curUnit()->md5(), TransAnchor, + addTranslation(TransRec(sk, sk.unit()->md5(), TransAnchor, astart, asize, stubstart, stubsize)); if (m_profData) { m_profData->addTransAnchor(sk); @@ -1458,7 +1458,7 @@ static void interp_set_regs(ActRec* ar, Cell* sp, Offset pcOff) { tl_regState = VMRegState::CLEAN; vmfp() = (Cell*)ar; vmsp() = sp; - vmpc() = curUnit()->at(pcOff); + vmpc() = ar->unit()->at(pcOff); } TCA @@ -2362,7 +2362,7 @@ TranslatorX64::enterTC(TCA start, void* data) { // recognizes, or we luck out and the leaseholder exits. while (!start) { TRACE(2, "enterTC forwarding BB to interpreter\n"); - g_vmContext->m_pc = curUnit()->at(sk.offset()); + g_vmContext->m_pc = sk.unit()->at(sk.offset()); INC_TPC(interp_bb); g_vmContext->dispatchBB(); PC newPc = g_vmContext->getPC(); @@ -2550,7 +2550,7 @@ bool TranslatorX64::handleServiceRequest(TReqInfo& info, case REQ_INTERPRET: { Offset off = args[0]; int numInstrs = args[1]; - g_vmContext->m_pc = curUnit()->at(off); + g_vmContext->m_pc = liveUnit()->at(off); /* * We know the compilation unit has not changed; basic blocks do * not span files. I claim even exceptions do not violate this @@ -2610,8 +2610,8 @@ bool TranslatorX64::handleServiceRequest(TReqInfo& info, * need to use fpi regions to find the fcall. */ const FPIEnt* fe = curFunc()->findPrecedingFPI( - curUnit()->offsetOf(vmpc())); - vmpc() = curUnit()->at(fe->m_fcallOff); + liveUnit()->offsetOf(vmpc())); + vmpc() = liveUnit()->at(fe->m_fcallOff); assert(isFCallStar(toOp(*vmpc()))); raise_error("Stack overflow"); NOT_REACHED(); @@ -3163,7 +3163,7 @@ TranslatorX64::reachedTranslationLimit(SrcKey sk, if (debug && Trace::moduleEnabled(Trace::tx64, 2)) { const vector& tns = srcRec.translations(); TRACE(1, "Too many (%zd) translations: %s, BC offset %d\n", - tns.size(), curUnit()->filepath()->data(), + tns.size(), sk.unit()->filepath()->data(), sk.offset()); SKTRACE(2, sk, "{\n"); TCA topTrans = srcRec.getTopTranslation(); @@ -3229,11 +3229,12 @@ void dumpTranslationInfo(const Tracelet& t, TCA postGuards) { if (!debug) return; SrcKey sk = t.m_sk; + DEBUG_ONLY auto unit = sk.unit(); TRACE(3, "----------------------------------------------\n"); TRACE(3, " Translating from file %s:%d %s at %p:\n", - curUnit()->filepath()->data(), - curUnit()->getLineNumber(sk.offset()), + unit->filepath()->data(), + unit->getLineNumber(sk.offset()), curFunc()->name()->data(), postGuards); TRACE(3, " preconds:\n"); @@ -3267,7 +3268,7 @@ void dumpTranslationInfo(const Tracelet& t, TCA postGuards) { // found it since this code is debug-only, and we don't want behavior // to vary across the optimized/debug builds. PC oldPC = vmpc(); - vmpc() = curUnit()->at(sk.offset()); + vmpc() = unit->at(sk.offset()); TRACE(3, g_vmContext->prettyStack(string(" tx64 "))); vmpc() = oldPC; TRACE(3, "----------------------------------------------\n"); @@ -3396,7 +3397,7 @@ TranslatorX64::translateWork(const TranslArgs& args) { } m_pendingFixups.clear(); - addTranslation(TransRec(sk, curUnit()->md5(), transKind, t, start, + addTranslation(TransRec(sk, sk.unit()->md5(), transKind, t, start, a.frontier() - start, stubStart, astubs.frontier() - stubStart, counterStart, counterLen, @@ -4148,7 +4149,7 @@ bool TranslatorX64::addDbgGuards(const Unit* unit) { SrcRec& sr = *it->second; if (sr.unitMd5() == unit->md5() && !sr.hasDebuggerGuard() && - isSrcKeyInBL(unit, sk)) { + isSrcKeyInBL(sk)) { addDbgGuardImpl(sk, sr); } } @@ -4174,7 +4175,7 @@ bool TranslatorX64::addDbgGuard(const Func* func, Offset offset) { } } if (debug) { - if (!isSrcKeyInBL(func->unit(), sk)) { + if (!isSrcKeyInBL(sk)) { TRACE(5, "calling addDbgGuard on PC that is not in blacklist"); return false; } diff --git a/hphp/runtime/vm/jit/translator.cpp b/hphp/runtime/vm/jit/translator.cpp index 5995e64a6..ffd518c46 100755 --- a/hphp/runtime/vm/jit/translator.cpp +++ b/hphp/runtime/vm/jit/translator.cpp @@ -182,7 +182,7 @@ std::string Tracelet::toString() const { } SrcKey Tracelet::nextSk() const { - return m_instrStream.last->source.advanced(curUnit()); + return m_instrStream.last->nextSk(); } /* @@ -529,7 +529,7 @@ predictMVec(const NormalizedInstruction* ni) { auto& immVec = ni->immVec; StringData* name; MemberCode mc; - if (immVec.decodeLastMember(curUnit(), name, mc)) { + if (immVec.decodeLastMember(ni->m_unit, name, mc)) { auto pred = predictType(TypeProfileKey(mc, name)); TRACE(1, "prediction for CGetM %s named %s: %d, %f\n", mc == MET ? "elt" : "prop", @@ -614,7 +614,7 @@ predictOutputs(SrcKey startSk, if (ni->op() == OpClsCnsD) { const NamedEntityPair& cne = curFrame()->m_func->unit()->lookupNamedEntityPairId(ni->imm[1].u_SA); - StringData* cnsName = curUnit()->lookupLitstrId(ni->imm[0].u_SA); + StringData* cnsName = ni->m_unit->lookupLitstrId(ni->imm[0].u_SA); Class* cls = cne.second->getCachedClass(); if (cls) { DataType dt = cls->clsCnsType(cnsName); @@ -750,7 +750,7 @@ getDynLocType(const SrcKey startSk, // If it's a system constant, burn in its type. Otherwise we have // to accept prediction; use the translation-time value, or fall back // to the targetcache if none exists. - StringData *sd = curUnit()->lookupLitstrId(ni->imm[0].u_SA); + StringData *sd = ni->m_unit->lookupLitstrId(ni->imm[0].u_SA); assert(sd); const TypedValue* tv = Unit::lookupPersistentCns(sd); if (tv) { @@ -773,14 +773,14 @@ getDynLocType(const SrcKey startSk, case OutStringImm: { assert(ni->op() == OpString); - StringData *sd = curUnit()->lookupLitstrId(ni->imm[0].u_SA); + StringData *sd = ni->m_unit->lookupLitstrId(ni->imm[0].u_SA); assert(sd); return RuntimeType(sd); } case OutArrayImm: { assert(ni->op() == OpArray); - ArrayData *ad = curUnit()->lookupArrayId(ni->imm[0].u_AA); + ArrayData *ad = ni->m_unit->lookupArrayId(ni->imm[0].u_AA); assert(ad); return RuntimeType(ad); } @@ -2350,7 +2350,7 @@ RuntimeType TraceletContext::currentType(const Location& l) const { if (!mapGet(m_currentMap, l, &dl)) { assert(!mapContains(m_deletedSet, l)); assert(!mapContains(m_changeSet, l)); - return tx64->liveType(l, *curUnit()); + return tx64->liveType(l, *liveUnit()); } return dl->rtt; } @@ -2376,7 +2376,7 @@ DynLocation* TraceletContext::recordRead(const InputInfo& ii, // TODO: Once the region translator supports guard relaxation // (task #2598894), we can enable specialization for all modes. const bool specialize = tx64->mode() == TransLive; - RuntimeType rtt = tx64->liveType(l, *curUnit(), specialize); + RuntimeType rtt = tx64->liveType(l, *liveUnit(), specialize); assert(rtt.isIter() || !rtt.isVagueValue()); // Allocate a new DynLocation to represent this and store it in the // current map. @@ -2488,6 +2488,10 @@ std::string NormalizedInstruction::toString() const { return instrToString((Op*)pc(), unit()); } +SrcKey NormalizedInstruction::nextSk() const { + return source.advanced(m_unit); +} + void Translator::postAnalyze(NormalizedInstruction* ni, SrcKey& sk, Tracelet& t, TraceletContext& tas) { if (ni->op() == OpBareThis && @@ -3045,7 +3049,7 @@ void Translator::analyzeCallee(TraceletContext& tas, NormalizedInstruction* fcall) { auto const callerFunc = curFunc(); auto const fpi = callerFunc->findFPI(fcall->source.offset()); - auto const pushOp = curUnit()->getOpcode(fpi->m_fpushOff); + auto const pushOp = fcall->m_unit->getOpcode(fpi->m_fpushOff); if (!shouldAnalyzeCallee(fcall, fpi, pushOp)) return; @@ -3264,12 +3268,13 @@ static bool instrBreaksProfileBB(const NormalizedInstruction* instr) { std::unique_ptr Translator::analyze(SrcKey sk, const TypeMap& initialTypes) { std::unique_ptr retval(new Tracelet()); + auto unit = sk.unit(); auto& t = *retval; t.m_sk = sk; t.m_func = curFunc(); - DEBUG_ONLY const char* file = curUnit()->filepath()->data(); - DEBUG_ONLY const int lineNum = curUnit()->getLineNumber(t.m_sk.offset()); + DEBUG_ONLY const char* file = unit->filepath()->data(); + DEBUG_ONLY const int lineNum = unit->getLineNumber(t.m_sk.offset()); DEBUG_ONLY const char* funcName = curFunc()->fullName()->data(); TRACE(1, "Translator::analyze %s:%d %s\n", file, lineNum, funcName); @@ -3282,7 +3287,6 @@ std::unique_ptr Translator::analyze(SrcKey sk, t.m_numOpcodes = 0; Unit::MetaHandle metaHand; - const Unit *unit = curUnit(); for (;; sk.advance(unit)) { head: NormalizedInstruction* ni = t.newNormalizedInstruction(); @@ -3563,7 +3567,8 @@ Translator::Get() { } bool -Translator::isSrcKeyInBL(const Unit* unit, const SrcKey& sk) { +Translator::isSrcKeyInBL(const SrcKey& sk) { + auto unit = sk.unit(); Lock l(m_dbgBlacklistLock); if (m_dbgBLSrcKey.find(sk) != m_dbgBLSrcKey.end()) { return true; @@ -3596,12 +3601,6 @@ Translator::addDbgBLPC(PC pc) { return true; } -// Return the SrcKey for the operation that should follow the supplied -// NormalizedInstruction. -SrcKey nextSrcKey(const NormalizedInstruction& i) { - return i.source.advanced(curUnit()); -} - void populateImmediates(NormalizedInstruction& inst) { for (int i = 0; i < numImmediates(inst.op()); i++) { inst.imm[i] = getImm((Op*)inst.pc(), i); @@ -3994,7 +3993,7 @@ void Translator::addTranslation(const TransRec& transRec) { Trace::traceRelease("New translation: %" PRId64 " %s %u %u %d\n", Timer::GetCurrentTimeMicros() - m_createdTime, folly::format("{}:{}:{}", - curUnit()->filepath()->data(), + transRec.src.unit()->filepath()->data(), transRec.src.getFuncId(), transRec.src.offset()).str().c_str(), transRec.aLen, diff --git a/hphp/runtime/vm/jit/translator.h b/hphp/runtime/vm/jit/translator.h index 9ec90238d..70fca3640 100644 --- a/hphp/runtime/vm/jit/translator.h +++ b/hphp/runtime/vm/jit/translator.h @@ -348,6 +348,8 @@ class NormalizedInstruction { return m_dynLocs.back().get(); } + SrcKey nextSk() const; + private: smart::vector::type> m_dynLocs; }; @@ -923,7 +925,7 @@ protected: PCFilter m_dbgBLPC; hphp_hash_set m_dbgBLSrcKey; Mutex m_dbgBlacklistLock; - bool isSrcKeyInBL(const Unit* unit, const SrcKey& sk); + bool isSrcKeyInBL(const SrcKey& sk); TransKind m_mode; ProfData* m_profData; @@ -1074,8 +1076,6 @@ static inline bool isSmartPtrRef(DataType t) { t == KindOfArray || t == KindOfObject; } -SrcKey nextSrcKey(const NormalizedInstruction& i); - void populateImmediates(NormalizedInstruction&); void preInputApplyMetaData(Unit::MetaHandle, NormalizedInstruction*); enum class MetaMode {