From e80db133e31b7a4ebd502d5326e0d7ef01375e35 Mon Sep 17 00:00:00 2001 From: Omer Katz Date: Thu, 14 May 2020 01:20:23 +0200 Subject: [PATCH] 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 Reviewed-by: Anton Bikineev Reviewed-by: Michael Lippautz Cr-Commit-Position: refs/heads/master@{#67788} --- include/cppgc/internal/gc-info.h | 6 ++++-- src/heap/cppgc/gc-info-table.cc | 5 +++++ src/heap/cppgc/gc-info-table.h | 3 +++ src/heap/cppgc/gc-info.cc | 5 +++-- src/heap/cppgc/heap-page.cc | 3 +-- .../heap/cppgc/custom-spaces-unittest.cc | 12 +++++++++-- .../heap/cppgc/garbage-collected-unittest.cc | 5 ++++- test/unittests/heap/cppgc/gc-info-unittest.cc | 20 ++++++++++++------- test/unittests/heap/cppgc/heap-unittest.cc | 4 +++- test/unittests/heap/cppgc/sweeper-unittest.cc | 1 + test/unittests/heap/cppgc/visitor-unittest.cc | 2 +- 11 files changed, 48 insertions(+), 18 deletions(-) diff --git a/include/cppgc/internal/gc-info.h b/include/cppgc/internal/gc-info.h index 9aac136..3d361e6 100644 --- a/include/cppgc/internal/gc-info.h +++ b/include/cppgc/internal/gc-info.h @@ -8,6 +8,7 @@ #include #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::kCallback, std::is_polymorphic::value); + FinalizerTrait::kCallback, TraceTrait::Trace, + std::is_polymorphic::value); return registered_index.GetIndex(); } }; diff --git a/src/heap/cppgc/gc-info-table.cc b/src/heap/cppgc/gc-info-table.cc index dda5f0a..8f2ee96 100644 --- 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 index 25141f5..749f30b 100644 --- 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 index 007eab3..7097013 100644 --- 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 index e8afbaf..a553f8c 100644 --- 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(header) + header->GetSize()); - DCHECK_NE(kFreeListGCInfoIndex, - header->GetGCInfoIndex()); + 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 index 3fb0b13..432c89e 100644 --- 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 heap_; }; -class RegularGCed final : public GarbageCollected {}; +class RegularGCed final : public GarbageCollected { + public: + void Trace(Visitor*) const {} +}; class CustomGCed1 final : public GarbageCollected { public: ~CustomGCed1() { g_destructor_callcount++; } + void Trace(Visitor*) const {} }; class CustomGCed2 final : public GarbageCollected { public: ~CustomGCed2() { g_destructor_callcount++; } + void Trace(Visitor*) const {} }; -class CustomGCedBase : public GarbageCollected {}; +class CustomGCedBase : public GarbageCollected { + 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 index aadd3aa..d736911 100644 --- 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 {}; +class GCed : public GarbageCollected { + public: + void Trace(Visitor*) const {} +}; class NotGCed {}; class Mixin : public GarbageCollectedMixin {}; class GCedWithMixin : public GarbageCollected, public Mixin { diff --git a/test/unittests/heap/cppgc/gc-info-unittest.cc b/test/unittests/heap/cppgc/gc-info-unittest.cc index 199b42d..b45de5c 100644 --- 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 index ca8e225..160445e 100644 --- a/test/unittests/heap/cppgc/heap-unittest.cc +++ b/test/unittests/heap/cppgc/heap-unittest.cc @@ -36,6 +36,8 @@ class Foo : public GarbageCollected { Foo() { destructor_callcount = 0; } ~Foo() { destructor_callcount++; } + + void Trace(cppgc::Visitor*) const {} }; size_t Foo::destructor_callcount; @@ -43,7 +45,7 @@ size_t Foo::destructor_callcount; template class GCed : public GarbageCollected { 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 index 4a5232b..8cd4257 100644 --- 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 { // 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 index d4eb4b1..d6b93dc 100644 --- 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);