From 8dc7f249132997090c63e0fb16f56b1a995e0243 Mon Sep 17 00:00:00 2001 From: Maciej Goszczycki Date: Mon, 13 May 2019 17:13:44 +0100 Subject: [PATCH] Reland "[heap] Skip read-only space in Heap::Contains" This is a reland of 2b24cd035a64d8930463b239c5c357b5e5371d6a Original change's description: > [heap] Skip read-only space in Heap::Contains > > Bug: v8:7464 > Change-Id: I27e82cdf0f8cc56ff68dcfaecab9644fe74916c7 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1559861 > Commit-Queue: Maciej Goszczycki > Reviewed-by: Dan Elphick > Reviewed-by: Ulan Degenbaev > Cr-Commit-Position: refs/heads/master@{#61350} Bug: v8:7464 Change-Id: Ic5a9221f62537c1711c70b48fc0069288bfda80f Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1601509 Reviewed-by: Dan Elphick Reviewed-by: Ulan Degenbaev Commit-Queue: Maciej Goszczycki Cr-Commit-Position: refs/heads/master@{#61489} --- src/arm/simulator-arm.cc | 4 +++- src/arm64/simulator-arm64.cc | 4 +++- src/heap/combined-heap.h | 5 +++++ src/heap/heap.cc | 12 +++--------- src/heap/spaces.cc | 3 ++- src/mips/simulator-mips.cc | 4 +++- src/mips64/simulator-mips64.cc | 4 +++- src/objects-debug.cc | 4 ++-- src/objects/string.cc | 3 ++- src/ppc/simulator-ppc.cc | 4 +++- src/s390/simulator-s390.cc | 3 ++- src/snapshot/partial-serializer.cc | 3 ++- test/cctest/heap/test-heap.cc | 5 ++--- test/cctest/heap/test-iterators.cc | 5 +++-- 14 files changed, 38 insertions(+), 25 deletions(-) diff --git a/src/arm/simulator-arm.cc b/src/arm/simulator-arm.cc index 4c312f2362..235da9f519 100644 --- a/src/arm/simulator-arm.cc +++ b/src/arm/simulator-arm.cc @@ -15,6 +15,7 @@ #include "src/base/bits.h" #include "src/base/lazy-instance.h" #include "src/disasm.h" +#include "src/heap/combined-heap.h" #include "src/macro-assembler.h" #include "src/objects-inl.h" #include "src/ostreams.h" @@ -332,7 +333,8 @@ void ArmDebugger::Debug() { reinterpret_cast(cur), *cur, *cur); Object obj(*cur); Heap* current_heap = sim_->isolate_->heap(); - if (obj.IsSmi() || current_heap->Contains(HeapObject::cast(obj))) { + if (obj.IsSmi() || + IsValidHeapObject(current_heap, HeapObject::cast(obj))) { PrintF(" ("); if (obj.IsSmi()) { PrintF("smi %d", Smi::ToInt(obj)); diff --git a/src/arm64/simulator-arm64.cc b/src/arm64/simulator-arm64.cc index 55cfe257d1..214780cd8b 100644 --- a/src/arm64/simulator-arm64.cc +++ b/src/arm64/simulator-arm64.cc @@ -15,6 +15,7 @@ #include "src/assembler-inl.h" #include "src/base/lazy-instance.h" #include "src/disasm.h" +#include "src/heap/combined-heap.h" #include "src/macro-assembler.h" #include "src/objects-inl.h" #include "src/ostreams.h" @@ -3289,7 +3290,8 @@ void Simulator::Debug() { reinterpret_cast(cur), *cur, *cur); Object obj(*cur); Heap* current_heap = isolate_->heap(); - if (obj.IsSmi() || current_heap->Contains(HeapObject::cast(obj))) { + if (obj.IsSmi() || + IsValidHeapObject(current_heap, HeapObject::cast(obj))) { PrintF(" ("); if (obj.IsSmi()) { PrintF("smi %" PRId32, Smi::ToInt(obj)); diff --git a/src/heap/combined-heap.h b/src/heap/combined-heap.h index 19cf52b744..645162ebd4 100644 --- a/src/heap/combined-heap.h +++ b/src/heap/combined-heap.h @@ -30,6 +30,11 @@ class V8_EXPORT_PRIVATE CombinedHeapIterator final { ReadOnlyHeapIterator ro_heap_iterator_; }; +V8_WARN_UNUSED_RESULT inline bool IsValidHeapObject(Heap* heap, + HeapObject object) { + return ReadOnlyHeap::Contains(object) || heap->Contains(object); +} + } // namespace internal } // namespace v8 diff --git a/src/heap/heap.cc b/src/heap/heap.cc index c237cb3102..87ff3b12af 100644 --- a/src/heap/heap.cc +++ b/src/heap/heap.cc @@ -25,6 +25,7 @@ #include "src/heap/array-buffer-tracker-inl.h" #include "src/heap/barrier.h" #include "src/heap/code-stats.h" +#include "src/heap/combined-heap.h" #include "src/heap/concurrent-marking.h" #include "src/heap/embedder-tracing.h" #include "src/heap/gc-idle-time-handler.h" @@ -3595,13 +3596,6 @@ const char* Heap::GarbageCollectionReasonToString( } bool Heap::Contains(HeapObject value) { - // Check RO_SPACE first because IsOutsideAllocatedSpace cannot account for a - // shared RO_SPACE. - // TODO(v8:7464): Exclude read-only space. Use ReadOnlyHeap::Contains where - // appropriate. - if (read_only_space_ != nullptr && read_only_space_->Contains(value)) { - return true; - } if (memory_allocator()->IsOutsideAllocatedSpace(value->address())) { return false; } @@ -4842,7 +4836,7 @@ void Heap::RegisterExternallyReferencedObject(Address* location) { Object object(*location); if (!object->IsHeapObject()) return; HeapObject heap_object = HeapObject::cast(object); - DCHECK(Contains(heap_object)); + DCHECK(IsValidHeapObject(this, heap_object)); if (FLAG_incremental_marking_wrappers && incremental_marking()->IsMarking()) { incremental_marking()->WhiteToGreyAndPush(heap_object); } else { @@ -5663,7 +5657,7 @@ void VerifyPointersVisitor::VisitRootPointers(Root root, } void VerifyPointersVisitor::VerifyHeapObjectImpl(HeapObject heap_object) { - CHECK(heap_->Contains(heap_object)); + CHECK(IsValidHeapObject(heap_, heap_object)); CHECK(heap_object->map()->IsMap()); } diff --git a/src/heap/spaces.cc b/src/heap/spaces.cc index 0e73566001..3af9eff564 100644 --- a/src/heap/spaces.cc +++ b/src/heap/spaces.cc @@ -12,6 +12,7 @@ #include "src/base/template-utils.h" #include "src/counters.h" #include "src/heap/array-buffer-tracker.h" +#include "src/heap/combined-heap.h" #include "src/heap/concurrent-marking.h" #include "src/heap/gc-tracer.h" #include "src/heap/heap-controller.h" @@ -3760,7 +3761,7 @@ void LargeObjectSpace::Verify(Isolate* isolate) { Object element = array->get(j); if (element->IsHeapObject()) { HeapObject element_object = HeapObject::cast(element); - CHECK(heap()->Contains(element_object)); + CHECK(IsValidHeapObject(heap(), element_object)); CHECK(element_object->map()->IsMap()); } } diff --git a/src/mips/simulator-mips.cc b/src/mips/simulator-mips.cc index 0683ab1fdf..386051a250 100644 --- a/src/mips/simulator-mips.cc +++ b/src/mips/simulator-mips.cc @@ -16,6 +16,7 @@ #include "src/base/bits.h" #include "src/base/lazy-instance.h" #include "src/disasm.h" +#include "src/heap/combined-heap.h" #include "src/macro-assembler.h" #include "src/mips/constants-mips.h" #include "src/ostreams.h" @@ -536,7 +537,8 @@ void MipsDebugger::Debug() { reinterpret_cast(cur), *cur, *cur); Object obj(*cur); Heap* current_heap = sim_->isolate_->heap(); - if (obj.IsSmi() || current_heap->Contains(HeapObject::cast(obj))) { + if (obj.IsSmi() || + IsValidHeapObject(current_heap, HeapObject::cast(obj))) { PrintF(" ("); if (obj.IsSmi()) { PrintF("smi %d", Smi::ToInt(obj)); diff --git a/src/mips64/simulator-mips64.cc b/src/mips64/simulator-mips64.cc index 1e736e51ef..0b9cf9d7fa 100644 --- a/src/mips64/simulator-mips64.cc +++ b/src/mips64/simulator-mips64.cc @@ -15,6 +15,7 @@ #include "src/assembler-inl.h" #include "src/base/bits.h" #include "src/disasm.h" +#include "src/heap/combined-heap.h" #include "src/macro-assembler.h" #include "src/mips64/constants-mips64.h" #include "src/ostreams.h" @@ -466,7 +467,8 @@ void MipsDebugger::Debug() { reinterpret_cast(cur), *cur, *cur); Object obj(*cur); Heap* current_heap = sim_->isolate_->heap(); - if (obj.IsSmi() || current_heap->Contains(HeapObject::cast(obj))) { + if (obj.IsSmi() || + IsValidHeapObject(current_heap, HeapObject::cast(obj))) { PrintF(" ("); if (obj.IsSmi()) { PrintF("smi %d", Smi::ToInt(obj)); diff --git a/src/objects-debug.cc b/src/objects-debug.cc index 3a57b50e78..4949ae4f39 100644 --- a/src/objects-debug.cc +++ b/src/objects-debug.cc @@ -12,6 +12,7 @@ #include "src/disassembler.h" #include "src/elements.h" #include "src/field-type.h" +#include "src/heap/combined-heap.h" #include "src/heap/heap-write-barrier-inl.h" #include "src/ic/handler-configuration-inl.h" #include "src/layout-descriptor.h" @@ -488,8 +489,7 @@ void HeapObject::HeapObjectVerify(Isolate* isolate) { // static void HeapObject::VerifyHeapPointer(Isolate* isolate, Object p) { CHECK(p->IsHeapObject()); - HeapObject ho = HeapObject::cast(p); - CHECK(isolate->heap()->Contains(ho)); + CHECK(IsValidHeapObject(isolate->heap(), HeapObject::cast(p))); } void Symbol::SymbolVerify(Isolate* isolate) { diff --git a/src/objects/string.cc b/src/objects/string.cc index 93b4d85d9b..5c7c7fded3 100644 --- a/src/objects/string.cc +++ b/src/objects/string.cc @@ -8,6 +8,7 @@ #include "src/conversions.h" #include "src/handles-inl.h" #include "src/heap/heap-inl.h" // For LooksValid implementation. +#include "src/heap/read-only-heap.h" #include "src/objects/map.h" #include "src/objects/oddball.h" #include "src/objects/string-comparator.h" @@ -396,7 +397,7 @@ bool String::LooksValid() { // basically the same logic as the way we access the heap in the first place. MemoryChunk* chunk = MemoryChunk::FromHeapObject(*this); // RO_SPACE objects should always be valid. - if (chunk->owner()->identity() == RO_SPACE) return true; + if (ReadOnlyHeap::Contains(*this)) return true; if (chunk->heap() == nullptr) return false; return chunk->heap()->Contains(*this); } diff --git a/src/ppc/simulator-ppc.cc b/src/ppc/simulator-ppc.cc index 4710897a03..35343eb06f 100644 --- a/src/ppc/simulator-ppc.cc +++ b/src/ppc/simulator-ppc.cc @@ -14,6 +14,7 @@ #include "src/base/bits.h" #include "src/base/lazy-instance.h" #include "src/disasm.h" +#include "src/heap/combined-heap.h" #include "src/macro-assembler.h" #include "src/objects-inl.h" #include "src/ostreams.h" @@ -401,7 +402,8 @@ void PPCDebugger::Debug() { reinterpret_cast(cur), *cur, *cur); Object obj(*cur); Heap* current_heap = sim_->isolate_->heap(); - if (obj.IsSmi() || current_heap->Contains(HeapObject::cast(obj))) { + if (obj.IsSmi() || + IsValidHeapObject(current_heap, HeapObject::cast(obj))) { PrintF(" ("); if (obj.IsSmi()) { PrintF("smi %d", Smi::ToInt(obj)); diff --git a/src/s390/simulator-s390.cc b/src/s390/simulator-s390.cc index f2600cca31..bfc685fc7f 100644 --- a/src/s390/simulator-s390.cc +++ b/src/s390/simulator-s390.cc @@ -15,6 +15,7 @@ #include "src/base/bits.h" #include "src/base/once.h" #include "src/disasm.h" +#include "src/heap/combined-heap.h" #include "src/macro-assembler.h" #include "src/objects-inl.h" #include "src/ostreams.h" @@ -424,7 +425,7 @@ void S390Debugger::Debug() { Heap* current_heap = sim_->isolate_->heap(); if (obj.IsSmi()) { PrintF(" (smi %d)", Smi::ToInt(obj)); - } else if (current_heap->Contains(HeapObject::cast(obj))) { + } else if (IsValidHeapObject(current_heap, HeapObject::cast(obj))) { PrintF(" ("); obj->ShortPrint(); PrintF(")"); diff --git a/src/snapshot/partial-serializer.cc b/src/snapshot/partial-serializer.cc index 6c650be4c1..d63e584bad 100644 --- a/src/snapshot/partial-serializer.cc +++ b/src/snapshot/partial-serializer.cc @@ -6,6 +6,7 @@ #include "src/snapshot/startup-serializer.h" #include "src/api-inl.h" +#include "src/heap/combined-heap.h" #include "src/math-random.h" #include "src/microtask-queue.h" #include "src/objects-inl.h" @@ -171,7 +172,7 @@ bool PartialSerializer::SerializeJSObjectWithEmbedderFields(Object obj) { original_embedder_values.emplace_back(embedder_data_slot.load_raw(no_gc)); Object object = embedder_data_slot.load_tagged(); if (object->IsHeapObject()) { - DCHECK(isolate()->heap()->Contains(HeapObject::cast(object))); + DCHECK(IsValidHeapObject(isolate()->heap(), HeapObject::cast(object))); serialized_data.push_back({nullptr, 0}); } else { // If no serializer is provided and the field was empty, we serialize it diff --git a/test/cctest/heap/test-heap.cc b/test/cctest/heap/test-heap.cc index 1b76e66048..444968e483 100644 --- a/test/cctest/heap/test-heap.cc +++ b/test/cctest/heap/test-heap.cc @@ -38,6 +38,7 @@ #include "src/field-type.h" #include "src/global-handles.h" #include "src/hash-seed-inl.h" +#include "src/heap/combined-heap.h" #include "src/heap/factory.h" #include "src/heap/gc-tracer.h" #include "src/heap/heap-inl.h" @@ -74,9 +75,7 @@ static const int kPretenureCreationCount = static void CheckMap(Map map, int type, int instance_size) { CHECK(map->IsHeapObject()); -#ifdef DEBUG - CHECK(CcTest::heap()->Contains(map)); -#endif + DCHECK(IsValidHeapObject(CcTest::heap(), map)); CHECK_EQ(ReadOnlyRoots(CcTest::heap()).meta_map(), map->map()); CHECK_EQ(type, map->instance_type()); CHECK_EQ(instance_size, map->instance_size()); diff --git a/test/cctest/heap/test-iterators.cc b/test/cctest/heap/test-iterators.cc index 9b5a8b58aa..e6eb917492 100644 --- a/test/cctest/heap/test-iterators.cc +++ b/test/cctest/heap/test-iterators.cc @@ -51,7 +51,6 @@ Object CreateWritableObject() { } } // namespace -// TODO(v8:7464): Add more CHECKs once Contains doesn't include read-only space. TEST(ReadOnlyHeapIterator) { CcTest::InitializeVM(); HandleScope handle_scope(CcTest::i_isolate()); @@ -61,6 +60,7 @@ TEST(ReadOnlyHeapIterator) { for (HeapObject obj = iterator.next(); !obj.is_null(); obj = iterator.next()) { CHECK(ReadOnlyHeap::Contains(obj)); + CHECK(!CcTest::heap()->Contains(obj)); CHECK_NE(sample_object, obj); } } @@ -75,6 +75,7 @@ TEST(HeapIterator) { for (HeapObject obj = iterator.next(); !obj.is_null(); obj = iterator.next()) { CHECK(!ReadOnlyHeap::Contains(obj)); + CHECK(CcTest::heap()->Contains(obj)); if (sample_object == obj) seen_sample_object = true; } CHECK(seen_sample_object); @@ -89,7 +90,7 @@ TEST(CombinedHeapIterator) { for (HeapObject obj = iterator.next(); !obj.is_null(); obj = iterator.next()) { - CHECK(CcTest::heap()->Contains(obj)); + CHECK(IsValidHeapObject(CcTest::heap(), obj)); if (sample_object == obj) seen_sample_object = true; } CHECK(seen_sample_object);