From 8b9fb5565bdd18c46ef4700bf4c501dd157c948e Mon Sep 17 00:00:00 2001 From: Gang Zhao Date: Wed, 4 Sep 2024 23:41:15 -0700 Subject: [PATCH] Add owning cell to parameters of GCHermesValueBase::set() Differential Revision: D62196480 --- include/hermes/VM/ArrayStorage.h | 2 +- include/hermes/VM/HermesValue-inline.h | 23 +++++++++++++------- include/hermes/VM/HermesValue.h | 29 ++++++++++++++++++-------- include/hermes/VM/JSObject.h | 4 ++-- include/hermes/VM/SegmentedArray.h | 2 +- include/hermes/VM/StringPrimitive.h | 2 +- lib/VM/ArrayStorage.cpp | 9 +++++--- lib/VM/Interpreter.cpp | 12 +++++------ lib/VM/JSObject.cpp | 3 ++- lib/VM/OrderedHashMap.cpp | 6 +++--- lib/VM/SegmentedArray.cpp | 17 ++++++++++----- lib/VM/StaticH.cpp | 7 ++++--- 12 files changed, 73 insertions(+), 43 deletions(-) diff --git a/include/hermes/VM/ArrayStorage.h b/include/hermes/VM/ArrayStorage.h index 6247d0e2c84..8efec26a125 100644 --- a/include/hermes/VM/ArrayStorage.h +++ b/include/hermes/VM/ArrayStorage.h @@ -151,7 +151,7 @@ class ArrayStorageBase final template 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 diff --git a/include/hermes/VM/HermesValue-inline.h b/include/hermes/VM/HermesValue-inline.h index 4bd456aeca1..9090c55248b 100644 --- a/include/hermes/VM/HermesValue-inline.h +++ b/include/hermes/VM/HermesValue-inline.h @@ -49,13 +49,15 @@ GCHermesValueBase::GCHermesValueBase(HVType hv, GC &gc, std::nullptr_t) template template -inline void GCHermesValueBase::set(HVType hv, GC &gc) { +inline void +GCHermesValueBase::set(HVType hv, GC &gc, const GCCell *cell) { 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)cell; if (NeedsBarriers::value) gc.writeBarrier(this, hv); HVType::setNoBarrier(hv); @@ -81,10 +83,11 @@ inline void GCHermesValueBase::fill( InputIt start, InputIt end, HVType fill, - GC &gc) { + GC &gc, + const GCCell *cell) { if (fill.isPointer()) { for (auto cur = start; cur != end; ++cur) { - cur->set(fill, gc); + cur->set(fill, gc, cell); } } else { for (auto cur = start; cur != end; ++cur) { @@ -120,7 +123,8 @@ inline OutputIt GCHermesValueBase::copy( InputIt first, InputIt last, OutputIt result, - GC &gc) { + GC &gc, + const GCCell *cell) { #if !defined(HERMESVM_GC_HADES) && !defined(HERMESVM_GC_RUNTIME) static_assert( !std::is_same::value || @@ -128,7 +132,7 @@ inline OutputIt GCHermesValueBase::copy( "Pointer arguments must invoke pointer overload."); #endif for (; first != last; ++first, (void)++result) { - result->set(*first, gc); + result->set(*first, gc, cell); } return result; } @@ -160,13 +164,15 @@ inline GCHermesValueBase *GCHermesValueBase::copy( GCHermesValueBase *first, GCHermesValueBase *last, GCHermesValueBase *result, - GC &gc) { + GC &gc, + const GCCell *cell) { // 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. + (void)cell; gc.writeBarrierRange(result, last - first); std::memmove( reinterpret_cast(result), @@ -208,9 +214,10 @@ inline OutputIt GCHermesValueBase::copy_backward( InputIt first, InputIt last, OutputIt result, - GC &gc) { + GC &gc, + const GCCell *cell) { while (first != last) { - (--result)->set(*--last, gc); + (--result)->set(*--last, gc, cell); } return result; } diff --git a/include/hermes/VM/HermesValue.h b/include/hermes/VM/HermesValue.h index 9a892e90cc7..d7a1591d48f 100644 --- a/include/hermes/VM/HermesValue.h +++ b/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 - inline void set(HVType hv, GC &gc); + inline void set(HVType hv, GC &gc, const GCCell *cell); /// The HermesValue \p hv must not be an object pointer. Assign the /// value. @@ -552,7 +553,8 @@ 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 - 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 *cell); /// Same as \p fill except the range expressed by [\p first, \p last) has not /// been previously initialized. Cannot use this on previously initialized @@ -563,8 +565,12 @@ class GCHermesValueBase final : public HVType { /// Copies a range of values and performs a write barrier on each. template - static inline OutputIt - copy(InputIt first, InputIt last, OutputIt result, GC &gc); + static inline OutputIt copy( + InputIt first, + InputIt last, + OutputIt result, + GC &gc, + const GCCell *cell); /// Same as \p copy, but the range [result, result + (last - first)) has not /// been previously initialized. Cannot use this on previously initialized @@ -579,7 +585,8 @@ class GCHermesValueBase final : public HVType { GCHermesValueBase *first, GCHermesValueBase *last, GCHermesValueBase *result, - GC &gc); + GC &gc, + const GCCell *cell); #endif /// Same as \p uninitialized_copy, but specialized for raw pointers. This is @@ -595,8 +602,12 @@ class GCHermesValueBase final : public HVType { /// Copies a range of values and performs a write barrier on each. template - 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 *cell); /// Same as \c unreachableWriteBarrier, but for a range of values all becoming /// unreachable. diff --git a/include/hermes/VM/JSObject.h b/include/hermes/VM/JSObject.h index e60bb4bec13..2782e43f28d 100644 --- a/include/hermes/VM/JSObject.h +++ b/include/hermes/VM/JSObject.h @@ -1736,7 +1736,7 @@ template 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( - value, runtime.getHeap()); + value, runtime.getHeap(), self); } inline void JSObject::setNamedSlotValueIndirectUnsafe( diff --git a/include/hermes/VM/SegmentedArray.h b/include/hermes/VM/SegmentedArray.h index 8b17b6fdbd6..9949cea4e07 100644 --- a/include/hermes/VM/SegmentedArray.h +++ b/include/hermes/VM/SegmentedArray.h @@ -288,7 +288,7 @@ class SegmentedArrayBase final : public VariableSizeRuntimeCell, /// Sets the element located at \p index to \p val. template void set(Runtime &runtime, TotalIndex index, HVType val) { - atRef(runtime, index).set(val, runtime.getHeap()); + atRef(runtime, index).set(val, runtime.getHeap(), this); } template void setNonPtr(Runtime &runtime, TotalIndex index, HVType val) { diff --git a/include/hermes/VM/StringPrimitive.h b/include/hermes/VM/StringPrimitive.h index e98cec3f75e..e3e0a8d72b6 100644 --- a/include/hermes/VM/StringPrimitive.h +++ b/include/hermes/VM/StringPrimitive.h @@ -743,7 +743,7 @@ class BufferedStringPrimitive final : public StringPrimitive { Handle> 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"); diff --git a/lib/VM/ArrayStorage.cpp b/lib/VM/ArrayStorage.cpp index c655bf0166e..868a52448e8 100644 --- a/lib/VM/ArrayStorage.cpp +++ b/lib/VM/ArrayStorage.cpp @@ -185,7 +185,8 @@ ExecutionStatus ArrayStorageBase::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. @@ -193,7 +194,8 @@ ExecutionStatus ArrayStorageBase::shift( self->data() + fromFirst, self->data() + fromFirst + copySize, self->data() + toFirst + copySize, - runtime.getHeap()); + runtime.getHeap(), + self); } // Initialize the elements which were emptied in front. @@ -201,7 +203,8 @@ ExecutionStatus ArrayStorageBase::shift( 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) { diff --git a/lib/VM/Interpreter.cpp b/lib/VM/Interpreter.cpp index 8e00660452b..3179b150a39 100644 --- a/lib/VM/Interpreter.cpp +++ b/lib/VM/Interpreter.cpp @@ -2077,16 +2077,16 @@ CallResult Interpreter::interpretFunction( } CASE(StoreToEnvironment) { - vmcast(O1REG(StoreToEnvironment)) - ->slot(ip->iStoreToEnvironment.op2) - .set(O3REG(StoreToEnvironment), runtime.getHeap()); + auto *environment = vmcast(O1REG(StoreToEnvironment)); + environment->slot(ip->iStoreToEnvironment.op2) + .set(O3REG(StoreToEnvironment), runtime.getHeap(), environment); ip = NEXTINST(StoreToEnvironment); DISPATCH; } CASE(StoreToEnvironmentL) { - vmcast(O1REG(StoreToEnvironmentL)) - ->slot(ip->iStoreToEnvironmentL.op2) - .set(O3REG(StoreToEnvironmentL), runtime.getHeap()); + auto *environment = vmcast(O1REG(StoreToEnvironmentL)); + environment->slot(ip->iStoreToEnvironmentL.op2) + .set(O3REG(StoreToEnvironmentL), runtime.getHeap(), environment); ip = NEXTINST(StoreToEnvironmentL); DISPATCH; } diff --git a/lib/VM/JSObject.cpp b/lib/VM/JSObject.cpp index 86791f701f0..aae02e442b6 100644 --- a/lib/VM/JSObject.cpp +++ b/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; } diff --git a/lib/VM/OrderedHashMap.cpp b/lib/VM/OrderedHashMap.cpp index b1d55758289..004336afcbf 100644 --- a/lib/VM/OrderedHashMap.cpp +++ b/lib/VM/OrderedHashMap.cpp @@ -270,7 +270,7 @@ ExecutionStatus OrderedHashMapBase::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::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) { auto v = SmallHermesValue::encodeHermesValue(value.getHermesValue(), runtime); - newMapEntry->value.set(v, runtime.getHeap()); + newMapEntry->value.set(v, runtime.getHeap(), *newMapEntry); } // After here, no allocation diff --git a/lib/VM/SegmentedArray.cpp b/lib/VM/SegmentedArray.cpp index 1ef68f43a1d..0e3a96dd178 100644 --- a/lib/VM/SegmentedArray.cpp +++ b/lib/VM/SegmentedArray.cpp @@ -265,7 +265,7 @@ void SegmentedArrayBase::allocateSegment( "Allocating into a non-empty segment"); PseudoHandle c = Segment::create(runtime); self->segmentAtPossiblyUnallocated(segment)->set( - HVType::encodeObjectValue(c.get(), runtime), runtime.getHeap()); + HVType::encodeObjectValue(c.get(), runtime), runtime.getHeap(), c.get()); } template @@ -327,7 +327,8 @@ ExecutionStatus SegmentedArrayBase::growLeft( self->begin(runtime), self->end(runtime), newSegmentedArray->begin(runtime) + amount, - runtime.getHeap()); + runtime.getHeap(), + *self); // Assign back to self. self = newSegmentedArray.get(); return ExecutionStatus::RETURNED; @@ -348,13 +349,15 @@ void SegmentedArrayBase::growLeftWithinCapacity( self->begin(runtime), self->end(runtime) - amount, self->end(runtime), - runtime.getHeap()); + runtime.getHeap(), + self.get()); // Fill the beginning with empty values. GCHVType::fill( self->begin(runtime), self->begin(runtime) + amount, HVType::encodeEmptyValue(), - runtime.getHeap()); + runtime.getHeap(), + self.get()); } template @@ -370,7 +373,11 @@ void SegmentedArrayBase::shrinkLeft( size_type amount) { // Copy the end values leftwards to the beginning. GCHVType::copy( - begin(runtime) + amount, end(runtime), begin(runtime), runtime.getHeap()); + begin(runtime) + amount, + end(runtime), + begin(runtime), + runtime.getHeap(), + this); // Now that all the values are moved down, fill the end with empty values. decreaseSize(runtime, amount); } diff --git a/lib/VM/StaticH.cpp b/lib/VM/StaticH.cpp index 5783041cef4..1f0bd718486 100644 --- a/lib/VM/StaticH.cpp +++ b/lib/VM/StaticH.cpp @@ -519,9 +519,10 @@ extern "C" void _sh_ljs_store_to_env( SHLegacyValue env, SHLegacyValue val, uint32_t index) { - vmcast(HermesValue::fromRaw(env.raw)) - ->slot(index) - .set(HermesValue::fromRaw(val.raw), getRuntime(shr).getHeap()); + auto *environment = vmcast(HermesValue::fromRaw(env.raw)); + + environment->slot(index).set( + HermesValue::fromRaw(val.raw), getRuntime(shr).getHeap(), environment); } extern "C" void _sh_ljs_store_np_to_env(