Revert "cppgc: Implement basic Member and Persistent checks"

This reverts commit 7458e67c.

Reason for revert: Crash on windows https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Win64%20-%20debug/37698/overview

Original change's description:
> cppgc: Implement basic Member and Persistent checks
>
> Adds check for
> - same heap on assignment
> - header and containment
>
> The verification state is eagerly created for on-heap Member
> references using caged heap and lazily created on first assignment for
> all others.
>
> Bug: chromium:1056170
> Change-Id: I38ee18eeb7ac489f69a46670cc5e5abe07f62dfa
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2878745
> Reviewed-by: Omer Katz <omerkatz@chromium.org>
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#74449}

Bug: chromium:1056170
Change-Id: I466522a7d879560c99dabbd96c3b097894743a87
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2880149
Auto-Submit: Zhi An Ng <zhin@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#74450}

include/cppgc/cross-thread-persistent.h

@@ -139,7 +139,7 @@ class BasicCrossThreadPersistent final : public PersistentBase,
     GetNode()->UpdateOwner(this);
     other.SetValue(nullptr);
     other.SetNode(nullptr);
-    this->CheckPointer(Get());
+    this->CheckPointer(GetValue());
     return *this;
   }
 

include/cppgc/internal/pointer-policies.h

@@ -9,9 +9,7 @@
 #include <type_traits>
 
 #include "cppgc/internal/write-barrier.h"
-#include "cppgc/sentinel-pointer.h"
 #include "cppgc/source-location.h"
