Cleanup MethCaller stuff in Func

Summary:
- I can't find a reason that we need Func::kMethCallerBit when we already is have isMethCaller that uses Attrs.
- Instead of implementing LowPtr stuff again just use LowPtr and AtomicLowPtr.

Reviewed By: ricklavoie

Differential Revision: D78199925

fbshipit-source-id: 62eec407a012c84aa3d0793a040cf942f27f7a93

Author: WizKid

hphp/runtime/vm/func-inl.h

@@ -146,16 +146,15 @@ inline Unit* Func::unit() const {
 }
 
 inline Class* Func::cls() const {
-  return !isMethCaller() ? m_u.cls() : nullptr;
+  return !isMethCaller() ? clsData().m_impl : nullptr;
 }
 
 inline PreClass* Func::preClass() const {
   return shared()->m_preClass;
 }
 
 inline Class* Func::baseCls() const {
-  return !(m_baseCls & kMethCallerBit) ?
-    reinterpret_cast<Class*>(m_baseCls) : nullptr;
+  return !isMethCaller() ? clsData().m_base : nullptr;
 }
 
 inline Class* Func::implCls() const {
@@ -216,27 +215,26 @@ inline void invalidFuncConversion(const char* type) {
 
 inline NamedFunc* Func::getNamedFunc() {
   assertx(!shared()->m_preClass);
-  return *reinterpret_cast<LowPtr<NamedFunc>*>(&m_namedFunc);
+  return const_cast<NamedFunc*>(m_namedFunc.get());
 }
 
 inline const NamedFunc* Func::getNamedFunc() const {
   assertx(!shared()->m_preClass);
-  return *reinterpret_cast<const LowPtr<const NamedFunc>*>(&m_namedFunc);
+  return m_namedFunc;
 }
 
 inline void Func::setNamedFunc(const NamedFunc* e) {
-  *reinterpret_cast<LowPtr<const NamedFunc>*>(&m_namedFunc) = e;
+  m_namedFunc = e;
 }
 
 inline const StringData* Func::methCallerClsName() const {
   assertx(isMethCaller() && isBuiltin());
-  return m_u.name();
+  return methCallerData().m_clsName;
 }
 
 inline const StringData* Func::methCallerMethName() const {
-  assertx(isMethCaller() && isBuiltin() &&
-          (m_methCallerMethName & kMethCallerBit));
-  return reinterpret_cast<StringData*>(m_methCallerMethName - kMethCallerBit);
+  assertx(isMethCaller() && isBuiltin());
+  return methCallerData().m_methName;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -855,7 +853,7 @@ inline void Func::setAttrs(Attr attrs) {
 }
 
 inline void Func::setBaseCls(Class* baseCls) {
-  m_baseCls = to_low(baseCls);
+  clsData().m_base = baseCls;
 }
 
 inline void Func::setHasPrivateAncestor(bool b) {

hphp/runtime/vm/func.cpp

@@ -122,8 +122,6 @@ Func::Func(
   Unit& unit, const StringData* name, Attr attrs,
   const StringData *methCallerCls, const StringData *methCallerMeth)
   : m_name(name)
-  , m_methCallerMethName(to_low(methCallerMeth, kMethCallerBit))
-  , m_u(methCallerCls)
   , m_isPreFunc(false)
   , m_hasPrivateAncestor(false)
   , m_shouldSampleJit(StructuredLog::coinflip(Cfg::Jit::SampleRate))
@@ -135,6 +133,8 @@ Func::Func(
 {
   assertx(methCallerCls != nullptr);
   assertx(methCallerMeth != nullptr);
+  methCallerData().m_methName = methCallerMeth;
+  methCallerData().m_clsName = methCallerCls;
 }
 
 Func::~Func() {
@@ -229,7 +229,7 @@ Func* Func::clone(Class* cls, const StringData* name) const {
   f->m_cloned.flag.test_and_set();
   f->initProloguesAndFuncEntry(numParams);
   if (name) f->m_name = name;
-  f->m_u.setCls(cls);
+  f->clsData().m_impl = cls;
   f->setFullName(numParams);
 
   if (f != this) {
@@ -246,7 +246,7 @@ Func* Func::clone(Class* cls, const StringData* name) const {
 }
 
 void Func::rescope(Class* ctx) {
-  m_u.setCls(ctx);
+  clsData().m_impl = ctx;
   setFullName(numParams());
 }
 

hphp/runtime/vm/func.h

@@ -1275,16 +1275,19 @@ struct Func final {
   OFF(inoutBits)
   OFF(shared)
   OFF(unit)
-  OFF(methCallerMethName)
   OFF(funcEntry)
 #undef OFF
 
   static constexpr ptrdiff_t clsOff() {
-    return offsetof(Func, m_u);
+    return offsetof(Func, m_u.m_cls.m_impl);
+  }
+
+  static constexpr ptrdiff_t methCallerMethNameOff() {
+    return offsetof(Func, m_u.m_methCaller.m_methName);
   }
 
   static constexpr ptrdiff_t methCallerClsNameOff() {
-    return offsetof(Func, m_u);
+    return offsetof(Func, m_u.m_methCaller.m_clsName);
   }
 
   static constexpr ptrdiff_t sharedAllFlags() {
@@ -1595,79 +1598,10 @@ struct Func final {
   };
 
 public:
-#ifdef USE_LOWPTR
-  using low_storage_t = uint32_t;
-#else
-  using low_storage_t = uintptr_t;
-#endif
-
-private:
-  /*
-   * Lowptr wrapper around std::atomic<Union> for Class* or StringData*
-   */
-  struct UnionWrapper {
-    union U {
-     low_storage_t m_cls;
-     low_storage_t m_methCallerClsName;
-    };
-    std::atomic<U> m_u;
-
-    // constructors
-    explicit UnionWrapper(Class *cls)
-      : m_u([](Class *cls){
-        U u;
-        u.m_cls = to_low(cls);
-        return u; }(cls)) {}
-    explicit UnionWrapper(const StringData *name)
-      : m_u([](const StringData *n){
-        U u;
-        u.m_methCallerClsName = to_low(n, kMethCallerBit);
-        return u; }(name)) {}
-    /* implicit */ UnionWrapper(std::nullptr_t /*px*/)
-      : m_u([](){
-        U u;
-        u.m_cls = 0;
-        return u; }()) {}
-    UnionWrapper(const UnionWrapper& r) :
-      m_u(r.m_u.load()) {
-    }
-
-    // Assignments
-    UnionWrapper& operator=(UnionWrapper r) {
-      m_u.store(r.m_u, std::memory_order_release);
-      return *this;
-    }
-
-    // setter & getter
-    void setCls(Class *cls) {
-      U u;
-      u.m_cls = to_low(cls);
-      m_u.store(u, std::memory_order_release);
-    }
-    Class* cls() const {
-      auto cls = m_u.load(std::memory_order_acquire).m_cls;
-      assertx(!(cls & kMethCallerBit));
-      return reinterpret_cast<Class*>(cls);
-    }
-    StringData* name() const {
-     auto n = m_u.load(std::memory_order_acquire).m_methCallerClsName;
-     assertx(n & kMethCallerBit);
-     return reinterpret_cast<StringData*>(n - kMethCallerBit);
-    }
-  };
-
-  template <class T>
-  static Func::low_storage_t to_low(T* px, Func::low_storage_t bit = 0) {
-    Func::low_storage_t ones = ~0;
-    auto ptr = reinterpret_cast<uintptr_t>(px) | bit;
-    always_assert((ptr & ones) == ptr);
-    return (Func::low_storage_t)(ptr);
-  }
 
   /////////////////////////////////////////////////////////////////////////////
   // Atomic Flags.
 
-public:
   enum Flags : uint8_t {
     None             = 0,
     Optimized        = 1 << 0,
@@ -1792,8 +1726,6 @@ struct Func final {
   static std::atomic<bool>     s_treadmill;
   static std::atomic<uint32_t> s_totalClonedClosures;
 
-  // To conserve space, we use unions for pairs of mutually exclusive fields
-  static auto constexpr kMethCallerBit = 0x1;  // set for m_methCaller
   /////////////////////////////////////////////////////////////////////////////
   // Data members.
   //
@@ -1812,23 +1744,78 @@ struct Func final {
   mutable AtomicLowPtr<const StringData> m_fullName{nullptr};
   LowStringPtr m_name{nullptr};
 
-  union {
-    // The first Class in the inheritance hierarchy that declared this method.
-    // Note that this may be an abstract class that did not provide an
-    // implementation.
-    low_storage_t m_baseCls{0};
-    // m_methCallerMethName can be accessed by meth_caller() only
-    low_storage_t m_methCallerMethName;
+  struct ClsOrMethCaller {
+    struct Cls {
+      // The first Class in the inheritance hierarchy that declared this method.
+      // Note that this may be an abstract class that did not provide an
+      // implementation.
+      LowPtr<Class> m_base{nullptr};
+      // Class that provided this method implementation
+      AtomicLowPtr<Class> m_impl{nullptr};
+
+      Cls() {}
+
+      Cls(const Cls& o) {
+        m_base = o.m_base;
+        m_impl = o.m_impl;
+      }
+
+      Cls& operator=(const Cls& o) {
+        m_base = o.m_base;
+        m_impl = o.m_impl;
+        return *this;
+      }
+    };
+    struct MethCaller {
+      LowStringPtr m_methName;
+      // Class name provided by meth_caller()
+      LowStringPtr m_clsName;
+    };
+
+    // The first part of the union is valid if !isMethCaller
+    // and the second part is valid if isMethCaller
+    union {
+      Cls m_cls;
+      MethCaller m_methCaller;
+    };
+
+    ClsOrMethCaller() {
+      m_cls.m_base = nullptr;
+      m_cls.m_impl = nullptr;
+    }
+
+    ClsOrMethCaller(const ClsOrMethCaller& o) {
+      m_cls = o.m_cls;
+    }
   };
 
-  // m_u is used to represent
-  // the Class that provided this method implementation, or
-  // the class name provided by meth_caller()
-  UnionWrapper m_u{nullptr};
+  ClsOrMethCaller::Cls& clsData() {
+    assertx(!isMethCaller());
+    return m_u.m_cls;
+  }
+
+  const ClsOrMethCaller::Cls& clsData() const {
+    assertx(!isMethCaller());
+    return m_u.m_cls;
+  }
+
+  ClsOrMethCaller::MethCaller& methCallerData() {
+    assertx(isMethCaller());
+    return m_u.m_methCaller;
+  }
+
+  const ClsOrMethCaller::MethCaller& methCallerData() const {
+    assertx(isMethCaller());
+    return m_u.m_methCaller;
+  }
+
+  ClsOrMethCaller m_u;
+
   union {
     Slot m_methodSlot{0};
-    LowPtr<const NamedFunc>::storage_type m_namedFunc;
+    LowPtr<const NamedFunc> m_namedFunc;
   };
+
   mutable ClonedFlag m_cloned;
   mutable AtomicFlags m_atomicFlags;
   bool m_isPreFunc : 1;

hphp/runtime/vm/jit/irlower-class-func.cpp

@@ -225,24 +225,20 @@ void cgLdFuncName(IRLS& env, const IRInstruction* inst) {
 }
 
 void cgLdMethCallerName(IRLS& env, const IRInstruction* inst) {
-  static_assert(Func::kMethCallerBit == 1,
-                "Fix the decq if you change kMethCallerBit");
   auto const dst = dstLoc(env, inst, 0).reg();
   auto const func = srcLoc(env, inst, 0).reg();
   auto const isCls = inst->extra<MethCallerData>()->isCls;
   auto& v = vmain(env);
   auto const off = isCls ?
     Func::methCallerClsNameOff() : Func::methCallerMethNameOff();
-  auto const tmp = v.makeReg();
-  emitLdLowPtr(v, func[off], tmp, sizeof(Func::low_storage_t));
-  v << decq{tmp, dst, v.makeReg()};
+  emitLdLowPtr(v, func[off], dst, sizeof(LowStringPtr));
 }
 
 void cgLdFuncCls(IRLS& env, const IRInstruction* inst) {
   auto const func = srcLoc(env, inst, 0).reg();
   auto const dst = dstLoc(env, inst, 0).reg();
   auto& v = vmain(env);
-  emitLdLowPtr(v, func[Func::clsOff()], dst, sizeof(Func::low_storage_t));
+  emitLdLowPtr(v, func[Func::clsOff()], dst, sizeof(LowPtr<Class>));
 }
 
 void cgFuncHasAttr(IRLS& env, const IRInstruction* inst) {

hphp/tools/lldb/utils.py

@@ -710,8 +710,7 @@ def _full_func_name(func: lldb.SBValue) -> str:
     if attrs.unsigned & Enum("HPHP::Attr", "AttrIsMethCaller", func.target).unsigned:
         cls = ""
     else:
-        m_u = atomic_get(get(func, "m_u", "m_u"))
-        cls = get(m_u, "m_cls")
+        cls = atomic_get(get(func, "m_u", "m_cls", "m_impl", "m_s"))
         if cls.unsigned == 0:
             cls = ""
         else: