Implement VectorTranslator::UnsetElem

Fairly straightforward. The only interesting part is the
addition of ElemUX, which behaves similarly to ElemDX in terms of
side-effects (but not exactly the same).

hphp/doc/ir.specification

@@ -871,6 +871,10 @@ ReleaseVVOrExit S0:StkPtr -> L
   null, does nothing.  If it is an ExtraArgs, deallocates the
   ExtraArgs structure.  Otherwise jumps to the exit-trace label L.
 
+RaiseError S0:Str
+
+  Raises a fatal error with the text in S0 as its message.
+
 D:StkPtr = GenericRetDecRefs S0:StkPtr S1:Gen S2:ConstInt
 
   Does decrefs of all the current function's locals, where S0 is a
@@ -1300,6 +1304,12 @@ D:PtrToGen, D:StkPtr = ElemDXStk S0:ConstTCA S1:PtrToGen S2:Gen S3:PtrToCell
   Like ElemX, but used for intermediate element lookups that may
   modify the base.
 
+D:PtrToGen = ElemUX S0:ConstTCA S1:PtrToGen S2:Gen S3:PtrToCell
+D:PtrToGen, D:StkPtr = ElemUXStk S0:ConstTCA S1:PtrToGen S2:Gen S3:PtrToCell
+
+  Like ElemX, but used for intermediate element lookups that may
+  modify the base as part of an unset operation.
+
 D:Cell = ArrayGet S0:ConstTCA S1:Arr S2:{Int|Str}
 
   Get element with key S2 from base S1.
@@ -1337,6 +1347,11 @@ D:Vector, D:StkPtr = SetElemStk S0:ConstTCA S1:PtrToGen S2:Gen S3:Cell
 
   Set element with key S2 in S1 to S3.
 
+UnsetElem S0:ConstTCA S1:PtrToGen S2:Gen
+D:StkPtr = UnsetElemStk S0:ConstTCA S1:PtrToGen S2:Gen
+
+  Unsets the element at key S2 in the base S1.
+
 D:Cell = SetOpElem S0:ConstTCA S1:PtrToGen S2:Gen S3:Cell S4:PtrToCell
 D:Cell, D:StkPtr = SetOpElemStk S0:ConstTCA S1:PtrToGen S2:Gen S3:Cell
                                 S4:PtrToCell
@@ -1360,6 +1375,10 @@ D:StkPtr = BindNewElemStk S0:PtrToGen S1:BoxedCell S2:PtrToCell
 
   Append the reference in S1 to S0. S2 should point to an MInstrState struct.
 
+D:Bool = ArrayIsset S0:ConstTCA S1:Arr S2:{Int|Str}
+
+  Returns true iff the element at key S2 in the base S1 is set.
+
 D:Bool = IssetElem S0:ConstTCA S1:PtrToGen S2:Gen S3:PtrToCell
 
   Returns true iff the element at key S2 in S1 is set.

hphp/runtime/base/strings.h

@@ -52,6 +52,10 @@ const char* const METHOD_NAME_MUST_BE_STRING =
   "Method name must be a string";
 const char* const MISSING_ARGUMENT =
   "Missing argument %d to %s()";
+const char* const CANT_UNSET_STRING =
+  "Cannot unset string offsets";
+const char* const OP_NOT_SUPPORTED_STRING =
+  "Operator not supported for strings";
 
 } // namespace Strings
 } // namespace HPHP

hphp/runtime/vm/member_operations.h