-#include "cppgc/type-traits.h"
 #include "v8config.h"  // NOLINT(build/include_directory)
 
 namespace cppgc {
@@ -50,57 +48,24 @@ struct NoWriteBarrierPolicy {
   static void AssigningBarrier(const void*, const void*) {}
 };
 
-class V8_EXPORT EnabledCheckingPolicyBase {
+class V8_EXPORT EnabledCheckingPolicy {
  protected:
-  EnabledCheckingPolicyBase() = default;
-  explicit EnabledCheckingPolicyBase(void* state) : state_(state) {}
-
-  template <typename T>
-  void CheckPointer(const T* ptr) {
-    if (!ptr || (kSentinelPointer == ptr)) return;
-
-    CheckPointersImplTrampoline<T>::Call(this, ptr);
-  }
+  EnabledCheckingPolicy();
+  void CheckPointer(const void* ptr);
 
  private:
-  void CheckPointerImpl(const void* ptr, bool points_to_payload);
-
-  template <typename T, bool = IsCompleteV<T>>
-  struct CheckPointersImplTrampoline {
-    static void Call(EnabledCheckingPolicyBase* policy, const T* ptr) {
-      policy->CheckPointerImpl(ptr, false);
-    }
-  };
-
-  template <typename T>
-  struct CheckPointersImplTrampoline<T, true> {
-    static void Call(EnabledCheckingPolicyBase* policy, const T* ptr) {
-      policy->CheckPointerImpl(ptr, IsGarbageCollectedTypeV<T>);
-    }
-  };
-
-  void* state_ = nullptr;
-};
-
-class V8_EXPORT EnabledMemberCheckingPolicy : public EnabledCheckingPolicyBase {
- protected:
-  EnabledMemberCheckingPolicy();
+  void* impl_;
 };
 
-class V8_EXPORT EnabledPersistentCheckingPolicy
-    : public EnabledCheckingPolicyBase {};
-
 class DisabledCheckingPolicy {
  protected:
   void CheckPointer(const void* raw) {}
 };
 
 #if V8_ENABLE_CHECKS
-using DefaultMemberCheckingPolicy = EnabledMemberCheckingPolicy;
-using DefaultPersistentCheckingPolicy = EnabledPersistentCheckingPolicy;
+using DefaultCheckingPolicy = EnabledCheckingPolicy;
 #else
-using DefaultMemberCheckingPolicy = DisabledCheckingPolicy;
-using DefaultPersistentCheckingPolicy = DisabledCheckingPolicy;
+using DefaultCheckingPolicy = DisabledCheckingPolicy;
 #endif
 
 class KeepLocationPolicy {
@@ -168,10 +133,10 @@ template <typename T, typename WeaknessPolicy,
 class BasicCrossThreadPersistent;
 template <typename T, typename WeaknessPolicy,
           typename LocationPolicy = DefaultLocationPolicy,
-          typename CheckingPolicy = DefaultPersistentCheckingPolicy>
+          typename CheckingPolicy = DefaultCheckingPolicy>
 class BasicPersistent;
 template <typename T, typename WeaknessTag, typename WriteBarrierPolicy,
-          typename CheckingPolicy = DefaultMemberCheckingPolicy>
+          typename CheckingPolicy = DefaultCheckingPolicy>
 class BasicMember;
 
 }  // namespace internal

include/cppgc/type-traits.h

@@ -7,7 +7,6 @@
 
 // This file should stay with minimal dependencies to allow embedder to check
 // against Oilpan types without including any other parts.
-#include <cstddef>
 #include <type_traits>
 
 namespace cppgc {
@@ -165,18 +164,6 @@ struct IsUntracedMemberType : std::false_type {};
 template <typename T>
 struct IsUntracedMemberType<T, true> : std::true_type {};
 
-template <typename T>
-struct IsComplete {
- private:
-  template <typename U, size_t = sizeof(U)>
-  static std::true_type IsSizeOfKnown(U*);
-  static std::false_type IsSizeOfKnown(...);
-
- public:
-  static constexpr bool value =
-      decltype(IsSizeOfKnown(std::declval<T*>()))::value;
-};
-
 }  // namespace internal
 
 /**
@@ -236,12 +223,6 @@ constexpr bool IsWeakMemberTypeV = internal::IsWeakMemberType<T>::value;
 template <typename T>
 constexpr bool IsWeakV = internal::IsWeak<T>::value;
 
-/**
- * Value is true for types that are complete, and false otherwise.
- */
-template <typename T>
-constexpr bool IsCompleteV = internal::IsComplete<T>::value;
-
 }  // namespace cppgc
 
 #endif  // INCLUDE_CPPGC_TYPE_TRAITS_H_

src/heap/cppgc/pointer-policies.cc

@@ -4,85 +4,21 @@
 
 #include "include/cppgc/internal/pointer-policies.h"
 
-#include "include/cppgc/internal/caged-heap-local-data.h"
 #include "include/cppgc/internal/persistent-node.h"
-#include "src/base/logging.h"
 #include "src/base/macros.h"
-#include "src/base/platform/platform.h"
-#include "src/heap/cppgc/heap-object-header.h"
 #include "src/heap/cppgc/heap-page.h"
 #include "src/heap/cppgc/heap.h"
 
 namespace cppgc {
 namespace internal {
 
-namespace {
-
-// Gets the state (HeapBase) for on-heap slots.
-void* TryGetStateFromSlot(void* slot) {
-#ifdef CPPGC_CAGED_HEAP
-  if (v8::base::Stack::GetCurrentStackPosition() <= slot &&
-      slot < v8::base::Stack::GetStackStart())
-    return nullptr;
-
-  // `slot` may reside in a regular or large object. Get to the heap using the
-  // cage.
-  return reinterpret_cast<CagedHeapLocalData*>(
-             reinterpret_cast<uintptr_t>(slot) &
-             ~(api_constants::kCagedHeapReservationAlignment - 1))
-      ->heap_base;
-#else   // !CPPGC_CAGED_HEAP
-  return nullptr;
-#endif  // !CPPGC_CAGED_HEAP
+EnabledCheckingPolicy::EnabledCheckingPolicy() {
+  USE(impl_);
+  // TODO(chromium:1056170): Save creating heap state.
 }
 
-}  // namespace
-
-// We know that Member is only allowed on heap and on-stack in rare cases. Use
-// this information to eagerly populate a verification state already on policy
-// creation.
-EnabledMemberCheckingPolicy::EnabledMemberCheckingPolicy()
-    : EnabledCheckingPolicyBase(TryGetStateFromSlot(this)) {}
-
-void EnabledCheckingPolicyBase::CheckPointerImpl(const void* ptr,
-                                                 bool points_to_payload) {
-  auto* base_page = BasePage::FromPayload(ptr);
-  // Large objects do not support mixins. This also means that `base_page` is
-  // valid for large objects.
-  DCHECK_IMPLIES(base_page->is_large(), points_to_payload);
-
-  if (!state_) {
-    state_ = base_page->heap();
-    // Member references are used from within objects that cannot change their
-    // heap association which means that state is immutable once it is set.
-    //
-    // TODO(chromium:1056170): Binding state late allows for getting the initial
-    // state wrong which requires a check that `this` is contained in heap that
-    // is itself expensive. Investigate options on non-caged builds to improve
-    // coverage.
-  }
-
-  HeapBase* heap = static_cast<HeapBase*>(state_);
-  if (!heap) return;
-
-  // Member references should never mix heaps.
-  DCHECK_EQ(heap, base_page->heap());
-
-  // Header checks.
-  const HeapObjectHeader* header = nullptr;
-  if (points_to_payload) {
-    header = &HeapObjectHeader::FromPayload(ptr);
-  } else if (!heap->sweeper().IsSweepingInProgress()) {
-    // Mixin case.
-    header = &base_page->ObjectHeaderFromInnerAddress(ptr);
-    DCHECK_LE(header->Payload(), ptr);
-    DCHECK_GT(header->PayloadEnd(), ptr);
-  }
-  if (header) {
-    DCHECK(!header->IsFree());
-  }
-
-  // TODO(v8:11749): Check mark bits when during pre-finalizer phase.
+void EnabledCheckingPolicy::CheckPointer(const void* ptr) {
+  // TODO(chromium:1056170): Provide implementation.
 }
 
 PersistentRegion& StrongPersistentPolicy::GetPersistentRegion(

test/unittests/heap/cppgc/garbage-collected-unittest.cc

@@ -84,17 +84,6 @@ TEST(GarbageCollectedTest, GarbageCollectedWithMixinTrait) {
   STATIC_ASSERT(IsGarbageCollectedWithMixinTypeV<GCWithMergedMixins>);
 }
 
-namespace {
-
-class ForwardDeclaredType;
-
-}  // namespace
-
-TEST(GarbageCollectedTest, CompleteTypeTrait) {
-  STATIC_ASSERT(IsCompleteV<GCed>);
-  STATIC_ASSERT(!IsCompleteV<ForwardDeclaredType>);
-}
-
 TEST_F(GarbageCollectedTestWithHeap, GetObjectStartReturnsCurrentAddress) {
   GCed* gced = MakeGarbageCollected<GCed>(GetAllocationHandle());
   GCedWithMixin* gced_with_mixin =

test/unittests/heap/cppgc/member-unittest.cc

@@ -506,51 +506,5 @@ TEST_F(MemberHeapTest, ConstWeakRefIsClearedOnGC) {
   EXPECT_FALSE(persistent->weak_member());
 }
 
-#if V8_ENABLE_CHECKS
-
-namespace {
-class MemberHeapDeathTest : public testing::TestWithHeap {};
-
-class LinkedNode final : public GarbageCollected<LinkedNode> {
- public:
-  explicit LinkedNode(LinkedNode* next) : next_(next) {}
-  void Trace(Visitor* v) const { v->Trace(next_); }
-
-  void SetNext(LinkedNode* next) { next_ = next; }
-
- private:
-  Member<LinkedNode> next_;
-};
-
-}  // namespace
-
-TEST_F(MemberHeapDeathTest, AssignDifferentHeapValues) {
-  auto* o1 = MakeGarbageCollected<LinkedNode>(GetAllocationHandle(), nullptr);
-  auto* o2 = MakeGarbageCollected<LinkedNode>(GetAllocationHandle(), o1);
-  {
-    auto tmp_heap = cppgc::Heap::Create(platform_);
-    auto* o3 = MakeGarbageCollected<LinkedNode>(tmp_heap->GetAllocationHandle(),
-                                                nullptr);
-    EXPECT_DEATH_IF_SUPPORTED(o2->SetNext(o3), "");
-  }
-}
-
-#ifdef CPPGC_CAGED_HEAP
-
-TEST_F(MemberHeapDeathTest, VerificationStateDoesNotRequireValue) {
-  // For caged heap setups the verification state is constructed from Member
-  // itself and does not require an initial value.
-  auto* o1 = MakeGarbageCollected<LinkedNode>(GetAllocationHandle(), nullptr);
-  {
-    auto tmp_heap = cppgc::Heap::Create(platform_);
-    EXPECT_DEATH_IF_SUPPORTED(
-        MakeGarbageCollected<LinkedNode>(tmp_heap->GetAllocationHandle(), o1),
-        "");
-  }
-}
-
-#endif  // CPPGC_CAGED_HEAP
-#endif  // V8_ENABLE_CHECKS
-
 }  // namespace internal
 }  // namespace cppgc

test/unittests/heap/cppgc/persistent-family-unittest.cc

@@ -74,12 +74,12 @@ template <typename T>
 using LocalizedPersistent =
     internal::BasicPersistent<T, internal::StrongPersistentPolicy,
                               internal::KeepLocationPolicy,
-                              internal::DefaultPersistentCheckingPolicy>;
+                              internal::DefaultCheckingPolicy>;
 
 template <typename T>
 using LocalizedCrossThreadPersistent = internal::BasicCrossThreadPersistent<
     T, internal::StrongCrossThreadPersistentPolicy,
-    internal::KeepLocationPolicy, internal::DisabledCheckingPolicy>;
+    internal::KeepLocationPolicy, internal::DefaultCheckingPolicy>;
 
 class RootVisitor final : public VisitorBase {
  public: