From 75e8b239895ccea7853718b930c71608b0abab9a Mon Sep 17 00:00:00 2001 From: Michael Grier Date: Tue, 24 Feb 2026 14:14:57 -0500 Subject: [PATCH] Fix CAS bug, add tests --- .../arefc_ptr/include/m/arefc_ptr/arefc_ptr.h | 17 +- .../arefc_ptr/test/test_arefc_ptr.cpp | 523 +++++++++++++++++- 2 files changed, 532 insertions(+), 8 deletions(-) diff --git a/src/libraries/arefc_ptr/include/m/arefc_ptr/arefc_ptr.h b/src/libraries/arefc_ptr/include/m/arefc_ptr/arefc_ptr.h index b69eb670..c8be4181 100644 --- a/src/libraries/arefc_ptr/include/m/arefc_ptr/arefc_ptr.h +++ b/src/libraries/arefc_ptr/include/m/arefc_ptr/arefc_ptr.h @@ -602,6 +602,8 @@ namespace m T* old_e = e; // save a copy so we don't have to re-load + // Pre-increment d's refcount so that if the CAS succeeds, m_ptr holds + // a valid reference to d without a window where the refcount is zero. increment_ref(d); if (m_ptr.compare_exchange_strong(e, d, std::memory_order_acq_rel)) @@ -613,8 +615,12 @@ namespace m return true; } - // The compare_exchange "failed", so now we need to update - // "expected" to the new value. + // The CAS failed: m_ptr still holds its current value (now captured in `e`). + // `d` was pre-incremented above but was never stored in m_ptr, so we must + // undo that increment to avoid a permanent ref-count leak on `desired`. + decrement_ref(d); + + // Update `expected` to reflect the actual current value of m_ptr. expected.reset(e); return false; @@ -692,6 +698,13 @@ namespace m std::atomic m_ptr{nullptr}; + // All specializations of arefc_ptr are friends of each other so that the + // cross-type converting constructor and assignment operators can call the + // private addref() / put() members on a related specialization. + template + requires(arefc_ptr_requirements) + friend class arefc_ptr; + template requires(arefc_ptr_requirements) friend arefc_ptr diff --git a/src/libraries/arefc_ptr/test/test_arefc_ptr.cpp b/src/libraries/arefc_ptr/test/test_arefc_ptr.cpp index bf6cbbd2..72415624 100644 --- a/src/libraries/arefc_ptr/test/test_arefc_ptr.cpp +++ b/src/libraries/arefc_ptr/test/test_arefc_ptr.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -19,16 +20,64 @@ using namespace std::chrono_literals; using namespace std::string_literals; using namespace std::string_view_literals; -struct alignas(128) BiglyAlignedStruct +// ============================================================================ +// Test helpers +// ============================================================================ + +namespace { - int m_x; -}; + struct alignas(128) BiglyAlignedStruct + { + int m_x; + }; + + // Tracks how many live instances exist. Used to verify that constructors + // and destructors are called the right number of times. + struct LifetimeTracker + { + inline static int s_live_count = 0; + + int m_id; + + explicit LifetimeTracker(int id) : m_id{id} { ++s_live_count; } + ~LifetimeTracker() { --s_live_count; } + }; + + // Plain struct with no special alignment requirements (small_control_area path). + struct Plain + { + int value = 0; + }; + + // Struct with a constructor taking arguments. + struct WithArgs + { + int x; + int y; + explicit WithArgs(int x_, int y_) : x{x_}, y{y_} {} + }; + + // Empty base used to test to() type coercion. + // DerivedFromEmpty is standard layout because the only class in the + // hierarchy that has data members is DerivedFromEmpty itself + // (EmptyBase has none), satisfying the C++17 standard-layout rules. + struct EmptyBase + {}; + + struct DerivedFromEmpty : EmptyBase + { + int value; + }; + +} // namespace + +// ============================================================================ +// Legacy tests (kept as-is) +// ============================================================================ TEST(TestRefCount, First) { - // auto p = m::mmake_arefc("Hello there"); - - // + // placeholder — intentionally empty } TEST(TestRefCount, TryAlignedStruct) @@ -37,3 +86,465 @@ TEST(TestRefCount, TryAlignedStruct) EXPECT_EQ(p->m_x, 10); } + +// ============================================================================ +// Default construction — null state +// ============================================================================ + +TEST(AreFcPtr_Null, DefaultConstructIsNull) +{ + m::arefc_ptr p; + EXPECT_EQ(p.get(), nullptr); + EXPECT_FALSE(static_cast(p)); + EXPECT_TRUE(!p); +} + +// ============================================================================ +// Basic construction, operator bool / operator!, get() +// ============================================================================ + +TEST(AreFcPtr_Basic, MakePtrIsNonNull) +{ + auto p = m::mmake_arefc(); + EXPECT_NE(p.get(), nullptr); + EXPECT_TRUE(static_cast(p)); + EXPECT_FALSE(!p); +} + +TEST(AreFcPtr_Basic, ConstructWithArgs) +{ + auto p = m::mmake_arefc(3, 7); + EXPECT_EQ(p->x, 3); + EXPECT_EQ(p->y, 7); +} + +// ============================================================================ +// Dereference: operator* and operator-> +// ============================================================================ + +TEST(AreFcPtr_Deref, OperatorArrow) +{ + auto p = m::mmake_arefc(); + p->value = 42; + EXPECT_EQ(p->value, 42); +} + +TEST(AreFcPtr_Deref, OperatorStar) +{ + auto p = m::mmake_arefc(); + p->value = 99; + EXPECT_EQ((*p).value, 99); +} + +// ============================================================================ +// Lifetime / ref counting: destructor called exactly once +// ============================================================================ + +TEST(AreFcPtr_Lifetime, DestructorCalledOnce) +{ + LifetimeTracker::s_live_count = 0; + { + auto p1 = m::mmake_arefc(1); + EXPECT_EQ(LifetimeTracker::s_live_count, 1); + { + auto p2 = p1; // copy — refcount = 2 + EXPECT_EQ(LifetimeTracker::s_live_count, 1); // still one object + { + auto p3 = p2; // copy — refcount = 3 + EXPECT_EQ(LifetimeTracker::s_live_count, 1); + } // p3 drops — refcount = 2; object still alive + EXPECT_EQ(LifetimeTracker::s_live_count, 1); + } // p2 drops — refcount = 1; object still alive + EXPECT_EQ(LifetimeTracker::s_live_count, 1); + } // p1 drops — refcount = 0; destructor called + EXPECT_EQ(LifetimeTracker::s_live_count, 0); +} + +// ============================================================================ +// Copy construction +// ============================================================================ + +TEST(AreFcPtr_CopyConstruct, SharesObject) +{ + auto p1 = m::mmake_arefc(); + p1->value = 77; + auto p2 = p1; + // Both point to the same underlying object + EXPECT_EQ(p2.get(), p1.get()); + EXPECT_EQ(p2->value, 77); +} + +TEST(AreFcPtr_CopyConstruct, FromNull) +{ + m::arefc_ptr null; + auto p2 = null; + EXPECT_EQ(p2.get(), nullptr); +} + +TEST(AreFcPtr_CopyConstruct, TypeCoercingCtor) +{ + // Constructing arefc_ptr from arefc_ptr + // exercises the templated converting copy constructor. + auto derived = m::mmake_arefc(); + derived->value = 55; + m::arefc_ptr base(derived); + // Both should point to the same storage (standard layout, EBO applies) + EXPECT_EQ(base.get(), static_cast(derived.get())); + EXPECT_NE(base.get(), nullptr); +} + +// ============================================================================ +// Move construction +// ============================================================================ + +TEST(AreFcPtr_MoveConstruct, TransfersOwnership) +{ + auto p1 = m::mmake_arefc(); + p1->value = 7; + Plain* raw = p1.get(); + + auto p2 = std::move(p1); + + EXPECT_EQ(p2.get(), raw); + EXPECT_EQ(p1.get(), nullptr); // moved-from is null + EXPECT_EQ(p2->value, 7); +} + +TEST(AreFcPtr_MoveConstruct, FromNull) +{ + m::arefc_ptr null; + auto p2 = std::move(null); + EXPECT_EQ(p2.get(), nullptr); + EXPECT_EQ(null.get(), nullptr); +} + +TEST(AreFcPtr_MoveConstruct, NoExtraDestruction) +{ + // Moving should not change the live count — no copy of the object is made + // and no premature destruction occurs. + LifetimeTracker::s_live_count = 0; + { + auto p1 = m::mmake_arefc(1); + EXPECT_EQ(LifetimeTracker::s_live_count, 1); + auto p2 = std::move(p1); + EXPECT_EQ(LifetimeTracker::s_live_count, 1); + } + EXPECT_EQ(LifetimeTracker::s_live_count, 0); +} + +// ============================================================================ +// Copy assignment +// ============================================================================ + +TEST(AreFcPtr_CopyAssign, AssignNonNull) +{ + auto p1 = m::mmake_arefc(); + p1->value = 5; + Plain* raw = p1.get(); + + m::arefc_ptr p2; + p2 = p1; + EXPECT_EQ(p2.get(), raw); + EXPECT_EQ(p2->value, 5); +} + +TEST(AreFcPtr_CopyAssign, SelfAssign) +{ + auto p = m::mmake_arefc(); + p->value = 11; + Plain* raw = p.get(); + + // Self-assignment must be a no-op. + // Assign through a reference to suppress the clang -Wself-assign-overloaded + // diagnostic while still exercising the self-assignment code path. + auto& pref = p; + p = pref; + EXPECT_EQ(p.get(), raw); + EXPECT_EQ(p->value, 11); +} + +TEST(AreFcPtr_CopyAssign, OverwritesExisting) +{ + // Verify that the previously held object is released when p2 is overwritten. + LifetimeTracker::s_live_count = 0; + { + auto p1 = m::mmake_arefc(1); + auto p2 = m::mmake_arefc(2); + EXPECT_EQ(LifetimeTracker::s_live_count, 2); + p2 = p1; // p2 releases its old object; refcount of p1's object goes up + EXPECT_EQ(LifetimeTracker::s_live_count, 1); // old p2 object destroyed + EXPECT_EQ(p2.get(), p1.get()); + } + EXPECT_EQ(LifetimeTracker::s_live_count, 0); +} + +// ============================================================================ +// Move assignment +// ============================================================================ + +TEST(AreFcPtr_MoveAssign, TransfersOwnership) +{ + auto p1 = m::mmake_arefc(); + p1->value = 13; + Plain* raw = p1.get(); + + m::arefc_ptr p2; + p2 = std::move(p1); + + EXPECT_EQ(p2.get(), raw); + EXPECT_EQ(p2->value, 13); + EXPECT_EQ(p1.get(), nullptr); +} + +// ============================================================================ +// reset() +// ============================================================================ + +TEST(AreFcPtr_Reset, ToNullDestroysObject) +{ + LifetimeTracker::s_live_count = 0; + { + auto p = m::mmake_arefc(42); + EXPECT_EQ(LifetimeTracker::s_live_count, 1); + p.reset(); + // Object should be destroyed immediately — reset() drops the last ref. + EXPECT_EQ(p.get(), nullptr); + EXPECT_EQ(LifetimeTracker::s_live_count, 0); + } + EXPECT_EQ(LifetimeTracker::s_live_count, 0); +} + +TEST(AreFcPtr_Reset, ResetNullIsNoOp) +{ + m::arefc_ptr p; // null + p.reset(); // must not crash or assert + EXPECT_EQ(p.get(), nullptr); +} + +TEST(AreFcPtr_Reset, ResetDoesNotDestroyWhileOtherHolds) +{ + LifetimeTracker::s_live_count = 0; + { + auto p1 = m::mmake_arefc(1); + auto p2 = p1; + p1.reset(); // drops one ref; p2 still holds the object + EXPECT_EQ(LifetimeTracker::s_live_count, 1); + EXPECT_EQ(p1.get(), nullptr); + } // p2 drops here → refcount 0 → destroy + EXPECT_EQ(LifetimeTracker::s_live_count, 0); +} + +// ============================================================================ +// to() — type-coercing view +// ============================================================================ + +TEST(AreFcPtr_TypeCoercion, ToBaseGivesSameAddress) +{ + auto derived = m::mmake_arefc(); + derived->value = 77; + + m::arefc_ptr base = derived.to(); + + // Standard layout + EBO: base subobject is at offset 0, so pointers are equal. + EXPECT_EQ(base.get(), static_cast(derived.get())); + EXPECT_NE(base.get(), nullptr); +} + +TEST(AreFcPtr_TypeCoercion, ToBaseKeepsObjectAlive) +{ + LifetimeTracker::s_live_count = 0; + + // LifetimeTracker does not inherit from anything so we cannot use it here. + // Use DerivedFromEmpty instead and check liveness via a side-channel. + { + auto derived = m::mmake_arefc(); + { + auto base = derived.to(); + // Both point to the same object; dropping derived should keep it alive. + derived.reset(); + EXPECT_NE(base.get(), nullptr); + } // base drops here — object should be freed (no crash / asan) + } +} + +// ============================================================================ +// mmake_arefc_ex — extra bytes +// ============================================================================ + +TEST(AreFcPtr_MakeEx, FnIsCalledAndObjectIsAccessible) +{ + bool fn_called = false; + + auto p = m::mmake_arefc_ex( + 64, + [&fn_called](m::byte_span s, int v) -> Plain* { + fn_called = true; + EXPECT_GE(s.size(), sizeof(Plain)); + return ::new (s.data()) Plain{.value = v}; + }, + 42); + + EXPECT_TRUE(fn_called); + EXPECT_EQ(p->value, 42); +} + +// ============================================================================ +// Over-aligned type — exercises big_control_area path +// ============================================================================ + +TEST(AreFcPtr_OverAligned, AlignmentIsRespected) +{ + auto p = m::mmake_arefc(55); + EXPECT_EQ(p->m_x, 55); + // The returned pointer must satisfy the requested alignment. + EXPECT_EQ(reinterpret_cast(p.get()) % 128u, 0u); +} + +TEST(AreFcPtr_OverAligned, CopySharesObjectAndLifetime) +{ + auto p1 = m::mmake_arefc(100); + auto p2 = p1; + EXPECT_EQ(p2.get(), p1.get()); + EXPECT_EQ(p2->m_x, 100); + // Dropping p1 must not destroy the object while p2 still holds it. + p1.reset(); + EXPECT_EQ(p2->m_x, 100); +} + +// ============================================================================ +// compare_exchange_strong — success case +// ============================================================================ + +TEST(AreFcPtr_CAS, SuccessUpdatesTarget) +{ + auto p1 = m::mmake_arefc(); + p1->value = 1; + auto p2 = m::mmake_arefc(); + p2->value = 2; + + m::arefc_ptr target = p1; + m::arefc_ptr expected = p1; + + // target == expected, so CAS should swap target to p2. + bool ok = target.compare_exchange_strong(expected, p2); + + EXPECT_TRUE(ok); + EXPECT_EQ(target.get(), p2.get()); + // On success, `expected` is not modified. + EXPECT_EQ(expected.get(), p1.get()); +} + +TEST(AreFcPtr_CAS, SuccessNoRefLeak) +{ + // Verify that a successful CAS leaves every object with the correct + // final refcount — none should linger when their last holder is dropped. + LifetimeTracker::s_live_count = 0; + { + auto p1 = m::mmake_arefc(1); + auto p2 = m::mmake_arefc(2); + EXPECT_EQ(LifetimeTracker::s_live_count, 2); + + m::arefc_ptr target = p1; + m::arefc_ptr expected = p1; + + bool ok = target.compare_exchange_strong(expected, p2); + EXPECT_TRUE(ok); + + target.reset(); + expected.reset(); + EXPECT_EQ(LifetimeTracker::s_live_count, 2); // p1 and p2 still held by locals + } + EXPECT_EQ(LifetimeTracker::s_live_count, 0); +} + +// ============================================================================ +// compare_exchange_strong — failure case +// ============================================================================ + +TEST(AreFcPtr_CAS, FailureUpdatesExpected) +{ + auto p1 = m::mmake_arefc(); + p1->value = 1; + auto p2 = m::mmake_arefc(); + p2->value = 2; + auto p3 = m::mmake_arefc(); + p3->value = 3; + + m::arefc_ptr target = p3; // holds p3 + m::arefc_ptr expected = p1; // but we think it holds p1 — mismatch + + bool ok = target.compare_exchange_strong(expected, p2); + + EXPECT_FALSE(ok); + EXPECT_EQ(target.get(), p3.get()); // target unchanged + EXPECT_EQ(expected.get(), p3.get()); // expected updated to actual current value +} + +TEST(AreFcPtr_CAS, FailureNoRefLeakOnDesired) +{ + // This test exposes a ref-count leak bug that was present on the CAS failure + // path: increment_ref(desired) was called unconditionally before the CAS, + // but if the CAS failed, the increment was never balanced. + // + // After the fix (decrement_ref(d) on the failure path), `desired`'s refcount + // must return to exactly 1 (held only by the local `p2` variable) so that + // when `p2` drops at the end of the scope, the object is destroyed. + LifetimeTracker::s_live_count = 0; + { + auto p1 = m::mmake_arefc(1); + auto p2 = m::mmake_arefc(2); // this is `desired` + auto p3 = m::mmake_arefc(3); + EXPECT_EQ(LifetimeTracker::s_live_count, 3); + + m::arefc_ptr target = p3; + m::arefc_ptr expected = p1; + + bool ok = target.compare_exchange_strong(expected, p2); + EXPECT_FALSE(ok); + + // Drop the CAS temporaries so they hold no extra references. + target.reset(); + expected.reset(); + + // Still 3 live objects — p1, p2, p3 each held by one local variable. + EXPECT_EQ(LifetimeTracker::s_live_count, 3); + } + // All locals dropped — all 3 objects must be destroyed. + // If the ref-count leak is present, p2 will not be destroyed here and + // s_live_count will remain 1. + EXPECT_EQ(LifetimeTracker::s_live_count, 0); +} + +// ============================================================================ +// Thread safety — concurrent copies and drops must not corrupt the ref count +// ============================================================================ + +TEST(AreFcPtr_Threaded, ConcurrentCopyAndDrop) +{ + LifetimeTracker::s_live_count = 0; + { + auto shared = m::mmake_arefc(1); + + std::vector threads; + threads.reserve(8); + for (int i = 0; i < 8; ++i) + { + threads.emplace_back( + [&shared]() + { + // Each thread takes its own copy and holds it briefly. + auto local = shared; + std::this_thread::sleep_for(1ms); + // local drops here + }); + } + + for (auto& t : threads) + t.join(); + + // Only `shared` (in this scope) still holds the object. + EXPECT_EQ(LifetimeTracker::s_live_count, 1); + } + // `shared` drops — object destroyed. + EXPECT_EQ(LifetimeTracker::s_live_count, 0); +}