From 19633c4e2c4096faefb589943d974dcf56b0b164 Mon Sep 17 00:00:00 2001 From: Tobias Tebbi Date: Wed, 23 Mar 2022 20:16:39 +0000 Subject: [PATCH] Revert "cppgc: Add regression test and check for object start bitmap" This reverts commit 164a040a2a06b854b19cb11fee4be6f81d1ce55f. Reason for revert: roll failure: https://ci.chromium.org/ui/p/chromium/builders/try/cast_shell_linux/1164753/overview Original change's description: > cppgc: Add regression test and check for object start bitmap > > Access to the object start bitmap is only safe during marking until > sweeping is started as the concurrent sweeper may clear and rebuild > the bitmap at any time during sweeping. > > Adds a DCHECK and an additional test for a previously broken > pre-finalizer scenario. > > Bug: chromium:1307471 > Change-Id: If67ade43f7cdad6de4720c0efeac11bfe8c22b3c > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3535782 > Reviewed-by: Nikolaos Papaspyrou > Reviewed-by: Omer Katz > Commit-Queue: Michael Lippautz > Cr-Commit-Position: refs/heads/main@{#79550} Bug: chromium:1307471 Change-Id: I181e63a34eae9369184fb86112bc64e53b8bfad5 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3545317 Owners-Override: Tobias Tebbi Reviewed-by: Michael Lippautz Commit-Queue: Tobias Tebbi Cr-Commit-Position: refs/heads/main@{#79590} --- src/heap/cppgc/heap-base.cc | 9 ---- src/heap/cppgc/heap-base.h | 9 ---- src/heap/cppgc/heap-page.cc | 3 +- src/heap/cppgc/heap-page.h | 18 +++---- src/heap/cppgc/trace-trait.cc | 2 - .../heap/cppgc/ephemeron-pair-unittest.cc | 2 - .../heap/cppgc/garbage-collected-unittest.cc | 1 - .../heap/cppgc/marking-visitor-unittest.cc | 6 +-- .../heap/cppgc/prefinalizer-unittest.cc | 51 ------------------- test/unittests/heap/cppgc/tests.cc | 15 ------ test/unittests/heap/cppgc/tests.h | 10 ---- test/unittests/heap/cppgc/visitor-unittest.cc | 18 +++---- 12 files changed, 17 insertions(+), 127 deletions(-) diff --git a/src/heap/cppgc/heap-base.cc b/src/heap/cppgc/heap-base.cc index 57549177fd..96c8f07336 100644 --- a/src/heap/cppgc/heap-base.cc +++ b/src/heap/cppgc/heap-base.cc @@ -195,15 +195,6 @@ void HeapBase::Terminate() { CHECK_EQ(0u, weak_cross_thread_persistent_region_.NodesInUse()); } -bool HeapBase::CanLookupObjectStartInBitmap() const { - // Object start bitmap access is only safe during marking and until the - // sweeper has been started. User code cannot distinguish between sweeping or - // not sweeping, which is why it's only valid to use the bitmap during - // marking. - return marker() || (in_atomic_pause() && !sweeper().IsSweepingInProgress()) || - allow_lookup_of_object_start_in_bitmap_; -} - HeapStatistics HeapBase::CollectStatistics( HeapStatistics::DetailLevel detail_level) { if (detail_level == HeapStatistics::DetailLevel::kBrief) { diff --git a/src/heap/cppgc/heap-base.h b/src/heap/cppgc/heap-base.h index c368ae21f0..0c6fe757f8 100644 --- a/src/heap/cppgc/heap-base.h +++ b/src/heap/cppgc/heap-base.h @@ -214,11 +214,6 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle { MarkingType marking_support() const { return marking_support_; } SweepingType sweeping_support() const { return sweeping_support_; } - bool CanLookupObjectStartInBitmap() const; - void set_allow_lookup_of_object_start_in_bitmap(bool value) { - allow_lookup_of_object_start_in_bitmap_ = value; - } - protected: // Used by the incremental scheduler to finalize a GC if supported. virtual void FinalizeIncrementalGarbageCollectionIfNeeded( @@ -289,10 +284,6 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle { int creation_thread_id_ = v8::base::OS::GetCurrentThreadId(); - // Used to explicitly allow lookups to the object start bitmap for testing - // code or internal code that otherwise verifies that access is safe. - bool allow_lookup_of_object_start_in_bitmap_ = false; - const MarkingType marking_support_; const SweepingType sweeping_support_; diff --git a/src/heap/cppgc/heap-page.cc b/src/heap/cppgc/heap-page.cc index 168faa806e..c7af4e971e 100644 --- a/src/heap/cppgc/heap-page.cc +++ b/src/heap/cppgc/heap-page.cc @@ -106,7 +106,8 @@ const HeapObjectHeader* BasePage::TryObjectHeaderFromInnerAddress( } // |address| is on the heap, so we FromInnerAddress can get the header. - const HeapObjectHeader* header = ObjectHeaderFromInnerAddressImpl(address); + const HeapObjectHeader* header = + ObjectHeaderFromInnerAddressImpl(this, address); if (header->IsFree()) return nullptr; DCHECK_NE(kFreeListGCInfoIndex, header->GetGCInfoIndex()); return header; diff --git a/src/heap/cppgc/heap-page.h b/src/heap/cppgc/heap-page.h index 1ea23ea709..5ee54f2aef 100644 --- a/src/heap/cppgc/heap-page.h +++ b/src/heap/cppgc/heap-page.h @@ -90,10 +90,6 @@ class V8_EXPORT_PRIVATE BasePage { BasePage(HeapBase&, BaseSpace&, PageType); private: - template - const HeapObjectHeader* ObjectHeaderFromInnerAddressImpl( - const void* address) const; - HeapBase& heap_; BaseSpace& space_; PageType type_; @@ -275,14 +271,14 @@ const BasePage* BasePage::FromPayload(const void* payload) { kGuardPageSize); } -template -const HeapObjectHeader* BasePage::ObjectHeaderFromInnerAddressImpl( - const void* address) const { - if (is_large()) { - return LargePage::From(this)->ObjectHeader(); +template +const HeapObjectHeader* ObjectHeaderFromInnerAddressImpl(const BasePage* page, + const void* address) { + if (page->is_large()) { + return LargePage::From(page)->ObjectHeader(); } const PlatformAwareObjectStartBitmap& bitmap = - NormalPage::From(this)->object_start_bitmap(); + NormalPage::From(page)->object_start_bitmap(); const HeapObjectHeader* header = bitmap.FindHeader(static_cast(address)); DCHECK_LT(address, reinterpret_cast(header) + @@ -307,7 +303,7 @@ const HeapObjectHeader& BasePage::ObjectHeaderFromInnerAddress( // reference to a mixin type SynchronizedLoad(); const HeapObjectHeader* header = - ObjectHeaderFromInnerAddressImpl(address); + ObjectHeaderFromInnerAddressImpl(this, address); DCHECK_NE(kFreeListGCInfoIndex, header->GetGCInfoIndex()); return *header; } diff --git a/src/heap/cppgc/trace-trait.cc b/src/heap/cppgc/trace-trait.cc index 9c5e377155..df14e3698b 100644 --- a/src/heap/cppgc/trace-trait.cc +++ b/src/heap/cppgc/trace-trait.cc @@ -5,7 +5,6 @@ #include "include/cppgc/trace-trait.h" #include "src/heap/cppgc/gc-info-table.h" -#include "src/heap/cppgc/heap-base.h" #include "src/heap/cppgc/heap-page.h" namespace cppgc { @@ -16,7 +15,6 @@ TraceDescriptor TraceTraitFromInnerAddressImpl::GetTraceDescriptor( // address is guaranteed to be on a normal page because this is used only for // mixins. const BasePage* page = BasePage::FromPayload(address); - DCHECK(page->heap().CanLookupObjectStartInBitmap()); page->SynchronizedLoad(); const HeapObjectHeader& header = page->ObjectHeaderFromInnerAddress(address); diff --git a/test/unittests/heap/cppgc/ephemeron-pair-unittest.cc b/test/unittests/heap/cppgc/ephemeron-pair-unittest.cc index ba35318da3..096de19f33 100644 --- a/test/unittests/heap/cppgc/ephemeron-pair-unittest.cc +++ b/test/unittests/heap/cppgc/ephemeron-pair-unittest.cc @@ -222,7 +222,6 @@ TEST_F(EphemeronPairTest, EphemeronPairWithMixinKey) { key, value); EXPECT_NE(static_cast(key), holder->ephemeron_pair().key.Get()); EXPECT_NE(static_cast(value), holder->ephemeron_pair().value.Get()); - testing::AllowLookupOfObjectStartInBitmap allow_access(*GetHeap()); InitializeMarker(*Heap::From(GetHeap()), GetPlatformHandle().get()); FinishSteps(); EXPECT_FALSE(HeapObjectHeader::FromObject(value).IsMarked()); @@ -239,7 +238,6 @@ TEST_F(EphemeronPairTest, EphemeronPairWithEmptyMixinValue) { key, nullptr); EXPECT_NE(static_cast(key), holder->ephemeron_pair().key.Get()); EXPECT_TRUE(HeapObjectHeader::FromObject(key).TryMarkAtomic()); - testing::AllowLookupOfObjectStartInBitmap allow_access(*GetHeap()); InitializeMarker(*Heap::From(GetHeap()), GetPlatformHandle().get()); FinishSteps(); FinishMarking(); diff --git a/test/unittests/heap/cppgc/garbage-collected-unittest.cc b/test/unittests/heap/cppgc/garbage-collected-unittest.cc index c0a1703ff1..0bfad3b2f0 100644 --- a/test/unittests/heap/cppgc/garbage-collected-unittest.cc +++ b/test/unittests/heap/cppgc/garbage-collected-unittest.cc @@ -99,7 +99,6 @@ TEST_F(GarbageCollectedTestWithHeap, GetObjectStartReturnsCurrentAddress) { GCed* gced = MakeGarbageCollected(GetAllocationHandle()); GCedWithMixin* gced_with_mixin = MakeGarbageCollected(GetAllocationHandle()); - testing::AllowLookupOfObjectStartInBitmap allow_access(*GetHeap()); const void* base_object_payload = TraceTrait::GetTraceDescriptor( static_cast(gced_with_mixin)) .base_object_payload; diff --git a/test/unittests/heap/cppgc/marking-visitor-unittest.cc b/test/unittests/heap/cppgc/marking-visitor-unittest.cc index 95b93f9855..7310134139 100644 --- a/test/unittests/heap/cppgc/marking-visitor-unittest.cc +++ b/test/unittests/heap/cppgc/marking-visitor-unittest.cc @@ -50,14 +50,10 @@ class TestMarkingVisitor : public MutatorMarkingVisitor { public: explicit TestMarkingVisitor(Marker* marker) : MutatorMarkingVisitor(marker->heap(), - marker->MutatorMarkingStateForTesting()), - allow_access_(marker->heap()) {} + marker->MutatorMarkingStateForTesting()) {} ~TestMarkingVisitor() { marking_state_.Publish(); } BasicMarkingState& marking_state() { return marking_state_; } - - private: - testing::AllowLookupOfObjectStartInBitmap allow_access_; }; } // namespace diff --git a/test/unittests/heap/cppgc/prefinalizer-unittest.cc b/test/unittests/heap/cppgc/prefinalizer-unittest.cc index e42b57d734..16dcb525e7 100644 --- a/test/unittests/heap/cppgc/prefinalizer-unittest.cc +++ b/test/unittests/heap/cppgc/prefinalizer-unittest.cc @@ -369,56 +369,5 @@ TEST_F(PrefinalizerTest, VirtualPrefinalizer) { EXPECT_LT(0u, GCedInherited::prefinalizer_count_); } -namespace { - -class MixinCallingGC : public GarbageCollectedMixin { - public: - template - explicit MixinCallingGC(GCFunction gc) { - gc(); - } - - void Trace(Visitor*) const override {} -}; - -class MixinWithPrefinalizer : public GarbageCollectedMixin { - CPPGC_USING_PRE_FINALIZER(MixinWithPrefinalizer, PreFinalize); - - public: - MixinWithPrefinalizer() = default; - void PreFinalize() {} - - void Trace(Visitor*) const override {} -}; - -class BaseWithMixins : public GarbageCollected, - public MixinCallingGC, - public MixinWithPrefinalizer { - public: - template - explicit BaseWithMixins(GCFunction gc) : MixinCallingGC(gc) {} - - void Trace(Visitor* v) const override { - MixinCallingGC::Trace(v); - MixinWithPrefinalizer::Trace(v); - } -}; - -} // namespace - -TEST_F(PrefinalizerTest, GCBeforePrefinalizerRegistration) { - // Regression test: https://crbug.com/1307471 - MakeGarbageCollected(GetAllocationHandle(), [this]() { - internal::Heap::From(GetHeap())->CollectGarbage( - {internal::GarbageCollector::Config::CollectionType::kMajor, - cppgc::Heap::StackState::kMayContainHeapPointers, - cppgc::Heap::MarkingType::kAtomic, - cppgc::Heap::SweepingType::kIncrementalAndConcurrent, - internal::GarbageCollector::Config::FreeMemoryHandling:: - kDiscardWherePossible, - internal::GarbageCollector::Config::IsForcedGC::kForced}); - }); -} - } // namespace internal } // namespace cppgc diff --git a/test/unittests/heap/cppgc/tests.cc b/test/unittests/heap/cppgc/tests.cc index b0c01d8702..b2bed85f1d 100644 --- a/test/unittests/heap/cppgc/tests.cc +++ b/test/unittests/heap/cppgc/tests.cc @@ -38,21 +38,6 @@ void TestWithHeap::ResetLinearAllocationBuffers() { TestSupportingAllocationOnly::TestSupportingAllocationOnly() : no_gc_scope_(GetHeap()->GetHeapHandle()) {} -AllowLookupOfObjectStartInBitmap::AllowLookupOfObjectStartInBitmap( - cppgc::Heap& heap) - : AllowLookupOfObjectStartInBitmap( - *static_cast(Heap::From(&heap))) {} - -AllowLookupOfObjectStartInBitmap::AllowLookupOfObjectStartInBitmap( - HeapBase& heap) - : heap_(heap) { - heap_.set_allow_lookup_of_object_start_in_bitmap(true); -} - -AllowLookupOfObjectStartInBitmap::~AllowLookupOfObjectStartInBitmap() { - heap_.set_allow_lookup_of_object_start_in_bitmap(false); -} - } // namespace testing } // namespace internal } // namespace cppgc diff --git a/test/unittests/heap/cppgc/tests.h b/test/unittests/heap/cppgc/tests.h index f2ea6ebab0..7fe91397fc 100644 --- a/test/unittests/heap/cppgc/tests.h +++ b/test/unittests/heap/cppgc/tests.h @@ -125,16 +125,6 @@ class TestSupportingAllocationOnly : public TestWithHeap { subtle::NoGarbageCollectionScope no_gc_scope_; }; -class AllowLookupOfObjectStartInBitmap final { - public: - explicit AllowLookupOfObjectStartInBitmap(cppgc::Heap&); - explicit AllowLookupOfObjectStartInBitmap(HeapBase&); - ~AllowLookupOfObjectStartInBitmap(); - - private: - HeapBase& heap_; -}; - } // namespace testing } // namespace internal } // namespace cppgc diff --git a/test/unittests/heap/cppgc/visitor-unittest.cc b/test/unittests/heap/cppgc/visitor-unittest.cc index 39b09ad066..9f716df314 100644 --- a/test/unittests/heap/cppgc/visitor-unittest.cc +++ b/test/unittests/heap/cppgc/visitor-unittest.cc @@ -68,7 +68,6 @@ TEST_F(TraceTraitTest, GetObjectStartGCedMixin) { auto* gced_mixin_app = MakeGarbageCollected(GetAllocationHandle()); auto* gced_mixin = static_cast(gced_mixin_app); - testing::AllowLookupOfObjectStartInBitmap allow_access(*GetHeap()); EXPECT_EQ(gced_mixin_app, TraceTrait::GetTraceDescriptor(gced_mixin) .base_object_payload); @@ -103,7 +102,6 @@ TEST_F(TraceTraitTest, TraceGCedMixinThroughTraceDescriptor) { MakeGarbageCollected(GetAllocationHandle()); auto* gced_mixin = static_cast(gced_mixin_app); EXPECT_EQ(0u, GCed::trace_callcount); - testing::AllowLookupOfObjectStartInBitmap allow_access(*GetHeap()); TraceDescriptor desc = TraceTrait::GetTraceDescriptor(gced_mixin); desc.callback(nullptr, desc.base_object_payload); EXPECT_EQ(1u, GCed::trace_callcount); @@ -121,7 +119,6 @@ TEST_F(TraceTraitTest, MixinInstanceWithoutTrace) { auto* mixin_without_trace = MakeGarbageCollected(GetAllocationHandle()); auto* mixin = static_cast(mixin_without_trace); - testing::AllowLookupOfObjectStartInBitmap allow_access(*GetHeap()); EXPECT_EQ(0u, GCedMixin::trace_callcount); TraceDescriptor mixin_without_trace_desc = TraceTrait::GetTraceDescriptor( @@ -141,8 +138,8 @@ namespace { class DispatchingVisitor final : public VisitorBase { public: - DispatchingVisitor(cppgc::Heap& heap, const void* object, const void* payload) - : allow_access_(heap), object_(object), payload_(payload) {} + DispatchingVisitor(const void* object, const void* payload) + : object_(object), payload_(payload) {} protected: void Visit(const void* t, TraceDescriptor desc) final { @@ -160,7 +157,6 @@ class DispatchingVisitor final : public VisitorBase { } private: - testing::AllowLookupOfObjectStartInBitmap allow_access_; const void* object_; const void* payload_; }; @@ -169,7 +165,7 @@ class DispatchingVisitor final : public VisitorBase { TEST_F(VisitorTest, DispatchTraceGCed) { Member ref = MakeGarbageCollected(GetAllocationHandle()); - DispatchingVisitor visitor(*GetHeap(), ref, ref); + DispatchingVisitor visitor(ref, ref); EXPECT_EQ(0u, GCed::trace_callcount); visitor.Trace(ref); EXPECT_EQ(1u, GCed::trace_callcount); @@ -182,7 +178,7 @@ TEST_F(VisitorTest, DispatchTraceGCedMixin) { // Ensure that we indeed test dispatching an inner object. EXPECT_NE(static_cast(gced_mixin_app), static_cast(gced_mixin)); Member ref = gced_mixin; - DispatchingVisitor visitor(*GetHeap(), gced_mixin, gced_mixin_app); + DispatchingVisitor visitor(gced_mixin, gced_mixin_app); EXPECT_EQ(0u, GCed::trace_callcount); visitor.Trace(ref); EXPECT_EQ(1u, GCed::trace_callcount); @@ -190,7 +186,7 @@ TEST_F(VisitorTest, DispatchTraceGCedMixin) { TEST_F(VisitorTest, DispatchTraceWeakGCed) { WeakMember ref = MakeGarbageCollected(GetAllocationHandle()); - DispatchingVisitor visitor(*GetHeap(), ref, ref); + DispatchingVisitor visitor(ref, ref); visitor.Trace(ref); // No marking, so reference should be cleared. EXPECT_EQ(nullptr, ref.Get()); @@ -203,7 +199,7 @@ TEST_F(VisitorTest, DispatchTraceWeakGCedMixin) { // Ensure that we indeed test dispatching an inner object. EXPECT_NE(static_cast(gced_mixin_app), static_cast(gced_mixin)); WeakMember ref = gced_mixin; - DispatchingVisitor visitor(*GetHeap(), gced_mixin, gced_mixin_app); + DispatchingVisitor visitor(gced_mixin, gced_mixin_app); visitor.Trace(ref); // No marking, so reference should be cleared. EXPECT_EQ(nullptr, ref.Get()); @@ -294,7 +290,7 @@ class GCedWithComposite final : public GarbageCollected { TEST_F(VisitorTest, DispatchToCompositeObject) { Member ref = MakeGarbageCollected(GetAllocationHandle()); - DispatchingVisitor visitor(*GetHeap(), ref, ref); + DispatchingVisitor visitor(ref, ref); EXPECT_EQ(0u, Composite::callback_callcount); visitor.Trace(ref); EXPECT_EQ(1u, Composite::callback_callcount);