Fix type-puns in FCallBuiltin

Avoid punning TypedValue* to String&/Array&/Object& in FCallBuiltin
(all three implementations).  Our native function calling conventions
require passing pointers into a TypedValue for these types, and
pointers-to-scratch for return values.

In the HHIR case, I removed the optional "return pointer" argument
from the IR CallBuiltin instruction.  The C++ value-passing ABI details
are now handled in cgCallBuiltin and are no longer exposed in the IR.
The argument types are still PtrTo*, but we handle the address fixups
in CodeGenerator.

hphp/doc/ir.specification

@@ -811,13 +811,30 @@ NativeImpl = S0:ConstFunc S1:StkPtr
 
   Execute a call to the native builtin specified by the func in S0.
 
-D:T = CallBuiltin<T> S0:FuncPtr S1:ConstInt S2...SN
+D:T = CallBuiltin<T> S0:FuncPtr S1...SN
 
-  Call builtin function. Operands:
+  Call builtin function with N-1 arguments.  Operands:
 
     S0: callee Func
-    S1: number of arguments to builtin
-    S2..SN: arguments to builtin function
+    S1..SN: arguments to builtin function
+
+  The source and destination types correspond to C++ parameter and return
+  types as follows:
+
+    C++ type            HHIR type         Position
+    -----------------   ---------         --------
+    bool                Bool              source, destination
+    int64_t             Int               source, destination
+    double              Dbl               source, destination
+    const String&       PtrToString       source
+    const Array&        PtrToArray        source
+    const Object&       PtrToObject       source
+    const Variant&      PtrToGen          source
+    Variant&            PtrToGen          source (ref param)
+    String              {Str|InitNull}    destination
+    Array               {Arr|InitNull}    destination
+    Object              {Obj|InitNull}    destination
+    Variant             {Gen-UninitNull}  destination
 
 D:RetAddr = LdRetAddr S0:StkPtr
 

hphp/runtime/vm/bytecode.cpp