@@ -403,13 +403,14 @@ static inline TypedValue* ElemU(TypedValue& tvScratch, TypedValue& tvRef,
   case KindOfBoolean:
   case KindOfInt64:
   case KindOfDouble: {
-    tvWriteUninit(&tvScratch);
-    result = &tvScratch;
+    // Unset on a null base never modifies the base, but the
+    // const_cast is necessary to placate the type system.
+    result = const_cast<TypedValue*>(null_variant.asTypedValue());
     break;
   }
   case KindOfStaticString:
   case KindOfString: {
-    raise_error("Operator not supported for strings");
+    raise_error(Strings::OP_NOT_SUPPORTED_STRING);
     break;
   }
   case KindOfArray: {
@@ -440,8 +441,7 @@ static inline TypedValue* ElemU(TypedValue& tvScratch, TypedValue& tvRef,
     break;
   }
   default: {
-    assert(false);
-    result = nullptr;
+    not_reached();
   }
   }
   return result;
@@ -1227,7 +1227,7 @@ static inline void UnsetElem(TypedValue* base, TypedValue* member) {
   switch (type) {
   case KindOfStaticString:
   case KindOfString: {
-    raise_error("Cannot unset string offsets");
+    raise_error(Strings::CANT_UNSET_STRING);
   }
   case KindOfArray: {
     UnsetElemArray<keyType>(base, member);

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

@@ -322,6 +322,7 @@ CALL_OPCODE(RaiseUninitLoc)
 CALL_OPCODE(WarnNonObjProp)
 CALL_OPCODE(ThrowNonObjProp)
 CALL_OPCODE(RaiseUndefProp)
+CALL_OPCODE(RaiseError)
 
 // Vector instruction helpers
 CALL_OPCODE(BaseG)
@@ -337,6 +338,7 @@ CALL_OPCODE(EmptyProp)
 CALL_OPCODE(IssetProp)
 CALL_OPCODE(ElemX)
 CALL_STK_OPCODE(ElemDX)
+CALL_STK_OPCODE(ElemUX)
 CALL_OPCODE(ArrayGet)
 CALL_OPCODE(CGetElem)
 CALL_STK_OPCODE(VGetElem)
@@ -344,6 +346,7 @@ CALL_STK_OPCODE(BindElem)
 CALL_OPCODE(ArraySet)
 CALL_OPCODE(ArraySetRef)
 CALL_STK_OPCODE(SetElem)
+CALL_STK_OPCODE(UnsetElem)
 CALL_STK_OPCODE(SetOpElem)
 CALL_STK_OPCODE(IncDecElem)
 CALL_STK_OPCODE(SetNewElem)

hphp/runtime/vm/translator/hopt/ir.h

@@ -226,6 +226,7 @@ O(JmpIndirect,                      ND, S(TCA),                          T|E) \
 O(ExitWhenSurprised,                ND, NA,                                E) \
 O(ExitOnVarEnv,                     ND, S(StkPtr),                         E) \
 O(ReleaseVVOrExit,                  ND, S(StkPtr),                       N|E) \
+O(RaiseError,                       ND, S(Str),               E|N|Mem|Refs|T) \
 O(CheckInit,                        ND, S(Gen),                           NF) \
 O(CheckInitMem,                     ND, S(PtrToGen) C(Int),               NF) \
 O(Unbox,                     DUnbox(0), S(Gen),                           NF) \
@@ -452,6 +453,10 @@ O_STK(ElemDX,              D(PtrToGen), C(TCA)                                \
                                           S(PtrToGen)                         \
                                           S(Gen)                              \
                                           S(PtrToCell),VElem|E|N|Mem|Refs|Er) \
+O_STK(ElemUX,              D(PtrToGen), C(TCA)                                \
+                                          S(PtrToGen)                         \
+                                          S(Gen)                              \
+                                          S(PtrToCell),VElem|E|N|Mem|Refs|Er) \
 O(ArrayGet,                    D(Cell), C(TCA)                                \
                                           S(Arr)                              \
                                           S(Int,Str),    E|N|PRc|Refs|Mem|Er) \
@@ -481,6 +486,9 @@ O_STK(SetElem,                 DVector, C(TCA)                                \
                                           S(PtrToGen)                         \
                                           S(Gen)                              \
                                           S(Cell),     VElem|E|N|Mem|Refs|Er) \
+O_STK(UnsetElem,                    ND, C(TCA)                                \
+                                          S(PtrToGen)                         \
+                                          S(Gen),      VElem|E|N|Mem|Refs|Er) \
 O_STK(SetOpElem,               D(Cell), C(TCA)                                \
                                           S(PtrToGen)                         \
                                           S(Gen)                              \

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

@@ -563,17 +563,6 @@ void MemMap::processInstruction(IRInstruction* inst, bool isPseudoMain) {
       killPropInfo(inst);
       break;
     }
-    case SetProp:
-    case SetElem:
-    case SetNewElem:
-    case ElemDX: {
-      VectorEffects::get(inst,
-                         /* storeLocValue callback */
-                         storeLocal,
-                         /* setLocType callback. Erases the value for now. */
-                         [&](uint32_t id, Type t) { storeLocal(id, nullptr); });
-      break;
-    }
     case DecRef:
     case DecRefNZ: {
       SSATmp* ref = inst->getSrc(0);
@@ -611,7 +600,15 @@ void MemMap::processInstruction(IRInstruction* inst, bool isPseudoMain) {
       // otherwise fall through to default case. a DecRef of an Obj{&}/Arr{&}
       // can modify refs if the source hits zero and runs a destructor
     }
-    default: {
+    default:
+      if (VectorEffects::supported(inst)) {
+        VectorEffects::get(inst,
+                           /* storeLocValue callback */
+                           storeLocal,
+                           /* setLocType callback. Erases the value for now. */
+                           [&](uint32_t id, Type t) { storeLocal(id, nullptr); }
+                          );
+      }
       if (inst->mayModifyRefs()) {
 
         // escape any boxes that are on the right hand side of the current
@@ -630,7 +627,6 @@ void MemMap::processInstruction(IRInstruction* inst, bool isPseudoMain) {
         clearLocalsMap();
       }
       break;
-    }
   }
 }
 

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

@@ -86,6 +86,7 @@ static CallMap s_callMap({
     {ThrowNonObjProp,    (TCA)throw_null_object_prop, DNone, SSync, {}},
     {RaiseUndefProp,     (TCA)raiseUndefProp, DNone, SSync,
                            {{SSA, 0}, {SSA, 1}}},
+    {RaiseError,         (TCA)raise_error_sd, DNone, SSync, {{SSA, 0}}},
 
     /* Switch helpers */
     {LdSwitchDblIndex,   (TCA)switchDoubleHelper, DSSA, SSync,
@@ -128,6 +129,8 @@ static CallMap s_callMap({
                  {{SSA, 1}, {VecKeyIS, 2}, {SSA, 3}}},
     {ElemDX,   {FSSA, 0}, DSSA, SSync,
                  {{SSA, 1}, {VecKeyIS, 2}, {SSA, 3}}},
+    {ElemUX,   {FSSA, 0}, DSSA, SSync,
+                 {{SSA, 1}, {VecKeyIS, 2}, {SSA, 3}}},
     {ArrayGet, {FSSA, 0}, DTV, SSync,
                  {{SSA, 1}, {SSA, 2}}},
     {CGetElem, {FSSA, 0}, DTV, SSync,
@@ -142,6 +145,8 @@ static CallMap s_callMap({
                  {{SSA, 1}, {SSA, 2}, {TV, 3}, {SSA, 4}}},
     {SetElem,  {FSSA, 0}, DTV, SSync,
                  {{SSA, 1}, {VecKeyIS, 2}, {TV, 3}}},
+    {UnsetElem, {FSSA, 0}, DNone, SSync,
+                 {{SSA, 1}, {VecKeyIS, 2}}},
     {SetOpElem, {FSSA, 0}, DTV, SSync,
                  {{SSA, 1}, {VecKeyIS, 2}, {TV, 3}, {SSA, 4}}},
     {IncDecElem, {FSSA, 0}, DTV, SSync,

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

@@ -87,6 +87,16 @@ void stripBase(Type& baseType, bool& basePtr, bool& baseBoxed) {
   baseBoxed = baseType.isBoxed();
   baseType = baseBoxed ? baseType.innerType() : baseType;
 }
+
+Opcode canonicalOp(Opcode op) {
+  if (op == ElemUX || op == ElemUXStk ||
+      op == UnsetElem || op == UnsetElemStk) {
+    return UnsetElem;
+  }
+  return opcodeHasFlags(op, VectorProp) ? SetProp
+       : opcodeHasFlags(op, VectorElem) || op == ArraySet ? SetElem
+       : bad_value<Opcode>();
+}
 }
 
 void VectorEffects::init(Opcode op, const Type origBase,
@@ -103,14 +113,11 @@ void VectorEffects::init(Opcode op, const Type origBase,
   baseTypeChanged = baseValChanged = valTypeChanged = false;
 
   // Canonicalize the op to SetProp or SetElem
-  op = opcodeHasFlags(op, VectorProp) ? SetProp
-     : opcodeHasFlags(op, VectorElem) || op == ArraySet ? SetElem
-     : bad_value<Opcode>();
-  assert(op == SetProp || op == SetElem);
+  op = canonicalOp(op);
   assert(key.equals(Type::None) || key.isKnownDataType());
   assert(origVal.equals(Type::None) || origVal.isKnownDataType());
 
-  if (baseType.maybe(Type::Null | Type::Bool | Type::Str)) {
+  if (op != UnsetElem && baseType.maybe(Type::Null | Type::Bool | Type::Str)) {
     // stdClass or array promotion might happen
     auto newBase = op == SetElem ? Type::Arr : Type::Obj;
     // If the base is known to be null, promotion will happen. If we can ever
@@ -129,13 +136,13 @@ void VectorEffects::init(Opcode op, const Type origBase,
     }
     baseValChanged = true;
   }
-  if (op == SetElem && baseType.maybe(Type::Arr)) {
+  if ((op == SetElem || op == UnsetElem) && baseType.maybe(Type::Arr)) {
     // possible COW when modifying an array, unless the base was boxed. If the
     // base is a box then the value of the box itself won't change, just what
     // it points to.
     baseValChanged = !baseBoxed;
   }
-  if (op == SetElem && baseType.maybe(Type::StaticArr)) {
+  if ((op == SetElem || op == UnsetElem) && baseType.maybe(Type::StaticArr)) {
     // the base might get promoted to a CountedArr. We can ignore the change if
     // the base is boxed, (for the same reasons as above).
     baseType = baseType | Type::CountedArr;
@@ -211,8 +218,10 @@ int vectorValIdx(Opcode opc) {
     case IncDecProp: case IncDecPropStk:
     case PropDX: case PropDXStk:
     case VGetElem: case VGetElemStk:
+    case UnsetElem: case UnsetElemStk:
     case IncDecElem: case IncDecElemStk:
     case ElemDX: case ElemDXStk:
+    case ElemUX: case ElemUXStk:
       return -1;
 
     case ArraySet: return 3;
@@ -300,6 +309,7 @@ void HhbcTranslator::VectorTranslator::checkMIState() {
   const bool isCGetM = m_ni.mInstrOp() == OpCGetM;
   const bool isSetM = m_ni.mInstrOp() == OpSetM;
   const bool isIssetM = m_ni.mInstrOp() == OpIssetM;
+  const bool isUnsetM = m_ni.mInstrOp() == OpUnsetM;
   const bool isSingle = m_ni.immVecM.size() == 1;
 
   assert(baseType.isBoxed() || baseType.notBoxed());
@@ -322,6 +332,12 @@ void HhbcTranslator::VectorTranslator::checkMIState() {
   const bool simpleArrayIsset = isIssetM && singleElem &&
     baseType.subtypeOf(Type::Arr);
 
+  // UnsetM on an array with one vector element
+  const bool simpleArrayUnset = isUnsetM && singleElem;
+
+  // UnsetM on a non-standard base. Always a noop or fatal.
+  const bool badUnset = isUnsetM && baseType.not(Type::Arr | Type::Obj);
+
   // CGetM on an array with a base that won't use MInstrState. Str
   // will use tvScratch and baseStrOff and Obj will fatal or use
   // tvRef.
@@ -330,6 +346,7 @@ void HhbcTranslator::VectorTranslator::checkMIState() {
 
   if (simpleProp || singlePropSet ||
       simpleArraySet || simpleArrayGet ||
+      simpleArrayUnset || badUnset ||
       simpleArrayIsset) {
     setNoMIState();
   }
@@ -632,10 +649,14 @@ void HhbcTranslator::VectorTranslator::emitProp() {
   const Class* knownCls = nullptr;
   const int propOffset  = getPropertyOffset(m_ni, knownCls, m_mii,
                                             m_mInd, m_iInd);
+  auto mia = m_mii.getAttr(m_ni.immVecM[m_mInd]);
+  if (mia & Unset) {
+    PUNT(emitProp-Unset);
+  }
   if (propOffset == -1) {
     emitPropGeneric();
   } else {
-    emitPropSpecialized(m_mii.getAttr(m_ni.immVecM[m_mInd]), propOffset);
+    emitPropSpecialized(mia, propOffset);
   }
 }
 
@@ -909,11 +930,34 @@ void HhbcTranslator::VectorTranslator::emitElem() {
   MInstrAttr mia = MInstrAttr(m_mii.getAttr(mCode) & MIA_intermediate);
   SSATmp* key = getInput(m_iInd);
 
+  const bool unset = mia & Unset;
+  const bool define = mia & Define;
+  assert(!(define && unset));
+  if (unset) {
+    ConstData cdata(&null_variant);
+    SSATmp* uninit = m_tb.gen(DefConst, Type::PtrToUninit, &cdata);
+    Type baseType = m_base->getType().deref().unbox();
+    if (baseType.subtypeOf(Type::Str)) {
+      m_ht.spillStack();
+      m_tb.gen(
+        RaiseError,
+        cns(StringData::GetStaticString(Strings::OP_NOT_SUPPORTED_STRING))
+      );
+      m_base = uninit;
+      return;
+    }
+    if (baseType.not(Type::Arr | Type::Obj)) {
+      m_base = uninit;
+      return;
+    }
+  }
+
   typedef TypedValue* (*OpFunc)(TypedValue*, TypedValue, MInstrState*);
   BUILD_OPTAB_HOT(getKeyTypeIS(key), key->isBoxed(), mia);
   m_ht.spillStack();
-  if (mia & Define) {
-    m_base = genStk(ElemDX, cns((TCA)opFunc), ptr(m_base), key, genMisPtr());
+  if (define || unset) {
+    m_base = genStk(define ? ElemDX : ElemUX, cns((TCA)opFunc), ptr(m_base),
+                    key, genMisPtr());
   } else {
     m_base = m_tb.gen(ElemX, cns((TCA)opFunc), ptr(m_base), key, genMisPtr());
   }
@@ -1171,7 +1215,7 @@ static inline TypedValue setPropImpl(Class* ctx, TypedValue* base,
                                      TypedValue keyVal, Cell val) {
   TypedValue* key = keyPtr<keyType>(keyVal);
   key = unbox<keyType, unboxKey>(key);
-  HPHP::VM::SetProp<true, isObj, keyType>(ctx, base, key, &val);
+  VM::SetProp<true, isObj, keyType>(ctx, base, key, &val);
   return val;
 }
 
@@ -1912,10 +1956,54 @@ void HhbcTranslator::VectorTranslator::emitBindElem() {
 }
 #undef HELPER_TABLE
 
-void HhbcTranslator::VectorTranslator::emitUnsetElem() {
-  SPUNT(__func__);
+template <KeyType keyType, bool unboxKey>
+static inline void unsetElemImpl(TypedValue* base, TypedValue keyVal) {
+  TypedValue* key = keyPtr<keyType>(keyVal);
+  key = unbox<keyType, unboxKey>(key);
+  VM::UnsetElem<keyType>(base, key);
 }
 
+#define HELPER_TABLE(m)                                       \
+  /* name           hot       keyType unboxKey */             \
+  m(unsetElemC,   ,            AnyKey,  false)                \
+  m(unsetElemI,   HOT_FUNC_VM, IntKey,  false)                \
+  m(unsetElemL,   ,            AnyKey,   true)                \
+  m(unsetElemLI,  ,            IntKey,   true)                \
+  m(unsetElemLS,  ,            StrKey,   true)                \
+  m(unsetElemS,   HOT_FUNC_VM, StrKey,  false)
+
+#define ELEM(nm, hot, ...)                                      \
+hot                                                             \
+void nm(TypedValue* base, TypedValue key) {                     \
+  unsetElemImpl<__VA_ARGS__>(base, key);                        \
+}
+namespace VectorHelpers {
+HELPER_TABLE(ELEM)
+}
+#undef ELEM
+
+void HhbcTranslator::VectorTranslator::emitUnsetElem() {
+  SSATmp* key = getInput(m_iInd);
+
+  Type baseType = m_base->getType().deref().unbox();
+  if (baseType.subtypeOf(Type::Str)) {
+    m_ht.spillStack();
+    m_tb.gen(RaiseError,
+             cns(StringData::GetStaticString(Strings::CANT_UNSET_STRING)));
+    return;
+  }
+  if (baseType.not(Type::Arr | Type::Obj)) {
+    // Noop
+    return;
+  }
+
+  typedef void (*OpFunc)(TypedValue*, TypedValue);
+  BUILD_OPTAB_HOT(getKeyTypeIS(key), key->isBoxed());
+  m_ht.spillStack();
+  genStk(UnsetElem, cns((TCA)opFunc), m_base, key);
+}
+#undef HELPER_TABLE
+
 void HhbcTranslator::VectorTranslator::emitNotSuppNewElem() {
   not_reached();
 }
@@ -1978,9 +2066,12 @@ void HhbcTranslator::VectorTranslator::emitMPost() {
   // Pop off all stack inputs
   m_ht.discard(nStack);
 
-  // Push result
-  assert(m_result);
-  m_ht.push(m_result);
+  // Push result, if one was produced
+  if (m_result) {
+    m_ht.push(m_result);
+  } else {
+    assert(m_ni.mInstrOp() == OpUnsetM);
+  }
 
   // Clean up tvRef(2)
   if (nLogicalRatchets() > 1) {

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

@@ -61,6 +61,10 @@ void raiseUndefProp(ObjectData* base, const StringData* name) {
   static_cast<Instance*>(base)->raiseUndefProp(name);
 }
 
+void raise_error_sd(const StringData *msg) {
+  raise_error("%s", msg->data());
+}
+
 void VerifyParamTypeFail(int paramNum) {
   VMRegAnchor _;
   const ActRec* ar = curFrame();

hphp/runtime/vm/translator/translator-runtime.h

@@ -64,6 +64,8 @@ void VerifyParamTypeCallable(TypedValue value, int param);
 void VerifyParamTypeSlow(const Class* cls, const Class* constraint, int param);
 
 
+void raise_error_sd(const StringData* sd);
+
 int64_t switchDoubleHelper(int64_t val, int64_t base, int64_t nTargets);
 int64_t switchStringHelper(StringData* s, int64_t base, int64_t nTargets);
 int64_t switchObjHelper(ObjectData* o, int64_t base, int64_t nTargets);

hphp/test/vm/unset_badbase.php

@@ -0,0 +1,17 @@
+<?php
+// Copyright 2004-present Facebook. All Rights Reserved.
+
+function main() {
+  $a = null;
+  unset($a['foo']);
+  unset($o->foo['blah']);
+  unset($o['basd']['sadfsadf']);
+  return $a;
+}
+function main2() {
+  $a = 'hello';
+  unset($a['bar']);
+}
+var_dump(main());
+main2();
+

hphp/test/vm/unset_badbase.php.exp

@@ -0,0 +1,2 @@
+NULL
+HipHop Fatal error: Cannot unset string offsets in hphp/test/vm/unset_badbase.php on line 13

hphp/test/vm/vector-stack-base.php

@@ -11,6 +11,9 @@ class dumper {
   }
   function __destruct() {
     printf("dumper %d destructing\n", $this->n);
+    if (isset($this->arr)) {
+      var_dump($this->arr);
+    }
     var_dump($this);
   }
 }
@@ -54,11 +57,15 @@ function main() {
   var_dump($ref);
   $ref = null;
 
+  // SetM and CGetM with an object base on the stack
   makeObj()->prop = 'foo';
   var_dump(makeObj()->prop);
   makeObjRef()->prop = 'bar';
   var_dump(makeObjRef()->prop[2]);
 
+  // UnsetM
+  unset(makeArr()['dumper']);
+
   echo "Done with main\n";
 }
 

hphp/test/vm/vector-stack-base.php.exp

@@ -148,5 +148,13 @@ object(dumper)#5 (2) {
   string(13) "default value"
 }
 string(1) "f"
+dumper 11 constructing
+dumper 11 destructing
+object(dumper)#5 (2) {
+  ["n":"dumper":private]=>
+  int(11)
+  ["prop"]=>
+  string(13) "default value"
+}
 Done with main
 Main has returned