[heap] Verify filler slots don't need clearing

When creating a filler, we pass through whether we need to clear slots
in the old to new remembered set.

This patch adds a verification check that, when we claim we don't need
to clear slots, checks that no slots are set in the remembered set for
the range of the filler. Effectively, this is a range counterpart to
VerifyClearedSlot.

Change-Id: Id994c56d941988cc282463304bc7307a51943e99
Bug: chromium:1075999
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2139572
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67453}

src/heap/heap.cc

@@ -2935,6 +2935,23 @@ void Heap::FlushNumberStringCache() {
   }
 }
 
+namespace {
+
+#ifdef DEBUG
+void VerifyNoNeedToClearSlots(Address start, Address end) {
+  MemoryChunk* chunk = MemoryChunk::FromAddress(start);
+  // TODO(ulan): Support verification of large pages.
+  if (chunk->InYoungGeneration() || chunk->IsLargePage()) return;
+  Space* space = chunk->owner();
+  if (static_cast<PagedSpace*>(space)->is_off_thread_space()) return;
+  space->heap()->VerifySlotRangeHasNoRecordedSlots(start, end);
+}
+#else
+void VerifyNoNeedToClearSlots(Address start, Address end) {}
+#endif  // DEBUG
+
+}  // namespace
+
 HeapObject Heap::CreateFillerObjectAt(Address addr, int size,
                                       ClearRecordedSlots clear_slots_mode,
                                       ClearFreedMemoryMode clear_memory_mode) {
@@ -2966,9 +2983,12 @@ HeapObject Heap::CreateFillerObjectAt(Address addr, int size,
                    (size / kTaggedSize) - 2);
     }
   }
-  if (clear_slots_mode == ClearRecordedSlots::kYes &&
-      !V8_ENABLE_THIRD_PARTY_HEAP_BOOL) {
-    ClearRecordedSlotRange(addr, addr + size);
+  if (!V8_ENABLE_THIRD_PARTY_HEAP_BOOL) {
+    if (clear_slots_mode == ClearRecordedSlots::kYes) {
+      ClearRecordedSlotRange(addr, addr + size);
+    } else {
+      VerifyNoNeedToClearSlots(addr, addr + size);
+    }
   }
 
   // At this point, we may be deserializing the heap from a snapshot, and
@@ -5756,6 +5776,15 @@ void Heap::VerifyClearedSlot(HeapObject object, ObjectSlot slot) {
                 page->RegisteredObjectWithInvalidatedSlots<OLD_TO_OLD>(object));
 #endif
 }
+
+void Heap::VerifySlotRangeHasNoRecordedSlots(Address start, Address end) {
+#ifndef V8_DISABLE_WRITE_BARRIERS
+  Page* page = Page::FromAddress(start);
+  DCHECK(!page->IsLargePage());
+  DCHECK(!page->InYoungGeneration());
+  RememberedSet<OLD_TO_NEW>::CheckNoneInRange(page, start, end);
+#endif
+}
 #endif
 
 void Heap::ClearRecordedSlotRange(Address start, Address end) {

src/heap/heap.h

@@ -951,6 +951,7 @@ class Heap {
 
 #ifdef DEBUG
   void VerifyClearedSlot(HeapObject object, ObjectSlot slot);
+  void VerifySlotRangeHasNoRecordedSlots(Address start, Address end);
 #endif
 
   // ===========================================================================

src/heap/remembered-set.h

@@ -7,8 +7,10 @@
 
 #include <memory>
 
+#include "src/base/bounds.h"
 #include "src/base/memory.h"
 #include "src/codegen/reloc-info.h"
+#include "src/common/globals.h"
 #include "src/heap/heap.h"
 #include "src/heap/slot-set.h"
 #include "src/heap/spaces.h"
@@ -35,8 +37,8 @@ class RememberedSetOperations {
                      SlotSet::EmptyBucketMode mode) {
     int slots = 0;
     if (slot_set != nullptr) {
-      slots +=
-          slot_set->Iterate(chunk->address(), chunk->buckets(), callback, mode);
+      slots += slot_set->Iterate(chunk->address(), 0, chunk->buckets(),
+                                 callback, mode);
     }
     return slots;
   }
@@ -59,6 +61,25 @@ class RememberedSetOperations {
                             mode);
     }
   }
