HHBBC shouldn't step through unreachable code while inserting RAT ass…

…ertions

Summary:
While inserting RAT type assertions at the end of the optimize phase, HHBBC
shouldn't step through unreachable bytecode. There's an invariant that at this
point, the analysis of the function should have reached a fixed
point. Therefore, stepping through the bytecodes should not infer anything new
about the program. We do assert this in a few places (related to function
folding). However, unreachable parts of the function may not be analyzed fully,
and therefore stepping through them might break that invariant.

Instead, as soon as we hit an unreachable part of the program, just insert a
static analysis error guard and stop processing that block (same as we do
elsewhere).

Include a test case which triggers an assertion without this. The root cause of
why this test case triggers it is because of the lack of a pattern-match
optimization for IsType[C/L]; Not; Jmp[N]Z during analysis. The peephole
optimizer will remove the Not and flip the jump, but only during the
optimization phase. Add a suitable pattern matching optimization as well.

Reviewed By: markw65

Differential Revision: D6905679

fbshipit-source-id: f4e30540e589158216dc8117c742b340507f21f5

hphp/hhbbc/interp.cpp

@@ -772,11 +772,8 @@ void sameImpl(ISS& env) {
   push(env, std::move(pair.first));
 }
 
-}
-
 template<class Same, class JmpOp>
-void group(ISS& env, const Same& same, const JmpOp& jmp) {
-
+void sameJmpImpl(ISS& env, const Same& same, const JmpOp& jmp) {
   auto bail = [&] { impl(env, same, jmp); };
 
   constexpr auto NSame = Same::op == Op::NSame;
@@ -875,6 +872,21 @@ void group(ISS& env, const Same& same, const JmpOp& jmp) {
   sameIsJmpTarget ? handle_differ() : handle_same();
 }
 
+bc::JmpNZ invertJmp(const bc::JmpZ& jmp) { return bc::JmpNZ { jmp.target }; }
+bc::JmpZ invertJmp(const bc::JmpNZ& jmp) { return bc::JmpZ { jmp.target }; }
+
+}
+
+template<class Same, class JmpOp>
+void group(ISS& env, const Same& same, const JmpOp& jmp) {
+  sameJmpImpl(env, same, jmp);
+}
+
+template<class Same, class JmpOp>
+void group(ISS& env, const Same& same, const bc::Not&, const JmpOp& jmp) {
+  sameJmpImpl(env, same, invertJmp(jmp));
+}
+
 void in(ISS& env, const bc::Same&)  { sameImpl<false>(env); }
 void in(ISS& env, const bc::NSame&) { sameImpl<true>(env); }
 
@@ -1286,12 +1298,12 @@ void typeTestPropagate(ISS& env, Type valTy, Type testTy,
   push(env, std::move(takenOnSuccess ? failTy : testTy));
 }
 
-}
-
 // After a StaticLocCheck, we know the local is bound on the true path,
 // and not changed on the false path.
 template<class JmpOp>
-void group(ISS& env, const bc::StaticLocCheck& slc, const JmpOp& jmp) {
+void staticLocCheckJmpImpl(ISS& env,
+                           const bc::StaticLocCheck& slc,
+                           const JmpOp& jmp) {
   auto const takenOnInit = jmp.op == Op::JmpNZ;
   auto save = env.state;
 
@@ -1319,11 +1331,30 @@ void group(ISS& env, const bc::StaticLocCheck& slc, const JmpOp& jmp) {
   }
 }
 
+}
+
+template<class JmpOp>
+void group(ISS& env, const bc::StaticLocCheck& slc, const JmpOp& jmp) {
+  staticLocCheckJmpImpl(env, slc, jmp);
+}
+
+template<class JmpOp>
+void group(ISS& env, const bc::StaticLocCheck& slc,
+           const bc::Not&, const JmpOp& jmp) {
+  staticLocCheckJmpImpl(env, slc, invertJmp(jmp));
+}
+
 template<class JmpOp>
 void group(ISS& env, const bc::IsTypeL& istype, const JmpOp& jmp) {
   isTypeHelper(env, istype.subop2, istype.loc1, istype, jmp);
 }
 
+template<class JmpOp>
+void group(ISS& env, const bc::IsTypeL& istype,
+           const bc::Not&, const JmpOp& jmp) {
+  isTypeHelper(env, istype.subop2, istype.loc1, istype, invertJmp(jmp));
+}
+
 // If we duplicate a value, and then test its type and Jmp based on that result,
 // we can narrow the type of the top of the stack. Only do this for null checks
 // right now (because its useful in memoize wrappers).
