Speedup ForbidDynamicCall implementation

Summary: The existing dynamic call checking infrastructure has high overhead when turned on, mainly because it inserts a dynamic call check in the prologue of every function. Improve this by moving the dynamic call check to the caller (instead of the callee). When making a function call which is known to be dynamic, check if the target function is dynamically callable, and if not, emit the warning. The dynamic call checks for frame accessing functions and for builtins still remain in the prologue. Its more optimal to do those there as they benefit more from knowing the func statically. Therefore we now have "caller dynamic call checks" and "callee dynamic call checks". Reviewed By: paulbiss Differential Revision: D7170451 fbshipit-source-id: d8378ae4e82876fe04bb73f35e4a91dd9fd6a011
facebook · Mar 9, 2018 · 5966d1131484594bfba00aa1b0dde0f8114f1f44 · 5966d11
1 parent 000059a
commit 5966d1131484594bfba00aa1b0dde0f8114f1f44
diff --git a/hphp/doc/ir.specification b/hphp/doc/ir.specification
@@ -1411,6 +1411,11 @@ To string conversions:
  Loads the m_this/m_cls member of an ActRec. S0 is the base address, with
  `offset' cells to the ActRec.

+| LdARIsDynamic<offset>, D(Bool), S(StkPtr), NF
+
+  Loads the dynamic call flag of a pre-live ActRec. S0 is the base address,
+  with `offset' cells to the ActRec.
+
 | LdARNumParams, D(Int), S(FramePtr), NF

  Loads the number of params from an ActRec. S0 is the address of the ActRec
@@ -1433,6 +1438,10 @@ To string conversions:
  Load the address of the static local variable named 'staticLocalName' for
  function 'func' in RDS. The variable must be initialized

+| IsFuncDynCallable, D(Bool), S(Func), NF
+
+  Tests for Func::m_attrs & AttrDynamicallyCallable.
+
 8. Allocation

 | AllocObj, DAllocObj, S(Cls), PRc|Er
@@ -2052,6 +2061,12 @@ To string conversions:
  Raise a warning indicating that the reffiness of a parameter was incorrectly
  annotated at the callsite.

+| RaiseForbiddenDynCall, ND, S(Func), Er
+
+  Depending on the setting of the `ForbidDynamicCalls` runtime option, either
+  raise a warning or throw an exception indicating that the func specified in
+  S0 was called dynamically (and should not be).
+
 | CheckStaticLoc<func,staticLocalName>, ND, NA, B

  Check whether the static local variable named 'staticLocalName' for
diff --git a/hphp/runtime/base/execution-context.cpp b/hphp/runtime/base/execution-context.cpp
@@ -1535,6 +1535,8 @@ TypedValue ExecutionContext::invokeFuncImpl(const Func* f,
  VMRegAnchor _;
  auto const reentrySP = vmStack().top();

+  if (dynamic) callerDynamicCallChecks(f);
+
  if (thiz != nullptr) thiz->incRefCount();

  TypedValue retval;
diff --git a/hphp/runtime/vm/bytecode.cpp b/hphp/runtime/vm/bytecode.cpp
@@ -1656,7 +1656,7 @@ void enterVMAtFunc(ActRec* enterFnAr, StackArgsState stk, VarEnv* varEnv) {

  if (!EventHook::FunctionCall(enterFnAr, EventHook::NormalFunc)) return;
  checkStack(vmStack(), enterFnAr->m_func, 0);
-  checkForDynamicCall(enterFnAr);
+  calleeDynamicCallChecks(enterFnAr);
  checkForRequiredCallM(enterFnAr);
  assert(vmfp()->func()->contains(vmpc()));

@@ -5286,7 +5286,7 @@ bool doFCall(ActRec* ar, PC& pc) {
    pc = vmpc();
    return false;
  }
-  checkForDynamicCall(ar);
+  calleeDynamicCallChecks(ar);
  checkForRequiredCallM(ar);
  return true;
 }
@@ -5302,6 +5302,7 @@ OPTBLD_INLINE void iopFCall(PC& pc, ActRec* ar, uint32_t numArgs) {
    ar->setUseWeakTypes();
  }
  assert(numArgs == ar->numArgs());
+  if (ar->isDynamicCall()) callerDynamicCallChecks(ar->func());
  checkStack(vmStack(), ar->m_func, 0);
  ar->setReturn(vmfp(), pc, jit::tc::ustubs().retHelper);
  doFCall(ar, pc);
@@ -5317,6 +5318,7 @@ void iopFCallD(PC& pc, ActRec* ar, uint32_t numArgs,
             ar->m_func == ar->m_func->cls()->getCtor()));
  }
  assert(numArgs == ar->numArgs());