+
+  static void CheckNoneInRange(SlotSet* slot_set, MemoryChunk* chunk,
+                               Address start, Address end) {
+    if (slot_set != nullptr) {
+      size_t start_bucket = SlotSet::BucketForSlot(start - chunk->address());
+      // Both 'end' and 'end_bucket' are exclusive limits, so do some index
+      // juggling to make sure we get the right bucket even if the end address
+      // is at the start of a bucket.
+      size_t end_bucket =
+          SlotSet::BucketForSlot(end - chunk->address() - kTaggedSize) + 1;
+      slot_set->Iterate(
+          chunk->address(), start_bucket, end_bucket,
+          [start, end](MaybeObjectSlot slot) {
+            CHECK(!base::IsInRange(slot.address(), start, end + 1));
+            return KEEP_SLOT;
+          },
+          SlotSet::KEEP_EMPTY_BUCKETS);
+    }
+  }
 };
 
 // TODO(ulan): Investigate performance of de-templatizing this class.
@@ -89,6 +110,11 @@ class RememberedSet : public AllStatic {
     return slot_set->Contains(offset);
   }
 
+  static void CheckNoneInRange(MemoryChunk* chunk, Address start, Address end) {
+    SlotSet* slot_set = chunk->slot_set<type>();
+    RememberedSetOperations::CheckNoneInRange(slot_set, chunk, start, end);
+  }
+
   // Given a page and a slot in that page, this function removes the slot from
   // the remembered set.
   // If the slot was never added, then the function does nothing.
