cppgc: Pass on source location when tracing roots

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

include/cppgc/visitor.h

@@ -154,9 +154,9 @@ class V8_EXPORT Visitor {
   virtual void Visit(const void* self, TraceDescriptor) {}
   virtual void VisitWeak(const void* self, TraceDescriptor, WeakCallback,
                          const void* weak_member) {}
-  virtual void VisitRoot(const void*, TraceDescriptor) {}
+  virtual void VisitRoot(const void*, TraceDescriptor, const SourceLocation&) {}
   virtual void VisitWeakRoot(const void* self, TraceDescriptor, WeakCallback,
-                             const void* weak_root) {}
+                             const void* weak_root, const SourceLocation&) {}
 
  private:
   template <typename T, void (T::*method)(const LivenessBroker&)>
@@ -190,7 +190,8 @@ class V8_EXPORT Visitor {
     if (!p.Get()) {
       return;
     }
-    VisitRoot(p.Get(), TraceTrait<PointeeType>::GetTraceDescriptor(p.Get()));
+    VisitRoot(p.Get(), TraceTrait<PointeeType>::GetTraceDescriptor(p.Get()),
+              loc);
   }
 
   template <
@@ -204,7 +205,7 @@ class V8_EXPORT Visitor {
                   "Persistent's pointee type must be GarbageCollected or "
                   "GarbageCollectedMixin");
     VisitWeakRoot(p.Get(), TraceTrait<PointeeType>::GetTraceDescriptor(p.Get()),
-                  &HandleWeak<WeakPersistent>, &p);
+                  &HandleWeak<WeakPersistent>, &p, loc);
   }
 
   template <typename T>

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

@@ -58,14 +58,16 @@ MutatorUnifiedHeapMarkingVisitor::MutatorUnifiedHeapMarkingVisitor(
                                     unified_heap_marking_state) {}
 
 void MutatorUnifiedHeapMarkingVisitor::VisitRoot(const void* object,
-                                                 TraceDescriptor desc) {
+                                                 TraceDescriptor desc,
+                                                 const SourceLocation&) {
   this->Visit(object, desc);
 }
 
 void MutatorUnifiedHeapMarkingVisitor::VisitWeakRoot(const void* object,
                                                      TraceDescriptor desc,
                                                      WeakCallback weak_callback,
-                                                     const void* weak_root) {
+                                                     const void* weak_root,
+                                                     const SourceLocation&) {
   static_cast<MutatorMarkingState&>(marking_state_)
       .InvokeWeakRootsCallbackIfNeeded(object, desc, weak_callback, weak_root);
 }

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

@@ -12,6 +12,9 @@
 #include "src/heap/cppgc/marking-visitor.h"
 
 namespace cppgc {
+
+class SourceLocation;
+
 namespace internal {
 class ConcurrentMarkingState;
 class MarkingStateBase;
@@ -22,6 +25,7 @@ class MutatorMarkingState;
 namespace v8 {
 namespace internal {
 
+using cppgc::SourceLocation;
 using cppgc::TraceDescriptor;
 using cppgc::WeakCallback;
 using cppgc::internal::ConcurrentMarkingState;
@@ -56,9 +60,9 @@ class V8_EXPORT_PRIVATE MutatorUnifiedHeapMarkingVisitor final
   ~MutatorUnifiedHeapMarkingVisitor() override = default;
 
  protected:
-  void VisitRoot(const void*, TraceDescriptor) final;
-  void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback,
-                     const void*) final;
+  void VisitRoot(const void*, TraceDescriptor, const SourceLocation&) final;
+  void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback, const void*,
+                     const SourceLocation&) final;
 };
 
 class V8_EXPORT_PRIVATE ConcurrentUnifiedHeapMarkingVisitor final
@@ -69,9 +73,11 @@ class V8_EXPORT_PRIVATE ConcurrentUnifiedHeapMarkingVisitor final
   ~ConcurrentUnifiedHeapMarkingVisitor() override = default;
 
  protected:
-  void VisitRoot(const void*, TraceDescriptor) final { UNREACHABLE(); }
-  void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback,
-                     const void*) final {
+  void VisitRoot(const void*, TraceDescriptor, const SourceLocation&) final {
+    UNREACHABLE();
+  }
+  void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback, const void*,
+                     const SourceLocation&) final {
     UNREACHABLE();
   }
 

src/heap/cppgc/marking-visitor.cc

@@ -46,15 +46,16 @@ MutatorMarkingVisitor::MutatorMarkingVisitor(HeapBase& heap,
                                              MutatorMarkingState& marking_state)
     : MarkingVisitorBase(heap, marking_state) {}
 
