cppgc: Allow MarkingVerifier to be specialized for unified heap

Follow the marker pattern where actual logic is moved into a dedicated
state class and the visitors merely forward to that class.

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

BUILD.gn

@@ -2599,6 +2599,8 @@ v8_source_set("v8_base_without_compiler") {
     "src/heap/cppgc-js/cpp-snapshot.cc",
     "src/heap/cppgc-js/cpp-snapshot.h",
     "src/heap/cppgc-js/unified-heap-marking-state.h",
+    "src/heap/cppgc-js/unified-heap-marking-verifier.cc",
+    "src/heap/cppgc-js/unified-heap-marking-verifier.h",
     "src/heap/cppgc-js/unified-heap-marking-visitor.cc",
     "src/heap/cppgc-js/unified-heap-marking-visitor.h",
     "src/heap/embedder-tracing.cc",

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

@@ -14,6 +14,7 @@
 #include "src/heap/base/stack.h"
 #include "src/heap/cppgc-js/cpp-snapshot.h"
 #include "src/heap/cppgc-js/unified-heap-marking-state.h"
+#include "src/heap/cppgc-js/unified-heap-marking-verifier.h"
 #include "src/heap/cppgc-js/unified-heap-marking-visitor.h"
 #include "src/heap/cppgc/concurrent-marker.h"
 #include "src/heap/cppgc/gc-info-table.h"
@@ -209,7 +210,8 @@ void CppHeap::TraceEpilogue(TraceSummary* trace_summary) {
   marker_.reset();
   // TODO(chromium:1056170): replace build flag with dedicated flag.
 #if DEBUG
-  VerifyMarking(cppgc::Heap::StackState::kNoHeapPointers);
+  UnifiedHeapMarkingVerifier verifier(*this);
+  verifier.Run(cppgc::Heap::StackState::kNoHeapPointers);
 #endif
   {
     NoGCScope no_gc(*this);

src/heap/cppgc-js/unified-heap-marking-verifier.cc

@@ -0,0 +1,57 @@
+
+
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/heap/cppgc-js/unified-heap-marking-verifier.h"
+
+#include "include/v8-cppgc.h"
+#include "src/heap/cppgc/marking-verifier.h"
+
+namespace v8 {
+namespace internal {
+
+namespace {
+
+class UnifiedHeapVerificationVisitor final : public JSVisitor {
+ public:
+  explicit UnifiedHeapVerificationVisitor(
+      cppgc::internal::VerificationState& state)
+      : JSVisitor(cppgc::internal::VisitorFactory::CreateKey()),
+        state_(state) {}
+
+  void Visit(const void*, cppgc::TraceDescriptor desc) final {
+    state_.VerifyMarked(desc.base_object_payload);
+  }
+
+  void VisitWeak(const void*, cppgc::TraceDescriptor desc, cppgc::WeakCallback,
+                 const void*) final {
+    // Weak objects should have been cleared at this point. As a consequence,
+    // all objects found through weak references have to point to live objects
+    // at this point.
+    state_.VerifyMarked(desc.base_object_payload);
+  }
+  void Visit(const internal::JSMemberBase& ref) final {
+    // TODO(chromium:1056170): Verify V8 object is indeed marked.
+  }
+
+ private:
+  cppgc::internal::VerificationState& state_;
+};
+
+}  // namespace
+
+UnifiedHeapMarkingVerifier::UnifiedHeapMarkingVerifier(
+    cppgc::internal::HeapBase& heap_base)
+    : MarkingVerifierBase(
+          heap_base, std::make_unique<UnifiedHeapVerificationVisitor>(state_)) {
+}
+
+void UnifiedHeapMarkingVerifier::SetCurrentParent(
+    const cppgc::internal::HeapObjectHeader* parent) {
+  state_.SetCurrentParent(parent);
+}
+
+}  // namespace internal
+}  // namespace v8

src/heap/cppgc-js/unified-heap-marking-verifier.h

@@ -0,0 +1,30 @@
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef V8_HEAP_CPPGC_JS_UNIFIED_HEAP_MARKING_VERIFIER_H_
+#define V8_HEAP_CPPGC_JS_UNIFIED_HEAP_MARKING_VERIFIER_H_
+
+#include "src/heap/cppgc/marking-verifier.h"
+
+namespace v8 {
+namespace internal {
+
+class V8_EXPORT_PRIVATE UnifiedHeapMarkingVerifier final
+    : public cppgc::internal::MarkingVerifierBase {
+ public:
+  explicit UnifiedHeapMarkingVerifier(cppgc::internal::HeapBase&);
+  ~UnifiedHeapMarkingVerifier() final = default;
+
+  void SetCurrentParent(const cppgc::internal::HeapObjectHeader*) final;
+
+ private:
+  // TODO(chromium:1056170): Use a verification state that can handle JS
+  // references.
+  cppgc::internal::VerificationState state_;
+};
+
+}  // namespace internal
+}  // namespace v8
+
+#endif  // V8_HEAP_CPPGC_JS_UNIFIED_HEAP_MARKING_VERIFIER_H_

src/heap/cppgc/heap-base.cc

@@ -88,10 +88,6 @@ HeapBase::NoGCScope::NoGCScope(HeapBase& heap) : heap_(heap) {
 
 HeapBase::NoGCScope::~NoGCScope() { heap_.no_gc_scope_--; }
 
-void HeapBase::VerifyMarking(cppgc::Heap::StackState stack_state) {
-  MarkingVerifier verifier(*this, stack_state);
-}
-
 void HeapBase::AdvanceIncrementalGarbageCollectionOnAllocationIfNeeded() {
   if (marker_) marker_->AdvanceMarkingOnAllocation();
 }

src/heap/cppgc/heap-base.h

@@ -137,8 +137,6 @@ class V8_EXPORT_PRIVATE HeapBase {
   void AdvanceIncrementalGarbageCollectionOnAllocationIfNeeded();
 
  protected:
-  void VerifyMarking(cppgc::Heap::StackState);
-
   virtual void FinalizeIncrementalGarbageCollectionIfNeeded(
       cppgc::Heap::StackState) = 0;
 

src/heap/cppgc/heap.cc

@@ -10,6 +10,7 @@
 #include "src/heap/cppgc/heap-object-header.h"
 #include "src/heap/cppgc/heap-visitor.h"
 #include "src/heap/cppgc/marker.h"
+#include "src/heap/cppgc/marking-verifier.h"
 #include "src/heap/cppgc/prefinalizer-handler.h"
 
 namespace cppgc {
@@ -162,7 +163,8 @@ void Heap::FinalizeGarbageCollection(Config::StackState stack_state) {
   marker_.reset();
   // TODO(chromium:1056170): replace build flag with dedicated flag.
 #if DEBUG
-  VerifyMarking(stack_state);
+  MarkingVerifier verifier(*this);
+  verifier.Run(stack_state);
 #endif
   {
     NoGCScope no_gc(*this);

src/heap/cppgc/marking-verifier.cc

@@ -6,36 +6,28 @@
 
 #include "src/base/logging.h"
 #include "src/heap/cppgc/gc-info-table.h"
+#include "src/heap/cppgc/heap-object-header.h"
 #include "src/heap/cppgc/heap.h"
+#include "src/heap/cppgc/marking-visitor.h"
 
 namespace cppgc {
 namespace internal {
 
-MarkingVerifier::MarkingVerifier(HeapBase& heap,
-                                 Heap::Config::StackState stack_state)
-    : cppgc::Visitor(VisitorFactory::CreateKey()),
-      ConservativeTracingVisitor(heap, *heap.page_backend(), *this) {
-  Traverse(&heap.raw_heap());
+MarkingVerifierBase::MarkingVerifierBase(
+    HeapBase& heap, std::unique_ptr<cppgc::Visitor> visitor)
+    : ConservativeTracingVisitor(heap, *heap.page_backend(), *visitor.get()),
+      visitor_(std::move(visitor)) {}
+
+void MarkingVerifierBase::Run(Heap::Config::StackState stack_state) {
+  Traverse(&heap_.raw_heap());
   if (stack_state == Heap::Config::StackState::kMayContainHeapPointers) {
     in_construction_objects_ = &in_construction_objects_stack_;
-    heap.stack()->IteratePointers(this);
+    heap_.stack()->IteratePointers(this);
     CHECK_EQ(in_construction_objects_stack_, in_construction_objects_heap_);
   }
 }
 
-void MarkingVerifier::Visit(const void* object, TraceDescriptor desc) {
-  VerifyChild(desc.base_object_payload);
-}
-
-void MarkingVerifier::VisitWeak(const void* object, TraceDescriptor desc,
-                                WeakCallback, const void*) {
-  // Weak objects should have been cleared at this point. As a consequence, all
-  // objects found through weak references have to point to live objects at this
-  // point.
-  VerifyChild(desc.base_object_payload);
-}
-
-void MarkingVerifier::VerifyChild(const void* base_object_payload) {
+void VerificationState::VerifyMarked(const void* base_object_payload) const {
   const HeapObjectHeader& child_header =
       HeapObjectHeader::FromPayload(base_object_payload);
 
@@ -50,27 +42,27 @@ void MarkingVerifier::VerifyChild(const void* base_object_payload) {
   }
 }
 
-void MarkingVerifier::VisitConservatively(
+void MarkingVerifierBase::VisitConservatively(
     HeapObjectHeader& header, TraceConservativelyCallback callback) {
   CHECK(header.IsMarked());
   in_construction_objects_->insert(&header);
   callback(this, header);
 }
 
-void MarkingVerifier::VisitPointer(const void* address) {
+void MarkingVerifierBase::VisitPointer(const void* address) {
   TraceConservativelyIfNeeded(address);
 }
 
-bool MarkingVerifier::VisitHeapObjectHeader(HeapObjectHeader* header) {
+bool MarkingVerifierBase::VisitHeapObjectHeader(HeapObjectHeader* header) {
   // Verify only non-free marked objects.
   if (!header->IsMarked()) return true;
 
   DCHECK(!header->IsFree());
 
-  parent_ = header;
+  SetCurrentParent(header);
 
   if (!header->IsInConstruction()) {
-    header->Trace(this);
+    header->Trace(visitor_.get());
   } else {
     // Dispatches to conservative tracing implementation.
     TraceConservativelyIfNeeded(*header);
@@ -79,5 +71,38 @@ bool MarkingVerifier::VisitHeapObjectHeader(HeapObjectHeader* header) {
   return true;
 }
 
+namespace {
+
+class VerificationVisitor final : public cppgc::Visitor {
+ public:
+  explicit VerificationVisitor(VerificationState& state)
+      : cppgc::Visitor(VisitorFactory::CreateKey()), state_(state) {}
+
+  void Visit(const void*, TraceDescriptor desc) final {
+    state_.VerifyMarked(desc.base_object_payload);
+  }
+
+  void VisitWeak(const void*, TraceDescriptor desc, WeakCallback,
+                 const void*) final {
+    // Weak objects should have been cleared at this point. As a consequence,
+    // all objects found through weak references have to point to live objects
+    // at this point.
+    state_.VerifyMarked(desc.base_object_payload);
+  }
+
+ private:
+  VerificationState& state_;
+};
+
+}  // namespace
+
+MarkingVerifier::MarkingVerifier(HeapBase& heap_base)
+    : MarkingVerifierBase(heap_base,
+                          std::make_unique<VerificationVisitor>(state_)) {}
+
+void MarkingVerifier::SetCurrentParent(const HeapObjectHeader* parent) {
+  state_.SetCurrentParent(parent);
+}
+
 }  // namespace internal
 }  // namespace cppgc

src/heap/cppgc/marking-verifier.h

@@ -8,6 +8,7 @@
 #include <unordered_set>
 
 #include "src/heap/base/stack.h"
+#include "src/heap/cppgc/heap-object-header.h"
 #include "src/heap/cppgc/heap-visitor.h"
 #include "src/heap/cppgc/heap.h"
 #include "src/heap/cppgc/visitor.h"
@@ -15,29 +16,42 @@
 namespace cppgc {
 namespace internal {
 
-class V8_EXPORT_PRIVATE MarkingVerifier final
-    : private HeapVisitor<MarkingVerifier>,
-      public cppgc::Visitor,
-      public ConservativeTracingVisitor,
-      public heap::base::StackVisitor {
-  friend class HeapVisitor<MarkingVerifier>;
-
+class VerificationState {
  public:
-  explicit MarkingVerifier(HeapBase&, Heap::Config::StackState);
-
-  void Visit(const void*, TraceDescriptor) final;
-  void VisitWeak(const void*, TraceDescriptor, WeakCallback, const void*) final;
+  void VerifyMarked(const void*) const;
+  void SetCurrentParent(const HeapObjectHeader* header) { parent_ = header; }
 
  private:
-  void VerifyChild(const void*);
+  const HeapObjectHeader* parent_ = nullptr;
+};
 
+class V8_EXPORT_PRIVATE MarkingVerifierBase
+    : private HeapVisitor<MarkingVerifierBase>,
+      public ConservativeTracingVisitor,
+      public heap::base::StackVisitor {
+  friend class HeapVisitor<MarkingVerifierBase>;
+
+ public:
+  ~MarkingVerifierBase() override = default;
+
+  MarkingVerifierBase(const MarkingVerifierBase&) = delete;
+  MarkingVerifierBase& operator=(const MarkingVerifierBase&) = delete;
+
+  void Run(Heap::Config::StackState);
+
+ protected:
+  MarkingVerifierBase(HeapBase&, std::unique_ptr<cppgc::Visitor>);
+
+  virtual void SetCurrentParent(const HeapObjectHeader*) = 0;
+
+ private:
   void VisitConservatively(HeapObjectHeader&,
                            TraceConservativelyCallback) final;
   void VisitPointer(const void*) final;
 
   bool VisitHeapObjectHeader(HeapObjectHeader*);
 
-  HeapObjectHeader* parent_ = nullptr;
+  std::unique_ptr<cppgc::Visitor> visitor_;
 
   std::unordered_set<const HeapObjectHeader*> in_construction_objects_heap_;
   std::unordered_set<const HeapObjectHeader*> in_construction_objects_stack_;
@@ -45,6 +59,17 @@ class V8_EXPORT_PRIVATE MarkingVerifier final
       &in_construction_objects_heap_;
 };
 
+class V8_EXPORT_PRIVATE MarkingVerifier final : public MarkingVerifierBase {
+ public:
+  explicit MarkingVerifier(HeapBase&);
+  ~MarkingVerifier() final = default;
+
+  void SetCurrentParent(const HeapObjectHeader*) final;
+
+ private:
+  VerificationState state_;
+};
+
 }  // namespace internal
 }  // namespace cppgc
 

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

@@ -23,7 +23,8 @@ class MarkingVerifierTest : public testing::TestWithHeap {
 
   void VerifyMarking(HeapBase& heap, StackState stack_state) {
     Heap::From(GetHeap())->object_allocator().ResetLinearAllocationBuffers();
-    MarkingVerifier verifier(heap, stack_state);
+    MarkingVerifier verifier(heap);
+    verifier.Run(stack_state);
   }
 };