Fix root being possibly uncounted in check_gc_root

CMakeLists.txt

@@ -36,8 +36,8 @@ if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME)
   include(CTest)
 endif()
 
-add_compile_options(-fsanitize=address,leak,undefined)
+# add_compile_options(-fsanitize=address,leak,undefined)
-add_link_options(-fsanitize=address,leak,undefined)
+# add_link_options(-fsanitize=address,leak,undefined)
 
 configure_file(include/refcount/config.h.in include/refcount/config.h @ONLY)
 

include/refcount/config.h.in

@@ -3,10 +3,4 @@
 
 #cmakedefine REFCOUNT_HAS_THREADS
 
-#if defined(REFCOUNT_HAS_THREADS) \
-    && (!defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L)
-#    error \
-        "RefCount needs to be compiled with at least C11 for thread support to work."
-#endif
-
 #endif

include/refcount/refcount.h

@@ -51,6 +51,8 @@ typedef void (*refcount_destroy_callback_t)(void *obj, void *user_data);
  */
 typedef struct RefcountContext {
     size_t entry_offset; //!< Offset to the #RefcountEntry member.
+    void *void_val; //!< Value returned by #refcount_context_unref when a
+                    //!< reference count falls to 0.
     refcount_held_refs_callback_t
         held_refs_callback; //!< Callback to list an object's held references.
     refcount_destroy_callback_t
@@ -66,13 +68,16 @@ typedef struct RefcountContext {
 #ifdef REFCOUNT_HAS_THREADS
     mtx_t so_mtx; //<! Mutex protecting static_objects.
     mtx_t gr_mtx; //<! Mutex protecting gc_roots.
+    _Atomic bool doing_gc;
+#else
+    bool doing_gc;
 #endif
 } RefcountContext;
 
 extern RefcountContext *refcount_default_context;
 
 RefcountContext *
-refcount_make_context(size_t entry_offset,
+refcount_make_context(size_t entry_offset, void *void_val,
                       refcount_held_refs_callback_t held_refs_callback,
                       refcount_destroy_callback_t destroy_callback,
                       void *user_data, const RefcountAllocator *alloc);
@@ -159,7 +164,20 @@ static inline bool refcount_context_is_static(const RefcountContext *ctx,
  */
 static inline uint64_t refcount_context_num_refs(const RefcountContext *ctx,
                                                  void *obj) {
-    return REFCOUNT_OBJECT_ENTRY(ctx, obj)->impl.counted.ref_count;
+#ifdef REFCOUNT_HAS_THREADS
+    if (!mtx_lock(&REFCOUNT_OBJECT_ENTRY(ctx, obj)->weak_ref->mtx)) {
+        // just try our best to estimate, if we are on a platform where 64bit
+        // stores are not atomic (or this is not aligned properly), this might
+        // race and give a bogus number
+        return REFCOUNT_OBJECT_ENTRY(ctx, obj)->impl.counted.ref_count;
+    }
+#endif
+    const uint64_t count =
+        REFCOUNT_OBJECT_ENTRY(ctx, obj)->impl.counted.ref_count;
+#ifdef REFCOUNT_HAS_THREADS
+    mtx_unlock(&REFCOUNT_OBJECT_ENTRY(ctx, obj)->weak_ref->mtx);
+#endif
+    return count;
 }
 
 bool refcount_context_init_obj(const RefcountContext *ctx, void *obj);
@@ -195,6 +213,20 @@ static inline void refcount_context_unref_as_callback(void *obj, void *ctx) {
 
 ptrdiff_t refcount_context_garbage_collect(RefcountContext *ctx);
 
+/**
+ * Test weather a give context is currently performing a GC operation.
+ * @param ctx The #RefcountContext
+ * @return True if the context is currently doing a GC operation, false
+ * otherwise
+ */
+static bool refcount_context_is_doing_gc(const RefcountContext *ctx) {
+#ifdef REFCOUNT_HAS_THREADS
+    return atomic_load_explicit(&ctx->doing_gc, memory_order_relaxed);
+#else
+    return ctx->doing_gc;
+#endif
+}
+
 RefcountWeakref *refcount_context_make_weakref(const RefcountContext *ctx,
                                                void *obj);
 
@@ -320,7 +352,7 @@ static inline void refcount_unref_as_callback(void *obj) {
 }
 
 /**
- * Same as #refcount_context_garbage_collect, but only operates on the globa
+ * Same as #refcount_context_garbage_collect, but only operates on the global
  * context.
  * @return The number of object's freed, or -1 if an error occurred
  */
@@ -328,6 +360,16 @@ static inline ptrdiff_t refcount_garbage_collect(void) {
     return refcount_context_garbage_collect(refcount_default_context);
 }
 
+/**
+ * Same as #refcount_context_is_doing_gc, but only operates on the global
+ * context.
+ * @return True if the context is currently doing a GC operation, false
+ * otherwise
+ */
+static inline bool refcount_is_doing_gc(void) {
+    return refcount_context_is_doing_gc(refcount_default_context);
+}
+
 /**
  * Same as #refcount_context_make_weakref, but operates on the global context.
  * @param obj The object to reference
@@ -414,6 +456,52 @@ static inline bool refcount_remove_destructor(void *obj, void *key) {
                                               key);
 }
 
+// Debug Functions
+/**
+ * Callback used from #refcount_debug_context_walk_tree.
+ * @param cur The current object in the walk
+ * @param trail The parent objects, up to the root object
+ * @return True if the walk should stop, false otherwise
+ */
+typedef bool (*refcount_debug_walk_callback_t)(void *cur,
+                                               const RefcountList *trail,
+                                               void *user_data);
+
+bool refcount_debug_context_walk_tree(const RefcountContext *ctx, void *obj,
+                                      refcount_debug_walk_callback_t callback,
+                                      void *user_data);
+
+/**
+ * Same as #refcount_debug_context_walk_tree, but only operates on the global
+ * context.
+ * @param obj The root object
+ * @param callback The callback
+ * @param user_data Extra data to pass to the callback
+ * @return True if the walk ended early because a callback returned true, false
+ */
+static inline bool
+refcount_debug_walk_tree(void *obj, refcount_debug_walk_callback_t callback,
+                         void *user_data) {
+    return refcount_debug_context_walk_tree(refcount_default_context, obj,
+                                            callback, user_data);
+}
+
+uint64_t refcount_debug_context_count_object(const RefcountContext *ctx,
+                                             void *obj, void *target);
+
+/**
+ * Same as #refcount_debug_context_count_object, but only operates on the global
+ * context.
+ * @param obj The root object
+ * @param target The object to look for
+ * @return The number of times the target appeared in the reference tree of the
+ * root
+ */
+static inline uint64_t refcount_debug_count_object(void *obj, void *target) {
+    return refcount_debug_context_count_object(refcount_default_context, obj,
+                                               target);
+}
+
 #ifdef __cplusplus
 }
 #endif

src/refcount.c

@@ -26,19 +26,24 @@
         (atomic_fetch_sub_explicit(obj, 1, memory_order_relaxed))
 #    define lock_entry_mtx(obj)   (lock_mutex(&obj->weak_ref->mtx))
 #    define unlock_entry_mtx(obj) (unlock_mutex(&obj->weak_ref->mtx))
+#    define store_flag_maybe_atomic(obj, value) \
+        (atomic_store_explicit(obj, value, memory_order_relaxed))
 #else
-#    define lock_mutex(m)          true
+#    define lock_mutex(m)                       true
-#    define unlock_mutex(m)        true
+#    define unlock_mutex(m)                     true
-#    define store_wr_count(obj, c) (*(obj) = (c))
+#    define store_wr_count(obj, c)              (*(obj) = (c))
-#    define inc_wr_count(obj)      ((*(obj))++)
+#    define inc_wr_count(obj)                   ((*(obj))++)
-#    define dec_wr_count(obj)      ((*(obj))--)
+#    define dec_wr_count(obj)                   ((*(obj))--)
-#    define lock_entry_mtx(obj)    true
+#    define lock_entry_mtx(obj)                 true
-#    define unlock_entry_mtx(obj)  true
+#    define unlock_entry_mtx(obj)               true
+#    define store_flag_maybe_atomic(obj, value) (*(obj) = (value))
 #endif
 
 /**
  * Default context to use for functions that do not take a context. You must
  * initialize this before use.
+ *
+ * > The default value of this is NULL.
  */
 RefcountContext *refcount_default_context = NULL;
 
@@ -54,7 +59,7 @@ RefcountContext *refcount_default_context = NULL;
  * @return The new context, or NULL in the case of an error
  */
 RefcountContext *
-refcount_make_context(size_t entry_offset,
+refcount_make_context(size_t entry_offset, void *void_val,
                       refcount_held_refs_callback_t held_refs_callback,
                       refcount_destroy_callback_t destroy_callback,
                       void *user_data, const RefcountAllocator *alloc) {
@@ -77,6 +82,7 @@ refcount_make_context(size_t entry_offset,
     }
 #endif
     ctx->entry_offset = entry_offset;
+    ctx->void_val = void_val;
     ctx->held_refs_callback = held_refs_callback;
     ctx->destroy_callback = destroy_callback;
     ctx->user_data = user_data;
@@ -85,27 +91,37 @@ refcount_make_context(size_t entry_offset,
 
     ctx->static_objects = NULL;
     ctx->gc_roots = NULL;
+    ctx->doing_gc = false;
     return ctx;
 }
 
 /**
- * Cleanup a #RefcountContext and free any associated resources. This also
+ * Callback that deinits a static, but sets its static_entry field to NULL
- * causes all remaining references, both static and non-static, to be dropped
+ * first. Used in #refcount_context_destroy.
- * and the context's destroy callback to be called on each object.
+ */
+static void deinit_static_for_context_destroy(void *obj, void *ctx_raw) {
+    RefcountContext *ctx = ctx_raw;
+    ENTRY->impl.static_entry = NULL;
+    refcount_context_deinit_static(ctx, obj);
+}
+
+/**
+ * Cleanup a #RefcountContext and free any associated resources. This first
+ * frees all static objects, then runs the garbage collector.
  * @param ctx The #RefcountContext
  */
 void refcount_context_destroy(RefcountContext *ctx) {
-    refcount_list_free_with_data_full(
+    refcount_list_free_with_data_full(ctx->static_objects,
-        ctx->static_objects, refcount_context_deinit_static_as_callback, ctx,
+                                      deinit_static_for_context_destroy, ctx,
-        &ctx->alloc);
+                                      &ctx->alloc);
+
+    refcount_context_garbage_collect(ctx);
 
 #ifdef REFCOUNT_HAS_THREADS
     mtx_destroy(&ctx->so_mtx);
     mtx_destroy(&ctx->gr_mtx);
 #endif
 
-    refcount_context_garbage_collect(ctx);
-
     refcount_free(&ctx->alloc, ctx);
 }
 
@@ -235,7 +251,7 @@ end:
  * @param refs Where to store the refs
  * @return True on success
  */
-static inline bool obj_held_refs(RefcountContext *ctx, void *obj,
+static inline bool obj_held_refs(const RefcountContext *ctx, void *obj,
                                  RefcountList **refs) {
     if (ctx->held_refs_callback) {
         return ctx->held_refs_callback(obj, refs, ctx->user_data);
@@ -319,8 +335,11 @@ bool refcount_context_deinit_static(RefcountContext *ctx, void *obj) {
     ht_free(ENTRY->destructors);
     unref_weakref(ctx, ENTRY->weak_ref);
     unlock_entry_mtx(ENTRY);
-    ctx->static_objects = refcount_list_remove_full(
+    // this is set to null if we are destroying the context
-        ctx->static_objects, ENTRY->impl.static_entry, NULL, &ctx->alloc);
+    if (ENTRY->impl.static_entry) {
+        ctx->static_objects = refcount_list_remove_full(
+            ctx->static_objects, ENTRY->impl.static_entry, NULL, &ctx->alloc);
+    }
     refcount_list_free_with_data_full(
         held_refs, refcount_context_unref_as_callback, ctx, &ctx->alloc);
     success = true;
@@ -337,7 +356,7 @@ end:
  */
 void *refcount_context_ref(const RefcountContext *ctx, void *obj) {
     if (!obj) {
-        return NULL;
+        return ctx->void_val;
     }
     if (!lock_entry_mtx(ENTRY)) {
         return obj;
@@ -401,7 +420,7 @@ static void remove_gc_root(RefcountContext *ctx, void *obj) {
 static void *unref_to_queue(RefcountContext *ctx, void *obj,
                             RefcountList **queue) {
     if (!obj) {
-        return NULL;
+        return ctx->void_val;
     } else if (ENTRY->is_static) {
         return obj;
     }
@@ -419,7 +438,7 @@ static void *unref_to_queue(RefcountContext *ctx, void *obj,
             *queue = refcount_list_push_full(*queue, obj, &ctx->alloc);
         }
         unlock_entry_mtx(ENTRY);
-        return NULL;
+        return ctx->void_val;
     } else {
         --ENTRY->impl.counted.ref_count;
         track_gc_root(ctx, obj);
@@ -505,7 +524,7 @@ static void process_unref_queue(RefcountContext *ctx, RefcountList *queue,
  */
 void *refcount_context_unref(RefcountContext *ctx, void *obj) {
     if (!obj) {
-        return NULL;
+        return ctx->void_val;
     }
     RefcountList *queue = NULL;
     void *retval = unref_to_queue(ctx, obj, &queue);
@@ -629,14 +648,18 @@ static ptrdiff_t check_gc_root(RefcountContext *ctx, RefcountList **root_ptr) {
         *root_ptr = (*root_ptr)->next;
         return -1;
     }
-    size_t seen_objects = 0;
+    // initialize information for the root
+    ht_insert(
+        counts, root->data,
+        HT_STUFF(
+            REFCOUNT_OBJECT_ENTRY(ctx, root->data)->impl.counted.ref_count));
     size_t clear_objects = 0;
     // ignore allocation errors until I decide how to deal with them (in the far
     // future)
     while (queue) {
         void *obj = queue->data;
+        queue = refcount_list_pop_full(queue, NULL, &ctx->alloc);
         if (!obj || refcount_context_is_static(ctx, obj)) {
-            queue = refcount_list_pop_full(queue, NULL, &ctx->alloc);
             continue;
         }
         uintptr_t count;
@@ -644,17 +667,17 @@ static ptrdiff_t check_gc_root(RefcountContext *ctx, RefcountList **root_ptr) {
             count = HT_UUNSTUFF(ht_get(counts, obj));
         } else {
             count = REFCOUNT_OBJECT_ENTRY(ctx, obj)->impl.counted.ref_count;
-            ++seen_objects;
+            // don't recuse into objects multiple times
+            obj_held_refs(ctx, obj, &queue);
         }
-        queue = refcount_list_pop_full(queue, NULL, &ctx->alloc);
         if (count > 0) {
             ht_insert(counts, obj, HT_STUFF(--count));
             if (count == 0) {
                 ++clear_objects;
             }
-            obj_held_refs(ctx, obj, &queue);
         }
     }
+    size_t seen_objects = ht_count(counts);
     ptrdiff_t freed_count = 0;
     if (seen_objects == clear_objects
         && !call_destructors_for_gc(ctx, counts)) {
@@ -689,6 +712,7 @@ ptrdiff_t refcount_context_garbage_collect(RefcountContext *ctx) {
     if (!lock_mutex(&ctx->gr_mtx)) {
         return -1;
     }
+    store_flag_maybe_atomic(&ctx->doing_gc, true);
     ptrdiff_t total_cleared = 0;
     RefcountList *root = ctx->gc_roots;
     while (root) {
@@ -698,6 +722,7 @@ ptrdiff_t refcount_context_garbage_collect(RefcountContext *ctx) {
         }
         total_cleared += res;
     }
+    store_flag_maybe_atomic(&ctx->doing_gc, false);
     unlock_mutex(&ctx->gr_mtx);
     return total_cleared;
 }
@@ -823,3 +848,105 @@ bool refcount_context_remove_destructor(const RefcountContext *ctx, void *obj,
     unlock_entry_mtx(ENTRY);
     return success;
 }
+
+// Debug Functions
+static const HTTableFunctions DEBUG_SEEN_FNS = {
+    .destroy_key = NULL,
+    .destroy_value = NULL,
+    .equal = ht_intptr_equal_callback,
+    .hash = ht_intptr_hash_callback,
+    .user_data = NULL,
+};
+
+/**
+ * Walk the tree held references tree of a given object, calling a callback for
+ * each object found. The root object will also be called. This is for debug
+ * purposes only.
+ * @param ctx The #RefcountContext
+ * @param obj The root object
+ * @param callback The callback
+ * @param user_data Extra data to pass to the callback
+ * @return True if the walk ended early because a callback returned true, false
+ * otherwise
+ */
+bool refcount_debug_context_walk_tree(const RefcountContext *ctx, void *obj,
+                                      refcount_debug_walk_callback_t callback,
+                                      void *user_data) {
+    static int frame_start_mark, frame_end_mark;
+    if (!obj) {
+        return false;
+    }
+    RefcountList *queue = NULL;
+    queue = refcount_list_push_full(queue, obj, &ctx->alloc);
+    HTTable *seen = ht_new(&DEBUG_SEEN_FNS, &ctx->ht_alloc, NULL);
+    RefcountList *trail = NULL;
+    while (queue) {
+        void *cur = queue->data;
+        queue = refcount_list_pop_full(queue, NULL, &ctx->alloc);
+        if (cur == &frame_start_mark) {
+            void *to_add = queue->data;
+            trail = refcount_list_push_full(trail, to_add, &ctx->alloc);
+            queue = refcount_list_pop_full(queue, NULL, &ctx->alloc);
+        } else if (cur == &frame_end_mark) {
+            trail = refcount_list_pop_full(trail, NULL, &ctx->alloc);
+        } else {
+            if (callback) {
+                if (callback(cur, trail, user_data)) {
+                    goto early_return;
+                }
+            }
+            if (cur && !ht_has(seen, cur)) {
+                ht_insert(seen, cur, NULL);
+                queue = refcount_list_push_full(queue, &frame_end_mark,
+                                                &ctx->alloc);
+                obj_held_refs(ctx, cur, &queue);
+                queue = refcount_list_push_full(queue, cur, &ctx->alloc);
+                queue = refcount_list_push_full(queue, &frame_start_mark,
+                                                &ctx->alloc);
+            }
+        }
+    }
+    ht_free(seen);
+    return false;
+early_return:
+    ht_free(seen);
+    refcount_list_free_full(queue, NULL, &ctx->alloc);
+    refcount_list_free_full(trail, NULL, &ctx->alloc);
+    return true;
+}
+
+/**
+ * Count all instances of a target object by walking the references of some root
+ * object. This is for debug purposes only. The root is not included in the
+ * count (as in, if `obj == target`, it will not be counted).
+ * @param ctx The #RefcountContext
+ * @param obj The root object
+ * @param target The object to look for
+ * @return The number of times the target appeared in the reference tree of the
+ * root
+ */
+uint64_t refcount_debug_context_count_object(const RefcountContext *ctx,
+                                             void *obj, void *target) {
+    if (!obj) {
+        return 0;
+    }
+    RefcountList *queue = NULL;
+    obj_held_refs(ctx, obj, &queue);
+    uint64_t total_count = 0;
+    HTTable *seen = ht_new(&DEBUG_SEEN_FNS, &ctx->ht_alloc, NULL);
+    while (queue) {
+        void *cur = queue->data;
+        queue = refcount_list_pop_full(queue, NULL, &ctx->alloc);
+        // count NULL
+        if (cur == target) {
+            ++total_count;
+        }
+        // but don't try to descend into it
+        if (cur && !ht_has(seen, cur)) {
+            ht_insert(seen, cur, NULL);
+            obj_held_refs(ctx, cur, &queue);
+        }
+    }
+    ht_free(seen);
+    return total_count;
+}

test/test_refcount.c

@@ -26,7 +26,16 @@ bool held_refs_callback(void *a_raw, RefcountList **out, void *ignored) {
     return true;
 }
 
-void destroy_callback(void *a_raw, void *ignored) {
+struct ContextAndFlag {
+    const RefcountContext *ctx;
+    bool should_be_doing_gc;
+};
+
+void destroy_callback(void *a_raw, void *ctx_and_flag_raw) {
+    struct ContextAndFlag *ctx_and_flag = ctx_and_flag_raw;
+    if (ctx_and_flag->should_be_doing_gc) {
+        assert(refcount_context_is_doing_gc(ctx_and_flag->ctx));
+    }
     A *a = a_raw;
     counting_free(a->str);
     counting_free(a);
@@ -37,17 +46,39 @@ void reref_destructor(void *a, void *ctx_raw) {
     refcount_context_ref(ctx, a);
 }
 
-int main(int argc, const char **argv) {
+UNUSED bool print_tree_walk_callback(void *obj, const RefcountList *trail,
-    RefcountContext *c =
+                                     void *ignored) {
-        refcount_make_context(offsetof(A, refcount), held_refs_callback,
+    size_t len = refcount_list_length(trail);
-                              destroy_callback, NULL, &COUNTING_ALLOCATOR);
+    for (size_t i = 0; i < len; ++i) {
+        fprintf(stderr, " ");
+    }
+    fprintf(stderr, "- %p\n", obj);
+    return false;
+}
 
-    A static_a = {
+int main(int argc, const char **argv) {
-        .num = 0,
+    struct ContextAndFlag ctx_and_flag = {.should_be_doing_gc = false};
-        .str = counting_strdup("static"),
+
-        .next = make_a(c, 0, "in static"),
+    RefcountContext *c = refcount_make_context(
-    };
+        offsetof(A, refcount), NULL, held_refs_callback, destroy_callback,
-    refcount_context_init_static(c, &static_a);
+        &ctx_and_flag, &COUNTING_ALLOCATOR);
+
+    ctx_and_flag.ctx = c;
+
+#define STATIC_A(id)                            \
+    A static_a_##id = {                         \
+        .num = __LINE__,                        \
+        .str = counting_strdup("static " #id),  \
+        .next = make_a(c, 0, "in static " #id), \
+    };                                          \
+    refcount_context_init_static(c, &static_a_##id)
+    STATIC_A(1);
+    STATIC_A(2);
+    STATIC_A(3);
+    assert(refcount_debug_context_count_object(c, &static_a_1, static_a_1.next)
+           == 1);
+    assert(refcount_debug_context_count_object(c, &static_a_1, static_a_2.next)
+           == 0);
 
     A *a = make_a(c, 10, "Hello world\n");
     assert(!refcount_context_is_static(c, a));
@@ -107,6 +138,7 @@ int main(int argc, const char **argv) {
     first->next = a;
     a = first;
     assert(refcount_context_num_refs(c, a) == 1);
+    assert(refcount_debug_context_count_object(c, a, a) == 1);
 
     refcount_context_ref(c, a);
     refcount_context_unref(c, a);
@@ -127,7 +159,16 @@ int main(int argc, const char **argv) {
     assert(refcount_context_garbage_collect(c) == 0);
     assert(refcount_context_num_refs(c, a_with_destructor) == 1);
     assert(refcount_context_remove_destructor(c, a_with_destructor, &key));
+    assert(refcount_context_ref(c, a_with_destructor));
+    assert(refcount_context_garbage_collect(c) == 0);
+    assert(refcount_context_unref(c, a_with_destructor));
+    assert(!refcount_context_is_doing_gc(c));
+    /* refcount_debug_context_walk_tree(c, a_with_destructor, */
+    /*                                  print_tree_walk_callback, NULL); */
+    ctx_and_flag.should_be_doing_gc = true;
     assert(refcount_context_garbage_collect(c) == 26);
+    ctx_and_flag.should_be_doing_gc = false;
+    assert(!refcount_context_is_doing_gc(c));
 
     a = make_a(c, 0, "a");
 
@@ -170,10 +211,12 @@ int main(int argc, const char **argv) {
     assert(refcount_context_num_refs(c, a) == 1);
     assert(!refcount_context_unref(c, a));
 
-    refcount_context_deinit_static(c, &static_a);
+    refcount_context_deinit_static(c, &static_a_1);
-    counting_free(static_a.str);
+    counting_free(static_a_1.str);
 
     refcount_context_destroy(c);
+    counting_free(static_a_2.str);
+    counting_free(static_a_3.str);
 
     check_allocator_status();
     return 0;