+  if (ar->isDynamicCall()) callerDynamicCallChecks(ar->func());
  checkStack(vmStack(), ar->m_func, 0);
  ar->setReturn(vmfp(), pc, jit::tc::ustubs().retHelper);
  doFCall(ar, pc);
@@ -5330,6 +5332,7 @@ void iopFCallAwait(PC& pc, ActRec* ar, uint32_t numArgs,
    assert(ar->m_func->name()->isame(funcName));
  }
  assert(numArgs == ar->numArgs());
+  if (ar->isDynamicCall()) callerDynamicCallChecks(ar->func());
  checkStack(vmStack(), ar->m_func, 0);
  ar->setReturn(vmfp(), pc, jit::tc::ustubs().retHelper);
  ar->setFCallAwait();
@@ -5445,7 +5448,7 @@ static bool doFCallArray(PC& pc, ActRec* ar, int numStackValues,
    pc = vmpc();
    return false;
  }
-  checkForDynamicCall(ar);
+  calleeDynamicCallChecks(ar);
  checkForRequiredCallM(ar);
  return true;
 }
@@ -5466,11 +5469,13 @@ bool doFCallArrayTC(PC pc, int32_t numArgs, void* retAddr) {
 }

 OPTBLD_INLINE void iopFCallArray(PC& pc, ActRec* ar) {
+  if (ar->isDynamicCall()) callerDynamicCallChecks(ar->func());
  doFCallArray(pc, ar, 1, CallArrOnInvalidContainer::CastToArray);
 }

 OPTBLD_INLINE void iopFCallUnpack(PC& pc, ActRec* ar, uint32_t numArgs) {
  assert(numArgs == ar->numArgs());
+  if (ar->isDynamicCall()) callerDynamicCallChecks(ar->func());
  checkStack(vmStack(), ar->m_func, 0);
  doFCallArray(pc, ar, numArgs, CallArrOnInvalidContainer::WarnAndContinue);
 }
diff --git a/hphp/runtime/vm/interp-helpers.h b/hphp/runtime/vm/interp-helpers.h
@@ -58,27 +58,32 @@ inline void setTypesFlag(ActRec* fp, ActRec* ar) {
  if (!callUsesStrictTypes(fp)) ar->setUseWeakTypes();
 }

