cppgc: Add TraceCallback to GCInfo

This is needed to trace objects found durinbg stack scanning. Bug: chromium:1056170 Change-Id: I1280d98f2fe69281c514b3a7d4a57f909a2eed96 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2190425 Commit-Queue: Omer Katz <[email protected]> Reviewed-by: Anton Bikineev <[email protected]> Reviewed-by: Michael Lippautz <[email protected]> Cr-Commit-Position: refs/heads/master@{#67788}
AntonBikineev · Jun 23, 2020 · e80db13 · e80db13
1 parent b92c330
commit e80db13
Show file tree

Hide file tree

Showing 11 changed files with 48 additions and 18 deletions.
diff --git a/include/cppgc/internal/gc-info.h b/include/cppgc/internal/gc-info.h
@@ -8,6 +8,7 @@
 #include <stdint.h>
 
 #include "cppgc/internal/finalizer-trait.h"
+#include "cppgc/trace-trait.h"
 #include "v8config.h"  // NOLINT(build/include_directory)
 
 namespace cppgc {
@@ -18,7 +19,7 @@ using GCInfoIndex = uint16_t;
 class V8_EXPORT RegisteredGCInfoIndex final {
  public:
   RegisteredGCInfoIndex(FinalizationCallback finalization_callback,
-                        bool has_v_table);
+                        TraceCallback trace_callback, bool has_v_table);
   GCInfoIndex GetIndex() const { return index_; }
 
  private:
@@ -32,7 +33,8 @@ struct GCInfoTrait {
   static GCInfoIndex Index() {
     static_assert(sizeof(T), "T must be fully defined");
     static const RegisteredGCInfoIndex registered_index(
-        FinalizerTrait<T>::kCallback, std::is_polymorphic<T>::value);
+        FinalizerTrait<T>::kCallback, TraceTrait<T>::Trace,
+        std::is_polymorphic<T>::value);
     return registered_index.GetIndex();
   }
 };

diff --git a/src/heap/cppgc/gc-info-table.cc b/src/heap/cppgc/gc-info-table.cc
@@ -18,6 +18,11 @@ namespace internal {
 
 namespace {
 
+// GCInfoTable::table_, the table which holds GCInfos, is maintained as a
+// contiguous array reserved upfront. Subparts of the array are (re-)committed
+// as read/write or read-only in OS pages, whose size is a power of 2. To avoid
+// having GCInfos that cross the boundaries between these subparts we force the
+// size of GCInfo to be a power of 2 as well.
 constexpr size_t kEntrySize = sizeof(GCInfo);
 static_assert(v8::base::bits::IsPowerOfTwo(kEntrySize),
               "GCInfoTable entries size must be power of "

diff --git a/src/heap/cppgc/gc-info-table.h b/src/heap/cppgc/gc-info-table.h
@@ -22,7 +22,10 @@ namespace internal {
 // inherit from GarbageCollected.
 struct GCInfo final {
   FinalizationCallback finalize;
+  TraceCallback trace;
   bool has_v_table;
+  // Keep sizeof(GCInfo) a power of 2.
+  size_t padding = 0;
 };
 
 class V8_EXPORT GCInfoTable final {

diff --git a/src/heap/cppgc/gc-info.cc b/src/heap/cppgc/gc-info.cc
@@ -10,9 +10,10 @@ namespace cppgc {
 namespace internal {
 
 RegisteredGCInfoIndex::RegisteredGCInfoIndex(
-    FinalizationCallback finalization_callback, bool has_v_table)
+    FinalizationCallback finalization_callback, TraceCallback trace_callback,
+    bool has_v_table)
     : index_(GlobalGCInfoTable::GetMutable().RegisterNewGCInfo(
-          {finalization_callback, has_v_table})) {}
+          {finalization_callback, trace_callback, has_v_table})) {}
 
 }  // namespace internal
 }  // namespace cppgc
diff --git a/src/heap/cppgc/heap-page.cc b/src/heap/cppgc/heap-page.cc
@@ -61,8 +61,7 @@ const HeapObjectHeader* BasePage::ObjectHeaderFromInnerAddress(
   DCHECK_LT(address,
             reinterpret_cast<ConstAddress>(header) +
                 header->GetSize<HeapObjectHeader::AccessMode::kAtomic>());
-  DCHECK_NE(kFreeListGCInfoIndex,
-            header->GetGCInfoIndex<HeapObjectHeader::AccessMode::kAtomic>());
+  DCHECK_NE(kFreeListGCInfoIndex, header->GetGCInfoIndex());
   return header;
 }
 

diff --git a/test/unittests/heap/cppgc/custom-spaces-unittest.cc b/test/unittests/heap/cppgc/custom-spaces-unittest.cc
@@ -48,18 +48,26 @@ class TestWithHeapWithCustomSpaces : public testing::TestWithPlatform {
   std::unique_ptr<cppgc::Heap> heap_;
 };
 
-class RegularGCed final : public GarbageCollected<RegularGCed> {};
+class RegularGCed final : public GarbageCollected<RegularGCed> {
+ public:
+  void Trace(Visitor*) const {}
+};
 
 class CustomGCed1 final : public GarbageCollected<CustomGCed1> {
  public:
   ~CustomGCed1() { g_destructor_callcount++; }
+  void Trace(Visitor*) const {}
 };
 class CustomGCed2 final : public GarbageCollected<CustomGCed2> {
  public:
   ~CustomGCed2() { g_destructor_callcount++; }
+  void Trace(Visitor*) const {}
 };
 
-class CustomGCedBase : public GarbageCollected<CustomGCedBase> {};
+class CustomGCedBase : public GarbageCollected<CustomGCedBase> {
+ public:
+  void Trace(Visitor*) const {}
+};
 class CustomGCedFinal1 final : public CustomGCedBase {
  public:
   ~CustomGCedFinal1() { g_destructor_callcount++; }

diff --git a/test/unittests/heap/cppgc/garbage-collected-unittest.cc b/test/unittests/heap/cppgc/garbage-collected-unittest.cc
@@ -16,7 +16,10 @@ namespace internal {
 
 namespace {
 
-class GCed : public GarbageCollected<GCed> {};
+class GCed : public GarbageCollected<GCed> {
+ public:
+  void Trace(Visitor*) const {}
+};
 class NotGCed {};
 class Mixin : public GarbageCollectedMixin {};
 class GCedWithMixin : public GarbageCollected<GCedWithMixin>, public Mixin {

diff --git a/test/unittests/heap/cppgc/gc-info-unittest.cc b/test/unittests/heap/cppgc/gc-info-unittest.cc
@@ -23,7 +23,7 @@ TEST(GCInfoTableTest, InitialEmpty) {
 TEST(GCInfoTableTest, ResizeToMaxIndex) {
   v8::base::PageAllocator page_allocator;
   GCInfoTable table(&page_allocator);
-  GCInfo info = {nullptr, false};
+  GCInfo info = {nullptr, nullptr, false};
   for (GCInfoIndex i = GCInfoTable::kMinIndex; i < GCInfoTable::kMaxIndex;
        i++) {
     GCInfoIndex index = table.RegisterNewGCInfo(info);
@@ -34,7 +34,7 @@ TEST(GCInfoTableTest, ResizeToMaxIndex) {
 TEST(GCInfoTableDeathTest, MoreThanMaxIndexInfos) {
   v8::base::PageAllocator page_allocator;
   GCInfoTable table(&page_allocator);
-  GCInfo info = {nullptr, false};
+  GCInfo info = {nullptr, nullptr, false};
   // Create GCInfoTable::kMaxIndex entries.
   for (GCInfoIndex i = GCInfoTable::kMinIndex; i < GCInfoTable::kMaxIndex;
        i++) {
@@ -46,7 +46,7 @@ TEST(GCInfoTableDeathTest, MoreThanMaxIndexInfos) {
 TEST(GCInfoTableDeathTest, OldTableAreaIsReadOnly) {
   v8::base::PageAllocator page_allocator;
   GCInfoTable table(&page_allocator);
-  GCInfo info = {nullptr, false};
+  GCInfo info = {nullptr, nullptr, false};
   // Use up all slots until limit.
   GCInfoIndex limit = table.LimitForTesting();
   // Bail out if initial limit is already the maximum because of large committed
@@ -76,7 +76,7 @@ class ThreadRegisteringGCInfoObjects final : public v8::base::Thread {
         num_registrations_(num_registrations) {}
 
   void Run() final {
-    GCInfo info = {nullptr, false};
+    GCInfo info = {nullptr, nullptr, false};
     for (GCInfoIndex i = 0; i < num_registrations_; i++) {
       table_->RegisterNewGCInfo(info);
     }
@@ -101,7 +101,7 @@ TEST(GCInfoTableTest, MultiThreadedResizeToMaxIndex) {
 
   v8::base::PageAllocator page_allocator;
   GCInfoTable table(&page_allocator);
-  GCInfo info = {nullptr, false};
+  GCInfo info = {nullptr, nullptr, false};
   for (size_t i = 0; i < main_thread_initialized; i++) {
     table.RegisterNewGCInfo(info);
   }
@@ -126,8 +126,14 @@ namespace {
 
 class GCInfoTraitTest : public testing::TestWithPlatform {};
 
-class BasicType final {};
-class OtherBasicType final {};
+class BasicType final {
+ public:
+  void Trace(Visitor*) const {}
+};
+class OtherBasicType final {
+ public:
+  void Trace(Visitor*) const {}
+};
 
 }  // namespace
 

diff --git a/test/unittests/heap/cppgc/heap-unittest.cc b/test/unittests/heap/cppgc/heap-unittest.cc
@@ -36,14 +36,16 @@ class Foo : public GarbageCollected<Foo> {
 
   Foo() { destructor_callcount = 0; }
   ~Foo() { destructor_callcount++; }
+
+  void Trace(cppgc::Visitor*) const {}
 };
 
 size_t Foo::destructor_callcount;
 
 template <size_t Size>
 class GCed : public GarbageCollected<Foo> {
  public:
-  void Visit(cppgc::Visitor*) {}
+  void Trace(cppgc::Visitor*) const {}
   char buf[Size];
 };
 

diff --git a/test/unittests/heap/cppgc/sweeper-unittest.cc b/test/unittests/heap/cppgc/sweeper-unittest.cc
@@ -211,6 +211,7 @@ class GCInDestructor final : public GarbageCollected<GCInDestructor> {
     // well.
     heap_->CollectGarbage(internal::Heap::GCConfig::Default());
   }
+  void Trace(Visitor*) const {}
 
  private:
   Heap* heap_;

diff --git a/test/unittests/heap/cppgc/visitor-unittest.cc b/test/unittests/heap/cppgc/visitor-unittest.cc
@@ -202,7 +202,7 @@ class GCedWithCustomWeakCallback final
     WeakCallbackDispatcher::Call(broker, this);
   }
 
-  void Trace(cppgc::Visitor* visitor) {
+  void Trace(cppgc::Visitor* visitor) const {
     visitor->RegisterWeakCallbackMethod<
         GCedWithCustomWeakCallback,
         &GCedWithCustomWeakCallback::CustomWeakCallbackMethod>(this);