[handles] Add a PatchValue method for Handle

We currently have a pattern of setting a dereferenced Handle location to
update that Handle's value:

  *handle.location() = new_value.ptr()

This is slightly opaque, and definitely not type-safe, so add a new
Handle<T>::PatchValue method which does this operation.

Ideally we would make Handle::location() return a const pointer to
discourage this sort of use, but there's a bunch of places where that
location pointer is used and passed around as a Handle surrogate, so
those would have to be updated first.

Change-Id: I157f7e2473ed1b86f7a93cae260b0932fed0ad88
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2424249
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70088}

src/handles/handles.h

@@ -59,6 +59,8 @@ class HandleBase {
   V8_INLINE Address address() const { return bit_cast<Address>(location_); }
 
   // Returns the address to where the raw pointer is stored.
+  // TODO(leszeks): This should probably be a const Address*, to encourage using
+  // PatchValue for modifying the handle's value.
   V8_INLINE Address* location() const {
     SLOW_DCHECK(location_ == nullptr || IsDereferenceAllowed());
     return location_;
@@ -154,6 +156,13 @@ class Handle final : public HandleBase {
   // Location equality.
   bool equals(Handle<T> other) const { return address() == other.address(); }
 
+  // Patches this Handle's value, in-place, with a new value. All handles with
+  // the same location will see this update.
+  void PatchValue(T new_value) {
+    SLOW_DCHECK(location_ != nullptr && IsDereferenceAllowed());
+    *location_ = new_value.ptr();
+  }
+
   // Provide function object for location equality comparison.
   struct equal_to {
     V8_INLINE bool operator()(Handle<T> lhs, Handle<T> rhs) const {

src/heap/factory.cc

@@ -206,7 +206,7 @@ MaybeHandle<Code> Factory::CodeBuilder::BuildInternal(
         isolate_->builtins_constants_table_builder()->PatchSelfReference(
             self_reference, code);
       }
-      *(self_reference.location()) = code->ptr();
+      self_reference.PatchValue(*code);
     }
 
     // Likewise, any references to the basic block counters marker need to be

src/objects/allocation-site-scopes.h

@@ -27,9 +27,7 @@ class AllocationSiteContext {
   Isolate* isolate() { return isolate_; }
 
  protected:
-  void update_current_site(AllocationSite site) {
-    *(current_.location()) = site.ptr();
-  }
+  void update_current_site(AllocationSite site) { current_.PatchValue(site); }
 
   inline void InitializeTraversal(Handle<AllocationSite> site);
 

src/objects/js-objects.cc

@@ -252,7 +252,7 @@ V8_WARN_UNUSED_RESULT Maybe<bool> FastAssign(
         ASSIGN_RETURN_ON_EXCEPTION_VALUE(
             isolate, prop_value, Object::GetProperty(&it), Nothing<bool>());
         stable = from->map() == *map;
-        *descriptors.location() = map->instance_descriptors().ptr();
+        descriptors.PatchValue(map->instance_descriptors());
       }
     } else {
       // If the map did change, do a slower lookup. We are still guaranteed that
@@ -278,7 +278,7 @@ V8_WARN_UNUSED_RESULT Maybe<bool> FastAssign(
       if (result.IsNothing()) return result;
       if (stable) {
         stable = from->map() == *map;
-        *descriptors.location() = map->instance_descriptors().ptr();
+        descriptors.PatchValue(map->instance_descriptors());
       }
     } else {
       if (excluded_properties != nullptr &&
@@ -1879,7 +1879,7 @@ V8_WARN_UNUSED_RESULT Maybe<bool> FastGetOwnValuesOrEntries(
   // side-effects.
   bool stable = *map == object->map();
   if (stable) {
-    *descriptors.location() = map->instance_descriptors().ptr();
+    descriptors.PatchValue(map->instance_descriptors());
   }
 
   for (InternalIndex index : InternalIndex::Range(number_of_own_descriptors)) {
@@ -1913,7 +1913,7 @@ V8_WARN_UNUSED_RESULT Maybe<bool> FastGetOwnValuesOrEntries(
         ASSIGN_RETURN_ON_EXCEPTION_VALUE(
             isolate, prop_value, Object::GetProperty(&it), Nothing<bool>());
         stable = object->map() == *map;
-        *descriptors.location() = map->instance_descriptors().ptr();
+        descriptors.PatchValue(map->instance_descriptors());
       }
     } else {
       // If the map did change, do a slower lookup. We are still guaranteed that

src/objects/literal-objects.cc

@@ -347,7 +347,7 @@ class ObjectDescriptor {
       AddToDictionaryTemplate(isolate, properties_dictionary_template_, name,
                               value_index, value_kind, value);
     } else {
-      *temp_handle_.location() = value.ptr();
+      temp_handle_.PatchValue(value);
       AddToDescriptorArrayTemplate(isolate, descriptor_array_template_, name,
                                    value_kind, temp_handle_);
     }

src/strings/string-builder-inl.h

@@ -262,13 +262,13 @@ class IncrementalStringBuilder {
   V8_INLINE Handle<String> accumulator() { return accumulator_; }
 
   V8_INLINE void set_accumulator(Handle<String> string) {
-    *accumulator_.location() = string->ptr();
+    accumulator_.PatchValue(*string);
   }
 
   V8_INLINE Handle<String> current_part() { return current_part_; }
 
   V8_INLINE void set_current_part(Handle<String> string) {
-    *current_part_.location() = string->ptr();
+    current_part_.PatchValue(*string);
   }
 
   // Add the current part to the accumulator.