@@ -159,8 +185,9 @@ class RememberedSet : public AllStatic {
     if (slot_set != nullptr) {
       PossiblyEmptyBuckets* possibly_empty_buckets =
           chunk->possibly_empty_buckets();
-      slots += slot_set->IterateAndTrackEmptyBuckets(
-          chunk->address(), chunk->buckets(), callback, possibly_empty_buckets);
+      slots += slot_set->IterateAndTrackEmptyBuckets(chunk->address(), 0,
+                                                     chunk->buckets(), callback,
+                                                     possibly_empty_buckets);
       if (!possibly_empty_buckets->IsEmpty()) empty_chunks.Push(chunk);
     }
     return slots;

src/heap/slot-set.h

@@ -190,6 +190,12 @@ class SlotSet {
            (kTaggedSizeLog2 + kBitsPerBucketLog2);
   }
 
+  // Converts the slot offset into bucket index.
+  static size_t BucketForSlot(size_t slot_offset) {
+    DCHECK(IsAligned(slot_offset, kTaggedSize));
+    return slot_offset >> (kTaggedSizeLog2 + kBitsPerBucketLog2);
+  }
+
   // The slot offset specifies a slot at address page_start_ + slot_offset.
   // AccessMode defines whether there can be concurrent access on the buckets
   // or not.
@@ -335,9 +341,9 @@ class SlotSet {
   //
   // Releases memory for empty buckets with FREE_EMPTY_BUCKETS.
   template <typename Callback>
-  size_t Iterate(Address chunk_start, size_t buckets, Callback callback,
-                 EmptyBucketMode mode) {
-    return Iterate(chunk_start, buckets, callback,
+  size_t Iterate(Address chunk_start, size_t start_bucket, size_t end_bucket,
+                 Callback callback, EmptyBucketMode mode) {
+    return Iterate(chunk_start, start_bucket, end_bucket, callback,
                    [this, mode](size_t bucket_index) {
                      if (mode == EmptyBucketMode::FREE_EMPTY_BUCKETS) {
                        ReleaseBucket(bucket_index);
@@ -351,11 +357,11 @@ class SlotSet {
   // CheckPossiblyEmptyBuckets.
   template <typename Callback>
   size_t IterateAndTrackEmptyBuckets(
-      Address chunk_start, size_t buckets, Callback callback,
-      PossiblyEmptyBuckets* possibly_empty_buckets) {
-    return Iterate(chunk_start, buckets, callback,
-                   [possibly_empty_buckets, buckets](size_t bucket_index) {
-                     possibly_empty_buckets->Insert(bucket_index, buckets);
+      Address chunk_start, size_t start_bucket, size_t end_bucket,
+      Callback callback, PossiblyEmptyBuckets* possibly_empty_buckets) {
+    return Iterate(chunk_start, start_bucket, end_bucket, callback,
+                   [possibly_empty_buckets, end_bucket](size_t bucket_index) {
+                     possibly_empty_buckets->Insert(bucket_index, end_bucket);
                    });
   }
 
@@ -461,10 +467,11 @@ class SlotSet {
 
  private:
   template <typename Callback, typename EmptyBucketCallback>
-  size_t Iterate(Address chunk_start, size_t buckets, Callback callback,
-                 EmptyBucketCallback empty_bucket_callback) {
+  size_t Iterate(Address chunk_start, size_t start_bucket, size_t end_bucket,
+                 Callback callback, EmptyBucketCallback empty_bucket_callback) {
     size_t new_count = 0;
-    for (size_t bucket_index = 0; bucket_index < buckets; bucket_index++) {
+    for (size_t bucket_index = start_bucket; bucket_index < end_bucket;
+         bucket_index++) {
       Bucket* bucket = LoadBucket(bucket_index);
       if (bucket != nullptr) {
         size_t in_bucket_count = 0;

test/unittests/heap/slot-set-unittest.cc

@@ -85,7 +85,7 @@ TEST(SlotSet, Iterate) {
   }
 
   set->Iterate(
-      kNullAddress, SlotSet::kBucketsRegularPage,
+      kNullAddress, 0, SlotSet::kBucketsRegularPage,
       [](MaybeObjectSlot slot) {
         if (slot.address() % 3 == 0) {
           return KEEP_SLOT;
@@ -106,6 +106,40 @@ TEST(SlotSet, Iterate) {
   SlotSet::Delete(set, SlotSet::kBucketsRegularPage);
 }
 
+TEST(SlotSet, IterateFromHalfway) {
+  SlotSet* set = SlotSet::Allocate(SlotSet::kBucketsRegularPage);
+
+  for (int i = 0; i < Page::kPageSize; i += kTaggedSize) {
+    if (i % 7 == 0) {
+      set->Insert<AccessMode::ATOMIC>(i);
+    }
+  }
+
+  set->Iterate(
+      kNullAddress, SlotSet::kBucketsRegularPage / 2,
+      SlotSet::kBucketsRegularPage,
+      [](MaybeObjectSlot slot) {
+        if (slot.address() % 3 == 0) {
+          return KEEP_SLOT;
+        } else {
+          return REMOVE_SLOT;
+        }
+      },
+      SlotSet::KEEP_EMPTY_BUCKETS);
+
+  for (int i = 0; i < Page::kPageSize; i += kTaggedSize) {
+    if (i < Page::kPageSize / 2 && i % 7 == 0) {
+      EXPECT_TRUE(set->Lookup(i));
+    } else if (i >= Page::kPageSize / 2 && i % 21 == 0) {
+      EXPECT_TRUE(set->Lookup(i));
+    } else {
+      EXPECT_FALSE(set->Lookup(i));
+    }
+  }
+
+  SlotSet::Delete(set, SlotSet::kBucketsRegularPage);
+}
+
 TEST(SlotSet, Remove) {
   SlotSet* set = SlotSet::Allocate(SlotSet::kBucketsRegularPage);