cppgc: Provide Visitor and tracing infrastructure for Member

This CL adds the necessary traits to dispatch from Member through a
visitor implementation for GarabgeCollected and GarbageCollectedMixin.

Bug: chromium:1056170
Change-Id: I12680335044aaa842639fb5e8f9a3ac61587f51a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2138431Reviewed-by: Hannes Payer <hpayer@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67041}

BUILD.gn

@@ -3954,7 +3954,9 @@ v8_source_set("cppgc_base") {
     "include/cppgc/member.h",
     "include/cppgc/platform.h",
     "include/cppgc/source-location.h",
-    "include/cppgc/type_traits.h",
+    "include/cppgc/trace-trait.h",
+    "include/cppgc/type-traits.h",
+    "include/cppgc/visitor.h",
     "include/v8config.h",
     "src/heap/cppgc/allocation.cc",
     "src/heap/cppgc/gc-info-table.cc",

include/cppgc/finalizer-trait.h

@@ -7,7 +7,7 @@
 
 #include <type_traits>
 
-#include "include/cppgc/internals.h"
+#include "include/cppgc/type-traits.h"
 
 namespace cppgc {
 namespace internal {

include/cppgc/garbage-collected.h

@@ -9,35 +9,14 @@
 
 #include "include/cppgc/internals.h"
 #include "include/cppgc/platform.h"
-#include "include/cppgc/type_traits.h"
+#include "include/cppgc/trace-trait.h"
+#include "include/cppgc/type-traits.h"
 
 namespace cppgc {
-namespace internal {
-
-template <typename T, typename = void>
-struct IsGarbageCollectedMixinType : std::false_type {
-  static_assert(sizeof(T), "T must be fully defined");
-};
 
-template <typename T>
-struct IsGarbageCollectedMixinType<
-    T,
-    void_t<typename std::remove_const_t<T>::IsGarbageCollectedMixinTypeMarker>>
-    : std::true_type {
-  static_assert(sizeof(T), "T must be fully defined");
-};
-
-template <typename T, typename = void>
-struct IsGarbageCollectedType : IsGarbageCollectedMixinType<T> {
-  static_assert(sizeof(T), "T must be fully defined");
-};
+class Visitor;
 
-template <typename T>
-struct IsGarbageCollectedType<
-    T, void_t<typename std::remove_const_t<T>::IsGarbageCollectedTypeMarker>>
-    : std::true_type {
-  static_assert(sizeof(T), "T must be fully defined");
-};
+namespace internal {
 
 class GarbageCollectedBase {
  public:
@@ -75,17 +54,14 @@ class GarbageCollectedMixin : public internal::GarbageCollectedBase {
   // Sentinel used to mark not-fully-constructed mixins.
   static constexpr void* kNotFullyConstructedObject = nullptr;
 
-  // TODO(chromium:1056170): Add virtual Trace method.
+  virtual void Trace(cppgc::Visitor*) {}
 
- protected:
   // Provide default implementation that indicate that the vtable is not yet
   // set up properly. This is used to to get GCInfo objects for mixins so that
   // these objects can be processed later on.
-  virtual const void* GetObjectStart() const {
-    return kNotFullyConstructedObject;
+  virtual TraceDescriptor GetTraceDescriptor() const {
+    return {kNotFullyConstructedObject, nullptr};
   }
-
-  GarbageCollectedMixin() = default;
 };
 
 namespace internal {
@@ -101,13 +77,15 @@ class __thisIsHereToForceASemicolonAfterThisMacro {};
 #define USING_GARBAGE_COLLECTED_MIXIN()                                      \
  public:                                                                     \
   typedef int HasUsingGarbageCollectedMixinMacro;                            \
-  const void* GetObjectStart() const override {                              \
+                                                                             \
+  TraceDescriptor GetTraceDescriptor() const override {                      \
     static_assert(                                                           \
         internal::IsSubclassOfTemplate<                                      \
             std::remove_const_t<std::remove_pointer_t<decltype(this)>>,      \
             cppgc::GarbageCollected>::value,                                 \
         "Only garbage collected objects can have garbage collected mixins"); \
-    return this;                                                             \
+    return {this, TraceTrait<std::remove_const_t<                            \
+                      std::remove_pointer_t<decltype(this)>>>::Trace};       \
   }                                                                          \
                                                                              \
  private:                                                                    \
@@ -118,23 +96,23 @@ class __thisIsHereToForceASemicolonAfterThisMacro {};
 //  class A : public GarbageCollectedMixin {};
 //  class B : public GarbageCollectedMixin {};
 //  class C : public A, public B {
-//    // C::GetObjectStart is now ambiguous because there are two
-//    // candidates: A::GetObjectStart and B::GetObjectStart.  Ditto for
+//    // C::GetTraceDescriptor is now ambiguous because there are two
+//    // candidates: A::GetTraceDescriptor and B::GetTraceDescriptor.  Ditto for
 //    // other functions.
 //
 //    MERGE_GARBAGE_COLLECTED_MIXINS();
-//    // The macro defines C::GetObjectStart, similar to GarbageCollectedMixin,ß
-//    // so that they are no longer ambiguous.
+//    // The macro defines C::GetTraceDescriptor, similar to
+//    // GarbageCollectedMixin, so that they are no longer ambiguous.
 //    // USING_GARBAGE_COLLECTED_MIXIN() overrides them later and provides the
 //    // implementations.
 //  };
-#define MERGE_GARBAGE_COLLECTED_MIXINS()        \
- public:                                        \
-  const void* GetObjectStart() const override { \
-    return kNotFullyConstructedObject;          \
-  }                                             \
-                                                \
- private:                                       \
+#define MERGE_GARBAGE_COLLECTED_MIXINS()                \
+ public:                                                \
+  TraceDescriptor GetTraceDescriptor() const override { \
+    return {kNotFullyConstructedObject, nullptr};       \
+  }                                                     \
+                                                        \
+ private:                                               \
   friend class internal::__thisIsHereToForceASemicolonAfterThisMacro
 
 }  // namespace cppgc

include/cppgc/internals.h

@@ -13,14 +13,6 @@
 namespace cppgc {
 namespace internal {
 
-// Pre-C++17 custom implementation of std::void_t.
-template <typename... Ts>
-struct make_void {
-  typedef void type;
-};
-template <typename... Ts>
-using void_t = typename make_void<Ts...>::type;
-
 // Embedders should not rely on this code!
 
 // Internal constants to avoid exposing internal types on the API surface.

include/cppgc/member.h

@@ -12,6 +12,8 @@
 
 namespace cppgc {
 
+class Visitor;
+
 namespace internal {
 
 struct DijkstraWriteBarrierPolicy {
@@ -154,6 +156,8 @@ class BasicMember : private CheckingPolicy {
   }
 
   T* raw_ = nullptr;
+
+  friend class cppgc::Visitor;
 };
 
 template <typename T1, typename WeaknessTag1, typename WriteBarrierPolicy1,

include/cppgc/trace-trait.h

+// 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_TRACE_TRAIT_H_
+#define V8_HEAP_CPPGC_TRACE_TRAIT_H_
+
+#include <type_traits>
+#include "include/cppgc/type-traits.h"
+
+namespace cppgc {
+
+class Visitor;
+
+namespace internal {
+
+template <typename T,
+          bool =
+              IsGarbageCollectedMixinTypeV<typename std::remove_const<T>::type>>
+struct TraceTraitImpl;
+
+}  // namespace internal
+
+using TraceCallback = void (*)(Visitor*, const void*);
+
+// TraceDescriptor is used to describe how to trace an object.
+struct TraceDescriptor {
+  // The adjusted base pointer of the object that should be traced.
+  const void* base_object_payload;
+  // A callback for tracing the object.
+  TraceCallback callback;
+};
+
+template <typename T>
+struct TraceTrait {
+  static_assert(internal::IsTraceableV<T>, "T must have a Trace() method");
+
+  static TraceDescriptor GetTraceDescriptor(const void* self) {
+    return internal::TraceTraitImpl<T>::GetTraceDescriptor(
+        static_cast<const T*>(self));
+  }
+
+  static void Trace(Visitor* visitor, const void* self) {
+    // TODO(chromium:1056170): Remove const_cast when Trace() methods are const.
+    static_cast<T*>(const_cast<void*>(self))->Trace(visitor);
+  }
+};
+
+namespace internal {
+
+template <typename T>
+struct TraceTraitImpl<T, false> {
+  static TraceDescriptor GetTraceDescriptor(const void* self) {
+    return {self, TraceTrait<T>::Trace};
+  }
+};
+
+template <typename T>
+struct TraceTraitImpl<T, true> {
+  static TraceDescriptor GetTraceDescriptor(const void* self) {
+    return static_cast<const T*>(self)->GetTraceDescriptor();
+  }
+};
+
+}  // namespace internal
+}  // namespace cppgc
+
+#endif  // V8_HEAP_CPPGC_TRACE_TRAIT_H_

include/cppgc/type_traits.h → include/cppgc/type-traits.h

@@ -11,8 +11,18 @@
 
 namespace cppgc {
 
+class Visitor;
+
 namespace internal {
 
+// Pre-C++17 custom implementation of std::void_t.
+template <typename... Ts>
+struct make_void {
+  typedef void type;
+};
+template <typename... Ts>
+using void_t = typename make_void<Ts...>::type;
+
 // Not supposed to be specialized by the user.
 template <typename T>
 struct IsWeak : std::false_type {};
@@ -33,6 +43,52 @@ struct IsSubclassOfTemplate {
       decltype(SubclassCheck(std::declval<T*>()))::value;
 };
 
+template <typename T, typename = void>
+struct IsTraceable : std::false_type {
+  static_assert(sizeof(T), "T must be fully defined");
+};
+
+template <typename T>
+struct IsTraceable<
+    T, void_t<decltype(std::declval<T>().Trace(std::declval<Visitor*>()))>>
+    : std::true_type {};
+
+template <typename T>
+constexpr bool IsTraceableV = IsTraceable<T>::value;
+
+template <typename T, typename = void>
+struct IsGarbageCollectedMixinType : std::false_type {
+  static_assert(sizeof(T), "T must be fully defined");
+};
+
+template <typename T>
+struct IsGarbageCollectedMixinType<
+    T,
+    void_t<typename std::remove_const_t<T>::IsGarbageCollectedMixinTypeMarker>>
+    : std::true_type {
+  static_assert(sizeof(T), "T must be fully defined");
+};
+
+template <typename T, typename = void>
+struct IsGarbageCollectedType : IsGarbageCollectedMixinType<T> {
+  static_assert(sizeof(T), "T must be fully defined");
+};
+
+template <typename T>
+struct IsGarbageCollectedType<
+    T, void_t<typename std::remove_const_t<T>::IsGarbageCollectedTypeMarker>>
+    : std::true_type {
+  static_assert(sizeof(T), "T must be fully defined");
+};
+
+template <typename T>
+constexpr bool IsGarbageCollectedTypeV =
+    internal::IsGarbageCollectedType<T>::value;
+
+template <typename T>
+constexpr bool IsGarbageCollectedMixinTypeV =
+    internal::IsGarbageCollectedMixinType<T>::value;
+
 }  // namespace internal
 
 template <typename T>

include/cppgc/visitor.h

+// 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 INCLUDE_CPPGC_VISITOR_H_
+#define INCLUDE_CPPGC_VISITOR_H_
+
+#include "include/cppgc/garbage-collected.h"
+#include "include/cppgc/member.h"
+#include "include/cppgc/trace-trait.h"
+
+namespace cppgc {
+namespace internal {
+class VisitorBase;
+}  // namespace internal
+
+class Visitor {
+ public:
+  template <typename T>
+  void Trace(const Member<T>& member) {
+    const T* value = member.GetRawAtomic();
+    // TODO(chromium:1056170): DCHECK (or similar) for deleted values as they
+    // should come in at a different path.
+    Trace(value);
+  }
+
+ protected:
+  virtual void Visit(const void*, TraceDescriptor) {}
+
+ private:
+  Visitor() = default;
+
+  template <typename T>
+  void Trace(const T* t) {
+    static_assert(sizeof(T), "T must be fully defined");
+    static_assert(internal::IsGarbageCollectedType<T>::value,
+                  "T must be GarabgeCollected or GarbageCollectedMixin type");
+    if (!t) {
+      return;
+    }
+    Visit(t, TraceTrait<T>::GetTraceDescriptor(t));
+  }
+
+  friend class internal::VisitorBase;
+};
+
+}  // namespace cppgc
+
+#endif  // INCLUDE_CPPGC_VISITOR_H_

src/heap/cppgc/visitor.h

+// 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_VISITOR_H_
+#define V8_HEAP_CPPGC_VISITOR_H_
+
+#include "include/cppgc/visitor.h"
+
+namespace cppgc {
+namespace internal {
+
+// Base visitor that is allowed to create a public cppgc::Visitor object.
+class VisitorBase : public cppgc::Visitor {
+ public:
+  VisitorBase() = default;
+};
+
+}  // namespace internal
+}  // namespace cppgc
+
+#endif  // V8_HEAP_CPPGC_VISITOR_H_

test/unittests/BUILD.gn

@@ -54,6 +54,7 @@ v8_source_set("cppgc_unittests_sources") {
     "heap/cppgc/stack_unittest.cc",
     "heap/cppgc/tests.cc",
     "heap/cppgc/tests.h",
+    "heap/cppgc/visitor_unittest.cc",
   ]
 
   configs = [

test/unittests/heap/cppgc/garbage-collected_unittest.cc

@@ -16,19 +16,25 @@ namespace {
 
 class GCed : public GarbageCollected<GCed> {};
 class NotGCed {};
-class Mixin : public GarbageCollectedMixin {
- public:
-  using GarbageCollectedMixin::GetObjectStart;
-};
+class Mixin : public GarbageCollectedMixin {};
 class GCedWithMixin : public GarbageCollected<GCedWithMixin>, public Mixin {
   USING_GARBAGE_COLLECTED_MIXIN();
 };
 class OtherMixin : public GarbageCollectedMixin {};
 class MergedMixins : public Mixin, public OtherMixin {
   MERGE_GARBAGE_COLLECTED_MIXINS();
+
+ public:
+  void Trace(cppgc::Visitor* visitor) override {
+    Mixin::Trace(visitor);
+    OtherMixin::Trace(visitor);
+  }
 };
 class GCWithMergedMixins : public GCed, public MergedMixins {
   USING_GARBAGE_COLLECTED_MIXIN();
+
+ public:
+  void Trace(cppgc::Visitor* visitor) override { MergedMixins::Trace(visitor); }
 };
 
 class GarbageCollectedTestWithHeap : public testing::TestWithHeap {
@@ -66,9 +72,12 @@ TEST_F(GarbageCollectedTestWithHeap, GetObjectStartReturnsCorrentAddress) {
   GCed* gced = MakeGarbageCollected<GCed>(GetHeap());
   GCedWithMixin* gced_with_mixin =
       MakeGarbageCollected<GCedWithMixin>(GetHeap());
-  EXPECT_EQ(gced_with_mixin,
-            static_cast<Mixin*>(gced_with_mixin)->GetObjectStart());
-  EXPECT_NE(gced, static_cast<Mixin*>(gced_with_mixin)->GetObjectStart());
+  EXPECT_EQ(gced_with_mixin, static_cast<Mixin*>(gced_with_mixin)
+                                 ->GetTraceDescriptor()
+                                 .base_object_payload);
+  EXPECT_NE(gced, static_cast<Mixin*>(gced_with_mixin)
+                      ->GetTraceDescriptor()
+                      .base_object_payload);
 }
 
 #endif  // CPPGC_SUPPORTS_CONSERVATIVE_STACK_SCAN

test/unittests/heap/cppgc/member_unittests.cc

@@ -6,7 +6,7 @@
 
 #include "include/cppgc/garbage-collected.h"
 #include "include/cppgc/member.h"
-#include "include/cppgc/type_traits.h"
+#include "include/cppgc/type-traits.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
 

test/unittests/heap/cppgc/visitor_unittest.cc

+// 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/visitor.h"
+#include "include/cppgc/garbage-collected.h"
+#include "include/cppgc/trace-trait.h"
+#include "src/base/macros.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace cppgc {
+namespace internal {
+
+namespace {
+
+class GCed : public GarbageCollected<GCed> {
+ public:
+  static size_t trace_callcount;
+
+  GCed() { trace_callcount = 0; }
+
+  virtual void Trace(cppgc::Visitor* visitor) { trace_callcount++; }
+};
+
+size_t GCed::trace_callcount;
+
+class GCedMixin : public GarbageCollectedMixin {};
+
+class OtherPayload {
+ public:
+  virtual void* GetDummy() const { return nullptr; }
+};
+
+class GCedMixinApplication : public GCed,
+                             public OtherPayload,
+                             public GCedMixin {
+  USING_GARBAGE_COLLECTED_MIXIN();
+
+ public:
+  void Trace(cppgc::Visitor* visitor) override {
+    GCed::Trace(visitor);
+    GCedMixin::Trace(visitor);
+  }
+};
+
+class DispatchingVisitor final : public VisitorBase {
+ public:
+  DispatchingVisitor(const void* object, const void* payload)
+      : object_(object), payload_(payload) {}
+
+ protected:
+  void Visit(const void* t, TraceDescriptor desc) final {
+    EXPECT_EQ(object_, t);
+    EXPECT_EQ(payload_, desc.base_object_payload);
+    desc.callback(this, desc.base_object_payload);
+  }
+
+ private:
+  const void* object_;
+  const void* payload_;
+};
+
+}  // namespace
+
+TEST(TraceTraitTest, GetObjectStartGCed) {
+  GCed gced;
+  EXPECT_EQ(&gced,
+            TraceTrait<GCed>::GetTraceDescriptor(&gced).base_object_payload);
+}
+
+TEST(TraceTraitTest, GetObjectStartGCedMixin) {
+  GCedMixinApplication gced_mixin_app;
+  GCedMixin* gced_mixin = static_cast<GCedMixin*>(&gced_mixin_app);
+  EXPECT_EQ(&gced_mixin_app,
+            TraceTrait<GCedMixin>::GetTraceDescriptor(gced_mixin)
+                .base_object_payload);
+}
+
+TEST(TraceTraitTest, TraceGCed) {
+  GCed gced;
+  EXPECT_EQ(0u, GCed::trace_callcount);
+  TraceTrait<GCed>::Trace(nullptr, &gced);
+  EXPECT_EQ(1u, GCed::trace_callcount);
+}
+
+TEST(TraceTraitTest, TraceGCedMixin) {
+  GCedMixinApplication gced_mixin_app;
+  GCedMixin* gced_mixin = static_cast<GCedMixin*>(&gced_mixin_app);
+  EXPECT_EQ(0u, GCed::trace_callcount);
+  TraceTrait<GCedMixin>::Trace(nullptr, gced_mixin);
+  EXPECT_EQ(1u, GCed::trace_callcount);
+}
+
+TEST(TraceTraitTest, TraceGCedThroughTraceDescriptor) {
+  GCed gced;
+  EXPECT_EQ(0u, GCed::trace_callcount);
+  TraceDescriptor desc = TraceTrait<GCed>::GetTraceDescriptor(&gced);
+  desc.callback(nullptr, desc.base_object_payload);
+  EXPECT_EQ(1u, GCed::trace_callcount);
+}
+
+TEST(TraceTraitTest, TraceGCedMixinThroughTraceDescriptor) {
+  GCedMixinApplication gced_mixin_app;
+  GCedMixin* gced_mixin = static_cast<GCedMixin*>(&gced_mixin_app);
+  EXPECT_EQ(0u, GCed::trace_callcount);
+  TraceDescriptor desc = TraceTrait<GCedMixin>::GetTraceDescriptor(gced_mixin);
+  desc.callback(nullptr, desc.base_object_payload);
+  EXPECT_EQ(1u, GCed::trace_callcount);
+}
+
+TEST(VisitorTest, DispatchTraceGCed) {
+  GCed gced;
+  Member<GCed> ref(&gced);
+  DispatchingVisitor visitor(&gced, &gced);
+  EXPECT_EQ(0u, GCed::trace_callcount);
+  visitor.Trace(ref);
+  EXPECT_EQ(1u, GCed::trace_callcount);
+}
+
+TEST(VisitorTest, DispatchTraceGCedMixin) {
+  GCedMixinApplication gced_mixin_app;
+  GCedMixin* gced_mixin = static_cast<GCedMixin*>(&gced_mixin_app);
+  // Ensure that we indeed test dispatching an inner object.
+  EXPECT_NE(static_cast<void*>(&gced_mixin_app),
+            static_cast<void*>(gced_mixin));
+  Member<GCedMixin> ref(gced_mixin);
+  DispatchingVisitor visitor(gced_mixin, &gced_mixin_app);
+  EXPECT_EQ(0u, GCed::trace_callcount);
+  visitor.Trace(ref);
+  EXPECT_EQ(1u, GCed::trace_callcount);
+}
+
+}  // namespace internal
+}  // namespace cppgc