From 85aa9442acd23616c6d5c68a4ea5dd67db420361 Mon Sep 17 00:00:00 2001
From: Jordan DeLong <jdelong@fb.com>
Date: Mon, 27 May 2013 11:41:41 -0700
Subject: [PATCH] Fix AddNewElemC bugs in HHIR

emitAddNewElemC didn't properly implement the specified
behavior for this opcode.  It assumed topC(1) was always an non-static
array with refcount 1 or 0.  This changes to interp the case when it's
not an array, and make the helper support static arrays.  (If it
matters for perf, we should engineer the bytecode spec to require this
as well, since the point of this opcode is initializing array literals
that can't be done as static arrays.)  In practice I think this is
only working because it always comes after a NewArray with a capacity
hint != 1, which means it's a newly allocated non-static array.
Ignoring the capacity hint would've broken it.
---
 hphp/runtime/base/array/hphp_array.cpp        | 13 ++++++----
 .../vm/translator/hopt/hhbctranslator.cpp     |  9 ++++---
 hphp/test/quick/asm_array_elem.hhas           | 24 +++++++++++++++++++
 hphp/test/quick/asm_array_elem.hhas.expect    |  4 ++++
 hphp/test/quick/asm_bad_array_elem.hhas       | 18 ++++++++++++++
 .../quick/asm_bad_array_elem.hhas.expectf     |  1 +
 6 files changed, 61 insertions(+), 8 deletions(-)
 create mode 100644 hphp/test/quick/asm_array_elem.hhas
 create mode 100644 hphp/test/quick/asm_array_elem.hhas.expect
 create mode 100644 hphp/test/quick/asm_bad_array_elem.hhas
 create mode 100644 hphp/test/quick/asm_bad_array_elem.hhas.expectf

diff --git a/hphp/runtime/base/array/hphp_array.cpp b/hphp/runtime/base/array/hphp_array.cpp
index 6a0a92d0e..32663560d 100644
--- a/hphp/runtime/base/array/hphp_array.cpp
+++ b/hphp/runtime/base/array/hphp_array.cpp
@@ -1423,11 +1423,13 @@ ArrayData* HphpArray::append(CVarRef v, bool copy) {
  */
 static NEVER_INLINE
 ArrayData* genericAddNewElemC(ArrayData* a, TypedValue value) {
-  assert(a->getCount() <= 1);
-  ArrayData* UNUSED r = a->append(tvAsCVarRef(&value), false);
+  ArrayData* r = a->append(tvAsCVarRef(&value), a->getCount() != 1);
+  if (UNLIKELY(r != a)) {
+    r->incRefCount();
+    decRefArr(a);
+  }
   tvRefcountedDecRef(value);
-  assert(r == a);
-  return a;
+  return r;
 }
 
 /*
@@ -1436,12 +1438,13 @@ ArrayData* genericAddNewElemC(ArrayData* a, TypedValue value) {
  * hphp_array.h.
  */
 ArrayData* HphpArray::AddNewElemC(ArrayData* a, TypedValue value) {
-  assert(a->getCount() <= 1 && value.m_type != KindOfRef);
+  assert(value.m_type != KindOfRef);
   HphpArray* h;
   ElmInd* ei;
   int64_t k;
   if (LIKELY(a->isHphpArray()) &&
       ((h = (HphpArray*)a), LIKELY(h->m_pos >= 0)) &&
+      LIKELY(h->getCount() <= 1) &&
       LIKELY(!h->isFull()) &&
       ((k = h->m_nextKI), LIKELY(k >= 0)) &&
       ((ei = &h->m_hash[k & h->m_tableMask]), LIKELY(!validElmInd(*ei)))) {
diff --git a/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp b/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp
index cd1b802d8..ee0c69c1c 100644
--- a/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp
+++ b/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp
@@ -467,9 +467,12 @@ void HhbcTranslator::emitAddElemC() {
 }
 
 void HhbcTranslator::emitAddNewElemC() {
-  // TODO(#2437059): this is broken
-  SSATmp* val = popC();
-  SSATmp* arr = popC();
+  if (!topC(1)->isA(Type::Arr)) {
+    return emitInterpOneOrPunt(Type::Arr, 2, 0);
+  }
+
+  auto const val = popC();
+  auto const arr = popC();
   // The AddNewElem helper decrefs its args, so don't decref pop'ed values.
   push(gen(AddNewElem, arr, val));
 }
diff --git a/hphp/test/quick/asm_array_elem.hhas b/hphp/test/quick/asm_array_elem.hhas
new file mode 100644
index 000000000..7e20e46f5
--- /dev/null
+++ b/hphp/test/quick/asm_array_elem.hhas
@@ -0,0 +1,24 @@
+
+.main {
+  FPushFuncD 0 "main"
+  FCall 0
+  PopR
+  Int 1
+  RetC
+}
+
+.function main() {
+  NewArray
+  String "asd"
+  AddNewElemC
+  SetL $foo
+  PopC
+
+  FPushFuncD 1 "var_dump"
+  FPassL 0 $foo
+  FCall 1
+  PopR
+
+  Int 1
+  RetC
+}
diff --git a/hphp/test/quick/asm_array_elem.hhas.expect b/hphp/test/quick/asm_array_elem.hhas.expect
new file mode 100644
index 000000000..feeb8ee9e
--- /dev/null
+++ b/hphp/test/quick/asm_array_elem.hhas.expect
@@ -0,0 +1,4 @@
+array(1) {
+  [0]=>
+  string(3) "asd"
+}
diff --git a/hphp/test/quick/asm_bad_array_elem.hhas b/hphp/test/quick/asm_bad_array_elem.hhas
new file mode 100644
index 000000000..970424ced
--- /dev/null
+++ b/hphp/test/quick/asm_bad_array_elem.hhas
@@ -0,0 +1,18 @@
+
+.main {
+  FPushFuncD 0 "main"
+  FCall 0
+  PopR
+  Int 1
+  RetC
+}
+
+.function main() {
+  String "asd"
+  String "asd"
+  AddNewElemC
+  Print
+  PopC
+  Int 1
+  RetC
+}
diff --git a/hphp/test/quick/asm_bad_array_elem.hhas.expectf b/hphp/test/quick/asm_bad_array_elem.hhas.expectf
new file mode 100644
index 000000000..8dc4bd167
--- /dev/null
+++ b/hphp/test/quick/asm_bad_array_elem.hhas.expectf
@@ -0,0 +1 @@
+HipHop Fatal error: AddNewElemC: $2 must be an array in %s on line %s