Revert "cppgc: Properly clear (Weak)Peristent and WeakMember pointers"

This reverts commit e0c1a349.

Reason for revert: Fails on Linux 64 cfi https://ci.chromium.org/p/v8/builders/ci/V8%20Linux64%20-%20cfi/25283?

TBR=omerkatz@chromium.org,mlippautz@chromium.org,bikineev@chromium.org,bikineev@chromium.org

Change-Id: I2b208c4019979735925bff5e0551291fae6a14d6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2250320Reviewed-by: Zhi An Ng <zhin@chromium.org>
Commit-Queue: Zhi An Ng <zhin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68396}

include/cppgc/internal/persistent-node.h

@@ -56,11 +56,6 @@ class PersistentNode final {
 
   bool IsUsed() const { return trace_; }
 
-  void* owner() const {
-    CPPGC_DCHECK(IsUsed());
-    return owner_;
-  }
-
  private:
   // PersistentNode acts as a designated union:
   // If trace_ != nullptr, owner_ points to the corresponding Persistent handle.
@@ -72,13 +67,11 @@ class PersistentNode final {
   TraceCallback trace_ = nullptr;
 };
 
-class V8_EXPORT PersistentRegion final {
+class V8_EXPORT PersistentRegion {
   using PersistentNodeSlots = std::array<PersistentNode, 256u>;
 
  public:
   PersistentRegion() = default;
-  // Clears Persistent fields to avoid stale pointers after heap teardown.
-  ~PersistentRegion();
 
   PersistentRegion(const PersistentRegion&) = delete;
   PersistentRegion& operator=(const PersistentRegion&) = delete;

include/cppgc/liveness-broker.h

@@ -49,6 +49,12 @@ class V8_EXPORT LivenessBroker final {
                TraceTrait<T>::GetTraceDescriptor(object).base_object_payload);
   }
 
+  template <typename T>
+  bool IsHeapObjectAlive(const WeakMember<T>& weak_member) const {
+    return (weak_member != kSentinelPointer) &&
+           IsHeapObjectAlive<T>(weak_member.Get());
+  }
+
   template <typename T>
   bool IsHeapObjectAlive(const UntracedMember<T>& untraced_member) const {
     return (untraced_member != kSentinelPointer) &&

include/cppgc/member.h

@@ -19,43 +19,19 @@ class Visitor;
 
 namespace internal {
 
-class MemberBase {
- protected:
-  MemberBase() = default;
-  explicit MemberBase(void* value) : raw_(value) {}
-
-  void* const* GetRawSlot() const { return &raw_; }
-  void* GetRaw() const { return raw_; }
-  void SetRaw(void* value) { raw_ = value; }
-
-  void* GetRawAtomic() const {
-    return reinterpret_cast<const std::atomic<void*>*>(&raw_)->load(
-        std::memory_order_relaxed);
-  }
-  void SetRawAtomic(void* value) {
-    reinterpret_cast<std::atomic<void*>*>(&raw_)->store(
-        value, std::memory_order_relaxed);
-  }
-
-  void ClearFromGC() const { raw_ = nullptr; }
-
- private:
-  mutable void* raw_ = nullptr;
-};
-
 // The basic class from which all Member classes are 'generated'.
 template <typename T, typename WeaknessTag, typename WriteBarrierPolicy,
           typename CheckingPolicy>
-class BasicMember final : private MemberBase, private CheckingPolicy {
+class BasicMember : private CheckingPolicy {
  public:
   using PointeeType = T;
 
   constexpr BasicMember() = default;
   constexpr BasicMember(std::nullptr_t) {}     // NOLINT
-  BasicMember(SentinelPointer s) : MemberBase(s) {}  // NOLINT
-  BasicMember(T* raw) : MemberBase(raw) {            // NOLINT
+  BasicMember(SentinelPointer s) : raw_(s) {}  // NOLINT
+  BasicMember(T* raw) : raw_(raw) {            // NOLINT
     InitializingWriteBarrier();
-    this->CheckPointer(Get());
+    this->CheckPointer(raw_);
   }
   BasicMember(T& raw) : BasicMember(&raw) {}  // NOLINT
   BasicMember(const BasicMember& other) : BasicMember(other.Get()) {}
@@ -132,7 +108,7 @@ class BasicMember final : private MemberBase, private CheckingPolicy {
 
   T* Get() const {
     // Executed by the mutator, hence non atomic load.
-    return static_cast<T*>(MemberBase::GetRaw());
+    return raw_;
   }
 
   void Clear() { SetRawAtomic(nullptr); }
@@ -144,18 +120,25 @@ class BasicMember final : private MemberBase, private CheckingPolicy {
   }
 
  private:
+  void SetRawAtomic(T* raw) {
+    reinterpret_cast<std::atomic<T*>*>(&raw_)->store(raw,
+                                                     std::memory_order_relaxed);
+  }
   T* GetRawAtomic() const {
-    return static_cast<T*>(MemberBase::GetRawAtomic());
+    return reinterpret_cast<const std::atomic<T*>*>(&raw_)->load(
+        std::memory_order_relaxed);
   }
 
   void InitializingWriteBarrier() const {
-    WriteBarrierPolicy::InitializingBarrier(GetRawSlot(), GetRaw());
+    WriteBarrierPolicy::InitializingBarrier(
+        reinterpret_cast<const void*>(&raw_), static_cast<const void*>(raw_));
   }
   void AssigningWriteBarrier() const {
-    WriteBarrierPolicy::AssigningBarrier(GetRawSlot(), GetRaw());
+    WriteBarrierPolicy::AssigningBarrier(reinterpret_cast<const void*>(&raw_),
+                                         static_cast<const void*>(raw_));
   }
 
-  void ClearFromGC() const { MemberBase::ClearFromGC(); }
+  T* raw_ = nullptr;
 
   friend class cppgc::Visitor;
 };

include/cppgc/persistent.h

@@ -15,41 +15,12 @@
 #include "v8config.h"  // NOLINT(build/include_directory)
 
 namespace cppgc {
-
-class Visitor;
-
 namespace internal {
 
-class PersistentBase {
- protected:
-  PersistentBase() = default;
-  explicit PersistentBase(void* raw) : raw_(raw) {}
-
-  void* GetValue() const { return raw_; }
-  void SetValue(void* value) { raw_ = value; }
-
-  PersistentNode* GetNode() const { return node_; }
-  void SetNode(PersistentNode* node) { node_ = node; }
-
-  // Performs a shallow clear which assumes that internal persistent nodes are
-  // destroyed elsewhere.
-  void ClearFromGC() const {
-    raw_ = nullptr;
-    node_ = nullptr;
-  }
-
- private:
-  mutable void* raw_ = nullptr;
-  mutable PersistentNode* node_ = nullptr;
-
-  friend class PersistentRegion;
-};
-
 // The basic class from which all Persistent classes are generated.
 template <typename T, typename WeaknessPolicy, typename LocationPolicy,
           typename CheckingPolicy>
-class BasicPersistent final : public PersistentBase,
-                              public LocationPolicy,
+class BasicPersistent : public LocationPolicy,
                         private WeaknessPolicy,
                         private CheckingPolicy {
  public:
@@ -67,15 +38,15 @@ class BasicPersistent final : public PersistentBase,
 
   BasicPersistent(  // NOLINT
       SentinelPointer s, const SourceLocation& loc = SourceLocation::Current())
-      : PersistentBase(s), LocationPolicy(loc) {}
+      : LocationPolicy(loc), raw_(s) {}
 
-  // Raw value constructors.
+  // Raw value contstructors.
   BasicPersistent(T* raw,  // NOLINT
                   const SourceLocation& loc = SourceLocation::Current())
-      : PersistentBase(raw), LocationPolicy(loc) {
+      : LocationPolicy(loc), raw_(raw) {
     if (!IsValid()) return;
-    SetNode(WeaknessPolicy::GetPersistentRegion(GetValue())
-                .AllocateNode(this, &BasicPersistent::Trace));
+    node_ = WeaknessPolicy::GetPersistentRegion(raw_).AllocateNode(
+        this, &BasicPersistent::Trace);
     this->CheckPointer(Get());
   }
 
@@ -103,11 +74,13 @@ class BasicPersistent final : public PersistentBase,
   BasicPersistent(
       BasicPersistent&& other,
       const SourceLocation& loc = SourceLocation::Current()) noexcept
-      : PersistentBase(std::move(other)), LocationPolicy(std::move(other)) {
+      : LocationPolicy(std::move(other)),
+        raw_(std::move(other.raw_)),
+        node_(std::move(other.node_)) {
     if (!IsValid()) return;
-    GetNode()->UpdateOwner(this);
-    other.SetValue(nullptr);
-    other.SetNode(nullptr);
+    node_->UpdateOwner(this);
+    other.raw_ = nullptr;
+    other.node_ = nullptr;
     this->CheckPointer(Get());
   }
 
@@ -141,12 +114,13 @@ class BasicPersistent final : public PersistentBase,
   BasicPersistent& operator=(BasicPersistent&& other) {
     if (this == &other) return *this;
     Clear();
-    PersistentBase::operator=(std::move(other));
     LocationPolicy::operator=(std::move(other));
+    raw_ = std::move(other.raw_);
+    node_ = std::move(other.node_);
     if (!IsValid()) return *this;
-    GetNode()->UpdateOwner(this);
-    other.SetValue(nullptr);
-    other.SetNode(nullptr);
+    node_->UpdateOwner(this);
+    other.raw_ = nullptr;
+    other.node_ = nullptr;
     this->CheckPointer(Get());
     return *this;
   }
@@ -182,7 +156,7 @@ class BasicPersistent final : public PersistentBase,
   T* operator->() const { return Get(); }
   T& operator*() const { return *Get(); }
 
-  T* Get() const { return static_cast<T*>(GetValue()); }
+  T* Get() const { return raw_; }
 
   void Clear() { Assign(nullptr); }
 
@@ -202,35 +176,29 @@ class BasicPersistent final : public PersistentBase,
     // Ideally, handling kSentinelPointer would be done by the embedder. On the
     // other hand, having Persistent aware of it is beneficial since no node
     // gets wasted.
-    return GetValue() != nullptr && GetValue() != kSentinelPointer;
+    return raw_ != nullptr && raw_ != kSentinelPointer;
   }
 
   void Assign(T* ptr) {
     if (IsValid()) {
       if (ptr && ptr != kSentinelPointer) {
         // Simply assign the pointer reusing the existing node.
-        SetValue(ptr);
+        raw_ = ptr;
         this->CheckPointer(ptr);
         return;
       }
-      WeaknessPolicy::GetPersistentRegion(GetValue()).FreeNode(GetNode());
-      SetNode(nullptr);
+      WeaknessPolicy::GetPersistentRegion(raw_).FreeNode(node_);
+      node_ = nullptr;
     }
-    SetValue(ptr);
+    raw_ = ptr;
     if (!IsValid()) return;
-    SetNode(WeaknessPolicy::GetPersistentRegion(GetValue())
-                .AllocateNode(this, &BasicPersistent::Trace));
+    node_ = WeaknessPolicy::GetPersistentRegion(raw_).AllocateNode(
+        this, &BasicPersistent::Trace);
     this->CheckPointer(Get());
   }
 
-  void ClearFromGC() const {
-    if (IsValid()) {
-      WeaknessPolicy::GetPersistentRegion(GetValue()).FreeNode(GetNode());
-      PersistentBase::ClearFromGC();
-    }
-  }
-
-  friend class cppgc::Visitor;
+  T* raw_ = nullptr;
+  PersistentNode* node_ = nullptr;
 };
 
 template <typename T1, typename WeaknessPolicy1, typename LocationPolicy1,

include/cppgc/visitor.h

@@ -136,11 +136,12 @@ class Visitor {
   template <typename PointerType>
   static void HandleWeak(const LivenessBroker& info, const void* object) {
     const PointerType* weak = static_cast<const PointerType*>(object);
-    // Sentinel values are preserved for weak pointers.
-    if (*weak == kSentinelPointer) return;
     const auto* raw = weak->Get();
-    if (!info.IsHeapObjectAlive(raw)) {
-      weak->ClearFromGC();
+    if (raw && !info.IsHeapObjectAlive(raw)) {
+      // Object is passed down through the marker as const. Alternatives are
+      // - non-const Trace method;
+      // - mutable pointer in MemberBase;
+      const_cast<PointerType*>(weak)->Clear();
     }
   }
 

src/heap/cppgc/persistent-node.cc

@@ -7,21 +7,9 @@
 #include <algorithm>
 #include <numeric>
 
-#include "include/cppgc/persistent.h"
-
 namespace cppgc {
 namespace internal {
 
-PersistentRegion::~PersistentRegion() {
-  for (auto& slots : nodes_) {
-    for (auto& node : *slots) {
-      if (node.IsUsed()) {
-        static_cast<PersistentBase*>(node.owner())->ClearFromGC();
-      }
-    }
-  }
-}
-
 size_t PersistentRegion::NodesInUse() const {
   return std::accumulate(
       nodes_.cbegin(), nodes_.cend(), 0u, [](size_t acc, const auto& slots) {

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

@@ -5,7 +5,6 @@
 #include "src/heap/cppgc/marker.h"
 
 #include "include/cppgc/allocation.h"
-#include "include/cppgc/internal/pointer-policies.h"
 #include "include/cppgc/member.h"
 #include "include/cppgc/persistent.h"
 #include "src/heap/cppgc/heap-object-header-inl.h"
@@ -246,20 +245,5 @@ TEST_F(MarkerTest, InConstructionObjectIsEventuallyMarkedNonEmptyStack) {
       });
 }
 
-TEST_F(MarkerTest, SentinelNotClearedOnWeakPersistentHandling) {
-  Marker marker(Heap::From(GetHeap())->AsBase());
-  Persistent<GCed> root = MakeGarbageCollected<GCed>(GetAllocationHandle());
-  auto* tmp = MakeGarbageCollected<GCed>(GetAllocationHandle());
-  root->SetWeakChild(tmp);
-  static const Marker::MarkingConfig config = {
-      MarkingConfig::CollectionType::kMajor,
-      MarkingConfig::StackState::kNoHeapPointers};
-  marker.StartMarking(config);
-  marker.FinishMarking(config);
-  root->SetWeakChild(kSentinelPointer);
-  marker.ProcessWeakness();
-  EXPECT_EQ(kSentinelPointer, root->weak_child());
-}
-
 }  // namespace internal
 }  // namespace cppgc

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

@@ -8,7 +8,6 @@
 
 #include "include/cppgc/allocation.h"
 #include "include/cppgc/garbage-collected.h"
-#include "include/cppgc/internal/pointer-policies.h"
 #include "include/cppgc/member.h"
 #include "include/cppgc/type-traits.h"
 #include "src/heap/cppgc/heap.h"
@@ -71,7 +70,6 @@ class RootVisitor final : public VisitorBase {
  private:
   std::vector<std::pair<WeakCallback, const void*>> weak_callbacks_;
 };
-
 class PersistentTest : public testing::TestSupportingAllocationOnly {};
 
 }  // namespace
@@ -619,25 +617,6 @@ TEST_F(PersistentTest, TraceWeak) {
   EXPECT_EQ(0u, GetRegion<WeakPersistent>(heap).NodesInUse());
 }
 
-TEST_F(PersistentTest, ClearOnHeapDestruction) {
-  Persistent<GCed> persistent;
-  WeakPersistent<GCed> weak_persistent;
-
-  // Create another heap that can be destroyed during the test.
-  auto heap = Heap::Create(GetPlatformHandle());
-  persistent = MakeGarbageCollected<GCed>(heap->GetAllocationHandle());
-  weak_persistent = MakeGarbageCollected<GCed>(heap->GetAllocationHandle());
-  const Persistent<GCed> persistent_sentinel(kSentinelPointer);
-  const WeakPersistent<GCed> weak_persistent_sentinel(kSentinelPointer);
-  heap.reset();
-
-  EXPECT_EQ(nullptr, persistent);
-  EXPECT_EQ(nullptr, weak_persistent);
-  // Sentinel values survive as they do not represent actual heap objects.
-  EXPECT_EQ(kSentinelPointer, persistent_sentinel);
-  EXPECT_EQ(kSentinelPointer, weak_persistent_sentinel);
-}
-
 #if CPPGC_SUPPORTS_SOURCE_LOCATION
 TEST_F(PersistentTest, LocalizedPersistent) {
   GCed* gced = MakeGarbageCollected<GCed>(GetAllocationHandle());

test/unittests/heap/cppgc/tests.h

@@ -22,8 +22,6 @@ class TestWithPlatform : public ::testing::Test {
 
   TestPlatform& GetPlatform() const { return *platform_; }
 
-  std::shared_ptr<TestPlatform> GetPlatformHandle() const { return platform_; }
-
  protected:
   static std::shared_ptr<TestPlatform> platform_;
 };