diff --git a/include/cppgc/internal/persistent-node.h b/include/cppgc/internal/persistent-node.h index 11cf69623e..e05efe3621 100644 --- a/include/cppgc/internal/persistent-node.h +++ b/include/cppgc/internal/persistent-node.h @@ -56,6 +56,11 @@ 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. @@ -67,11 +72,13 @@ class PersistentNode final { TraceCallback trace_ = nullptr; }; -class V8_EXPORT PersistentRegion { +class V8_EXPORT PersistentRegion final { using PersistentNodeSlots = std::array; public: PersistentRegion() = default; + // Clears Persistent fields to avoid stale pointers after heap teardown. + ~PersistentRegion(); PersistentRegion(const PersistentRegion&) = delete; PersistentRegion& operator=(const PersistentRegion&) = delete; diff --git a/include/cppgc/liveness-broker.h b/include/cppgc/liveness-broker.h index 640a5519ff..883be46240 100644 --- a/include/cppgc/liveness-broker.h +++ b/include/cppgc/liveness-broker.h @@ -49,12 +49,6 @@ class V8_EXPORT LivenessBroker final { TraceTrait::GetTraceDescriptor(object).base_object_payload); } - template - bool IsHeapObjectAlive(const WeakMember& weak_member) const { - return (weak_member != kSentinelPointer) && - IsHeapObjectAlive(weak_member.Get()); - } - template bool IsHeapObjectAlive(const UntracedMember& untraced_member) const { return (untraced_member != kSentinelPointer) && diff --git a/include/cppgc/member.h b/include/cppgc/member.h index a183edb96f..22c1adc0af 100644 --- a/include/cppgc/member.h +++ b/include/cppgc/member.h @@ -19,19 +19,43 @@ 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*>(&raw_)->load( + std::memory_order_relaxed); + } + void SetRawAtomic(void* value) { + reinterpret_cast*>(&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 -class BasicMember : private CheckingPolicy { +class BasicMember final : private MemberBase, private CheckingPolicy { public: using PointeeType = T; constexpr BasicMember() = default; constexpr BasicMember(std::nullptr_t) {} // NOLINT - BasicMember(SentinelPointer s) : raw_(s) {} // NOLINT - BasicMember(T* raw) : raw_(raw) { // NOLINT + BasicMember(SentinelPointer s) : MemberBase(s) {} // NOLINT + BasicMember(T* raw) : MemberBase(raw) { // NOLINT InitializingWriteBarrier(); - this->CheckPointer(raw_); + this->CheckPointer(Get()); } BasicMember(T& raw) : BasicMember(&raw) {} // NOLINT BasicMember(const BasicMember& other) : BasicMember(other.Get()) {} @@ -106,9 +130,12 @@ class BasicMember : private CheckingPolicy { T* operator->() const { return Get(); } T& operator*() const { return *Get(); } - T* Get() const { + // CFI cast exemption to allow passing SentinelPointer through T* and support + // heterogeneous assignments between different Member and Persistent handles + // based on their actual types. + V8_CLANG_NO_SANITIZE("cfi-unrelated-cast") T* Get() const { // Executed by the mutator, hence non atomic load. - return raw_; + return static_cast(MemberBase::GetRaw()); } void Clear() { SetRawAtomic(nullptr); } @@ -120,25 +147,18 @@ class BasicMember : private CheckingPolicy { } private: - void SetRawAtomic(T* raw) { - reinterpret_cast*>(&raw_)->store(raw, - std::memory_order_relaxed); - } T* GetRawAtomic() const { - return reinterpret_cast*>(&raw_)->load( - std::memory_order_relaxed); + return static_cast(MemberBase::GetRawAtomic()); } void InitializingWriteBarrier() const { - WriteBarrierPolicy::InitializingBarrier( - reinterpret_cast(&raw_), static_cast(raw_)); + WriteBarrierPolicy::InitializingBarrier(GetRawSlot(), GetRaw()); } void AssigningWriteBarrier() const { - WriteBarrierPolicy::AssigningBarrier(reinterpret_cast(&raw_), - static_cast(raw_)); + WriteBarrierPolicy::AssigningBarrier(GetRawSlot(), GetRaw()); } - T* raw_ = nullptr; + void ClearFromGC() const { MemberBase::ClearFromGC(); } friend class cppgc::Visitor; }; diff --git a/include/cppgc/persistent.h b/include/cppgc/persistent.h index fc6b0b9d92..c2d8a7a5a6 100644 --- a/include/cppgc/persistent.h +++ b/include/cppgc/persistent.h @@ -15,14 +15,43 @@ #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 -class BasicPersistent : public LocationPolicy, - private WeaknessPolicy, - private CheckingPolicy { +class BasicPersistent final : public PersistentBase, + public LocationPolicy, + private WeaknessPolicy, + private CheckingPolicy { public: using typename WeaknessPolicy::IsStrongPersistent; using PointeeType = T; @@ -38,15 +67,15 @@ class BasicPersistent : public LocationPolicy, BasicPersistent( // NOLINT SentinelPointer s, const SourceLocation& loc = SourceLocation::Current()) - : LocationPolicy(loc), raw_(s) {} + : PersistentBase(s), LocationPolicy(loc) {} - // Raw value contstructors. + // Raw value constructors. BasicPersistent(T* raw, // NOLINT const SourceLocation& loc = SourceLocation::Current()) - : LocationPolicy(loc), raw_(raw) { + : PersistentBase(raw), LocationPolicy(loc) { if (!IsValid()) return; - node_ = WeaknessPolicy::GetPersistentRegion(raw_).AllocateNode( - this, &BasicPersistent::Trace); + SetNode(WeaknessPolicy::GetPersistentRegion(GetValue()) + .AllocateNode(this, &BasicPersistent::Trace)); this->CheckPointer(Get()); } @@ -74,13 +103,11 @@ class BasicPersistent : public LocationPolicy, BasicPersistent( BasicPersistent&& other, const SourceLocation& loc = SourceLocation::Current()) noexcept - : LocationPolicy(std::move(other)), - raw_(std::move(other.raw_)), - node_(std::move(other.node_)) { + : PersistentBase(std::move(other)), LocationPolicy(std::move(other)) { if (!IsValid()) return; - node_->UpdateOwner(this); - other.raw_ = nullptr; - other.node_ = nullptr; + GetNode()->UpdateOwner(this); + other.SetValue(nullptr); + other.SetNode(nullptr); this->CheckPointer(Get()); } @@ -114,13 +141,12 @@ class BasicPersistent : public LocationPolicy, 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; - node_->UpdateOwner(this); - other.raw_ = nullptr; - other.node_ = nullptr; + GetNode()->UpdateOwner(this); + other.SetValue(nullptr); + other.SetNode(nullptr); this->CheckPointer(Get()); return *this; } @@ -156,7 +182,12 @@ class BasicPersistent : public LocationPolicy, T* operator->() const { return Get(); } T& operator*() const { return *Get(); } - T* Get() const { return raw_; } + // CFI cast exemption to allow passing SentinelPointer through T* and support + // heterogeneous assignments between different Member and Persistent handles + // based on their actual types. + V8_CLANG_NO_SANITIZE("cfi-unrelated-cast") T* Get() const { + return static_cast(GetValue()); + } void Clear() { Assign(nullptr); } @@ -176,29 +207,35 @@ class BasicPersistent : public LocationPolicy, // 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 raw_ != nullptr && raw_ != kSentinelPointer; + return GetValue() != nullptr && GetValue() != kSentinelPointer; } void Assign(T* ptr) { if (IsValid()) { if (ptr && ptr != kSentinelPointer) { // Simply assign the pointer reusing the existing node. - raw_ = ptr; + SetValue(ptr); this->CheckPointer(ptr); return; } - WeaknessPolicy::GetPersistentRegion(raw_).FreeNode(node_); - node_ = nullptr; + WeaknessPolicy::GetPersistentRegion(GetValue()).FreeNode(GetNode()); + SetNode(nullptr); } - raw_ = ptr; + SetValue(ptr); if (!IsValid()) return; - node_ = WeaknessPolicy::GetPersistentRegion(raw_).AllocateNode( - this, &BasicPersistent::Trace); + SetNode(WeaknessPolicy::GetPersistentRegion(GetValue()) + .AllocateNode(this, &BasicPersistent::Trace)); this->CheckPointer(Get()); } - T* raw_ = nullptr; - PersistentNode* node_ = nullptr; + void ClearFromGC() const { + if (IsValid()) { + WeaknessPolicy::GetPersistentRegion(GetValue()).FreeNode(GetNode()); + PersistentBase::ClearFromGC(); + } + } + + friend class cppgc::Visitor; }; template static void HandleWeak(const LivenessBroker& info, const void* object) { const PointerType* weak = static_cast(object); + // Sentinel values are preserved for weak pointers. + if (*weak == kSentinelPointer) return; const auto* raw = weak->Get(); - 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(weak)->Clear(); + if (!info.IsHeapObjectAlive(raw)) { + weak->ClearFromGC(); } } diff --git a/include/v8config.h b/include/v8config.h index 9825232d6a..bbd1d6ce97 100644 --- a/include/v8config.h +++ b/include/v8config.h @@ -433,6 +433,16 @@ #define V8_WARN_UNUSED_RESULT /* NOT SUPPORTED */ #endif +// Helper macro to define no_sanitize attributes only with clang. +#if defined(__clang__) && defined(__has_attribute) +#if __has_attribute(no_sanitize) +#define V8_CLANG_NO_SANITIZE(what) __attribute__((no_sanitize(what))) +#endif +#endif +#if !defined(V8_CLANG_NO_SANITIZE) +#define V8_CLANG_NO_SANITIZE(what) +#endif + #if defined(BUILDING_V8_SHARED) && defined(USING_V8_SHARED) #error Inconsistent build configuration: To build the V8 shared library \ set BUILDING_V8_SHARED, to include its headers for linking against the \ diff --git a/src/base/macros.h b/src/base/macros.h index e22dd00895..4eb652cae5 100644 --- a/src/base/macros.h +++ b/src/base/macros.h @@ -171,22 +171,12 @@ V8_INLINE Dest bit_cast(Source const& source) { #endif #endif -// Helper macro to define no_sanitize attributes only with clang. -#if defined(__clang__) && defined(__has_attribute) -#if __has_attribute(no_sanitize) -#define CLANG_NO_SANITIZE(what) __attribute__((no_sanitize(what))) -#endif -#endif -#if !defined(CLANG_NO_SANITIZE) -#define CLANG_NO_SANITIZE(what) -#endif - // DISABLE_CFI_PERF -- Disable Control Flow Integrity checks for Perf reasons. -#define DISABLE_CFI_PERF CLANG_NO_SANITIZE("cfi") +#define DISABLE_CFI_PERF V8_CLANG_NO_SANITIZE("cfi") // DISABLE_CFI_ICALL -- Disable Control Flow Integrity indirect call checks, // useful because calls into JITed code can not be CFI verified. -#define DISABLE_CFI_ICALL CLANG_NO_SANITIZE("cfi-icall") +#define DISABLE_CFI_ICALL V8_CLANG_NO_SANITIZE("cfi-icall") #if V8_CC_GNU #define V8_IMMEDIATE_CRASH() __builtin_trap() diff --git a/src/heap/cppgc/persistent-node.cc b/src/heap/cppgc/persistent-node.cc index 299cefc521..9c5113f86a 100644 --- a/src/heap/cppgc/persistent-node.cc +++ b/src/heap/cppgc/persistent-node.cc @@ -7,9 +7,21 @@ #include #include +#include "include/cppgc/persistent.h" + namespace cppgc { namespace internal { +PersistentRegion::~PersistentRegion() { + for (auto& slots : nodes_) { + for (auto& node : *slots) { + if (node.IsUsed()) { + static_cast(node.owner())->ClearFromGC(); + } + } + } +} + size_t PersistentRegion::NodesInUse() const { return std::accumulate( nodes_.cbegin(), nodes_.cend(), 0u, [](size_t acc, const auto& slots) { diff --git a/test/unittests/heap/cppgc/marker-unittest.cc b/test/unittests/heap/cppgc/marker-unittest.cc index 362d1ae5ff..0b8a320bd3 100644 --- a/test/unittests/heap/cppgc/marker-unittest.cc +++ b/test/unittests/heap/cppgc/marker-unittest.cc @@ -5,6 +5,7 @@ #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" @@ -245,5 +246,20 @@ TEST_F(MarkerTest, InConstructionObjectIsEventuallyMarkedNonEmptyStack) { }); } +TEST_F(MarkerTest, SentinelNotClearedOnWeakPersistentHandling) { + Marker marker(Heap::From(GetHeap())->AsBase()); + Persistent root = MakeGarbageCollected(GetAllocationHandle()); + auto* tmp = MakeGarbageCollected(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 diff --git a/test/unittests/heap/cppgc/persistent-unittest.cc b/test/unittests/heap/cppgc/persistent-unittest.cc index 94d3351983..11c1bd02d1 100644 --- a/test/unittests/heap/cppgc/persistent-unittest.cc +++ b/test/unittests/heap/cppgc/persistent-unittest.cc @@ -8,6 +8,7 @@ #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" @@ -70,6 +71,7 @@ class RootVisitor final : public VisitorBase { private: std::vector> weak_callbacks_; }; + class PersistentTest : public testing::TestSupportingAllocationOnly {}; } // namespace @@ -617,6 +619,25 @@ TEST_F(PersistentTest, TraceWeak) { EXPECT_EQ(0u, GetRegion(heap).NodesInUse()); } +TEST_F(PersistentTest, ClearOnHeapDestruction) { + Persistent persistent; + WeakPersistent weak_persistent; + + // Create another heap that can be destroyed during the test. + auto heap = Heap::Create(GetPlatformHandle()); + persistent = MakeGarbageCollected(heap->GetAllocationHandle()); + weak_persistent = MakeGarbageCollected(heap->GetAllocationHandle()); + const Persistent persistent_sentinel(kSentinelPointer); + const WeakPersistent 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(GetAllocationHandle()); diff --git a/test/unittests/heap/cppgc/tests.h b/test/unittests/heap/cppgc/tests.h index 5f861a696c..d6e501d525 100644 --- a/test/unittests/heap/cppgc/tests.h +++ b/test/unittests/heap/cppgc/tests.h @@ -22,6 +22,8 @@ class TestWithPlatform : public ::testing::Test { TestPlatform& GetPlatform() const { return *platform_; } + std::shared_ptr GetPlatformHandle() const { return platform_; } + protected: static std::shared_ptr platform_; };