[ptr-compr] Remove i::GetIsolateFromWritableObject(HeapObject, Isolate*)

... and add i::GetIsolateFromHeapObject(HeapObject, Isolate*) and
i::IsReadOnlyHeapObject(HeapObject) instead.

Previously the removed function was also used for checking if given heap
object is a read only object. But if pointer compression is enabled
the i::GetIsolateFromHeapObject() will succeed for both read only and
writable heap objects.

Bug: v8:9379, v8:7703
Change-Id: Ib0a9babafe32f43716dac70620b51657dfb97d7c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1667416Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62291}

src/api/api.cc

@@ -5365,20 +5365,15 @@ Local<Value> Symbol::Name() const {
   i::Handle<i::Symbol> sym = Utils::OpenHandle(this);
 
   i::Isolate* isolate;
-  if (!i::GetIsolateFromWritableObject(*sym, &isolate)) {
-    // If the Symbol is in RO_SPACE, then its name must be too. Since RO_SPACE
-    // objects are immovable we can use the Handle(Address*) constructor with
-    // the address of the name field in the Symbol object without needing an
-    // isolate.
-#ifdef V8_COMPRESS_POINTERS
-    // Compressed fields can't serve as handle locations.
-    // TODO(ishell): get Isolate as a parameter.
-    isolate = i::Isolate::Current();
-#else
+  if (!i::GetIsolateFromHeapObject(*sym, &isolate)) {
+    // Symbol is in RO_SPACE, which means that its name is also in RO_SPACE.
+    // Since RO_SPACE objects are immovable we can use the Handle(Address*)
+    // constructor with the address of the name field in the Symbol object
+    // without needing an isolate.
+    DCHECK(!COMPRESS_POINTERS_BOOL);
     i::Handle<i::HeapObject> ro_name(reinterpret_cast<i::Address*>(
         sym->GetFieldAddress(i::Symbol::kNameOffset)));
     return Utils::ToLocal(ro_name);
-#endif
   }
 
   i::Handle<i::Object> name(sym->name(), isolate);
@@ -6238,8 +6233,7 @@ bool v8::String::MakeExternal(v8::String::ExternalStringResource* resource) {
 
   // It is safe to call GetIsolateFromWritableHeapObject because
   // SupportsExternalization already checked that the object is writable.
-  i::Isolate* isolate;
-  i::GetIsolateFromWritableObject(obj, &isolate);
+  i::Isolate* isolate = i::GetIsolateFromWritableObject(obj);
   ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
 
   CHECK(resource && resource->data());
@@ -6266,8 +6260,7 @@ bool v8::String::MakeExternal(
 
   // It is safe to call GetIsolateFromWritableHeapObject because
   // SupportsExternalization already checked that the object is writable.
-  i::Isolate* isolate;
-  i::GetIsolateFromWritableObject(obj, &isolate);
+  i::Isolate* isolate = i::GetIsolateFromWritableObject(obj);
   ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
 
   CHECK(resource && resource->data());

src/diagnostics/objects-printer.cc

@@ -2510,10 +2510,10 @@ void Map::MapPrint(std::ostream& os) {  // NOLINT
     layout_descriptor().ShortPrint(os);
   }
 
-  Isolate* isolate;
   // Read-only maps can't have transitions, which is fortunate because we need
   // the isolate to iterate over the transitions.
-  if (GetIsolateFromWritableObject(*this, &isolate)) {
+  if (!IsReadOnlyHeapObject(*this)) {
+    Isolate* isolate = GetIsolateFromWritableObject(*this);
     DisallowHeapAllocation no_gc;
     TransitionsAccessor transitions(isolate, *this, &no_gc);
     int nof_transitions = transitions.NumberOfTransitions();

src/handles/handles.cc

@@ -33,8 +33,8 @@ bool HandleBase::IsDereferenceAllowed(DereferenceCheckMode mode) const {
   Object object(*location_);
   if (object.IsSmi()) return true;
   HeapObject heap_object = HeapObject::cast(object);
-  Isolate* isolate;
-  if (!GetIsolateFromWritableObject(heap_object, &isolate)) return true;
+  if (IsReadOnlyHeapObject(heap_object)) return true;
+  Isolate* isolate = GetIsolateFromWritableObject(heap_object);
   RootIndex root_index;
   if (isolate->roots_table().IsRootHandleLocation(location_, &root_index) &&
       RootsTable::IsImmortalImmovable(root_index)) {

src/heap/heap-write-barrier-inl.h

@@ -239,19 +239,21 @@ inline Heap* GetHeapFromWritableObject(HeapObject object) {
 
 inline Isolate* GetIsolateFromWritableObject(HeapObject object) {
 #ifdef V8_COMPRESS_POINTERS
-  return Isolate::FromRoot(GetIsolateRoot(object.ptr()));
+  Isolate* isolate = Isolate::FromRoot(GetIsolateRoot(object.ptr()));
+  DCHECK_NOT_NULL(isolate);
+  return isolate;
 #else
   return Isolate::FromHeap(GetHeapFromWritableObject(object));
 #endif  // V8_COMPRESS_POINTERS
 }
 
-inline bool GetIsolateFromWritableObject(HeapObject obj, Isolate** isolate) {
+inline bool GetIsolateFromHeapObject(HeapObject object, Isolate** isolate) {
 #ifdef V8_COMPRESS_POINTERS
-  *isolate = GetIsolateFromWritableObject(obj);
+  *isolate = GetIsolateFromWritableObject(object);
   return true;
 #else
   heap_internals::MemoryChunk* chunk =
-      heap_internals::MemoryChunk::FromHeapObject(obj);
+      heap_internals::MemoryChunk::FromHeapObject(object);
   if (chunk->InReadOnlySpace()) {
     *isolate = nullptr;
     return false;
@@ -261,6 +263,12 @@ inline bool GetIsolateFromWritableObject(HeapObject obj, Isolate** isolate) {
 #endif  // V8_COMPRESS_POINTERS
 }
 
+inline bool IsReadOnlyHeapObject(HeapObject object) {
+  heap_internals::MemoryChunk* chunk =
+      heap_internals::MemoryChunk::FromHeapObject(object);
+  return chunk->InReadOnlySpace();
+}
+
 }  // namespace internal
 }  // namespace v8
 

src/heap/heap-write-barrier.h

@@ -44,6 +44,13 @@ void MarkingBarrierForDescriptorArray(Heap* heap, HeapObject host,
 inline Heap* GetHeapFromWritableObject(HeapObject object);
 inline Isolate* GetIsolateFromWritableObject(HeapObject object);
 
+// Returns true if it succeeded to obtain isolate from given object.
+// If it fails then the object is definitely a read-only object but it may also
+// succeed for read only objects if pointer compression is enabled.
+inline bool GetIsolateFromHeapObject(HeapObject object, Isolate** isolate);
+
+inline bool IsReadOnlyHeapObject(HeapObject object);
+
 }  // namespace internal
 }  // namespace v8
 

src/objects/string.cc

@@ -146,15 +146,15 @@ bool String::MakeExternal(v8::String::ExternalStringResource* resource) {
   int size = this->Size();  // Byte size of the original string.
   // Abort if size does not allow in-place conversion.
   if (size < ExternalString::kUncachedSize) return false;
-  Isolate* isolate;
   // Read-only strings cannot be made external, since that would mutate the
   // string.
-  if (!GetIsolateFromWritableObject(*this, &isolate)) return false;
-  Heap* heap = isolate->heap();
+  if (IsReadOnlyHeapObject(*this)) return false;
+  Isolate* isolate = GetIsolateFromWritableObject(*this);
   bool is_internalized = this->IsInternalizedString();
   bool has_pointers = StringShape(*this).IsIndirect();
+
   if (has_pointers) {
-    heap->NotifyObjectLayoutChange(*this, size, no_allocation);
+    isolate->heap()->NotifyObjectLayoutChange(*this, size, no_allocation);
   }
   // Morph the string to an external string by replacing the map and
   // reinitializing the fields.  This won't work if the space the existing
@@ -163,7 +163,7 @@ bool String::MakeExternal(v8::String::ExternalStringResource* resource) {
   // the address of the backing store.  When we encounter uncached external
   // strings in generated code, we need to bailout to runtime.
   Map new_map;
-  ReadOnlyRoots roots(heap);
+  ReadOnlyRoots roots(isolate);
   if (size < ExternalString::kSizeOfAllExternalStrings) {
     if (is_internalized) {
       new_map = roots.uncached_external_internalized_string_map();
@@ -177,10 +177,11 @@ bool String::MakeExternal(v8::String::ExternalStringResource* resource) {
 
   // Byte size of the external String object.
   int new_size = this->SizeFromMap(new_map);
-  heap->CreateFillerObjectAt(this->address() + new_size, size - new_size,
-                             ClearRecordedSlots::kNo);
+  isolate->heap()->CreateFillerObjectAt(
+      this->address() + new_size, size - new_size, ClearRecordedSlots::kNo);
   if (has_pointers) {
-    heap->ClearRecordedSlotRange(this->address(), this->address() + new_size);
+    isolate->heap()->ClearRecordedSlotRange(this->address(),
+                                            this->address() + new_size);
   }
 
   // We are storing the new map using release store after creating a filler for
@@ -189,7 +190,7 @@ bool String::MakeExternal(v8::String::ExternalStringResource* resource) {
 
   ExternalTwoByteString self = ExternalTwoByteString::cast(*this);
   self.SetResource(isolate, resource);
-  heap->RegisterExternalString(*this);
+  isolate->heap()->RegisterExternalString(*this);
   if (is_internalized) self.Hash();  // Force regeneration of the hash value.
   return true;
 }
@@ -218,18 +219,16 @@ bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) {
   int size = this->Size();  // Byte size of the original string.
   // Abort if size does not allow in-place conversion.
   if (size < ExternalString::kUncachedSize) return false;
-  Isolate* isolate;
   // Read-only strings cannot be made external, since that would mutate the
   // string.
-  if (!GetIsolateFromWritableObject(*this, &isolate)) return false;
-  Heap* heap = isolate->heap();
+  if (IsReadOnlyHeapObject(*this)) return false;
+  Isolate* isolate = GetIsolateFromWritableObject(*this);
   bool is_internalized = this->IsInternalizedString();
   bool has_pointers = StringShape(*this).IsIndirect();
 
   if (has_pointers) {
-    heap->NotifyObjectLayoutChange(*this, size, no_allocation);
+    isolate->heap()->NotifyObjectLayoutChange(*this, size, no_allocation);
   }
-
   // Morph the string to an external string by replacing the map and
   // reinitializing the fields.  This won't work if the space the existing
   // string occupies is too small for a regular external string.  Instead, we
@@ -237,7 +236,7 @@ bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) {
   // the address of the backing store.  When we encounter uncached external
   // strings in generated code, we need to bailout to runtime.
   Map new_map;
-  ReadOnlyRoots roots(heap);
+  ReadOnlyRoots roots(isolate);
   if (size < ExternalString::kSizeOfAllExternalStrings) {
     new_map = is_internalized
                   ? roots.uncached_external_one_byte_internalized_string_map()
@@ -250,10 +249,11 @@ bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) {
 
   // Byte size of the external String object.
   int new_size = this->SizeFromMap(new_map);
-  heap->CreateFillerObjectAt(this->address() + new_size, size - new_size,
-                             ClearRecordedSlots::kNo);
+  isolate->heap()->CreateFillerObjectAt(
+      this->address() + new_size, size - new_size, ClearRecordedSlots::kNo);
   if (has_pointers) {
-    heap->ClearRecordedSlotRange(this->address(), this->address() + new_size);
+    isolate->heap()->ClearRecordedSlotRange(this->address(),
+                                            this->address() + new_size);
   }
 
   // We are storing the new map using release store after creating a filler for
@@ -262,7 +262,7 @@ bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) {
 
   ExternalOneByteString self = ExternalOneByteString::cast(*this);
   self.SetResource(isolate, resource);
-  heap->RegisterExternalString(*this);
+  isolate->heap()->RegisterExternalString(*this);
   if (is_internalized) self.Hash();  // Force regeneration of the hash value.
   return true;
 }
@@ -272,9 +272,8 @@ bool String::SupportsExternalization() {
     return i::ThinString::cast(*this).actual().SupportsExternalization();
   }
 
-  Isolate* isolate;
   // RO_SPACE strings cannot be externalized.
-  if (!GetIsolateFromWritableObject(*this, &isolate)) {
+  if (IsReadOnlyHeapObject(*this)) {
     return false;
   }
 
@@ -290,6 +289,7 @@ bool String::SupportsExternalization() {
   DCHECK_LE(ExternalString::kUncachedSize, this->Size());
 #endif
 
+  Isolate* isolate = GetIsolateFromWritableObject(*this);
   return !isolate->heap()->IsInGCPostProcessing();
 }