Stop binding refs into args array for fb_intercept with inout handlers

Summary: They already return the arguments array back out as an `inout` parameter. Reviewed By: jano Differential Revision: D14999692 fbshipit-source-id: 01bf885731d343e24d75958db022d6a6f04e48e7
facebook · Apr 19, 2019 · 1ad7bc6 · 1ad7bc6
1 parent 0fe7010
commit 1ad7bc6
Show file tree

Hide file tree

Showing 5 changed files with 130 additions and 29 deletions.
diff --git a/hphp/runtime/vm/event-hook.cpp b/hphp/runtime/vm/event-hook.cpp
@@ -220,7 +220,7 @@ void runUserProfilerOnFunctionExit(const ActRec* ar, const TypedValue* retval,
 
 }
 
-static Array get_frame_args_with_ref(const ActRec* ar) {
+static Array get_frame_args(const ActRec* ar, bool with_ref) {
   int numNonVariadic = ar->func()->numNonVariadicParams();
   int numArgs = ar->numArgs();
 
@@ -233,7 +233,8 @@ static Array get_frame_args_with_ref(const ActRec* ar) {
   int i = 0;
   // The function's formal parameters are on the stack
   for (; i < numArgs && i < numNonVariadic; ++i) {
-    retArray.appendWithRef(tvAsCVarRef(local));
+    if (with_ref) retArray.appendWithRef(tvAsCVarRef(local));
+    else          retArray.append(tvAsCVarRef(local));
     --local;
   }
 
@@ -244,13 +245,15 @@ static Array get_frame_args_with_ref(const ActRec* ar) {
       // ... shuffled into a packed array stored in the variadic capture
       // param on the stack
       for (ArrayIter iter(tvAsCVarRef(local)); iter; ++iter) {
-        retArray.appendWithRef(iter.secondVal());
+        if (with_ref) retArray.appendWithRef(iter.secondVal());
+        else          retArray.append(iter.secondVal());
       }
     } else {
       // ... or moved into the ExtraArgs datastructure.
       for (; i < numArgs; ++i) {
-        retArray.appendWithRef(
-          tvAsCVarRef(ar->getExtraArg(i - numNonVariadic)));
+        auto const arg = ar->getExtraArg(i - numNonVariadic);
+        if (with_ref) retArray.appendWithRef(tvAsCVarRef(arg));
+        else          retArray.append(tvAsCVarRef(arg));
       }
     }
   }
@@ -263,7 +266,8 @@ static Variant call_intercept_handler(
   const Variant& called_on,
   Array& args,
   const Variant& ctx,
-  Variant& done
+  Variant& done,
+  ActRec* ar
 ) {
   ObjectData* obj = nullptr;
   Class* cls = nullptr;
@@ -277,14 +281,6 @@ static Variant call_intercept_handler(
     return uninit_null();
   }
 
-  PackedArrayInit par(5);
-  par.append(called);
-  par.append(called_on);
-  par.append(args);
-  par.append(ctx);
-
-  Variant intArgs;
-
   auto const inout = [&] {
     if (f->isInOutWrapper()) {
       auto const name = mangleInOutFuncName(f->name(), {2,4});
@@ -312,9 +308,19 @@ static Variant call_intercept_handler(
         return true;
       }
     }
-    return false;
+    return f->takesInOutParams();
   }();
 
+  args = get_frame_args(ar, !inout);
+
+  PackedArrayInit par(5);
+  par.append(called);
+  par.append(called_on);
+  par.append(args);
+  par.append(ctx);
+
+  Variant intArgs;
+
   if (inout) {
     intArgs = par.append(done).toArray();
   } else {
@@ -329,15 +335,14 @@ static Variant call_intercept_handler(
                           nullptr, invName, ExecutionContext::InvokeNormal,
                           dynamic, false)
   );
-  if (UNLIKELY(isRefType(ret.getRawType()))) {
-    tvUnbox(*ret.asTypedValue());
-  }
 
   if (inout) {
     auto& arr = ret.asCArrRef();
     if (arr[1].isArray()) args = arr[1].toArray();
     if (arr[2].isBoolean()) done = arr[2].toBoolean();
     return arr[0];
+  } else if (!ar->func()->takesInOutParams()) {
+    args.reset();
   }
   return ret;
 }
@@ -390,34 +395,55 @@ bool EventHook::RunInterceptHandler(ActRec* ar) {
     return init_null();
   }();
 
-  auto args = get_frame_args_with_ref(ar);
+  Array args;
   VarNR called(ar->func()->fullDisplayName());
 
   Variant ret = call_intercept_handler(
-    h->asCArrRef()[0], called, called_on, args, h->asCArrRef()[1], doneFlag
+    h->asCArrRef()[0], called, called_on, args, h->asCArrRef()[1], doneFlag, ar
   );
 
+  auto const rebind_locals = [&] {
+    auto local = reinterpret_cast<TypedValue*>(
+      uintptr_t(ar) - sizeof(TypedValue)
+    );
+    uint32_t param = 0;
+    IterateKV(args.get(), [&] (Cell, TypedValue v) {
+      if (param < func->numParams() && func->byRef(param++)) {
+        if (tvIsReferenced(*local)) {
+          cellSet(tvToCell(v), val(local).pref->cell());
+        }
+      }
+      --local;
+    });
+  };
+
   if (doneFlag.toBoolean()) {
     Offset pcOff;
     bool vmEntry;
     ActRec* outer = g_context->getPrevVMState(ar, &pcOff, nullptr, &vmEntry);
 
-    ar->setLocalsDecRefd();
-    frame_free_locals_no_hook(ar);
-
-    // Tear down the callee frame, then push the return value.
     Stack& stack = vmStack();
-    stack.trim((Cell*)(ar + 1));
+    auto const trim = [&] {
+      ar->setLocalsDecRefd();
+      frame_free_locals_no_hook(ar);
+
+      // Tear down the callee frame, then push the return value.
+      stack.trim((Cell*)(ar + 1));
+    };
+
     if (UNLIKELY(func->takesInOutParams())) {
+      assertx(!args.isNull());
       uint32_t count = func->numInOutParams();
 
+      trim(); // discard the callee frame before pushing inout values
       auto start = stack.topTV();
       auto const end = start + count;
 
       auto push = [&] (TypedValue v) {
+        auto const c = tvToCell(v);
         assertx(start < end);
-        tvIncRefGen(v);
-        *start++ = v;
+        tvIncRefGen(c);
+        *start++ = c;
       };
 
       uint32_t param = 0;
@@ -429,6 +455,11 @@ bool EventHook::RunInterceptHandler(ActRec* ar) {
       });
 
       while (start < end) push(make_tv<KindOfNull>());
+    } else if (!args.isNull()) {
+      rebind_locals();
+      trim(); // discard the callee frame after binding refs
+    } else {
+      trim(); // nothing to do throw away the frame
     }
 
     cellDup(*ret.toCell(), *stack.allocTV());
@@ -439,6 +470,8 @@ bool EventHook::RunInterceptHandler(ActRec* ar) {
     if (vmpc() && !vmEntry) vmpc() = skipCall(vmpc());
 
     return false;
+  } else if (!func->takesInOutParams() && !args.isNull()) {
+    rebind_locals();
   }
   vmfp() = ar;
   vmpc() = savePc;

diff --git a/hphp/test/slow/inout/fb-intercept-wrapper.php.expect b/hphp/test/slow/inout/fb-intercept-wrapper.php.expect
@@ -11,10 +11,10 @@ string(10) "inside bar"
 string(4) "meep"
 array(3) {
   [0]=>
-  &int(1)
+  int(1)
   [1]=>
   bool(true)
   [2]=>
-  &string(1) "c"
+  string(1) "c"
 }
 string(11) "inside meep"
diff --git a/hphp/test/slow/intercept/inout-handlers-2.php b/hphp/test/slow/intercept/inout-handlers-2.php
@@ -0,0 +1,61 @@
+<?hh
+
+function handler1($name, $target, inout $args, $ctx, inout $done) {
+  $done = true;
+  $args[0] = 'handler1';
+}
+
+class W {
+  static function handler2($name, $target, inout $args, $ctx, inout $done) {
+    $done = true;
+    $args[0] = 'handler2';
+  }
+
+  function handler3($name, $target, inout $args, $ctx, inout $done) {
+    $done = true;
+    $args[0] = 'handler3';
+  }
+
+  function make_closure() {
+    return ($name, $target, inout $args, $ctx, inout $done) ==> {
+      spl_object_hash($this);
+      $done = true;
+      $args[0] = 'handler4';
+    };
+  }
+
+  static function make_static_closure() {
+    return ($name, $target, inout $args, $ctx, inout $done) ==> {
+      $done = true;
+      $args[0] = 'handler5';
+    };
+  }
+}
+
+function foo(&$x) { echo "fail!\n"; }
+function bar(&$x) { echo "fail!\n"; }
+function fiz(&$x) { echo "fail!\n"; }
+function buz(&$x) { echo "fail!\n"; }
+function biz(&$x) { echo "fail!\n"; }
+function far(&$x) { echo "fail!\n"; }
+
+<<__EntryPoint>>
+function main() {
+  $handler6 = ($name, $target, inout $args, $ctx, inout $done) ==> {
+    $done = true;
+    $args[0] = 'handler6';
+  };
+  fb_intercept('foo', 'handler1');
+  fb_intercept('bar', 'W::handler2');
+  fb_intercept('fiz', [new W, 'handler3']);
+  fb_intercept('buz', (new W)->make_closure());
+  fb_intercept('biz', W::make_static_closure());
+  fb_intercept('far', $handler6);
+
+  $x = 'fail'; foo(&$x); echo "foo: $x\n";
+  $x = 'fail'; bar(&$x); echo "bar: $x\n";
+  $x = 'fail'; fiz(&$x); echo "fiz: $x\n";
+  $x = 'fail'; buz(&$x); echo "buz: $x\n";
+  $x = 'fail'; biz(&$x); echo "biz: $x\n";
+  $x = 'fail'; far(&$x); echo "far: $x\n";
+}
diff --git a/hphp/test/slow/intercept/inout-handlers-2.php.expect b/hphp/test/slow/intercept/inout-handlers-2.php.expect
@@ -0,0 +1,6 @@
+foo: handler1
+bar: handler2
+fiz: handler3
+buz: handler4
+biz: handler5
+far: handler6
diff --git a/hphp/test/slow/intercept/inout-handlers-2.php.hphp_opts b/hphp/test/slow/intercept/inout-handlers-2.php.hphp_opts
@@ -0,0 +1 @@
+-vJitEnableRenameFunction=1