@@ -6098,15 +6098,19 @@ template<class Ret, size_t NArgs, size_t CurArg> struct NativeFuncCaller {
     typedef NativeFuncCaller<Ret,NArgs - 1,CurArg + 1> NextArgT;
     DataType type = func->params()[CurArg].builtinType();
     if (type == KindOfDouble) {
-      // Doubles have a different calling convention.  So we need to
-      // tell the C++ type system about it.
+      // pass TV.m_data.dbl by value with C++ calling convention for doubles
       return NextArgT::call(func, tvs - 1, args..., tvs->m_data.dbl);
-    } else {
-      uintptr_t newArg = (type == KindOfInt64 || type == KindOfBoolean)
-        ? tvs->m_data.num
-        : uintptr_t(tvs);
-      return NextArgT::call(func, tvs - 1, args..., newArg);
     }
+    if (type == KindOfInt64 || type == KindOfBoolean) {
+      // pass TV.m_data.num by value
+      return NextArgT::call(func, tvs - 1, args..., tvs->m_data.num);
+    }
+    if (IS_STRING_TYPE(type) || type == KindOfArray || type == KindOfObject) {
+      // pass ptr to TV.m_data for String&, Array&, or Object&
+      return NextArgT::call(func, tvs - 1, args..., &tvs->m_data);
+    }
+    // final case is for passing full value as Variant&
+    return NextArgT::call(func, tvs - 1, args..., tvs);
   }
 };
 template<class Ret, size_t CurArg> struct NativeFuncCaller<Ret,0,CurArg> {
@@ -6133,7 +6137,8 @@ static Ret makeNativeCall(const Func* f, TypedValue* args, size_t numArgs) {
   not_reached();
 }
 
-static int makeNativeRefCall(const Func* f, TypedValue* ret,
+template<class Ret>
+static int makeNativeRefCall(const Func* f, Ret* ret,
                              TypedValue* args, size_t numArgs) {
   switch (numArgs) {
   case 0: return NativeFuncCaller<int64_t,0,0>::call(f, args, ret);
@@ -6193,9 +6198,10 @@ inline void OPTBLD_INLINE VMExecutionContext::iopFCallBuiltin(PC& pc) {
     ret.m_data.num = makeNativeCall<int64_t>(func, args, numArgs);
     break;
   case KindOfString:
+  case KindOfStaticString:
   case KindOfArray:
   case KindOfObject:
-    makeNativeRefCall(func, &ret, args, numArgs);
+    makeNativeRefCall(func, &ret.m_data, args, numArgs);
     if (ret.m_data.num == 0) {
       ret.m_type = KindOfNull;
     }

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

@@ -3238,36 +3238,64 @@ void CodeGenerator::cgCallBuiltin(IRInstruction* inst) {
   Type returnType       = inst->getTypeParam();
 
   const Func* func = f->getValFunc();
-
-  PhysReg returnBase = rsp;
+  DataType funcReturnType = func->returnType();
   int returnOffset = HHIR_MISOFF(tvBuiltinReturn);
 
-  // Load args into registers
+  // RSP points to the MInstrState we need to use.
+  // workaround the fact that rsp moves when we spill registers around call
+  PhysReg misReg = rScratch;
+  emitMovRegReg(m_as, reg::rsp, misReg);
+
   ArgGroup callArgs;
-  callArgs.ssas(inst, 1, numArgs);
-
-  // Call Builtin
-  BuiltinFunction nativeFuncPtr = func->nativeFuncPtr();
-  cgCallHelper(m_as,
-              (TCA)nativeFuncPtr,
-              dstReg,
-              kSyncPoint,
-              callArgs);
-
-  if (dstReg == InvalidReg) {
-    return;
+  if (isCppByRef(funcReturnType)) {
+    // first arg is pointer to storage for that return value
+    if (isSmartPtrRef(funcReturnType)) {
+      returnOffset += TVOFF(m_data);
+    }
+    // misReg is pointing to an MInstrState struct on the C stack.  Pass
+    // the address of tvBuiltinReturn to the native function as the location
+    // it can construct the return Array, String, Object, or Variant.
+    callArgs.addr(misReg, returnOffset); // &misReg[returnOffset]
   }
+
+  // non-pointer args are plain values passed by value.  String, Array,
+  // Object, and Variant are passed by const&, ie a pointer to stack memory
+  // holding the value, so expect PtrToT types for these.
+  // Pointers to smartptr types (String, Array, Object) need adjusting to
+  // point to &ptr->m_data.
+  for (int i = 0; i < numArgs; i++) {
+    const Func::ParamInfo& pi = func->params()[i];
+    if (TVOFF(m_data) && isSmartPtrRef(pi.builtinType())) {
+      assert(args[i]->getType().isPtr() && args[i]->getReg() != InvalidReg);
+      callArgs.addr(args[i]->getReg(), TVOFF(m_data));
+    } else {
+      callArgs.ssa(args[i]);
+    }
+  }
+
+  // if the return value is returned by reference, we don't need the
+  // return value from this call since we know where the value is.
+  cgCallHelper(m_as, Transl::Call((TCA)func->nativeFuncPtr()),
+               isCppByRef(funcReturnType) ? InvalidReg : dstReg,
+               kSyncPoint, callArgs);
+
   // load return value from builtin
-  // for primitive return types (int, bool, etc), the return value
+  // for primitive return types (int, bool), the return value
   // is already in dstReg (the builtin call returns in rax). For return
-  // by reference (String, Object, Array, etc), the builtin writes the
+  // by reference (String, Object, Array, Variant), the builtin writes the
   // return value into MInstrState::tvBuiltinReturn TV, from where it
   // has to be tested and copied.
-  if (returnType.isSimpleType()) {
+  if (dstReg == InvalidReg || returnType.isSimpleType()) {
     return;
   }
+  // after the call, RSP is back pointing to MInstrState and rSratch
+  // has been clobberred.
+  misReg = rsp;
+
   if (returnType.isReferenceType()) {
-    m_as.   loadq (returnBase[returnOffset], dstReg);
+    assert(isCppByRef(funcReturnType) && isSmartPtrRef(funcReturnType));
+    // return type is String, Array, or Object; fold nullptr to KindOfNull
+    m_as.   loadq (misReg[returnOffset], dstReg);
     emitLoadImm(m_as, returnType.toDataType(), dstType);
     emitLoadImm(m_as, KindOfNull, rScratch);
     m_as.   testq (dstReg, dstReg);
@@ -3276,12 +3304,14 @@ void CodeGenerator::cgCallBuiltin(IRInstruction* inst) {
   }
   if (returnType.subtypeOf(Type::Cell)
       || returnType.subtypeOf(Type::BoxedCell)) {
-    emitLoadTVType(m_as, returnBase[returnOffset + TVOFF(m_type)], dstType);
-    m_as.   loadq (returnBase[returnOffset + TVOFF(m_data)], dstReg);
+    // return type is Variant; fold KindOfUninit to KindOfNull
+    assert(isCppByRef(funcReturnType) && !isSmartPtrRef(funcReturnType));
+    assert(misReg != dstType);
+    emitLoadTVType(m_as, misReg[returnOffset + TVOFF(m_type)], dstType);
+    m_as.   loadq (misReg[returnOffset + TVOFF(m_data)], dstReg);
     emitLoadImm(m_as, KindOfNull, rScratch);
-    static_assert(KindOfUninit == 0,
-                  "CallBuiltin needs update for KindOfUninit");
-    m_as.   testl (r32(dstType), r32(dstType));
+    static_assert(KindOfUninit == 0, "KindOfUninit must be 0 for test");
+    m_as.   testb (rbyte(dstType), rbyte(dstType));
     m_as.   cmov_reg64_reg64 (CC_Z, rScratch, dstType);
     return;
   }

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

@@ -1542,15 +1542,6 @@ void HhbcTranslator::emitFCall(uint32_t numParams,
                 params);
 }
 
-// This is used to check that return types of builtins are not simple
-// types. This is different from IS_REFCOUNTED_TYPE because builtins
-// can return Variants, and we use KindOfUnknown to denote these
-// return types.
-static bool isCppByRef(DataType t) {
-  assert(t != KindOfDouble);
-  return t != KindOfBoolean && t != KindOfInt64 && t != KindOfNull;
-}
-
 void HhbcTranslator::emitFCallBuiltin(uint32_t numArgs,
                                       uint32_t numNonDefault, int32_t funcId) {
   const NamedEntityPair& nep = lookupNamedEntityPairId(funcId);
@@ -1584,31 +1575,20 @@ void HhbcTranslator::emitFCallBuiltin(uint32_t numArgs,
     }
   }
 
-
   // pass arguments for call
   SSATmp* args[numArgs + 1];
 
-  int isRefReturn = 0;
-  // if the function returns by reference, the first parameter
-  // has to be the fixed C++ location for the return value
-  if (isCppByRef(callee->returnType())) {
-    isRefReturn = 1;
-    SSATmp* misBase = m_tb->gen(DefMIStateBase);
-    SSATmp* returnAddr = m_tb->genLdAddr(misBase, HHIR_MISOFF(tvBuiltinReturn));
-    args[0] = returnAddr;
-  }
-
   for (int i = numArgs - 1; i >= 0; i--) {
     const Func::ParamInfo& pi = callee->params()[i];
     switch (pi.builtinType()) {
       case KindOfBoolean:
       case KindOfInt64:
-        args[isRefReturn + i] = top(Type::fromDataType(pi.builtinType(), KindOfInvalid),
-                                    numArgs - i - 1);
+        args[i] = top(Type::fromDataType(pi.builtinType(), KindOfInvalid),
+                      numArgs - i - 1);
         break;
       case KindOfDouble: assert(false);
       default:
-        args[isRefReturn + i] = loadStackAddr(numArgs - i - 1);
+        args[i] = loadStackAddr(numArgs - i - 1);
         break;
     }
   }
@@ -1616,7 +1596,7 @@ void HhbcTranslator::emitFCallBuiltin(uint32_t numArgs,
   SSATmp* func = m_tb->genDefConst<const Func*>(callee);
   Type type = Type::fromDataTypeWithRef(callee->returnType(),
                        (callee->attrs() & ClassInfo::IsReference));
-  SSATmp* ret = m_tb->genCallBuiltin(func, type, numArgs + isRefReturn, args);
+  SSATmp* ret = m_tb->genCallBuiltin(func, type, numArgs, args);
 
   // decref and free args
   for (int i = 0; i < numArgs; i++) {

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

@@ -9381,7 +9381,7 @@ TranslatorX64::translateThis(const Tracelet &t,
   }
 
   assert(!i.outLocal);
-  assert(curFunc()->isPseudoMain() || curFunc()->cls() || 
+  assert(curFunc()->isPseudoMain() || curFunc()->cls() ||
          curFunc()->isClosureBody());
   m_regMap.allocOutputRegs(i);
   PhysReg out = getReg(i.outStack->location);
@@ -9953,14 +9953,6 @@ void TranslatorX64::translateFCallArray(const Tracelet& t,
   }
 }
 
-// This is used to check that return types of builtins are not simple
-// types. This is different from IS_REFCOUNTED_TYPE because builtins
-// can return Variants, and we use KindOfUnknown to denote these
-// return types.
-static bool isCppByRef(DataType t) {
-  return t != KindOfBoolean && t != KindOfInt64 && t != KindOfNull;
-}
-
 void TranslatorX64::analyzeFCallBuiltin(Tracelet& t,
                                         NormalizedInstruction& i) {
   Id funcId = i.imm[2].u_SA;
@@ -10029,7 +10021,9 @@ void TranslatorX64::translateFCallBuiltin(const Tracelet& t,
   PhysReg returnBase = rsp;
   int returnOffset = offsetof(MInstrState, tvBuiltinReturn);
 
-  if (isCppByRef(func->returnType())) {
+  auto returnType = func->returnType();
+  if (isCppByRef(returnType)) {
+    if (isSmartPtrRef(returnType)) returnOffset += TVOFF(m_data);
     emitLea(a, returnBase, returnOffset, argNumToRegName[0]);
     refReturn = 1;
   }
@@ -10038,16 +10032,25 @@ void TranslatorX64::translateFCallBuiltin(const Tracelet& t,
   for (int i = 0; i < numArgs; i++) {
     const Func::ParamInfo& pi = func->params()[i];
     locToRegDisp(ni.inputs[numArgs - i - 1]->location, &base, &disp);
+    auto argReg = argNumToRegName[i + refReturn];
     switch (pi.builtinType()) {
+      case KindOfDouble:
+        assert(false);
       case KindOfBoolean:
-      case KindOfInt64: {
-        a.   loadq  (base[disp + TVOFF(m_data.num)],
-                       argNumToRegName[i + refReturn]);
-      } break;
-      case KindOfDouble:  assert(false);
-      default: {
-        emitLea(a, base, disp, argNumToRegName[i + refReturn]);
-      }
+      case KindOfInt64:
+        // pass by value
+        a.   loadq  (base[disp + TVOFF(m_data.num)], argReg);
+        break;
+      STRINGCASE():
+      case KindOfArray:
+      case KindOfObject:
+        // pass ptr to TV.m_data as String&, Array&, or Object&
+        emitLea(a, base, disp + TVOFF(m_data), argReg);
+        break;
+      default:
+        // pass ptr to TV as Variant&
+        emitLea(a, base, disp, argReg);
+        break;
     }
   }
   // Call builtin
@@ -10066,7 +10069,7 @@ void TranslatorX64::translateFCallBuiltin(const Tracelet& t,
     if (pi.builtinType() == KindOfUnknown) {
       emitDecRefGeneric(ni, base, disp);
     } else if (IS_REFCOUNTED_TYPE(pi.builtinType())) {
-      a.  loadq  (base[disp], rScratch);
+      a.  loadq  (base[disp + TVOFF(m_data)], rScratch);
       emitDecRef(ni, rScratch, pi.builtinType());
     }
   }
@@ -10077,36 +10080,40 @@ void TranslatorX64::translateFCallBuiltin(const Tracelet& t,
   // copy return value
   locToRegDisp(ni.outStack->location, &base, &disp);
 
-  switch (func->returnType()) {
+  switch (returnType) {
     // For bool return value, get the %al byte
     case KindOfBoolean:
-      a.  movzbl (al, eax);
+      a.  movzbl (al, eax);  // sign extend byte->qword
+      emitStoreTypedValue(a, func->returnType(), rax, disp, base, true);
+      break;
     case KindOfNull:  /* void return type */
     case KindOfInt64:
       emitStoreTypedValue(a, func->returnType(), rax, disp, base, true);
       break;
-    case BitwiseKindOfString:
+    STRINGCASE():
     case KindOfArray:
     case KindOfObject:
+      // returnOffset already has TVOFF(m_data) added if necessary.
       a.   loadq  (returnBase[returnOffset], rax);
       a.   testq  (rax, rax);
       {
         IfElseBlock<CC_Z> ifNotZero(a);
-        emitStoreTypedValue(a, func->returnType(), rax, disp, base, true);
+        emitStoreTypedValue(a, returnType, rax, disp, base, true);
 
         ifNotZero.Else();
         emitStoreTVType(a, KindOfNull, base[disp + TVOFF(m_type)]);
       }
       break;
-    case KindOfUnknown:
+    case KindOfUnknown: // return type was Variant
       emitLea(a, returnBase, returnOffset, rax);
-      emitLoadTVType(a, rax[TVOFF(m_type)], rScratch);
-      a.   cmpl   (KindOfUninit, r32(rScratch));
+      emitCmpTVType(a, KindOfUninit, rax[TVOFF(m_type)]);
       {
         IfElseBlock<CC_Z> ifNotUninit(a);
+        // copy 16-byte TypedValue
         emitCopyToAligned(a, rax, 0, base, disp);
 
         ifNotUninit.Else();
+        // result was KindOfUninit; convert to KindOfNull
         emitStoreTVType(a, KindOfNull, base[disp + TVOFF(m_type)]);
       }
       break;

hphp/runtime/vm/translator/translator.h

@@ -1103,6 +1103,20 @@ extern bool tc_dump();
 const Func* lookupImmutableMethod(const Class* cls, const StringData* name,
                                   bool& magicCall, bool staticLookup);
 
+// This is used to check that return types of builtins are not simple
+// types. This is different from IS_REFCOUNTED_TYPE because builtins
+// can return Variants, and we use KindOfUnknown to denote these
+// return types.
+static inline bool isCppByRef(DataType t) {
+  return t != KindOfBoolean && t != KindOfInt64 && t != KindOfNull;
+}
+
+// return true if type is passed in/out of C++ as String&/Array&/Object&
+static inline bool isSmartPtrRef(DataType t) {
+  return t == KindOfString || t == KindOfStaticString ||
+         t == KindOfArray || t == KindOfObject;
+}
+
 } } } // HPHP::VM::Transl
 
 #endif