Skip to content

Commit 8a27d9f

Browse files
ngzhianCommit Bot
ngzhian
authored and
Commit Bot
committed
Revert "cppgc: Properly clear (Weak)Peristent and WeakMember pointers"
This reverts commit e0c1a34. 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/+/2250320 Reviewed-by: Zhi An Ng <zhin@chromium.org> Commit-Queue: Zhi An Ng <zhin@chromium.org> Cr-Commit-Position: refs/heads/master@{#68396}
1 parent cad1845 commit 8a27d9f

9 files changed

Lines changed: 57 additions & 157 deletions

File tree

‎include/cppgc/internal/persistent-node.h‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -56,11 +56,6 @@ class PersistentNode final {
5656

5757
bool IsUsed() const { return trace_; }
5858

59-
void* owner() const {
60-
CPPGC_DCHECK(IsUsed());
61-
return owner_;
62-
}
63-
6459
private:
6560
// PersistentNode acts as a designated union:
6661
// If trace_ != nullptr, owner_ points to the corresponding Persistent handle.
@@ -72,13 +67,11 @@ class PersistentNode final {
7267
TraceCallback trace_ = nullptr;
7368
};
7469

75-
class V8_EXPORT PersistentRegion final {
70+
class V8_EXPORT PersistentRegion {
7671
using PersistentNodeSlots = std::array<PersistentNode, 256u>;
7772

7873
public:
7974
PersistentRegion() = default;
80-
// Clears Persistent fields to avoid stale pointers after heap teardown.
81-
~PersistentRegion();
8275

8376
PersistentRegion(const PersistentRegion&) = delete;
8477
PersistentRegion& operator=(const PersistentRegion&) = delete;

‎include/cppgc/liveness-broker.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,12 @@ class V8_EXPORT LivenessBroker final {
4949
TraceTrait<T>::GetTraceDescriptor(object).base_object_payload);
5050
}
5151

52+
template <typename T>
53+
bool IsHeapObjectAlive(const WeakMember<T>& weak_member) const {
54+
return (weak_member != kSentinelPointer) &&
55+
IsHeapObjectAlive<T>(weak_member.Get());
56+
}
57+
5258
template <typename T>
5359
bool IsHeapObjectAlive(const UntracedMember<T>& untraced_member) const {
5460
return (untraced_member != kSentinelPointer) &&

‎include/cppgc/member.h‎

Lines changed: 16 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -19,43 +19,19 @@ class Visitor;
1919

2020
namespace internal {
2121

22-
class MemberBase {
23-
protected:
24-
MemberBase() = default;
25-
explicit MemberBase(void* value) : raw_(value) {}
26-
27-
void* const* GetRawSlot() const { return &raw_; }
28-
void* GetRaw() const { return raw_; }
29-
void SetRaw(void* value) { raw_ = value; }
30-
31-
void* GetRawAtomic() const {
32-
return reinterpret_cast<const std::atomic<void*>*>(&raw_)->load(
33-
std::memory_order_relaxed);
34-
}
35-
void SetRawAtomic(void* value) {
36-
reinterpret_cast<std::atomic<void*>*>(&raw_)->store(
37-
value, std::memory_order_relaxed);
38-
}
39-
40-
void ClearFromGC() const { raw_ = nullptr; }
41-
42-
private:
43-
mutable void* raw_ = nullptr;
44-
};
45-
4622
// The basic class from which all Member classes are 'generated'.
4723
template <typename T, typename WeaknessTag, typename WriteBarrierPolicy,
4824
typename CheckingPolicy>
49-
class BasicMember final : private MemberBase, private CheckingPolicy {
25+
class BasicMember : private CheckingPolicy {
5026
public:
5127
using PointeeType = T;
5228

5329
constexpr BasicMember() = default;
5430
constexpr BasicMember(std::nullptr_t) {} // NOLINT
55-
BasicMember(SentinelPointer s) : MemberBase(s) {} // NOLINT
56-
BasicMember(T* raw) : MemberBase(raw) { // NOLINT
31+
BasicMember(SentinelPointer s) : raw_(s) {} // NOLINT
32+
BasicMember(T* raw) : raw_(raw) { // NOLINT
5733
InitializingWriteBarrier();
58-
this->CheckPointer(Get());
34+
this->CheckPointer(raw_);
5935
}
6036
BasicMember(T& raw) : BasicMember(&raw) {} // NOLINT
6137
BasicMember(const BasicMember& other) : BasicMember(other.Get()) {}
@@ -132,7 +108,7 @@ class BasicMember final : private MemberBase, private CheckingPolicy {
132108

133109
T* Get() const {
134110
// Executed by the mutator, hence non atomic load.
135-
return static_cast<T*>(MemberBase::GetRaw());
111+
return raw_;
136112
}
137113

138114
void Clear() { SetRawAtomic(nullptr); }
@@ -144,18 +120,25 @@ class BasicMember final : private MemberBase, private CheckingPolicy {
144120
}
145121

146122
private:
123+
void SetRawAtomic(T* raw) {
124+
reinterpret_cast<std::atomic<T*>*>(&raw_)->store(raw,
125+
std::memory_order_relaxed);
126+
}
147127
T* GetRawAtomic() const {
148-
return static_cast<T*>(MemberBase::GetRawAtomic());
128+
return reinterpret_cast<const std::atomic<T*>*>(&raw_)->load(
129+
std::memory_order_relaxed);
149130
}
150131

151132
void InitializingWriteBarrier() const {
152-
WriteBarrierPolicy::InitializingBarrier(GetRawSlot(), GetRaw());
133+
WriteBarrierPolicy::InitializingBarrier(
134+
reinterpret_cast<const void*>(&raw_), static_cast<const void*>(raw_));
153135
}
154136
void AssigningWriteBarrier() const {
155-
WriteBarrierPolicy::AssigningBarrier(GetRawSlot(), GetRaw());
137+
WriteBarrierPolicy::AssigningBarrier(reinterpret_cast<const void*>(&raw_),
138+
static_cast<const void*>(raw_));
156139
}
157140

158-
void ClearFromGC() const { MemberBase::ClearFromGC(); }
141+
T* raw_ = nullptr;
159142

160143
friend class cppgc::Visitor;
161144
};

‎include/cppgc/persistent.h‎

Lines changed: 29 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -15,43 +15,14 @@
1515
#include "v8config.h" // NOLINT(build/include_directory)
1616

1717
namespace cppgc {
18-
19-
class Visitor;
20-
2118
namespace internal {
2219

23-
class PersistentBase {
24-
protected:
25-
PersistentBase() = default;
26-
explicit PersistentBase(void* raw) : raw_(raw) {}
27-
28-
void* GetValue() const { return raw_; }
29-
void SetValue(void* value) { raw_ = value; }
30-
31-
PersistentNode* GetNode() const { return node_; }
32-
void SetNode(PersistentNode* node) { node_ = node; }
33-
34-
// Performs a shallow clear which assumes that internal persistent nodes are
35-
// destroyed elsewhere.
36-
void ClearFromGC() const {
37-
raw_ = nullptr;
38-
node_ = nullptr;
39-
}
40-
41-
private:
42-
mutable void* raw_ = nullptr;
43-
mutable PersistentNode* node_ = nullptr;
44-
45-
friend class PersistentRegion;
46-
};
47-
4820
// The basic class from which all Persistent classes are generated.
4921
template <typename T, typename WeaknessPolicy, typename LocationPolicy,
5022
typename CheckingPolicy>
51-
class BasicPersistent final : public PersistentBase,
52-
public LocationPolicy,
53-
private WeaknessPolicy,
54-
private CheckingPolicy {
23+
class BasicPersistent : public LocationPolicy,
24+
private WeaknessPolicy,
25+
private CheckingPolicy {
5526
public:
5627
using typename WeaknessPolicy::IsStrongPersistent;
5728
using PointeeType = T;
@@ -67,15 +38,15 @@ class BasicPersistent final : public PersistentBase,
6738

6839
BasicPersistent( // NOLINT
6940
SentinelPointer s, const SourceLocation& loc = SourceLocation::Current())
70-
: PersistentBase(s), LocationPolicy(loc) {}
41+
: LocationPolicy(loc), raw_(s) {}
7142

72-
// Raw value constructors.
43+
// Raw value contstructors.
7344
BasicPersistent(T* raw, // NOLINT
7445
const SourceLocation& loc = SourceLocation::Current())
75-
: PersistentBase(raw), LocationPolicy(loc) {
46+
: LocationPolicy(loc), raw_(raw) {
7647
if (!IsValid()) return;
77-
SetNode(WeaknessPolicy::GetPersistentRegion(GetValue())
78-
.AllocateNode(this, &BasicPersistent::Trace));
48+
node_ = WeaknessPolicy::GetPersistentRegion(raw_).AllocateNode(
49+
this, &BasicPersistent::Trace);
7950
this->CheckPointer(Get());
8051
}
8152

@@ -103,11 +74,13 @@ class BasicPersistent final : public PersistentBase,
10374
BasicPersistent(
10475
BasicPersistent&& other,
10576
const SourceLocation& loc = SourceLocation::Current()) noexcept
106-
: PersistentBase(std::move(other)), LocationPolicy(std::move(other)) {
77+
: LocationPolicy(std::move(other)),
78+
raw_(std::move(other.raw_)),
79+
node_(std::move(other.node_)) {
10780
if (!IsValid()) return;
108-
GetNode()->UpdateOwner(this);
109-
other.SetValue(nullptr);
110-
other.SetNode(nullptr);
81+
node_->UpdateOwner(this);
82+
other.raw_ = nullptr;
83+
other.node_ = nullptr;
11184
this->CheckPointer(Get());
11285
}
11386

@@ -141,12 +114,13 @@ class BasicPersistent final : public PersistentBase,
141114
BasicPersistent& operator=(BasicPersistent&& other) {
142115
if (this == &other) return *this;
143116
Clear();
144-
PersistentBase::operator=(std::move(other));
145117
LocationPolicy::operator=(std::move(other));
118+
raw_ = std::move(other.raw_);
119+
node_ = std::move(other.node_);
146120
if (!IsValid()) return *this;
147-
GetNode()->UpdateOwner(this);
148-
other.SetValue(nullptr);
149-
other.SetNode(nullptr);
121+
node_->UpdateOwner(this);
122+
other.raw_ = nullptr;
123+
other.node_ = nullptr;
150124
this->CheckPointer(Get());
151125
return *this;
152126
}
@@ -182,7 +156,7 @@ class BasicPersistent final : public PersistentBase,
182156
T* operator->() const { return Get(); }
183157
T& operator*() const { return *Get(); }
184158

185-
T* Get() const { return static_cast<T*>(GetValue()); }
159+
T* Get() const { return raw_; }
186160

187161
void Clear() { Assign(nullptr); }
188162

@@ -202,35 +176,29 @@ class BasicPersistent final : public PersistentBase,
202176
// Ideally, handling kSentinelPointer would be done by the embedder. On the
203177
// other hand, having Persistent aware of it is beneficial since no node
204178
// gets wasted.
205-
return GetValue() != nullptr && GetValue() != kSentinelPointer;
179+
return raw_ != nullptr && raw_ != kSentinelPointer;
206180
}
207181

208182
void Assign(T* ptr) {
209183
if (IsValid()) {
210184
if (ptr && ptr != kSentinelPointer) {
211185
// Simply assign the pointer reusing the existing node.
212-
SetValue(ptr);
186+
raw_ = ptr;
213187
this->CheckPointer(ptr);
214188
return;
215189
}
216-
WeaknessPolicy::GetPersistentRegion(GetValue()).FreeNode(GetNode());
217-
SetNode(nullptr);
190+
WeaknessPolicy::GetPersistentRegion(raw_).FreeNode(node_);
191+
node_ = nullptr;
218192
}
219-
SetValue(ptr);
193+
raw_ = ptr;
220194
if (!IsValid()) return;
221-
SetNode(WeaknessPolicy::GetPersistentRegion(GetValue())
222-
.AllocateNode(this, &BasicPersistent::Trace));
195+
node_ = WeaknessPolicy::GetPersistentRegion(raw_).AllocateNode(
196+
this, &BasicPersistent::Trace);
223197
this->CheckPointer(Get());
224198
}
225199

226-
void ClearFromGC() const {
227-
if (IsValid()) {
228-
WeaknessPolicy::GetPersistentRegion(GetValue()).FreeNode(GetNode());
229-
PersistentBase::ClearFromGC();
230-
}
231-
}
232-
233-
friend class cppgc::Visitor;
200+
T* raw_ = nullptr;
201+
PersistentNode* node_ = nullptr;
234202
};
235203

236204
template <typename T1, typename WeaknessPolicy1, typename LocationPolicy1,

‎include/cppgc/visitor.h‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -136,11 +136,12 @@ class Visitor {
136136
template <typename PointerType>
137137
static void HandleWeak(const LivenessBroker& info, const void* object) {
138138
const PointerType* weak = static_cast<const PointerType*>(object);
139-
// Sentinel values are preserved for weak pointers.
140-
if (*weak == kSentinelPointer) return;
141139
const auto* raw = weak->Get();
142-
if (!info.IsHeapObjectAlive(raw)) {
143-
weak->ClearFromGC();
140+
if (raw && !info.IsHeapObjectAlive(raw)) {
141+
// Object is passed down through the marker as const. Alternatives are
142+
// - non-const Trace method;
143+
// - mutable pointer in MemberBase;
144+
const_cast<PointerType*>(weak)->Clear();
144145
}
145146
}
146147

‎src/heap/cppgc/persistent-node.cc‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,21 +7,9 @@
77
#include <algorithm>
88
#include <numeric>
99

10-
#include "include/cppgc/persistent.h"
11-
1210
namespace cppgc {
1311
namespace internal {
1412

15-
PersistentRegion::~PersistentRegion() {
16-
for (auto& slots : nodes_) {
17-
for (auto& node : *slots) {
18-
if (node.IsUsed()) {
19-
static_cast<PersistentBase*>(node.owner())->ClearFromGC();
20-
}
21-
}
22-
}
23-
}
24-
2513
size_t PersistentRegion::NodesInUse() const {
2614
return std::accumulate(
2715
nodes_.cbegin(), nodes_.cend(), 0u, [](size_t acc, const auto& slots) {

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

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
#include "src/heap/cppgc/marker.h"
66

77
#include "include/cppgc/allocation.h"
8-
#include "include/cppgc/internal/pointer-policies.h"
98
#include "include/cppgc/member.h"
109
#include "include/cppgc/persistent.h"
1110
#include "src/heap/cppgc/heap-object-header-inl.h"
@@ -246,20 +245,5 @@ TEST_F(MarkerTest, InConstructionObjectIsEventuallyMarkedNonEmptyStack) {
246245
});
247246
}
248247

249-
TEST_F(MarkerTest, SentinelNotClearedOnWeakPersistentHandling) {
250-
Marker marker(Heap::From(GetHeap())->AsBase());
251-
Persistent<GCed> root = MakeGarbageCollected<GCed>(GetAllocationHandle());
252-
auto* tmp = MakeGarbageCollected<GCed>(GetAllocationHandle());
253-
root->SetWeakChild(tmp);
254-
static const Marker::MarkingConfig config = {
255-
MarkingConfig::CollectionType::kMajor,
256-
MarkingConfig::StackState::kNoHeapPointers};
257-
marker.StartMarking(config);
258-
marker.FinishMarking(config);
259-
root->SetWeakChild(kSentinelPointer);
260-
marker.ProcessWeakness();
261-
EXPECT_EQ(kSentinelPointer, root->weak_child());
262-
}
263-
264248
} // namespace internal
265249
} // namespace cppgc

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

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88

99
#include "include/cppgc/allocation.h"
1010
#include "include/cppgc/garbage-collected.h"
11-
#include "include/cppgc/internal/pointer-policies.h"
1211
#include "include/cppgc/member.h"
1312
#include "include/cppgc/type-traits.h"
1413
#include "src/heap/cppgc/heap.h"
@@ -71,7 +70,6 @@ class RootVisitor final : public VisitorBase {
7170
private:
7271
std::vector<std::pair<WeakCallback, const void*>> weak_callbacks_;
7372
};
74-
7573
class PersistentTest : public testing::TestSupportingAllocationOnly {};
7674

7775
} // namespace
@@ -619,25 +617,6 @@ TEST_F(PersistentTest, TraceWeak) {
619617
EXPECT_EQ(0u, GetRegion<WeakPersistent>(heap).NodesInUse());
620618
}
621619

622-
TEST_F(PersistentTest, ClearOnHeapDestruction) {
623-
Persistent<GCed> persistent;
624-
WeakPersistent<GCed> weak_persistent;
625-
626-
// Create another heap that can be destroyed during the test.
627-
auto heap = Heap::Create(GetPlatformHandle());
628-
persistent = MakeGarbageCollected<GCed>(heap->GetAllocationHandle());
629-
weak_persistent = MakeGarbageCollected<GCed>(heap->GetAllocationHandle());
630-
const Persistent<GCed> persistent_sentinel(kSentinelPointer);
631-
const WeakPersistent<GCed> weak_persistent_sentinel(kSentinelPointer);
632-
heap.reset();
633-
634-
EXPECT_EQ(nullptr, persistent);
635-
EXPECT_EQ(nullptr, weak_persistent);
636-
// Sentinel values survive as they do not represent actual heap objects.
637-
EXPECT_EQ(kSentinelPointer, persistent_sentinel);
638-
EXPECT_EQ(kSentinelPointer, weak_persistent_sentinel);
639-
}
640-
641620
#if CPPGC_SUPPORTS_SOURCE_LOCATION
642621
TEST_F(PersistentTest, LocalizedPersistent) {
643622
GCed* gced = MakeGarbageCollected<GCed>(GetAllocationHandle());

0 commit comments

Comments
 (0)