cppgc: Add build time option to verify live bytes

The marking verifier already traverses the whole heap using page
iteration. Add an option to allow checking that the verifier pass
finds the same amount of live bytes as the marker traversal.

Bug: chromium:1056170
Change-Id: I1dc4cd0c04147b8cd3e3eb7678276b665336e615
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2902724
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74630}

BUILD.gn

@@ -279,6 +279,10 @@ declare_args() {
   # Enable heap reservation of size 4GB. Only possible for 64bit archs.
   cppgc_enable_caged_heap = v8_current_cpu == "x64" || v8_current_cpu == "arm64"
 
+  # Enable verification of live bytes in the marking verifier.
+  # TODO(v8:11785): Enable by default when running with the verifier.
+  cppgc_enable_verify_live_bytes = false
+
   # Enable young generation in cppgc.
   cppgc_enable_young_generation = false
 
@@ -703,6 +707,10 @@ config("features") {
     ":cppgc_header_features",
   ]
 
+  if (cppgc_enable_verify_live_bytes) {
+    defines += [ "CPPGC_VERIFY_LIVE_BYTES" ]
+  }
+
   if (v8_embedder_string != "") {
     defines += [ "V8_EMBEDDER_STRING=\"$v8_embedder_string\"" ]
   }

src/heap/cppgc-js/cpp-heap.cc

@@ -378,7 +378,8 @@ void CppHeap::TraceEpilogue(TraceSummary* trace_summary) {
   // TODO(chromium:1056170): replace build flag with dedicated flag.
 #if DEBUG
   UnifiedHeapMarkingVerifier verifier(*this);
-  verifier.Run(stack_state_of_prev_gc(), stack_end_of_current_gc());
+  verifier.Run(stack_state_of_prev_gc(), stack_end_of_current_gc(),
+               stats_collector()->marked_bytes());
 #endif
 
   {

src/heap/cppgc/heap.cc

@@ -189,7 +189,8 @@ void Heap::FinalizeGarbageCollection(Config::StackState stack_state) {
   // TODO(chromium:1056170): replace build flag with dedicated flag.
 #if DEBUG
   MarkingVerifier verifier(*this);
-  verifier.Run(config_.stack_state, stack_end_of_current_gc());
+  verifier.Run(config_.stack_state, stack_end_of_current_gc(),
+               stats_collector()->marked_bytes());
 #endif
 
   subtle::NoGarbageCollectionScope no_gc(*this);

src/heap/cppgc/marking-verifier.cc

@@ -9,6 +9,7 @@
 #include "src/heap/cppgc/heap-object-header.h"
 #include "src/heap/cppgc/heap.h"
 #include "src/heap/cppgc/marking-visitor.h"
+#include "src/heap/cppgc/object-view.h"
 
 namespace cppgc {
 namespace internal {
@@ -21,7 +22,8 @@ MarkingVerifierBase::MarkingVerifierBase(
       visitor_(std::move(visitor)) {}
 
 void MarkingVerifierBase::Run(Heap::Config::StackState stack_state,
-                              uintptr_t stack_end) {
+                              uintptr_t stack_end,
+                              size_t expected_marked_bytes) {
   Traverse(&heap_.raw_heap());
   if (stack_state == Heap::Config::StackState::kMayContainHeapPointers) {
     in_construction_objects_ = &in_construction_objects_stack_;
@@ -36,6 +38,9 @@ void MarkingVerifierBase::Run(Heap::Config::StackState stack_state,
                in_construction_objects_heap_.find(header));
     }
   }
+#ifdef CPPGC_VERIFY_LIVE_BYTES
+  CHECK_EQ(expected_marked_bytes, found_marked_bytes_);
+#endif  // CPPGC_VERIFY_LIVE_BYTES
 }
 
 void VerificationState::VerifyMarked(const void* base_object_payload) const {
@@ -97,6 +102,8 @@ bool MarkingVerifierBase::VisitHeapObjectHeader(HeapObjectHeader* header) {
     TraceConservativelyIfNeeded(*header);
   }
 
+  found_marked_bytes_ += ObjectView(*header).Size() + sizeof(HeapObjectHeader);
+
   verification_state_.SetCurrentParent(nullptr);
 
   return true;

src/heap/cppgc/marking-verifier.h

@@ -40,7 +40,7 @@ class V8_EXPORT_PRIVATE MarkingVerifierBase
   MarkingVerifierBase(const MarkingVerifierBase&) = delete;
   MarkingVerifierBase& operator=(const MarkingVerifierBase&) = delete;
 
-  void Run(Heap::Config::StackState, uintptr_t);
+  void Run(Heap::Config::StackState, uintptr_t, size_t);
 
  protected:
   MarkingVerifierBase(HeapBase&, VerificationState&,
@@ -60,6 +60,7 @@ class V8_EXPORT_PRIVATE MarkingVerifierBase
   std::unordered_set<const HeapObjectHeader*> in_construction_objects_stack_;
   std::unordered_set<const HeapObjectHeader*>* in_construction_objects_ =
       &in_construction_objects_heap_;
+  size_t found_marked_bytes_ = 0;
 };
 
 class V8_EXPORT_PRIVATE MarkingVerifier final : public MarkingVerifierBase {

test/unittests/heap/cppgc/marking-verifier-unittest.cc

@@ -22,10 +22,12 @@ class MarkingVerifierTest : public testing::TestWithHeap {
  public:
   using StackState = Heap::Config::StackState;
 
-  V8_NOINLINE void VerifyMarking(HeapBase& heap, StackState stack_state) {
+  V8_NOINLINE void VerifyMarking(HeapBase& heap, StackState stack_state,
+                                 size_t expected_marked_bytes) {
     Heap::From(GetHeap())->object_allocator().ResetLinearAllocationBuffers();
     MarkingVerifier verifier(heap);
-    verifier.Run(stack_state, v8::base::Stack::GetCurrentStackPosition());
+    verifier.Run(stack_state, v8::base::Stack::GetCurrentStackPosition(),
+                 expected_marked_bytes);
   }
 };
 
@@ -54,28 +56,35 @@ V8_NOINLINE T access(volatile const T& t) {
 
 // Following tests should not crash.
 
-TEST_F(MarkingVerifierTest, DoesntDieOnMarkedOnStackReference) {
+TEST_F(MarkingVerifierTest, DoesNotDieOnMarkedOnStackReference) {
   GCed* object = MakeGarbageCollected<GCed>(GetAllocationHandle());
-  HeapObjectHeader::FromObject(object).TryMarkAtomic();
+  auto& header = HeapObjectHeader::FromObject(object);
+  ASSERT_TRUE(header.TryMarkAtomic());
   VerifyMarking(Heap::From(GetHeap())->AsBase(),
-                StackState::kMayContainHeapPointers);
+                StackState::kMayContainHeapPointers, header.AllocatedSize());
   access(object);
 }
 
-TEST_F(MarkingVerifierTest, DoesntDieOnMarkedMember) {
+TEST_F(MarkingVerifierTest, DoesNotDieOnMarkedMember) {
   Persistent<GCed> parent = MakeGarbageCollected<GCed>(GetAllocationHandle());
-  HeapObjectHeader::FromObject(parent.Get()).TryMarkAtomic();
+  auto& parent_header = HeapObjectHeader::FromObject(parent.Get());
+  ASSERT_TRUE(parent_header.TryMarkAtomic());
   parent->SetChild(MakeGarbageCollected<GCed>(GetAllocationHandle()));
-  HeapObjectHeader::FromObject(parent->child()).TryMarkAtomic();
-  VerifyMarking(Heap::From(GetHeap())->AsBase(), StackState::kNoHeapPointers);
+  auto& child_header = HeapObjectHeader::FromObject(parent->child());
+  ASSERT_TRUE(child_header.TryMarkAtomic());
+  VerifyMarking(Heap::From(GetHeap())->AsBase(), StackState::kNoHeapPointers,
+                parent_header.AllocatedSize() + child_header.AllocatedSize());
 }
 
-TEST_F(MarkingVerifierTest, DoesntDieOnMarkedWeakMember) {
+TEST_F(MarkingVerifierTest, DoesNotDieOnMarkedWeakMember) {
   Persistent<GCed> parent = MakeGarbageCollected<GCed>(GetAllocationHandle());
-  HeapObjectHeader::FromObject(parent.Get()).TryMarkAtomic();
+  auto& parent_header = HeapObjectHeader::FromObject(parent.Get());
+  ASSERT_TRUE(parent_header.TryMarkAtomic());
   parent->SetWeakChild(MakeGarbageCollected<GCed>(GetAllocationHandle()));
-  HeapObjectHeader::FromObject(parent->weak_child()).TryMarkAtomic();
-  VerifyMarking(Heap::From(GetHeap())->AsBase(), StackState::kNoHeapPointers);
+  auto& child_header = HeapObjectHeader::FromObject(parent->weak_child());
+  ASSERT_TRUE(child_header.TryMarkAtomic());
+  VerifyMarking(Heap::From(GetHeap())->AsBase(), StackState::kNoHeapPointers,
+                parent_header.AllocatedSize() + child_header.AllocatedSize());
 }
 
 namespace {
@@ -91,12 +100,14 @@ class GCedWithCallback : public GarbageCollected<GCedWithCallback> {
 
 }  // namespace
 
-TEST_F(MarkingVerifierTest, DoesntDieOnInConstructionOnObject) {
+TEST_F(MarkingVerifierTest, DoesNotDieOnInConstructionOnObject) {
   MakeGarbageCollected<GCedWithCallback>(
       GetAllocationHandle(), [this](GCedWithCallback* obj) {
-        HeapObjectHeader::FromObject(obj).TryMarkAtomic();
+        auto& header = HeapObjectHeader::FromObject(obj);
+        CHECK(header.TryMarkAtomic());
         VerifyMarking(Heap::From(GetHeap())->AsBase(),
-                      StackState::kMayContainHeapPointers);
+                      StackState::kMayContainHeapPointers,
+                      header.AllocatedSize());
       });
 }
 
@@ -156,30 +167,51 @@ class MarkingVerifierDeathTest : public MarkingVerifierTest {
 
 TEST_F(MarkingVerifierDeathTest, DieOnUnmarkedOnStackReference) {
   GCed* object = MakeGarbageCollected<GCed>(GetAllocationHandle());
+  auto& header = HeapObjectHeader::FromObject(object);
+  USE(header);
   EXPECT_DEATH_IF_SUPPORTED(VerifyMarking(Heap::From(GetHeap())->AsBase(),
-                                          StackState::kMayContainHeapPointers),
+                                          StackState::kMayContainHeapPointers,
+                                          header.AllocatedSize()),
                             "");
   access(object);
 }
 
 TEST_F(MarkingVerifierDeathTest, DieOnUnmarkedMember) {
   Persistent<GCed> parent = MakeGarbageCollected<GCed>(GetAllocationHandle());
-  HeapObjectHeader::FromObject(parent.Get()).TryMarkAtomic();
+  auto& parent_header = HeapObjectHeader::FromObject(parent);
+  ASSERT_TRUE(parent_header.TryMarkAtomic());
   parent->SetChild(MakeGarbageCollected<GCed>(GetAllocationHandle()));
-  EXPECT_DEATH_IF_SUPPORTED(VerifyMarking(Heap::From(GetHeap())->AsBase(),
-                                          StackState::kNoHeapPointers),
-                            "");
+  EXPECT_DEATH_IF_SUPPORTED(
+      VerifyMarking(Heap::From(GetHeap())->AsBase(),
+                    StackState::kNoHeapPointers, parent_header.AllocatedSize()),
+      "");
 }
 
 TEST_F(MarkingVerifierDeathTest, DieOnUnmarkedWeakMember) {
   Persistent<GCed> parent = MakeGarbageCollected<GCed>(GetAllocationHandle());
-  HeapObjectHeader::FromObject(parent.Get()).TryMarkAtomic();
+  auto& parent_header = HeapObjectHeader::FromObject(parent);
+  ASSERT_TRUE(parent_header.TryMarkAtomic());
   parent->SetWeakChild(MakeGarbageCollected<GCed>(GetAllocationHandle()));
+  EXPECT_DEATH_IF_SUPPORTED(
+      VerifyMarking(Heap::From(GetHeap())->AsBase(),
+                    StackState::kNoHeapPointers, parent_header.AllocatedSize()),
+      "");
+}
+
+#ifdef CPPGC_VERIFY_LIVE_BYTES
+
+TEST_F(MarkingVerifierDeathTest, DieOnUnexpectedLiveByteCount) {
+  GCed* object = MakeGarbageCollected<GCed>(GetAllocationHandle());
+  auto& header = HeapObjectHeader::FromObject(object);
+  ASSERT_TRUE(header.TryMarkAtomic());
   EXPECT_DEATH_IF_SUPPORTED(VerifyMarking(Heap::From(GetHeap())->AsBase(),
-                                          StackState::kNoHeapPointers),
+                                          StackState::kMayContainHeapPointers,
+                                          header.AllocatedSize() - 1),
                             "");
 }
 
+#endif  // CPPGC_VERIFY_LIVE_BYTES
+
 namespace {
 
 template <template <typename T> class Reference>
@@ -223,7 +255,9 @@ void MarkingVerifierDeathTest::TestResurrectingPreFinalizer() {
       MakeGarbageCollected<GCed>(GetAllocationHandle()));
   EXPECT_DEATH_IF_SUPPORTED(PreciseGC(), "");
 }
+
 #if DEBUG
+
 TEST_F(MarkingVerifierDeathTest, DiesOnResurrectedMember) {
   TestResurrectingPreFinalizer<Member>();
 }
@@ -231,6 +265,7 @@ TEST_F(MarkingVerifierDeathTest, DiesOnResurrectedMember) {
 TEST_F(MarkingVerifierDeathTest, DiesOnResurrectedWeakMember) {
   TestResurrectingPreFinalizer<WeakMember>();
 }
+
 #endif  // DEBUG
 
 }  // namespace internal

tools/cppgc/gen_cmake.py

@@ -244,6 +244,7 @@ set(CMAKE_CXX_STANDARD_REQUIRED ON)
 
 option(CPPGC_ENABLE_OBJECT_NAMES "Enable object names in cppgc for debug purposes" OFF)
 option(CPPGC_ENABLE_CAGED_HEAP "Enable heap reservation of size 4GB, only possible for 64bit archs" OFF)
+option(CPPGC_ENABLE_VERIFY_LIVE_BYTES " Enable verification of live bytes in the marking verifier" OFF)
 option(CPPGC_ENABLE_YOUNG_GENERATION "Enable young generation in cppgc" OFF)
 set(CPPGC_TARGET_ARCH "x64" CACHE STRING "Target architecture, possible options: x64, x86, arm, arm64, ppc64, s390x, mipsel, mips64el")
 
@@ -433,6 +434,9 @@ endif()
 if(CPPGC_ENABLE_CAGED_HEAP)
   target_compile_definitions({target.name} PRIVATE "-DCPPGC_CAGED_HEAP")
 endif()
+if(CPPGC_ENABLE_VERIFY_LIVE_BYTES)
+  target_compile_definitions({target.name} PRIVATE "-DCPPGC_VERIFY_LIVE_BYTES")
+endif()
 if(CPPGC_ENABLE_YOUNG_GENERATION)
   target_compile_definitions({target.name} PRIVATE "-DCPPGC_YOUNG_GENERATION")
 endif()"""