Pass the pointer of owning object in GCHermesValueBase::set()

Summary:
Add this parameter so that it could be passed to write barrier function
in following diffs. For not it's unused.

We don't create a overload or make a default parameter value because
it's error prone: new code may call wrong functions (unless reasoning
whether the object lives in normal segment or large segment), or forget
to update existing calls. It's actually easier to just always pass in
the owning object pointer, which in most cases is obvious to get.

Differential Revision: D62196480

include/hermes/VM/ArrayStorage.h

@@ -151,7 +151,7 @@ class ArrayStorageBase final
   template <Inline inl = Inline::No>
   void set(size_type index, HVType val, GC &gc) {
     assert(index < size() && "index out of range");
-    data()[index].set(val, gc);
+    data()[index].set(val, gc, this);
   }
 
   /// \return the element at index \p index

include/hermes/VM/HermesValue-inline.h

@@ -49,13 +49,15 @@ GCHermesValueBase<HVType>::GCHermesValueBase(HVType hv, GC &gc, std::nullptr_t)
 
 template <typename HVType>
 template <typename NeedsBarriers>
-inline void GCHermesValueBase<HVType>::set(HVType hv, GC &gc) {
+inline void
+GCHermesValueBase<HVType>::set(HVType hv, GC &gc, const GCCell *owningObj) {
   if (hv.isPointer()) {
     HERMES_SLOW_ASSERT(
         gc.validPointer(hv.getPointer(gc.getPointerBase())) &&
         "Setting an invalid pointer into a GCHermesValue");
   }
   assert(NeedsBarriers::value || !gc.needsWriteBarrier(this, hv));
+  (void)owningObj;
   if (NeedsBarriers::value)
     gc.writeBarrier(this, hv);
   HVType::setNoBarrier(hv);
@@ -81,10 +83,11 @@ inline void GCHermesValueBase<HVType>::fill(
     InputIt start,
     InputIt end,
     HVType fill,
-    GC &gc) {
+    GC &gc,
+    const GCCell *owningObj) {
   if (fill.isPointer()) {
     for (auto cur = start; cur != end; ++cur) {
-      cur->set(fill, gc);
+      cur->set(fill, gc, owningObj);
     }
   } else {
     for (auto cur = start; cur != end; ++cur) {
@@ -121,14 +124,13 @@ inline OutputIt GCHermesValueBase<HVType>::copy(
     InputIt last,
     OutputIt result,
     GC &gc) {
-#if !defined(HERMESVM_GC_HADES) && !defined(HERMESVM_GC_RUNTIME)
   static_assert(
       !std::is_same<InputIt, GCHermesValueBase *>::value ||
           !std::is_same<OutputIt, GCHermesValueBase *>::value,
       "Pointer arguments must invoke pointer overload.");
-#endif
   for (; first != last; ++first, (void)++result) {
-    result->set(*first, gc);
+    auto [hv, owningObj] = first.get_cell_value();
+    result->set(*first, gc, owningObj);
   }
   return result;
 }
@@ -150,31 +152,38 @@ inline OutputIt GCHermesValueBase<HVType>::uninitialized_copy(
   return result;
 }
 
-// Specializations using memmove can't be used in Hades, because the concurrent
-// write barrier needs strict control over how assignments are done to HV fields
-// which need to be atomically updated.
-#if !defined(HERMESVM_GC_HADES) && !defined(HERMESVM_GC_RUNTIME)
 /// Specialization for raw pointers to do a ranged write barrier.
 template <typename HVType>
 inline GCHermesValueBase<HVType> *GCHermesValueBase<HVType>::copy(
     GCHermesValueBase<HVType> *first,
     GCHermesValueBase<HVType> *last,
     GCHermesValueBase<HVType> *result,
-    GC &gc) {
+    GC &gc,
+    const GCCell *owningObj) {
+// Specializations using memmove can't be used in Hades, because the concurrent
+// write barrier needs strict control over how assignments are done to HV fields
+// which need to be atomically updated.
+#if !defined(HERMESVM_GC_HADES) && !defined(HERMESVM_GC_RUNTIME)
+  gc.writeBarrierRange(result, last - first);
   // We must use "raw" function such as memmove here, rather than a
   // function like std::copy (or copy_backward) that respects
   // constructors and operator=.  For HermesValue, those require the
   // contents not to contain pointers.  The range write barrier
   // before the copies ensure that sufficient barriers are
   // performed.
-  gc.writeBarrierRange(result, last - first);
+  (void)owningObj;
   std::memmove(
       reinterpret_cast<void *>(result),
       first,
       (last - first) * sizeof(GCHermesValueBase<HVType>));
   return result + (last - first);
-}
+#else
+  for (; first != last; ++first, (void)++result) {
+    result->set(*first, gc, owningObj);
+  }
+  return result;
 #endif
+}
 
 /// Specialization for raw pointers to do a ranged write barrier.
 template <typename HVType>
@@ -183,7 +192,7 @@ inline GCHermesValueBase<HVType> *GCHermesValueBase<HVType>::uninitialized_copy(
     GCHermesValueBase<HVType> *last,
     GCHermesValueBase<HVType> *result,
     GC &gc,
-    const GCCell *cell) {
+    const GCCell *owningObj) {
 #ifndef NDEBUG
   uintptr_t fromFirst = reinterpret_cast<uintptr_t>(first),
             fromLast = reinterpret_cast<uintptr_t>(last);
@@ -195,7 +204,7 @@ inline GCHermesValueBase<HVType> *GCHermesValueBase<HVType>::uninitialized_copy(
       "Uninitialized range cannot overlap with an initialized one.");
 #endif
 
-  gc.constructorWriteBarrierRange(cell, result, last - first);
+  gc.constructorWriteBarrierRange(owningObj, result, last - first);
   // memcpy is fine for an uninitialized copy.
   std::memcpy(
       reinterpret_cast<void *>(result), first, (last - first) * sizeof(HVType));
@@ -208,9 +217,10 @@ inline OutputIt GCHermesValueBase<HVType>::copy_backward(
     InputIt first,
     InputIt last,
     OutputIt result,
-    GC &gc) {
+    GC &gc,
+    const GCCell *owningObj) {
   while (first != last) {
-    (--result)->set(*--last, gc);
+    (--result)->set(*--last, gc, owningObj);
   }
   return result;
 }

include/hermes/VM/HermesValue.h

@@ -531,10 +531,11 @@ class GCHermesValueBase final : public HVType {
   GCHermesValueBase(HVType hv, GC &gc, std::nullptr_t);
   GCHermesValueBase(const HVType &) = delete;
 
-  /// The HermesValue \p hv may be an object pointer.  Assign the
-  /// value, and perform any necessary write barriers.
+  /// The HermesValue \p hv may be an object pointer. Assign the value, and
+  /// perform any necessary write barriers. \p cell is the object that contains
+  /// this GCHermesValueBase. It's needed by the write barrier.
   template <typename NeedsBarriers = std::true_type>
-  inline void set(HVType hv, GC &gc);
+  inline void set(HVType hv, GC &gc, const GCCell *owningObj);
 
   /// The HermesValue \p hv must not be an object pointer.  Assign the
   /// value.
@@ -552,7 +553,12 @@ class GCHermesValueBase final : public HVType {
   /// value \p fill.  If the fill value is an object pointer, must
   /// provide a non-null \p gc argument, to perform write barriers.
   template <typename InputIt>
-  static inline void fill(InputIt first, InputIt last, HVType fill, GC &gc);
+  static inline void fill(
+      InputIt first,
+      InputIt last,
+      HVType fill,
+      GC &gc,
+      const GCCell *owningObj);
 
   /// Same as \p fill except the range expressed by  [\p first, \p last) has not
   /// been previously initialized. Cannot use this on previously initialized
@@ -573,14 +579,13 @@ class GCHermesValueBase final : public HVType {
   static inline OutputIt
   uninitialized_copy(InputIt first, InputIt last, OutputIt result, GC &gc);
 
-#if !defined(HERMESVM_GC_HADES) && !defined(HERMESVM_GC_RUNTIME)
   /// Same as \p copy, but specialized for raw pointers.
   static inline GCHermesValueBase<HVType> *copy(
       GCHermesValueBase<HVType> *first,
       GCHermesValueBase<HVType> *last,
       GCHermesValueBase<HVType> *result,
-      GC &gc);
-#endif
+      GC &gc,
+      const GCCell *owningObj);
 
   /// Same as \p uninitialized_copy, but specialized for raw pointers. This is
   /// unsafe to use if the memory region being copied into (pointed to by
@@ -591,12 +596,16 @@ class GCHermesValueBase final : public HVType {
       GCHermesValueBase<HVType> *last,
       GCHermesValueBase<HVType> *result,
       GC &gc,
-      const GCCell *cell);
+      const GCCell *owningObj);
 
   /// Copies a range of values and performs a write barrier on each.
   template <typename InputIt, typename OutputIt>
-  static inline OutputIt
-  copy_backward(InputIt first, InputIt last, OutputIt result, GC &gc);
+  static inline OutputIt copy_backward(
+      InputIt first,
+      InputIt last,
+      OutputIt result,
+      GC &gc,
+      const GCCell *owningObj);
 
   /// Same as \c unreachableWriteBarrier, but for a range of values all becoming
   /// unreachable.

include/hermes/VM/JSObject.h

@@ -1736,7 +1736,7 @@ template <SlotIndex index>
 inline void
 JSObject::setDirectSlotValue(JSObject *self, SmallHermesValue value, GC &gc) {
   static_assert(index < DIRECT_PROPERTY_SLOTS, "Must be a direct property");
-  self->directProps()[index].set(value, gc);
+  self->directProps()[index].set(value, gc, self);
 }
 
 inline SmallHermesValue JSObject::getNamedSlotValueUnsafe(
@@ -1839,7 +1839,7 @@ inline void JSObject::setNamedSlotValueDirectUnsafe(
   // to namedSlotRef(), it is a slight performance regression, which is not
   // entirely unexpected.
   return self->directProps()[index].set<NeedsBarriers>(
-      value, runtime.getHeap());
+      value, runtime.getHeap(), self);
 }
 
 inline void JSObject::setNamedSlotValueIndirectUnsafe(

include/hermes/VM/SegmentedArray.h

@@ -161,6 +161,8 @@ class SegmentedArrayBase final : public VariableSizeRuntimeCell,
     using difference_type = ptrdiff_t;
     using pointer = GCHVType *;
     using reference = GCHVType &;
+    // Dereference a value
+    using cell_value_pair = std::pair<reference, GCCell *>;
 
     /// The SegmentedArray which owns this iterator. This iterator should never
     /// be compared against an iterator with a different owner. This is used to
@@ -238,6 +240,24 @@ class SegmentedArrayBase final : public VariableSizeRuntimeCell,
       }
     }
 
+    /// Return the value and the owning cell of that value (either owner_ or the
+    /// extra Segment allocated for the current index_).
+    /// Note: this is a temporary workaround to pass the correct owning object
+    /// pointer to write barrier. Once we support large allocation, we could
+    /// get rid of this (and probably the entire iterator here).
+    cell_value_pair get_cell_value() {
+      assert(
+          index_ < owner_->size(base_) &&
+          "Trying to read from an index outside the size");
+      // Almost all arrays fit entirely in the inline storage.
+      if (LLVM_LIKELY(index_ < kValueToSegmentThreshold)) {
+        return {owner_->inlineStorage()[index_], owner_};
+      } else {
+        auto *segment = owner_->segmentAt(base_, toSegment(index_));
+        return {segment->at(toInterior(index_)), segment};
+      }
+    }
+
     pointer operator->() {
       return &**this;
     }
@@ -288,7 +308,25 @@ class SegmentedArrayBase final : public VariableSizeRuntimeCell,
   /// Sets the element located at \p index to \p val.
   template <Inline inl = Inline::No>
   void set(Runtime &runtime, TotalIndex index, HVType val) {
-    atRef<inl>(runtime, index).set(val, runtime.getHeap());
+    assert(index < size(runtime) && "Out-of-bound access");
+    if constexpr (inl == Inline::Yes) {
+      assert(
+          index < kValueToSegmentThreshold &&
+          "Using the inline storage accessor when the index is larger than the "
+          "inline storage");
+      inlineStorage()[index].set(val, runtime.getHeap(), this);
+    }
+    // The caller may not set inl to Inline::Yes when the index is actually in
+    // inline storage.
+    if (index < kValueToSegmentThreshold) {
+      inlineStorage()[index].set(val, runtime.getHeap(), this);
+    } else {
+      auto segmentNumber = toSegment(index);
+      auto *segment = segmentAt(runtime, segmentNumber);
+      auto &elm = segment->at(toInterior(index));
+      // elm lives in segment, which is not the same cell as SegmentedArrayBase.
+      elm.set(val, runtime.getHeap(), segment);
+    }
   }
   template <Inline inl = Inline::No>
   void setNonPtr(Runtime &runtime, TotalIndex index, HVType val) {

include/hermes/VM/StringPrimitive.h

@@ -743,7 +743,7 @@ class BufferedStringPrimitive final : public StringPrimitive {
       Handle<ExternalStringPrimitive<T>> concatBuffer)
       : StringPrimitive(length) {
     concatBufferHV_.set(
-        HermesValue::encodeObjectValue(*concatBuffer), runtime.getHeap());
+        HermesValue::encodeObjectValue(*concatBuffer), runtime.getHeap(), this);
     assert(
         concatBuffer->contents_.size() >= length &&
         "length exceeds size of concatenation buffer");

lib/VM/ArrayStorage.cpp

@@ -185,23 +185,26 @@ ExecutionStatus ArrayStorageBase<HVType>::shift(
           self->data() + fromFirst,
           self->data() + fromFirst + copySize,
           self->data() + toFirst,
-          runtime.getHeap());
+          runtime.getHeap(),
+          self);
     } else if (fromFirst < toFirst) {
       // Copying to the right, need to copy backwards to avoid overwriting what
       // is being copied.
       GCHVType::copy_backward(
           self->data() + fromFirst,
           self->data() + fromFirst + copySize,
           self->data() + toFirst + copySize,
-          runtime.getHeap());
+          runtime.getHeap(),
+          self);
     }
 
     // Initialize the elements which were emptied in front.
     GCHVType::fill(
         self->data(),
         self->data() + toFirst,
         HVType::encodeEmptyValue(),
-        runtime.getHeap());
+        runtime.getHeap(),
+        self);
 
     // Initialize the elements between the last copied element and toLast.
     if (toFirst + copySize < toLast) {

lib/VM/Interpreter.cpp

@@ -2077,16 +2077,16 @@ CallResult<HermesValue> Interpreter::interpretFunction(
       }
 
       CASE(StoreToEnvironment) {
-        vmcast<Environment>(O1REG(StoreToEnvironment))
-            ->slot(ip->iStoreToEnvironment.op2)
-            .set(O3REG(StoreToEnvironment), runtime.getHeap());
+        auto *environment = vmcast<Environment>(O1REG(StoreToEnvironment));
+        environment->slot(ip->iStoreToEnvironment.op2)
+            .set(O3REG(StoreToEnvironment), runtime.getHeap(), environment);
         ip = NEXTINST(StoreToEnvironment);
         DISPATCH;
       }
       CASE(StoreToEnvironmentL) {
-        vmcast<Environment>(O1REG(StoreToEnvironmentL))
-            ->slot(ip->iStoreToEnvironmentL.op2)
-            .set(O3REG(StoreToEnvironmentL), runtime.getHeap());
+        auto *environment = vmcast<Environment>(O1REG(StoreToEnvironmentL));
+        environment->slot(ip->iStoreToEnvironmentL.op2)
+            .set(O3REG(StoreToEnvironmentL), runtime.getHeap(), environment);
         ip = NEXTINST(StoreToEnvironmentL);
         DISPATCH;
       }

lib/VM/JSObject.cpp

@@ -237,7 +237,8 @@ void JSObject::allocateNewSlotStorage(
   // If it is a direct property, just store the value and we are done.
   if (LLVM_LIKELY(newSlotIndex < DIRECT_PROPERTY_SLOTS)) {
     auto shv = SmallHermesValue::encodeHermesValue(*valueHandle, runtime);
-    selfHandle->directProps()[newSlotIndex].set(shv, runtime.getHeap());
+    selfHandle->directProps()[newSlotIndex].set(
+        shv, runtime.getHeap(), *selfHandle);
     return;
   }
 

lib/VM/OrderedHashMap.cpp

@@ -270,7 +270,7 @@ ExecutionStatus OrderedHashMapBase<BucketType, Derived>::insert(
         self->lookupInBucket(runtime, bucket, key.getHermesValue());
     if (entry) {
       // Element for the key already exists, update value and return.
-      entry->value.set(shv, runtime.getHeap());
+      entry->value.set(shv, runtime.getHeap(), entry);
       return ExecutionStatus::RETURNED;
     }
   }
@@ -337,11 +337,11 @@ ExecutionStatus OrderedHashMapBase<BucketType, Derived>::doInsert(
   // call it and set to newMapEntry one at a time.
   auto newMapEntry = runtime.makeHandle(std::move(*crtRes));
   auto k = SmallHermesValue::encodeHermesValue(key.getHermesValue(), runtime);
-  newMapEntry->key.set(k, runtime.getHeap());
+  newMapEntry->key.set(k, runtime.getHeap(), *newMapEntry);
   if constexpr (std::is_same_v<BucketType, HashMapEntry>) {
     auto v =
         SmallHermesValue::encodeHermesValue(value.getHermesValue(), runtime);
-    newMapEntry->value.set(v, runtime.getHeap());
+    newMapEntry->value.set(v, runtime.getHeap(), *newMapEntry);
   }
 
   // After here, no allocation