From b4e9d6c0a8a83b3b522b93c1ad06af34a18698a3 Mon Sep 17 00:00:00 2001 From: Igor Sheludko Date: Wed, 19 Jun 2019 16:40:18 +0200 Subject: [PATCH] [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/+/1667416 Reviewed-by: Ulan Degenbaev Commit-Queue: Igor Sheludko Cr-Commit-Position: refs/heads/master@{#62291} --- src/api/api.cc | 23 ++++++---------- src/diagnostics/objects-printer.cc | 4 +-- src/handles/handles.cc | 4 +-- src/heap/heap-write-barrier-inl.h | 16 +++++++++--- src/heap/heap-write-barrier.h | 7 +++++ src/objects/string.cc | 42 +++++++++++++++--------------- 6 files changed, 52 insertions(+), 44 deletions(-) diff --git a/src/api/api.cc b/src/api/api.cc index cd4682b7cb..18ab727f1d 100644 --- a/src/api/api.cc +++ b/src/api/api.cc @@ -5365,20 +5365,15 @@ Local Symbol::Name() const { i::Handle 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 ro_name(reinterpret_cast( sym->GetFieldAddress(i::Symbol::kNameOffset))); return Utils::ToLocal(ro_name); -#endif } i::Handle 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()); diff --git a/src/diagnostics/objects-printer.cc b/src/diagnostics/objects-printer.cc index f467e4bc6c..b767d4260a 100644 --- a/src/diagnostics/objects-printer.cc +++ b/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(); diff --git a/src/handles/handles.cc b/src/handles/handles.cc index e0a1f23b7b..7f320a271c 100644 --- a/src/handles/handles.cc +++ b/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)) { diff --git a/src/heap/heap-write-barrier-inl.h b/src/heap/heap-write-barrier-inl.h index 8270996432..b436c8b9fc 100644 --- a/src/heap/heap-write-barrier-inl.h +++ b/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 diff --git a/src/heap/heap-write-barrier.h b/src/heap/heap-write-barrier.h index e8d3384bc6..c9648d273c 100644 --- a/src/heap/heap-write-barrier.h +++ b/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 diff --git a/src/objects/string.cc b/src/objects/string.cc index 21bc3bb787..af838db06f 100644 --- a/src/objects/string.cc +++ b/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(); }