cppgc: Improve marking verifier

- Reset parent object and signal stack with nullptr sentinel
- Adjust FATAL messaging
- Fix dispatch for in-construction objects on stack

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

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

@@ -35,7 +35,7 @@ class UnifiedHeapVerificationVisitor final : public JSVisitor {
 
   void VisitWeakContainer(const void* object, cppgc::TraceDescriptor,
                           cppgc::TraceDescriptor weak_desc, cppgc::WeakCallback,
-                          const void*) {
+                          const void*) final {
     if (!object) return;
 
     // Contents of weak containers are found themselves through page iteration
@@ -58,13 +58,8 @@ class UnifiedHeapVerificationVisitor final : public JSVisitor {
 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);
-}
+          heap_base, state_,
+          std::make_unique<UnifiedHeapVerificationVisitor>(state_)) {}
 
 }  // namespace internal
 }  // namespace v8

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

@@ -16,8 +16,6 @@ class V8_EXPORT_PRIVATE UnifiedHeapMarkingVerifier final
   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.

src/heap/cppgc/marking-verifier.cc

@@ -14,8 +14,10 @@ namespace cppgc {
 namespace internal {
 
 MarkingVerifierBase::MarkingVerifierBase(
-    HeapBase& heap, std::unique_ptr<cppgc::Visitor> visitor)
+    HeapBase& heap, VerificationState& verification_state,
+    std::unique_ptr<cppgc::Visitor> visitor)
     : ConservativeTracingVisitor(heap, *heap.page_backend(), *visitor.get()),
+      verification_state_(verification_state),
       visitor_(std::move(visitor)) {}
 
 void MarkingVerifierBase::Run(Heap::Config::StackState stack_state) {
@@ -36,23 +38,38 @@ void VerificationState::VerifyMarked(const void* base_object_payload) const {
         "MarkingVerifier: Encountered unmarked object.\n"
         "#\n"
         "# Hint:\n"
-        "#   %s\n"
-        "#     \\-> %s",
-        parent_->GetName().value, child_header.GetName().value);
+        "#   %s (%p)\n"
+        "#     \\-> %s (%p)",
+        parent_ ? parent_->GetName().value : "Stack",
+        parent_ ? parent_->Payload() : nullptr, child_header.GetName().value,
+        child_header.Payload());
   }
 }
 
 void MarkingVerifierBase::VisitInConstructionConservatively(
     HeapObjectHeader& header, TraceConservativelyCallback callback) {
-  CHECK(header.IsMarked());
   if (in_construction_objects_->find(&header) !=
       in_construction_objects_->end())
     return;
   in_construction_objects_->insert(&header);
+
+  // Stack case: Parent is stack and this is merely ensuring that the object
+  // itself is marked. If the object is marked, then it is being processed by
+  // the on-heap phase.
+  if (verification_state_.IsParentOnStack()) {
+    verification_state_.VerifyMarked(header.Payload());
+    return;
+  }
+
+  // Heap case: Dispatching parent object that must be marked (pre-condition).
+  CHECK(header.IsMarked());
   callback(this, header);
 }
 
 void MarkingVerifierBase::VisitPointer(const void* address) {
+  // Entry point for stack walk. The conservative visitor dispatches as follows:
+  // - Fully constructed objects: Visit()
+  // - Objects in construction: VisitInConstructionConservatively()
   TraceConservativelyIfNeeded(address);
 }
 
@@ -62,7 +79,7 @@ bool MarkingVerifierBase::VisitHeapObjectHeader(HeapObjectHeader* header) {
 
   DCHECK(!header->IsFree());
 
-  SetCurrentParent(header);
+  verification_state_.SetCurrentParent(header);
 
   if (!header->IsInConstruction()) {
     header->Trace(visitor_.get());
@@ -71,6 +88,8 @@ bool MarkingVerifierBase::VisitHeapObjectHeader(HeapObjectHeader* header) {
     TraceConservativelyIfNeeded(*header);
   }
 
+  verification_state_.SetCurrentParent(nullptr);
+
   return true;
 }
 
@@ -112,12 +131,8 @@ class VerificationVisitor final : public cppgc::Visitor {
 }  // namespace
 
 MarkingVerifier::MarkingVerifier(HeapBase& heap_base)
-    : MarkingVerifierBase(heap_base,
+    : MarkingVerifierBase(heap_base, state_,
                           std::make_unique<VerificationVisitor>(state_)) {}
 
-void MarkingVerifier::SetCurrentParent(const HeapObjectHeader* parent) {
-  state_.SetCurrentParent(parent);
-}
-
 }  // namespace internal
 }  // namespace cppgc

src/heap/cppgc/marking-verifier.h

@@ -21,6 +21,9 @@ class VerificationState {
   void VerifyMarked(const void*) const;
   void SetCurrentParent(const HeapObjectHeader* header) { parent_ = header; }
 
+  // No parent means parent was on stack.
+  bool IsParentOnStack() const { return !parent_; }
+
  private:
   const HeapObjectHeader* parent_ = nullptr;
 };
@@ -40,9 +43,8 @@ class V8_EXPORT_PRIVATE MarkingVerifierBase
   void Run(Heap::Config::StackState);
 
  protected:
-  MarkingVerifierBase(HeapBase&, std::unique_ptr<cppgc::Visitor>);
-
-  virtual void SetCurrentParent(const HeapObjectHeader*) = 0;
+  MarkingVerifierBase(HeapBase&, VerificationState&,
+                      std::unique_ptr<cppgc::Visitor>);
 
  private:
   void VisitInConstructionConservatively(HeapObjectHeader&,
@@ -51,6 +53,7 @@ class V8_EXPORT_PRIVATE MarkingVerifierBase
 
   bool VisitHeapObjectHeader(HeapObjectHeader*);
 
+  VerificationState& verification_state_;
   std::unique_ptr<cppgc::Visitor> visitor_;
 
   std::unordered_set<const HeapObjectHeader*> in_construction_objects_heap_;
@@ -64,8 +67,6 @@ class V8_EXPORT_PRIVATE MarkingVerifier final : public MarkingVerifierBase {
   explicit MarkingVerifier(HeapBase&);
   ~MarkingVerifier() final = default;
 
-  void SetCurrentParent(const HeapObjectHeader*) final;
-
  private:
   VerificationState state_;
 };