@@ -1334,6 +1365,14 @@ void group(ISS& env, const bc::IsTypeC& istype, const JmpOp& jmp) {
   isTypeHelper(env, istype.subop1, location, istype, jmp);
 }
 
+template<class JmpOp>
+void group(ISS& env, const bc::IsTypeC& istype,
+           const bc::Not& negate, const JmpOp& jmp) {
+  auto const location = topStkEquiv(env);
+  if (location == NoLocalId) return impl(env, istype, negate, jmp);
+  isTypeHelper(env, istype.subop1, location, istype, invertJmp(jmp));
+}
+
 // If we do an IsUninit check and then Jmp based on the check, one branch will
 // be the original type minus the Uninit, and the other will be
 // Uninit. (IsUninit does not pop the value).
@@ -1343,6 +1382,12 @@ void group(ISS& env, const bc::IsUninit&, const JmpOp& jmp) {
   typeTestPropagate(env, valTy, TUninit, remove_uninit(valTy), jmp);
 }
 
+template<class JmpOp>
+void group(ISS& env, const bc::IsUninit&, const bc::Not&, const JmpOp& jmp) {
+  auto const valTy = popCU(env);
+  typeTestPropagate(env, valTy, TUninit, remove_uninit(valTy), invertJmp(jmp));
+}
+
 // A MemoGet, followed by an IsUninit, followed by a Jmp, can have the type of
 // the stack inferred very well. The IsUninit success path will be Uninit and
 // the failure path will be the inferred return type of the wrapped
@@ -1359,10 +1404,12 @@ void group(ISS& env, const bc::MemoGet& get, const bc::IsUninit& /*isuninit*/,
   typeTestPropagate(env, popCU(env), TUninit, memoizeImplRetType(env), jmp);
 }
 
