Reland "cppgc: Implement basic Member and Persistent checks"

This is a reland of 7458e67c

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: I9cecfcf7ba2cb70650fd51f345fbf740b96ff6ba
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2882804Reviewed-by: Omer Katz <omerkatz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74468}

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(GetValue());
+    this->CheckPointer(Get());
     return *this;
   }
 

include/cppgc/internal/pointer-policies.h

@@ -9,7 +9,9 @@
 #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 {
@@ -48,24 +50,57 @@ struct NoWriteBarrierPolicy {
   static void AssigningBarrier(const void*, const void*) {}
 };
 
-class V8_EXPORT EnabledCheckingPolicy {
+class V8_EXPORT EnabledCheckingPolicyBase {
  protected:
-  EnabledCheckingPolicy();
-  void CheckPointer(const void* ptr);
+  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);
+  }
 
  private:
-  void* impl_;
+  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();
 };
 
+class V8_EXPORT EnabledPersistentCheckingPolicy
+    : public EnabledCheckingPolicyBase {};
+
 class DisabledCheckingPolicy {
  protected:
   void CheckPointer(const void* raw) {}
 };
 
 #if V8_ENABLE_CHECKS
-using DefaultCheckingPolicy = EnabledCheckingPolicy;
+using DefaultMemberCheckingPolicy = EnabledMemberCheckingPolicy;
+using DefaultPersistentCheckingPolicy = EnabledPersistentCheckingPolicy;
 #else
-using DefaultCheckingPolicy = DisabledCheckingPolicy;
+using DefaultMemberCheckingPolicy = DisabledCheckingPolicy;
+using DefaultPersistentCheckingPolicy = DisabledCheckingPolicy;
 #endif
 
 class KeepLocationPolicy {
@@ -133,10 +168,10 @@ template <typename T, typename WeaknessPolicy,
 class BasicCrossThreadPersistent;
 template <typename T, typename WeaknessPolicy,
           typename LocationPolicy = DefaultLocationPolicy,
-          typename CheckingPolicy = DefaultCheckingPolicy>
+          typename CheckingPolicy = DefaultPersistentCheckingPolicy>
 class BasicPersistent;
 template <typename T, typename WeaknessTag, typename WriteBarrierPolicy,
-          typename CheckingPolicy = DefaultCheckingPolicy>
+          typename CheckingPolicy = DefaultMemberCheckingPolicy>
 class BasicMember;
 
 }  // namespace internal

include/cppgc/type-traits.h

@@ -7,6 +7,7 @@
 
 // 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 {
@@ -164,6 +165,18 @@ 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
 
 /**
@@ -223,6 +236,12 @@ 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,21 +4,92 @@
 
 #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 {
 
-EnabledCheckingPolicy::EnabledCheckingPolicy() {
-  USE(impl_);
-  // TODO(chromium:1056170): Save creating heap state.
+namespace {
+
+#if defined(CPPGC_CAGED_HEAP) || defined(DEBUG)
+bool IsOnStack(const void* address) {
+  return v8::base::Stack::GetCurrentStackPosition() <= address &&
+         address < v8::base::Stack::GetStackStart();
+}
+#endif  // defined(CPPGC_CAGED_HEAP) || defined(DEBUG)
+
+// Gets the state (HeapBase) for on-heap slots.
+void* TryGetStateFromSlot(void* slot) {
+#ifdef CPPGC_CAGED_HEAP
+  if (IsOnStack(slot)) 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
 }
 
-void EnabledCheckingPolicy::CheckPointer(const void* ptr) {
-  // TODO(chromium:1056170): Provide implementation.
+}  // 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) {
+  // `ptr` must not reside on stack.
+  DCHECK(!IsOnStack(ptr));
+  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.
 }
 
 PersistentRegion& StrongPersistentPolicy::GetPersistentRegion(

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

@@ -84,6 +84,17 @@ 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

@@ -122,10 +122,10 @@ void AtomicCtorTest(cppgc::Heap* heap) {
     EXPECT_EQ(gced, member.Get());
   }
   {
-    GCed gced;
-    MemberType<GCed> member(gced,
+    GCed* gced = MakeGarbageCollected<GCed>(heap->GetAllocationHandle());
+    MemberType<GCed> member(*gced,
                             typename MemberType<GCed>::AtomicInitializerTag());
-    EXPECT_EQ(&gced, member.Get());
+    EXPECT_EQ(gced, member.Get());
   }
   {
     MemberType<GCed> member(nullptr,
@@ -506,5 +506,51 @@ 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::DefaultCheckingPolicy>;
+                              internal::DefaultPersistentCheckingPolicy>;
 
 template <typename T>
 using LocalizedCrossThreadPersistent = internal::BasicCrossThreadPersistent<
     T, internal::StrongCrossThreadPersistentPolicy,
-    internal::KeepLocationPolicy, internal::DefaultCheckingPolicy>;
+    internal::KeepLocationPolicy, internal::DisabledCheckingPolicy>;
 
 class RootVisitor final : public VisitorBase {
  public: