From 2c377490818fa17716aa1bf1a9cb56cad1666a0b Mon Sep 17 00:00:00 2001 From: Michael Lippautz Date: Wed, 3 Aug 2022 17:27:49 +0200 Subject: [PATCH] [heap] Conservatively scan for TracedNode GlobalHandle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v8::TracedReference is supposed to be used from objects allocated on CppHeap. Such objects can be in construction during garbage collection, meaning that they are unable to invoke Trace(v8::TraceReference) as they have not been properly set up. It is thus necessary to use conservative tracing to find v8::TracedReference (backed by TracedNode in GlobalHandle) in in-construction objects. Change-Id: I5b4ac6e7805ff7ded33f63a405db65ea08d809ad Bug: v8:13141, chromium:1322114 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3806439 Commit-Queue: Michael Lippautz Reviewed-by: Dominik Inführ Reviewed-by: Anton Bikineev Cr-Commit-Position: refs/heads/main@{#82188} --- src/handles/global-handles.cc | 44 ++++++++++++- src/handles/global-handles.h | 5 ++ src/heap/cppgc-js/cpp-heap.cc | 63 +++++++++++++++++-- .../cppgc-js/unified-heap-marking-state-inl.h | 8 ++- .../cppgc-js/unified-heap-marking-state.h | 1 + src/heap/cppgc/visitor.h | 9 ++- .../heap/cppgc-js/unified-heap-unittest.cc | 40 ++++++++++++ 7 files changed, 157 insertions(+), 13 deletions(-) diff --git a/src/handles/global-handles.cc b/src/handles/global-handles.cc index ae9e70b3a8..5675657f65 100644 --- a/src/handles/global-handles.cc +++ b/src/handles/global-handles.cc @@ -6,6 +6,7 @@ #include #include +#include #include #include @@ -86,6 +87,9 @@ class GlobalHandles::NodeBlock final { NodeBlock* next() const { return next_; } NodeBlock* next_used() const { return next_used_; } + const void* begin_address() const { return nodes_; } + const void* end_address() const { return &nodes_[kBlockSize]; } + private: NodeType nodes_[kBlockSize]; NodeBlock* const next_; @@ -192,6 +196,7 @@ class GlobalHandles::NodeSpace final { public: using BlockType = NodeBlock; using iterator = NodeIterator; + using NodeBounds = GlobalHandles::NodeBounds; static NodeSpace* From(NodeType* node); static void Release(NodeType* node); @@ -208,6 +213,8 @@ class GlobalHandles::NodeSpace final { size_t TotalSize() const { return blocks_ * sizeof(NodeType) * kBlockSize; } size_t handles_count() const { return handles_count_; } + NodeBounds GetNodeBlockBounds() const; + private: void PutNodesOnFreeList(BlockType* block); V8_INLINE void Free(NodeType* node); @@ -250,6 +257,21 @@ NodeType* GlobalHandles::NodeSpace::Allocate() { return node; } +template +typename GlobalHandles::NodeSpace::NodeBounds +GlobalHandles::NodeSpace::GetNodeBlockBounds() const { + NodeBounds block_bounds; + for (BlockType* current = first_used_block_; current; + current = current->next_used()) { + block_bounds.push_back({current->begin_address(), current->end_address()}); + } + std::sort(block_bounds.begin(), block_bounds.end(), + [](const auto& pair1, const auto& pair2) { + return pair1.first < pair2.first; + }); + return block_bounds; +} + template void GlobalHandles::NodeSpace::PutNodesOnFreeList(BlockType* block) { for (int32_t i = kBlockSize - 1; i >= 0; --i) { @@ -1122,12 +1144,27 @@ GlobalHandles* GlobalHandles::From(const TracedNode* node) { : NodeBlock::From(node)->global_handles(); } +// static void GlobalHandles::MarkTraced(Address* location) { TracedNode* node = TracedNode::FromLocation(location); DCHECK(node->IsInUse()); node->set_markbit(); } +// static +Object GlobalHandles::MarkTracedConservatively( + Address* inner_location, Address* traced_node_block_base) { + // Compute the `TracedNode` address based on its inner pointer. + const ptrdiff_t delta = reinterpret_cast(inner_location) - + reinterpret_cast(traced_node_block_base); + const auto index = delta / sizeof(TracedNode); + TracedNode& node = + reinterpret_cast(traced_node_block_base)[index]; + if (!node.IsInUse()) return Smi::zero(); + node.set_markbit(); + return node.object(); +} + void GlobalHandles::Destroy(Address* location) { if (location != nullptr) { NodeSpace::Release(Node::FromLocation(location)); @@ -1542,8 +1579,11 @@ void GlobalHandles::IterateAllRootsForTesting( } } -DISABLE_CFI_PERF -void GlobalHandles::IterateTracedNodes( +GlobalHandles::NodeBounds GlobalHandles::GetTracedNodeBounds() const { + return traced_nodes_->GetNodeBlockBounds(); +} + +DISABLE_CFI_PERF void GlobalHandles::IterateTracedNodes( v8::EmbedderHeapTracer::TracedGlobalHandleVisitor* visitor) { for (TracedNode* node : *traced_nodes_) { if (node->IsInUse()) { diff --git a/src/handles/global-handles.h b/src/handles/global-handles.h index 66310ae50f..7016c45b9f 100644 --- a/src/handles/global-handles.h +++ b/src/handles/global-handles.h @@ -79,6 +79,8 @@ class V8_EXPORT_PRIVATE GlobalHandles final { static void CopyTracedReference(const Address* const* from, Address** to); static void DestroyTracedReference(Address* location); static void MarkTraced(Address* location); + static Object MarkTracedConservatively(Address* inner_location, + Address* traced_node_block_base); V8_INLINE static Object Acquire(Address* location); @@ -166,6 +168,9 @@ class V8_EXPORT_PRIVATE GlobalHandles final { void IterateAllRootsForTesting(v8::PersistentHandleVisitor* v); + using NodeBounds = std::vector>; + NodeBounds GetTracedNodeBounds() const; + #ifdef DEBUG void PrintStats(); void Print(); diff --git a/src/heap/cppgc-js/cpp-heap.cc b/src/heap/cppgc-js/cpp-heap.cc index 2dba6e6405..c5632165c9 100644 --- a/src/heap/cppgc-js/cpp-heap.cc +++ b/src/heap/cppgc-js/cpp-heap.cc @@ -24,6 +24,7 @@ #include "src/heap/base/stack.h" #include "src/heap/cppgc-js/cpp-marking-state.h" #include "src/heap/cppgc-js/cpp-snapshot.h" +#include "src/heap/cppgc-js/unified-heap-marking-state-inl.h" #include "src/heap/cppgc-js/unified-heap-marking-state.h" #include "src/heap/cppgc-js/unified-heap-marking-verifier.h" #include "src/heap/cppgc-js/unified-heap-marking-visitor.h" @@ -41,6 +42,7 @@ #include "src/heap/cppgc/stats-collector.h" #include "src/heap/cppgc/sweeper.h" #include "src/heap/cppgc/unmarker.h" +#include "src/heap/cppgc/visitor.h" #include "src/heap/embedder-tracing-inl.h" #include "src/heap/embedder-tracing.h" #include "src/heap/gc-tracer.h" @@ -245,6 +247,49 @@ void GlobalFatalOutOfMemoryHandlerImpl(const std::string& reason, V8::FatalProcessOutOfMemory(nullptr, reason.c_str()); } +class UnifiedHeapConservativeMarkingVisitor final + : public cppgc::internal::ConservativeMarkingVisitor { + public: + UnifiedHeapConservativeMarkingVisitor( + HeapBase& heap, MutatorMarkingState& mutator_marking_state, + cppgc::Visitor& visitor, UnifiedHeapMarkingState& marking_state) + : ConservativeMarkingVisitor(heap, mutator_marking_state, visitor), + marking_state_(marking_state) {} + ~UnifiedHeapConservativeMarkingVisitor() override = default; + + void SetTracedNodeBounds(GlobalHandles::NodeBounds traced_node_bounds) { + traced_node_bounds_ = std::move(traced_node_bounds); + } + + void TraceConservativelyIfNeeded(const void* address) override { + ConservativeMarkingVisitor::TraceConservativelyIfNeeded(address); + TraceTracedNodesConservatively(address); + } + + private: + void TraceTracedNodesConservatively(const void* address) { + const auto upper_it = + std::upper_bound(traced_node_bounds_.begin(), traced_node_bounds_.end(), + address, [](const void* needle, const auto& pair) { + return needle < pair.first; + }); + // Also checks emptiness as begin() == end() on empty maps. + if (upper_it == traced_node_bounds_.begin()) return; + + const auto bounds = std::next(upper_it, -1); + if (address < bounds->second) { + auto object = GlobalHandles::MarkTracedConservatively( + const_cast(reinterpret_cast(address)), + const_cast( + reinterpret_cast(bounds->first))); + marking_state_.MarkAndPush(object); + } + } + + GlobalHandles::NodeBounds traced_node_bounds_; + UnifiedHeapMarkingState& marking_state_; +}; + } // namespace class UnifiedHeapMarker final : public cppgc::internal::MarkerBase { @@ -269,11 +314,12 @@ class UnifiedHeapMarker final : public cppgc::internal::MarkerBase { return mutator_unified_heap_marking_state_; } - protected: - cppgc::Visitor& visitor() final { return *marking_visitor_; } - cppgc::internal::ConservativeTracingVisitor& conservative_visitor() final { + UnifiedHeapConservativeMarkingVisitor& conservative_visitor() final { return conservative_marking_visitor_; } + + protected: + cppgc::Visitor& visitor() final { return *marking_visitor_; } ::heap::base::StackVisitor& stack_visitor() final { return conservative_marking_visitor_; } @@ -281,7 +327,7 @@ class UnifiedHeapMarker final : public cppgc::internal::MarkerBase { private: UnifiedHeapMarkingState mutator_unified_heap_marking_state_; std::unique_ptr marking_visitor_; - cppgc::internal::ConservativeMarkingVisitor conservative_marking_visitor_; + UnifiedHeapConservativeMarkingVisitor conservative_marking_visitor_; }; UnifiedHeapMarker::UnifiedHeapMarker(Heap* v8_heap, @@ -298,7 +344,8 @@ UnifiedHeapMarker::UnifiedHeapMarker(Heap* v8_heap, heap, mutator_marking_state_, mutator_unified_heap_marking_state_)), conservative_marking_visitor_(heap, mutator_marking_state_, - *marking_visitor_) { + *marking_visitor_, + mutator_unified_heap_marking_state_) { concurrent_marker_ = std::make_unique( heap_, v8_heap, marking_worklists_, schedule_, platform_, mutator_unified_heap_marking_state_, config.collection_type); @@ -650,6 +697,12 @@ bool CppHeap::IsTracingDone() { return marking_done_; } void CppHeap::EnterFinalPause(cppgc::EmbedderStackState stack_state) { CHECK(!in_disallow_gc_scope()); in_atomic_pause_ = true; + auto* marker = static_cast(marker_.get()); + // Scan global handles conservatively in case we are attached to an Isolate. + if (isolate_) { + marker->conservative_visitor().SetTracedNodeBounds( + isolate()->global_handles()->GetTracedNodeBounds()); + } marker_->EnterAtomicPause(stack_state); if (isolate_ && *collection_type_ == CollectionType::kMinor) { // Visit V8 -> cppgc references. diff --git a/src/heap/cppgc-js/unified-heap-marking-state-inl.h b/src/heap/cppgc-js/unified-heap-marking-state-inl.h index 97ba6971f5..20524a32c5 100644 --- a/src/heap/cppgc-js/unified-heap-marking-state-inl.h +++ b/src/heap/cppgc-js/unified-heap-marking-state-inl.h @@ -43,9 +43,15 @@ void UnifiedHeapMarkingState::MarkAndPush( // non-empty `TracedReferenceBase` when `CppHeap` is in detached mode. Object object = BasicTracedReferenceExtractor::GetObjectForMarking(reference); + MarkAndPush(object); +} + +void UnifiedHeapMarkingState::MarkAndPush(Object object) { if (!object.IsHeapObject()) { // The embedder is not aware of whether numbers are materialized as heap - // objects are just passed around as Smis. + // objects are just passed around as Smis. This branch also filters out + // intentionally passed `Smi::zero()` that indicate that there's no object + // to mark. return; } HeapObject heap_object = HeapObject::cast(object); diff --git a/src/heap/cppgc-js/unified-heap-marking-state.h b/src/heap/cppgc-js/unified-heap-marking-state.h index af1320721b..08bf9ec3dd 100644 --- a/src/heap/cppgc-js/unified-heap-marking-state.h +++ b/src/heap/cppgc-js/unified-heap-marking-state.h @@ -25,6 +25,7 @@ class UnifiedHeapMarkingState final { void Update(MarkingWorklists::Local*); V8_INLINE void MarkAndPush(const TracedReferenceBase&); + V8_INLINE void MarkAndPush(v8::internal::Object); private: Heap* const heap_; diff --git a/src/heap/cppgc/visitor.h b/src/heap/cppgc/visitor.h index b5ceb5a78f..3412061ec6 100644 --- a/src/heap/cppgc/visitor.h +++ b/src/heap/cppgc/visitor.h @@ -46,7 +46,7 @@ class RootVisitorBase : public RootVisitor { }; // Regular visitor that additionally allows for conservative tracing. -class ConservativeTracingVisitor { +class V8_EXPORT_PRIVATE ConservativeTracingVisitor { public: ConservativeTracingVisitor(HeapBase&, PageBackend&, cppgc::Visitor&); virtual ~ConservativeTracingVisitor() = default; @@ -55,18 +55,17 @@ class ConservativeTracingVisitor { ConservativeTracingVisitor& operator=(const ConservativeTracingVisitor&) = delete; - void TraceConservativelyIfNeeded(const void*); + virtual void TraceConservativelyIfNeeded(const void*); void TraceConservativelyIfNeeded(HeapObjectHeader&); protected: using TraceConservativelyCallback = void(ConservativeTracingVisitor*, const HeapObjectHeader&); - virtual void V8_EXPORT_PRIVATE - VisitFullyConstructedConservatively(HeapObjectHeader&); + virtual void VisitFullyConstructedConservatively(HeapObjectHeader&); virtual void VisitInConstructionConservatively(HeapObjectHeader&, TraceConservativelyCallback) {} - void V8_EXPORT_PRIVATE TryTracePointerConservatively(Address address); + void TryTracePointerConservatively(Address address); HeapBase& heap_; PageBackend& page_backend_; diff --git a/test/unittests/heap/cppgc-js/unified-heap-unittest.cc b/test/unittests/heap/cppgc-js/unified-heap-unittest.cc index 080e32c20e..55933a7553 100644 --- a/test/unittests/heap/cppgc-js/unified-heap-unittest.cc +++ b/test/unittests/heap/cppgc-js/unified-heap-unittest.cc @@ -20,6 +20,7 @@ #include "include/v8-traced-handle.h" #include "src/api/api-inl.h" #include "src/base/platform/time.h" +#include "src/common/globals.h" #include "src/heap/cppgc-js/cpp-heap.h" #include "src/heap/cppgc/heap-object-header.h" #include "src/heap/cppgc/sweeper.h" @@ -349,5 +350,44 @@ TEST_F(UnifiedHeapWithCustomSpaceTest, CollectCustomSpaceStatisticsAtLastGC) { EXPECT_EQ(4u, StatisticsReceiver::num_calls_); } +namespace { + +class InConstructionObjectReferringToGlobalHandle final + : public cppgc::GarbageCollected< + InConstructionObjectReferringToGlobalHandle> { + public: + InConstructionObjectReferringToGlobalHandle(Heap* heap, + v8::Local wrapper) + : wrapper_(reinterpret_cast(heap->isolate()), wrapper) { + heap->CollectGarbage(OLD_SPACE, GarbageCollectionReason::kTesting); + heap->CollectGarbage(OLD_SPACE, GarbageCollectionReason::kTesting); + } + + void Trace(cppgc::Visitor* visitor) const { visitor->Trace(wrapper_); } + + TracedReference& GetWrapper() { return wrapper_; } + + private: + TracedReference wrapper_; +}; + +} // namespace + +TEST_F(UnifiedHeapTest, InConstructionObjectReferringToGlobalHandle) { + v8::HandleScope handle_scope(v8_isolate()); + v8::Local context = v8::Context::New(v8_isolate()); + v8::Context::Scope context_scope(context); + { + v8::HandleScope inner_handle_scope(v8_isolate()); + auto local = v8::Object::New(v8_isolate()); + auto* cpp_obj = cppgc::MakeGarbageCollected< + InConstructionObjectReferringToGlobalHandle>( + allocation_handle(), + reinterpret_cast(v8_isolate())->heap(), local); + CHECK_NE(kGlobalHandleZapValue, + *reinterpret_cast(*cpp_obj->GetWrapper())); + } +} + } // namespace internal } // namespace v8