Minor cleanup in srcdb.h

Mainly gets rid of the need to pass Asm& a and astubs
to all the patching functions.

hphp/runtime/vm/translator/hopt/codegen.cpp

@@ -2425,9 +2425,9 @@ void CodeGenerator::cgExitTrace(IRInstruction* inst) {
   emitMovRegReg(a, fp->getReg(), rVmFp);
 
   // Get the SrcKey for the dest
-  SrcKey  destSK(func->getValFunc(), pc->getValInt());
+  SrcKey destSK(func->getValFunc(), pc->getValInt());
 
-  switch(exitType) {
+  switch (exitType) {
     case TraceExitType::NormalCc:
       if (toSmash) {
         TCA smashAddr = toSmash->getTCA();
@@ -2510,10 +2510,12 @@ void CodeGenerator::cgExitTrace(IRInstruction* inst) {
         }
       }
       break;
-    case TraceExitType::GuardFailure:
+
+    case TraceExitType::GuardFailure: {
       SrcRec* destSR = m_tx64->getSrcRec(destSK);
       m_tx64->emitFallbackUncondJmp(a, *destSR);
       break;
+    }
   }
 }
 

hphp/runtime/vm/translator/hopt/ir.cpp

@@ -578,7 +578,7 @@ bool cmpOpTypesMayReenter(Opcode op, Type t0, Type t1) {
 
 TraceExitType::ExitType getExitType(Opcode opc) {
   assert(opc >= ExitTrace && opc <= ExitGuardFailure);
-  return (TraceExitType::ExitType)(opc - ExitTrace);
+  return TraceExitType::ExitType(opc - ExitTrace);
 }
 
 Opcode getExitOpcode(TraceExitType::ExitType type) {

hphp/runtime/vm/translator/hopt/ir.h

@@ -975,12 +975,13 @@ enum ExitType {
   NormalCc,
   Slow,
   SlowNoProgress,
-  GuardFailure
+  GuardFailure,
 };
 }
 
-extern TraceExitType::ExitType getExitType(Opcode opc);
-extern Opcode getExitOpcode(TraceExitType::ExitType);
+TraceExitType::ExitType getExitType(Opcode opc);
+Opcode getExitOpcode(TraceExitType::ExitType);
+
 inline bool isExitSlow(TraceExitType::ExitType t) {
   return t == TraceExitType::Slow || t == TraceExitType::SlowNoProgress;
 }

hphp/runtime/vm/translator/srcdb.cpp

@@ -48,35 +48,37 @@ TCA SrcRec::getFallbackTranslation() const {
   return m_anchorTranslation;
 }
 
-void SrcRec::chainFrom(Asm& a, IncomingBranch br) {
+void SrcRec::chainFrom(IncomingBranch br) {
   TCA destAddr = getTopTranslation();
   m_incomingBranches.push_back(br);
-  TRACE(1, "SrcRec(%p)::chainFrom %p -> %p; %zd incoming branches\n",
+  TRACE(1, "SrcRec(%p)::chainFrom %p -> %p (type %d); %zd incoming branches\n",
         this,
-        a.code.frontier, destAddr, m_incomingBranches.size());
-  patch(&a, br, destAddr);
+        br.toSmash(), destAddr, br.type(), m_incomingBranches.size());
+  patch(br, destAddr);
 }
 
-void SrcRec::emitFallbackJump(Asm &a, TCA from, int cc /* = -1 */) {
+void SrcRec::emitFallbackJump(TCA from, int cc /* = -1 */) {
   TCA destAddr = getFallbackTranslation();
-  IncomingBranch incoming(cc < 0 ? IncomingBranch::JMP : IncomingBranch::JCC,
-                          from);
+  auto incoming = cc < 0 ? IncomingBranch::jmpFrom(from)
+                         : IncomingBranch::jccFrom(from);
+  auto& a = tx64->getAsmFor(from);
+
   // emit dummy jump to be smashed via patch()
   if (cc < 0) {
     a.jmp(a.code.frontier);
   } else {
-    assert(incoming.m_type == IncomingBranch::JCC);
+    assert(incoming.type() == IncomingBranch::JCC);
     a.jcc((ConditionCode)cc, a.code.frontier);
   }
 
-  patch(&a, incoming, destAddr);
+  patch(incoming, destAddr);
 
   // We'll need to know the location of this jump later so we can
   // patch it to new translations added to the chain.
   m_inProgressTailJumps.push_back(incoming);
 }
 
-void SrcRec::newTranslation(Asm& a, Asm &astubs, TCA newStart) {
+void SrcRec::newTranslation(TCA newStart) {
   // When translation punts due to hitting limit, will generate one
   // more translation that will call the interpreter.
   assert(m_translations.size() <= kMaxTranslations);
@@ -86,7 +88,7 @@ void SrcRec::newTranslation(Asm& a, Asm &astubs, TCA newStart) {
   m_translations.push_back(newStart);
   if (!m_topTranslation) {
     atomic_release_store(&m_topTranslation, newStart);
-    patchIncomingBranches(a, astubs, newStart);
+    patchIncomingBranches(newStart);
   }
 
   /*
@@ -104,8 +106,7 @@ void SrcRec::newTranslation(Asm& a, Asm &astubs, TCA newStart) {
    * translation possibly for this same situation.)
    */
   for (size_t i = 0; i < m_tailFallbackJumps.size(); ++i) {
-    auto& as = asmChoose(m_tailFallbackJumps[i].m_src, a, astubs);
-    patch(&as, m_tailFallbackJumps[i], newStart);
+    patch(m_tailFallbackJumps[i], newStart);
   }
 
   // This is the new tail translation, so store the fallback jump list
@@ -114,15 +115,14 @@ void SrcRec::newTranslation(Asm& a, Asm &astubs, TCA newStart) {
   m_inProgressTailJumps.clear();
 }
 
-void SrcRec::addDebuggerGuard(Asm& a, Asm &astubs, TCA dbgGuard,
-                              TCA dbgBranchGuardSrc) {
+void SrcRec::addDebuggerGuard(TCA dbgGuard, TCA dbgBranchGuardSrc) {
   assert(!m_dbgBranchGuardSrc);
 
   TRACE(1, "SrcRec(%p)::addDebuggerGuard @%p, "
         "%zd incoming branches to rechain\n",
         this, dbgGuard, m_incomingBranches.size());
 
-  patchIncomingBranches(a, astubs, dbgGuard);
+  patchIncomingBranches(dbgGuard);
 
   // Set m_dbgBranchGuardSrc after patching, so we don't try to patch
   // the debug guard.
@@ -130,12 +130,14 @@ void SrcRec::addDebuggerGuard(Asm& a, Asm &astubs, TCA dbgGuard,
   atomic_release_store(&m_topTranslation, dbgGuard);
 }
 
-void SrcRec::patchIncomingBranches(Asm& a, Asm &astubs, TCA newStart) {
+void SrcRec::patchIncomingBranches(TCA newStart) {
   if (hasDebuggerGuard()) {
     // We have a debugger guard, so all jumps to us funnel through
     // this.  Just smash m_dbgBranchGuardSrc.
     TRACE(1, "smashing m_dbgBranchGuardSrc @%p\n", m_dbgBranchGuardSrc);
-    TranslatorX64::smashJmp(a, m_dbgBranchGuardSrc, newStart);
+    TranslatorX64::smashJmp(tx64->getAsmFor(m_dbgBranchGuardSrc),
+                            m_dbgBranchGuardSrc,
+                            newStart);
     return;
   }
 
@@ -144,46 +146,62 @@ void SrcRec::patchIncomingBranches(Asm& a, Asm &astubs, TCA newStart) {
   vector<IncomingBranch>& change = m_incomingBranches;
   for (unsigned i = 0; i < change.size(); ++i) {
     TRACE(1, "SrcRec(%p)::newTranslation rechaining @%p -> %p\n",
-          this, change[i].m_src, newStart);
-    Asm *as = change[i].m_type == IncomingBranch::ADDR ?
-      nullptr : &asmChoose(change[i].m_src, a, astubs);
-    patch(as, change[i], newStart);
+          this, change[i].toSmash(), newStart);
+    patch(change[i], newStart);
   }
 }
 
-void SrcRec::replaceOldTranslations(Asm& a, Asm& astubs) {
+void SrcRec::replaceOldTranslations() {
   // Everyone needs to give up on old translations; send them to the anchor,
-  // which is a REQ_RETRANSLATE
+  // which is a REQ_RETRANSLATE.
   m_translations.clear();
   m_tailFallbackJumps.clear();
   atomic_release_store(&m_topTranslation, static_cast<TCA>(0));
-  patchIncomingBranches(a, astubs, m_anchorTranslation);
+
+  /*
+   * It may seem a little weird that we're about to point every
+   * incoming branch at the anchor, since that's going to just
+   * unconditionally retranslate this SrcKey and never patch the
+   * incoming branch to do something else.
+   *
+   * The reason this is ok is this mechanism is only used in
+   * non-RepoAuthoritative mode, and the granularity of code
+   * invalidation there is such that we'll only have incoming branches
+   * like this basically within the same file since we don't have
+   * whole program analysis.
+   *
+   * This means all these incoming branches are about to go away
+   * anyway ...
+   *
+   * If we ever change that we'll have to change this to patch to
+   * some sort of rebind requests.
+   */
+  assert(!RuntimeOption::RepoAuthoritative);
+  patchIncomingBranches(m_anchorTranslation);
 }
 
-void SrcRec::patch(Asm* a, IncomingBranch branch, TCA dest) {
-  if (branch.m_type == IncomingBranch::ADDR) {
-    // Note that this effectively ignores a
-    atomic_release_store(branch.m_addr, dest);
-    return;
+void SrcRec::patch(IncomingBranch branch, TCA dest) {
+  switch (branch.type()) {
+  case IncomingBranch::JMP: {
+    auto& a = tx64->getAsmFor(branch.toSmash());
+    CodeCursor cg(a, branch.toSmash());
+    TranslatorX64::smashJmp(a, branch.toSmash(), dest);
+    break;
   }
 
-  // modifying reachable code
-  switch(branch.m_type) {
-    case IncomingBranch::JMP: {
-      CodeCursor cg(*a, branch.m_src);
-      TranslatorX64::smashJmp(*a, branch.m_src, dest);
-      break;
-    }
-    case IncomingBranch::JCC: {
-      // patch destination, but preserve the condition code
-      int32_t delta = safe_cast<int32_t>((dest - branch.m_src) -
-                                         TranslatorX64::kJmpccLen);
-      int32_t* addr = (int32_t*)(branch.m_src + TranslatorX64::kJmpccLen - 4);
-      atomic_release_store(addr, delta);
-      break;
-    }
-    default:
-      not_implemented();
+  case IncomingBranch::JCC: {
+    // patch destination, but preserve the condition code
+    int32_t delta = safe_cast<int32_t>((dest - branch.toSmash()) -
+                                       TranslatorX64::kJmpccLen);
+    int32_t* addr = (int32_t*)(branch.toSmash() +
+      TranslatorX64::kJmpccLen - 4);
+    atomic_release_store(addr, delta);
+    break;
+  }
+
+  case IncomingBranch::ADDR:
+    // Note that this effectively ignores a
+    atomic_release_store(reinterpret_cast<TCA*>(branch.toSmash()), dest);
   }
 }
 

hphp/runtime/vm/translator/srcdb.h

@@ -28,31 +28,49 @@ namespace HPHP {
 namespace VM {
 namespace Transl {
 
+/*
+ * Incoming branches between different translations are tracked using
+ * this structure.
+ *
+ * This allows us to smash them later to point to different things.
+ * We handle conditional and unconditional jumps, as well as pointers
+ * to code (via IncomingBranch::ADDR, used for example in a switch
+ * table).
+ *
+ * We don't need to track which condition code a conditional jump used
+ * because we take care to smash only the address and leave the code
+ * intact.
+ */
 struct IncomingBranch {
   enum BranchType {
     JMP,
     JCC,
     ADDR,
   };
-  IncomingBranch(BranchType t, TCA src)
-  : m_type(t), m_src(src) { }
-  IncomingBranch(TCA src)
-  : m_type(JMP), m_src(src) { }
-  IncomingBranch(TCA* addr)
-  : m_type(ADDR), m_addr(addr) { }
+
+  static IncomingBranch jmpFrom(TCA from) { return IncomingBranch(JMP, from); }
+  static IncomingBranch jccFrom(TCA from) { return IncomingBranch(JCC, from); }
+  static IncomingBranch addr(TCA* from) {
+    return IncomingBranch(ADDR, TCA(from));
+  }
+
+  BranchType type() const { return m_type; }
+  TCA toSmash()     const { return m_toSmash; }
+
+private:
+  explicit IncomingBranch(BranchType type, TCA toSmash)
+    : m_type(type)
+    , m_toSmash(toSmash)
+  {}
 
   BranchType m_type;
-  union {
-    TCA m_src;
-    TCA* m_addr;
-  };
+  TCA m_toSmash;
 };
 
 /*
  * SrcRec: record of translator output for a given source location.
  */
 struct SrcRec {
-  typedef X64Assembler Asm;
   static const unsigned int kMaxTranslations = 12;
 
   SrcRec()
@@ -79,12 +97,11 @@ struct SrcRec {
    * when holding the translator write lease.
    */
   void setFuncInfo(const Func* f);
-  void chainFrom(Asm& a, IncomingBranch br);
-  void emitFallbackJump(Asm &a, TCA from, int cc = -1);
-  void newTranslation(Asm& a, Asm &astubs, TCA newStart);
-  void replaceOldTranslations(Asm& a, Asm& astubs);
-  void addDebuggerGuard(Asm& a, Asm &astubs, TCA dbgGuard,
-                        TCA m_dbgBranchGuardSrc);
+  void chainFrom(IncomingBranch br);
+  void emitFallbackJump(TCA from, int cc = -1);
+  void newTranslation(TCA newStart);
+  void replaceOldTranslations();
+  void addDebuggerGuard(TCA dbgGuard, TCA m_dbgBranchGuardSrc);
   bool hasDebuggerGuard() const { return m_dbgBranchGuardSrc != nullptr; }
   const MD5& unitMd5() const { return m_unitMd5; }
 
@@ -92,6 +109,10 @@ struct SrcRec {
     return m_translations;
   }
 
+  /*
+   * The anchor translation is a retranslate request for the current
+   * SrcKey that will continue the tracelet chain.
+   */
   void setAnchorTranslation(TCA anc) {
     assert(!m_anchorTranslation);
     assert(m_tailFallbackJumps.empty());
@@ -108,8 +129,8 @@ struct SrcRec {
 
 private:
   TCA getFallbackTranslation() const;
-  void patch(Asm* a, IncomingBranch branch, TCA dest);
-  void patchIncomingBranches(Asm& a, Asm& astubs, TCA newStart);
+  void patch(IncomingBranch branch, TCA dest);
+  void patchIncomingBranches(TCA newStart);
 
 private:
   // This either points to the most recent translation in the

hphp/runtime/vm/translator/translator-x64.cpp

@@ -1223,10 +1223,7 @@ asm_label(a, release);
         size_t(a.code.frontier - m_dtorGenericStub));
 }
 
-/*
- * Translation call targets. It is a lot easier, and a bit more
- * portable, to use C linkage from assembly.
- */
+
 TCA TranslatorX64::retranslate(SrcKey sk, bool align, bool allowIR) {
   if (isDebuggerAttachedProcess() && isSrcKeyInBL(curUnit(), sk)) {
     // We are about to translate something known to be blacklisted by
@@ -2560,12 +2557,11 @@ TranslatorX64::bindJmp(TCA toSmash, SrcKey destSk,
   smashed = true;
   SrcRec* sr = getSrcRec(destSk);
   if (req == REQ_BIND_ADDR) {
-    sr->chainFrom(a, IncomingBranch((TCA*)toSmash));
+    sr->chainFrom(IncomingBranch::addr(reinterpret_cast<TCA*>(toSmash)));
   } else if (req == REQ_BIND_JCC) {
-    sr->chainFrom(getAsmFor(toSmash),
-                  IncomingBranch(IncomingBranch::JCC, toSmash));
+    sr->chainFrom(IncomingBranch::jccFrom(toSmash));
   } else {
-    sr->chainFrom(getAsmFor(toSmash), IncomingBranch(toSmash));
+    sr->chainFrom(IncomingBranch::jmpFrom(toSmash));
   }
   return tDest;
 }
@@ -2616,7 +2612,7 @@ TranslatorX64::bindJmpccFirst(TCA toSmash,
     emitServiceReq(SRFlags::SRNone, REQ_BIND_JMPCC_SECOND, 3,
                    toSmash, uint64_t(offWillDefer), uint64_t(cc));
 
-  Asm &as = getAsmFor(toSmash);
+  Asm& as = getAsmFor(toSmash);
   // Its not clear where chainFrom should go to if as is astubs
   assert(&as != &astubs);
 
@@ -2647,7 +2643,7 @@ TranslatorX64::bindJmpccFirst(TCA toSmash,
    */
   CodeCursor cg(as, toSmash);
   as.jcc(cc, stub);
-  getSrcRec(dest)->chainFrom(as, IncomingBranch(as.code.frontier));
+  getSrcRec(dest)->chainFrom(IncomingBranch::jmpFrom(as.code.frontier));
   TRACE(5, "bindJmpccFirst: overwrote with cc%02x taken %d\n", cc, taken);
   return tDest;
 }
@@ -2663,8 +2659,7 @@ TranslatorX64::bindJmpccSecond(TCA toSmash, const Offset off,
   if (branch && writer.acquire()) {
     smashed = true;
     SrcRec* destRec = getSrcRec(dest);
-    destRec->chainFrom(getAsmFor(toSmash),
-                       IncomingBranch(IncomingBranch::JCC, toSmash));
+    destRec->chainFrom(IncomingBranch::jccFrom(toSmash));
   }
   return branch;
 }
@@ -2824,19 +2819,19 @@ void
 TranslatorX64::emitFallbackJmp(Asm& as, SrcRec& dest,
                                ConditionCode cc /* = CC_NZ */) {
   prepareForSmash(as, kJmpccLen);
-  dest.emitFallbackJump(as, as.code.frontier, cc);
+  dest.emitFallbackJump(as.code.frontier, cc);
 }
 
 void
 TranslatorX64::emitFallbackUncondJmp(Asm& as, SrcRec& dest) {
   prepareForSmash(as, kJmpLen);
-  dest.emitFallbackJump(as, as.code.frontier);
+  dest.emitFallbackJump(as.code.frontier);
 }
 
 void
 TranslatorX64::emitFallbackCondJmp(Asm& as, SrcRec& dest, ConditionCode cc) {
   prepareForSmash(as, kJmpccLen);
-  dest.emitFallbackJump(as, as.code.frontier, cc);
+  dest.emitFallbackJump(as.code.frontier, cc);
 }
 
 void TranslatorX64::emitReqRetransNoIR(Asm& as, SrcKey& sk) {
@@ -3257,7 +3252,8 @@ bool TranslatorX64::handleServiceRequest(TReqInfo& info,
   case REQ_BIND_JMP:
   case REQ_BIND_JCC:
   case REQ_BIND_JMP_NO_IR:
-  case REQ_BIND_ADDR: {
+  case REQ_BIND_ADDR:
+  {
     TCA toSmash = (TCA)args[0];
     Offset off = args[1];
     sk = SrcKey(curFunc(), off);
@@ -3296,7 +3292,7 @@ bool TranslatorX64::handleServiceRequest(TReqInfo& info,
       if (writer) {
         smashed = true;
         SrcRec* sr = getSrcRec(sk);
-        sr->chainFrom(a, IncomingBranch(&rlsa->m_pseudoMain));
+        sr->chainFrom(IncomingBranch::addr(&rlsa->m_pseudoMain));
       }
     }
   } break;
@@ -11406,7 +11402,7 @@ TranslatorX64::translateTracelet(SrcKey sk, bool considerHHIR/*=true*/,
   // metadata is not yet visible.
   TRACE(1, "newTranslation: %p  sk: (func %d, bcOff %d)\n",
       start, sk.getFuncId(), sk.m_offset);
-  srcRec.newTranslation(a, astubs, start);
+  srcRec.newTranslation(start);
   TRACE(1, "tx64: %zd-byte tracelet\n", a.code.frontier - start);
   if (Trace::moduleEnabledRelease(Trace::tcspace, 1)) {
     Trace::traceRelease(getUsage().c_str());
@@ -12177,7 +12173,7 @@ void TranslatorX64::addDbgGuardImpl(SrcKey sk, SrcRec& srcRec) {
   TCA dbgBranchGuardSrc = a.code.frontier;
   a.   jmp(realCode);
   // Add it to srcRec
-  srcRec.addDebuggerGuard(a, astubs, dbgGuard, dbgBranchGuardSrc);
+  srcRec.addDebuggerGuard(dbgGuard, dbgBranchGuardSrc);
 }
 
 bool TranslatorX64::dumpTCCode(const char* filename) {
@@ -12367,7 +12363,7 @@ void TranslatorX64::invalidateSrcKey(SrcKey sk) {
    * just created some garbage in the TC. We currently have no mechanism
    * to reclaim this.
    */
-  sr->replaceOldTranslations(a, astubs);
+  sr->replaceOldTranslations();
 }
 
 void TranslatorX64::invalidateFileWork(Eval::PhpFile* f) {