+namespace {
+
 template<class JmpOp>
-void group(ISS& env,
-           const bc::InstanceOfD& inst,
-           const JmpOp& jmp) {
+void instanceOfJmpImpl(ISS& env,
+                       const bc::InstanceOfD& inst,
+                       const JmpOp& jmp) {
   auto bail = [&] { impl(env, inst, jmp); };
 
   auto const locId = topStkEquiv(env);
@@ -1394,6 +1441,23 @@ void group(ISS& env,
   refineLocation(env, locId, pre, jmp.target, post);
 }
 
+}
+
+template<class JmpOp>
+void group(ISS& env,
+           const bc::InstanceOfD& inst,
+           const JmpOp& jmp) {
+  instanceOfJmpImpl(env, inst, jmp);
+}
+
+template<class JmpOp>
+void group(ISS& env,
+           const bc::InstanceOfD& inst,
+           const bc::Not&,
+           const JmpOp& jmp) {
+  instanceOfJmpImpl(env, inst, invertJmp(jmp));
+}
+
 void in(ISS& env, const bc::Switch& op) {
   auto v = tv(popC(env));
 
@@ -3992,6 +4056,15 @@ void interpStep(ISS& env, Iterator& it, Iterator stop) {
   switch (o1) {
   case Op::InstanceOfD:
     switch (o2) {
+    case Op::Not:
+      switch (o3) {
+      case Op::JmpZ:
+        return group(env, it, it[0].InstanceOfD, it[1].Not, it[2].JmpZ);
+      case Op::JmpNZ:
+        return group(env, it, it[0].InstanceOfD, it[1].Not, it[2].JmpNZ);
+      default: break;
+      }
+      break;
     case Op::JmpZ:
       return group(env, it, it[0].InstanceOfD, it[1].JmpZ);
     case Op::JmpNZ:
@@ -4001,20 +4074,47 @@ void interpStep(ISS& env, Iterator& it, Iterator stop) {
     break;
   case Op::IsTypeL:
     switch (o2) {
+    case Op::Not:
+      switch (o3) {
+      case Op::JmpZ:
+        return group(env, it, it[0].IsTypeL, it[1].Not, it[2].JmpZ);
+      case Op::JmpNZ:
+        return group(env, it, it[0].IsTypeL, it[1].Not, it[2].JmpNZ);
+      default: break;
+      }
+      break;
     case Op::JmpZ:   return group(env, it, it[0].IsTypeL, it[1].JmpZ);
     case Op::JmpNZ:  return group(env, it, it[0].IsTypeL, it[1].JmpNZ);
     default: break;
     }
     break;
   case Op::IsUninit:
     switch (o2) {
+    case Op::Not:
+      switch (o3) {
+      case Op::JmpZ:
+        return group(env, it, it[0].IsUninit, it[1].Not, it[2].JmpZ);
+      case Op::JmpNZ:
+        return group(env, it, it[0].IsUninit, it[1].Not, it[2].JmpNZ);
+      default: break;
+      }
+      break;
     case Op::JmpZ:   return group(env, it, it[0].IsUninit, it[1].JmpZ);
     case Op::JmpNZ:  return group(env, it, it[0].IsUninit, it[1].JmpNZ);
     default: break;
     }
     break;
   case Op::IsTypeC:
     switch (o2) {
+    case Op::Not:
+      switch (o3) {
+      case Op::JmpZ:
+        return group(env, it, it[0].IsTypeC, it[1].Not, it[2].JmpZ);
+      case Op::JmpNZ:
+        return group(env, it, it[0].IsTypeC, it[1].Not, it[2].JmpNZ);
+      default: break;
+      }
+      break;
     case Op::JmpZ:
       return group(env, it, it[0].IsTypeC, it[1].JmpZ);
     case Op::JmpNZ:
@@ -4038,6 +4138,15 @@ void interpStep(ISS& env, Iterator& it, Iterator stop) {
     break;
   case Op::StaticLocCheck:
     switch (o2) {
+    case Op::Not:
+      switch (o3) {
+      case Op::JmpZ:
+        return group(env, it, it[0].StaticLocCheck, it[1].Not, it[2].JmpZ);
+      case Op::JmpNZ:
+        return group(env, it, it[0].StaticLocCheck, it[1].Not, it[2].JmpNZ);
+      default: break;
+      }
+      break;
     case Op::JmpZ:
       return group(env, it, it[0].StaticLocCheck, it[1].JmpZ);
     case Op::JmpNZ:
@@ -4047,6 +4156,15 @@ void interpStep(ISS& env, Iterator& it, Iterator stop) {
     break;
   case Op::Same:
     switch (o2) {
+    case Op::Not:
+      switch (o3) {
+      case Op::JmpZ:
+        return group(env, it, it[0].Same, it[1].Not, it[2].JmpZ);
+      case Op::JmpNZ:
+        return group(env, it, it[0].Same, it[1].Not, it[2].JmpNZ);
+      default: break;
+      }
+      break;
     case Op::JmpZ:
       return group(env, it, it[0].Same, it[1].JmpZ);
     case Op::JmpNZ:
@@ -4056,6 +4174,15 @@ void interpStep(ISS& env, Iterator& it, Iterator stop) {
     break;
   case Op::NSame:
     switch (o2) {
+    case Op::Not:
+      switch (o3) {
+      case Op::JmpZ:
+        return group(env, it, it[0].NSame, it[1].Not, it[2].JmpZ);
+      case Op::JmpNZ:
+        return group(env, it, it[0].NSame, it[1].Not, it[2].JmpNZ);
+      default: break;
+      }
+      break;
     case Op::JmpZ:
       return group(env, it, it[0].NSame, it[1].JmpZ);
     case Op::JmpNZ:

hphp/hhbbc/optimize.cpp

@@ -335,6 +335,16 @@ void insert_assertions(const Index& index,
       FTRACE(2, "   + {}\n", show(ctx.func, newBCs.back()));
     };
 
+    if (state.unreachable) {
+      blk->fallthrough = NoBlockId;
+      if (!(instrFlags(op.op) & TF)) {
+        gen(bc::BreakTraceHint {});
+        gen(bc::String { s_unreachable.get() });
+        gen(bc::Fatal { FatalOp::Runtime });
+      }
+      break;
+    }
+
     auto const preState = state;
     auto const flags    = step(interp, op);
 

hphp/test/slow/hhbbc/insert-assertions-unreachable.php

@@ -0,0 +1,16 @@
+<?hh
+// Copyright 2004-present Facebook. All Rights Reserved.
+
+function byref(&$x) {}
+function build() {
+  $d = dict[];
+  $k = __hhvm_intrinsics\launder_value('key');
+  while (__hhvm_intrinsics\launder_value(false)) {
+    $d[$k] = $d[$k] ?? dict[];
+    $d[$k] = $d[$k] ?? dict[];
+    byref(&$d[$k]['k2']);
+  }
+  return $d;
+}
+
+var_dump(build());

hphp/test/slow/hhbbc/insert-assertions-unreachable.php.expect

@@ -0,0 +1,2 @@
+dict(0) {
+}