-inline void checkForDynamicCall(const ActRec* ar) {
+inline void callerDynamicCallChecks(const Func* func) {
+  if (RuntimeOption::EvalForbidDynamicCalls <= 0) return;
+  if (func->isDynamicallyCallable()) return;
+
+  if (RuntimeOption::EvalForbidDynamicCalls >= 2) {
+    std::string msg;
+    string_printf(
+      msg,
+      Strings::FUNCTION_CALLED_DYNAMICALLY,
+      func->fullDisplayName()->data()
+    );
+    throw_invalid_operation_exception(makeStaticString(msg));
+  } else {
+    raise_notice(
+      Strings::FUNCTION_CALLED_DYNAMICALLY,
+      func->fullDisplayName()->data()
+    );
+  }
+}
+
+inline void calleeDynamicCallChecks(const ActRec* ar) {
  if (!ar->isDynamicCall()) return;
  auto const func = ar->func();

-  if (func->accessesCallerFrame()) raise_disallowed_dynamic_call(func);
-
-  if (!func->isDynamicallyCallable()) {
-    if (RuntimeOption::EvalForbidDynamicCalls >= 2) {
-      std::string msg;
-      string_printf(
-        msg,
-        Strings::FUNCTION_CALLED_DYNAMICALLY,
-        func->fullDisplayName()->data()
-      );
-      throw_invalid_operation_exception(makeStaticString(msg));
-    } else if (RuntimeOption::EvalForbidDynamicCalls == 1) {
-      raise_notice(
-        Strings::FUNCTION_CALLED_DYNAMICALLY,
-        func->fullDisplayName()->data()
-      );
-    }
+  if (func->accessesCallerFrame()) {
+    raise_disallowed_dynamic_call(func);
  }

  if (RuntimeOption::EvalNoticeOnBuiltinDynamicCalls && func->isBuiltin()) {
diff --git a/hphp/runtime/vm/jit/dce.cpp b/hphp/runtime/vm/jit/dce.cpp
@@ -192,6 +192,7 @@ bool canDCE(IRInstruction* inst) {
  case LdObjClass:
  case LdClsName:
  case LdARFuncPtr:
+  case LdARIsDynamic:
  case LdARNumParams:
  case LdFuncNumParams:
  case LdStrLen:
@@ -269,6 +270,7 @@ bool canDCE(IRInstruction* inst) {
  case LdCufIterDynamic:
  case LdStaticLoc:
  case LdARNumArgsAndFlags:
+  case IsFuncDynCallable:
  case StrictlyIntegerConv:
    assertx(!inst->isControlFlow());
    return true;
@@ -502,6 +504,7 @@ bool canDCE(IRInstruction* inst) {
  case RaiseVarEnvDynCall:
  case RaiseHackArrCompatNotice:
  case RaiseParamRefMismatch:
+  case RaiseForbiddenDynCall:
  case InitStaticLoc:
  case PrintStr:
  case PrintInt:
diff --git a/hphp/runtime/vm/jit/extra-data.h b/hphp/runtime/vm/jit/extra-data.h
@@ -1526,6 +1526,7 @@ X(StContArState,                GeneratorState);
 X(ContEnter,                    ContEnterData);
 X(DbgAssertARFunc,              IRSPRelOffsetData);
 X(LdARFuncPtr,                  IRSPRelOffsetData);
+X(LdARIsDynamic,                IRSPRelOffsetData);
 X(LdARCtx,                      IRSPRelOffsetData);
 X(EndCatch,                     IRSPRelOffsetData);
 X(EagerSyncVMRegs,              IRSPRelOffsetData);
diff --git a/hphp/runtime/vm/jit/frame-state.cpp b/hphp/runtime/vm/jit/frame-state.cpp
@@ -198,6 +198,12 @@ bool merge_into(FrameState& dst, const FrameState& src) {
      dstInfo.func = nullptr;
      changed = true;
    }
+
+    if (dstInfo.dynamicCall != nullptr &&
+        dstInfo.dynamicCall != srcInfo.dynamicCall) {
+      dstInfo.dynamicCall = nullptr;
+      changed = true;
+    }
  }

  // The frame may span a call if it could have done so in either state.
@@ -707,6 +713,7 @@ void FrameStateMgr::update(const IRInstruction* inst) {
                                         nullptr,
                                         extra.opcode,
                                         nullptr,
+                                         nullptr,
                                         false /* inlineEligible */,
                                         false /* spansCall */});
    } else if (isFCallStar(extra.opcode) && !cur().fpiStack.empty()) {
@@ -1769,10 +1776,13 @@ static const Func* getSpillFrameKnownCallee(const IRInstruction* inst) {
 void FrameStateMgr::spillFrameStack(IRSPRelOffset offset,
                                    FPInvOffset retOffset,
                                    const IRInstruction* inst) {
+  assertx(inst->is(SpillFrame));
+
  for (auto i = uint32_t{0}; i < kNumActRecCells; ++i) {
    setValue(stk(offset + i), nullptr);
  }
-  auto const ctx = inst->op() == SpillFrame ? inst->src(2) : nullptr;
+  auto const ctx = inst->src(2);
+  auto const dynamicCall = inst->src(4);

  const Func* func = getSpillFrameKnownCallee(inst);
  auto const opc = m_fpushOverride ?
@@ -1787,6 +1797,7 @@ void FrameStateMgr::spillFrameStack(IRSPRelOffset offset,
    ctx,
    opc,
    func,
+    dynamicCall,
    true /* inlineEligible */,
    false /* spans */
  });
diff --git a/hphp/runtime/vm/jit/frame-state.h b/hphp/runtime/vm/jit/frame-state.h
@@ -71,6 +71,11 @@ struct FPIInfo {
   */
  const Func* func;

+  /*
+   * Whether this func was pushed as part of a dynamic call
+   */
+  SSATmp* dynamicCall;
+
  bool inlineEligible;
  bool spansCall;
 };
diff --git a/hphp/runtime/vm/jit/ir-builder.cpp b/hphp/runtime/vm/jit/ir-builder.cpp
@@ -323,6 +323,22 @@ SSATmp* IRBuilder::preOptimizeLdARFuncPtr(IRInstruction* inst) {
  return nullptr;
 }

+SSATmp* IRBuilder::preOptimizeLdARIsDynamic(IRInstruction* inst) {
+  auto const& fpiStack = fs().fpiStack();
+  auto const arOff = inst->extra<LdARIsDynamic>()->offset;
+  auto const invOff = arOff.to<FPInvOffset>(fs().irSPOff()) - kNumActRecCells;
+
+  for (auto i = fpiStack.size(); i--; ) {
+    auto const& info = fpiStack[i];
+    if (info.returnSP == inst->src(0) &&
+        info.returnSPOff == invOff) {
+      return info.dynamicCall;
+    }
+  }
+
+  return nullptr;
+}
+
 SSATmp* IRBuilder::preOptimizeCheckCtxThis(IRInstruction* inst) {
  auto const func = inst->marker().func();
  if (!func->mayHaveThis()) {
@@ -467,6 +483,7 @@ SSATmp* IRBuilder::preOptimize(IRInstruction* inst) {
  X(CastStk)
  X(CoerceStk)
  X(LdARFuncPtr)
+  X(LdARIsDynamic)
  X(CheckCtxThis)
  X(LdCtx)
  X(LdCctx)
diff --git a/hphp/runtime/vm/jit/ir-builder.h b/hphp/runtime/vm/jit/ir-builder.h
@@ -309,6 +309,7 @@ struct IRBuilder {
  SSATmp* preOptimizeAssertLoc(IRInstruction*);
  SSATmp* preOptimizeAssertStk(IRInstruction*);
  SSATmp* preOptimizeLdARFuncPtr(IRInstruction*);
+  SSATmp* preOptimizeLdARIsDynamic(IRInstruction*);
  SSATmp* preOptimizeCheckCtxThis(IRInstruction*);
  SSATmp* preOptimizeLdCtxHelper(IRInstruction*);
  SSATmp* preOptimizeLdCtx(IRInstruction* i) {
diff --git a/hphp/runtime/vm/jit/irgen-call.cpp b/hphp/runtime/vm/jit/irgen-call.cpp
@@ -1556,6 +1556,50 @@ void emitFPassCW(IRGS& env, uint32_t argNum, FPassHint hint) {

 //////////////////////////////////////////////////////////////////////

+void emitCallerDynamicCallChecks(IRGS& env,
+                                 const Func* callee,
+                                 uint32_t numParams) {
+  if (RuntimeOption::EvalForbidDynamicCalls <= 0) return;
+  if (callee && callee->isDynamicallyCallable()) return;
+
+  SSATmp* func = nullptr;
+  ifElse(
+    env,
+    [&] (Block* skip) {
+      auto const calleeAROff = spOffBCFromIRSP(env) + numParams;
+      auto const dynamic = gen(
+        env,
+        LdARIsDynamic,
+        IRSPRelOffsetData { calleeAROff },
+        sp(env)
+      );
+      gen(env, JmpZero, skip, dynamic);
+
+      // If we do have a callee, we already know its not dynamically callable
+      // (checked above).
+      if (!callee) {
+        func = gen(
+          env,
+          LdARFuncPtr,
+          TFunc,
+          IRSPRelOffsetData { calleeAROff },
+          sp(env)
+        );
+        auto const dyncallable = gen(env, IsFuncDynCallable, func);
+        gen(env, JmpNZero, skip, dyncallable);
+      } else {
+        func = cns(env, callee);
+      }
+    },
+    [&] {
+      hint(env, Block::Hint::Unlikely);
+      gen(env, RaiseForbiddenDynCall, func);
+    }
+  );
+}
+
+//////////////////////////////////////////////////////////////////////
+
 void emitFCallArray(IRGS& env) {
  auto const callee = env.currentNormalizedInstruction->funcd;

@@ -1566,6 +1610,8 @@ void emitFCallArray(IRGS& env) {
    ? funcReadsLocals(callee)
    : callReadsLocals(*env.currentNormalizedInstruction, curFunc(env));

+  emitCallerDynamicCallChecks(env, callee, 1);
+
  auto const data = CallArrayData {
    spOffBCFromIRSP(env),
    0,
@@ -1590,6 +1636,8 @@ void implFCallUnpack(IRGS& env, uint32_t numParams, uint32_t numOut) {
    ? funcReadsLocals(callee)
    : callReadsLocals(*env.currentNormalizedInstruction, curFunc(env));

+  emitCallerDynamicCallChecks(env, callee, numParams);
+
  auto const data = CallArrayData {
    spOffBCFromIRSP(env),
    numParams,
@@ -1633,6 +1681,8 @@ SSATmp* implFCall(IRGS& env, uint32_t numParams, uint32_t numOut) {
      curFunc(env)
    );

+  emitCallerDynamicCallChecks(env, callee, numParams);
+
  auto op = curFunc(env)->unit()->getOp(bcOff(env));
  auto const retVal = gen(
    env,
diff --git a/hphp/runtime/vm/jit/irgen-call.h b/hphp/runtime/vm/jit/irgen-call.h
@@ -46,6 +46,10 @@ void emitDirectCall(IRGS& env, Func* callee, uint32_t numParams,

 SSATmp* implFCall(IRGS& env, uint32_t numParams, uint32_t numOut);

+void emitCallerDynamicCallChecks(IRGS& env,
+                                 const Func* callee,
+                                 uint32_t numParams);
+
 Type callReturnType(const Func* callee);

 //////////////////////////////////////////////////////////////////////