-void MutatorMarkingVisitor::VisitRoot(const void* object,
-                                      TraceDescriptor desc) {
+void MutatorMarkingVisitor::VisitRoot(const void* object, TraceDescriptor desc,
+                                      const SourceLocation&) {
   Visit(object, desc);
 }
 
 void MutatorMarkingVisitor::VisitWeakRoot(const void* object,
                                           TraceDescriptor desc,
                                           WeakCallback weak_callback,
-                                          const void* weak_root) {
+                                          const void* weak_root,
+                                          const SourceLocation&) {
   static_cast<MutatorMarkingState&>(marking_state_)
       .InvokeWeakRootsCallbackIfNeeded(object, desc, weak_callback, weak_root);
 }

src/heap/cppgc/marking-visitor.h

@@ -39,9 +39,9 @@ class V8_EXPORT_PRIVATE MutatorMarkingVisitor : public MarkingVisitorBase {
   ~MutatorMarkingVisitor() override = default;
 
  protected:
-  void VisitRoot(const void*, TraceDescriptor) final;
-  void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback,
-                     const void*) final;
+  void VisitRoot(const void*, TraceDescriptor, const SourceLocation&) final;
+  void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback, const void*,
+                     const SourceLocation&) final;
 };
 
 class V8_EXPORT_PRIVATE ConcurrentMarkingVisitor final
@@ -51,9 +51,11 @@ class V8_EXPORT_PRIVATE ConcurrentMarkingVisitor final
   ~ConcurrentMarkingVisitor() override = default;
 
  protected:
-  void VisitRoot(const void*, TraceDescriptor) final { UNREACHABLE(); }
-  void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback,
-                     const void*) final {
+  void VisitRoot(const void*, TraceDescriptor, const SourceLocation&) final {
+    UNREACHABLE();
+  }
+  void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback, const void*,
+                     const SourceLocation&) final {
     UNREACHABLE();
   }
 

src/heap/cppgc/visitor.h

@@ -31,14 +31,8 @@ class VisitorBase : public cppgc::Visitor {
   VisitorBase(const VisitorBase&) = delete;
   VisitorBase& operator=(const VisitorBase&) = delete;
 
-  template <typename T>
-  void TraceRootForTesting(const Persistent<T>& p, const SourceLocation& loc) {
-    TraceRoot(p, loc);
-  }
-
-  template <typename T>
-  void TraceRootForTesting(const WeakPersistent<T>& p,
-                           const SourceLocation& loc) {
+  template <typename Persistent>
+  void TraceRootForTesting(const Persistent& p, const SourceLocation& loc) {
     TraceRoot(p, loc);
   }
 };

test/unittests/heap/cppgc/persistent-family-unittest.cc

@@ -10,6 +10,7 @@
 #include "include/cppgc/internal/pointer-policies.h"
 #include "include/cppgc/member.h"
 #include "include/cppgc/persistent.h"
+#include "include/cppgc/source-location.h"
 #include "include/cppgc/type-traits.h"
 #include "src/heap/cppgc/heap.h"
 #include "src/heap/cppgc/liveness-broker.h"
@@ -94,11 +95,12 @@ class RootVisitor final : public VisitorBase {
   }
 
  protected:
-  void VisitRoot(const void* t, TraceDescriptor desc) final {
+  void VisitRoot(const void* t, TraceDescriptor desc,
+                 const SourceLocation&) final {
     desc.callback(this, desc.base_object_payload);
   }
   void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback callback,
-                     const void* object) final {
+                     const void* object, const SourceLocation&) final {
     weak_callbacks_.emplace_back(callback, object);
   }
 
@@ -814,7 +816,47 @@ TEST_F(PersistentTest, LocalizedPersistent) {
     EXPECT_EQ(expected_loc.Line(), p2.Location().Line());
   }
 }
+
 #endif
 
+namespace {
+
+class ExpectingLocationVisitor final : public VisitorBase {
+ public:
+  explicit ExpectingLocationVisitor(const SourceLocation& expected_location)
+      : expected_loc_(expected_location) {}
+
+ protected:
+  void VisitRoot(const void* t, TraceDescriptor desc,
+                 const SourceLocation& loc) final {
+    EXPECT_STREQ(expected_loc_.Function(), loc.Function());
+    EXPECT_STREQ(expected_loc_.FileName(), loc.FileName());
+    EXPECT_EQ(expected_loc_.Line(), loc.Line());
+  }
+
+ private:
+  const SourceLocation& expected_loc_;
+};
+
+}  // namespace
+
+TEST_F(PersistentTest, PersistentTraceLocation) {
+  GCed* gced = MakeGarbageCollected<GCed>(GetAllocationHandle());
+  {
+#if CPPGC_SUPPORTS_SOURCE_LOCATION
+    // Baseline for creating expected location which has a different line
+    // number.
+    const auto loc = SourceLocation::Current();
+    const auto expected_loc =
+        SourceLocation::Current(loc.Function(), loc.FileName(), loc.Line() + 6);
+#else   // !CCPPGC_SUPPORTS_SOURCE_LOCATION
+    const SourceLocation expected_loc;
+#endif  // !CCPPGC_SUPPORTS_SOURCE_LOCATION
+    LocalizedPersistent<GCed> p = gced;
+    ExpectingLocationVisitor visitor(expected_loc);
+    visitor.TraceRootForTesting(p, p.Location());
+  }
+}
+
 }  // namespace internal
 }  // namespace cppgc