From facdf01ac6c4941e3e5a0a54154be0bf02071307 Mon Sep 17 00:00:00 2001 From: Shokunin <30949000+tetrisgm@users.noreply.github.com> Date: Tue, 13 Oct 2026 00:10:16 -0700 Subject: [PATCH] Overlay draw callback: unregister waits for in-flight draws (lifetime contract) Review (#599): who keeps the callback context alive until render-thread calls are quiescent. Draw calls now run inside an in-flight count, and set_draw_cb (replace or NULL) swaps the record and waits until no call that read the old one is running, then frees it. When it returns, the registrant may free ctx and its GL objects; it must unregister before destroying them or the context. A change from inside the callback does not wait. Documented in host_overlay.h. Tests: a reader thread drawing while the owner unregisters and frees (300 rounds; fails without the wait), and self-unregister from the callback. Co-Authored-By: Claude Opus 5.5 --- runtime/CMakeLists.txt | 2 + runtime/include/host_overlay.h | 10 ++++- runtime/src/host_overlay.c | 54 ++++++++++++++++++++++---- runtime/tests/test_host_overlay.c | 63 +++++++++++++++++++++++++++++++ 4 files changed, 120 insertions(+), 9 deletions(-) diff --git a/runtime/CMakeLists.txt b/runtime/CMakeLists.txt index 4e645678..69f7b9f8 100644 --- a/runtime/CMakeLists.txt +++ b/runtime/CMakeLists.txt @@ -795,6 +795,8 @@ if(BUILD_TESTING) add_test(NAME pad_external_input_test COMMAND pad_external_input_test) add_executable(host_overlay_test tests/test_host_overlay.c src/host_overlay.c) target_include_directories(host_overlay_test PRIVATE include src) + find_package(Threads REQUIRED) + target_link_libraries(host_overlay_test PRIVATE Threads::Threads) add_test(NAME host_overlay_test COMMAND host_overlay_test) add_executable(gte_register_access_test diff --git a/runtime/include/host_overlay.h b/runtime/include/host_overlay.h index b144c5a1..89e44d62 100644 --- a/runtime/include/host_overlay.h +++ b/runtime/include/host_overlay.h @@ -30,7 +30,15 @@ void psx_host_pause_set_refuse_probe(PsxHostPauseRefuseFn fn); * swap, with the drawable size in pixels. It runs on the thread that owns the * GL context (the render thread when it is on, else the main thread), with the * window framebuffer bound. The callback must leave GL state as it found it. - * Vulkan and the SDL renderer never call it. NULL unregisters. */ + * Vulkan and the SDL renderer never call it. NULL unregisters. + * + * Lifetime contract: the registrant owns ctx. psx_host_overlay_set_draw_cb + * (replace or NULL) returns only after any draw call that may still be using + * the previous fn/ctx has finished, so the registrant may free ctx and the GL + * objects it draws with right after it returns, and must keep them alive + * until then. Unregister before destroying those objects or the GL context + * (e.g. on renderer shutdown). Called from inside the callback it does not + * wait; the current call is then the last one with the old ctx. */ typedef void (*PsxHostOverlayDrawFn)(int width, int height, void *ctx); void psx_host_overlay_set_draw_cb(PsxHostOverlayDrawFn fn, void *ctx); int psx_host_overlay_has_draw(void); diff --git a/runtime/src/host_overlay.c b/runtime/src/host_overlay.c index 9374c11e..3b6f1223 100644 --- a/runtime/src/host_overlay.c +++ b/runtime/src/host_overlay.c @@ -4,6 +4,11 @@ #include #include #include +#if defined(_WIN32) +#include +#else +#include +#endif /* ---- P1: host pause ------------------------------------------------------ */ #define PSX_HOST_PAUSE_MAX 8 @@ -33,26 +38,50 @@ void psx_host_pause_set_refuse_probe(PsxHostPauseRefuseFn fn) { s_pause_refuse = /* ---- P2: overlay draw callback -------------------------------------------- */ /* Registered on the main thread, read on the render / present thread. The * function and its context are published together as one immutable record - * behind an atomic pointer (release on store, acquire on load), so a reader - * can never pair a live function with a stale or NULL context. Records are - * never freed: registration happens a handful of times per process, and a - * reader may still hold the previous record when it is replaced. */ + * behind an atomic pointer, so a reader never pairs a live function with a + * stale context. + * + * Lifetime: every call runs inside an in-flight count. Replacing or removing + * the callback swaps the record and then waits until no call that could have + * read the old record is still running, so when psx_host_overlay_set_draw_cb + * returns, the previous fn will not run again and its ctx (and the GL objects + * it draws with) may be freed by the caller. The old record is freed there + * too. A change made from inside the callback itself does not wait (it would + * wait for itself); that caller's own frame is the only one still using the + * old ctx and it returns before the next frame reads the new record. */ typedef struct { PsxHostOverlayDrawFn fn; void *ctx; } PsxHostOverlayDraw; static _Atomic(PsxHostOverlayDraw *) s_draw = NULL; +static atomic_int s_draw_inflight = 0; +#if defined(_MSC_VER) +static __declspec(thread) int s_in_draw = 0; +#else +static _Thread_local int s_in_draw = 0; +#endif + +static void draw_yield(void) { +#if defined(_WIN32) + Sleep(0); +#else + sched_yield(); +#endif +} void psx_host_overlay_set_draw_cb(PsxHostOverlayDrawFn fn, void *ctx) { - PsxHostOverlayDraw *rec = NULL; + PsxHostOverlayDraw *rec = NULL, *old; if (fn) { rec = (PsxHostOverlayDraw *)malloc(sizeof(*rec)); if (!rec) return; rec->fn = fn; rec->ctx = ctx; } - atomic_store_explicit(&s_draw, rec, memory_order_release); + old = atomic_exchange(&s_draw, rec); /* seq_cst: ordered with the readers' count */ + if (s_in_draw) return; /* from inside the callback: see above; old stays (rare) */ + while (atomic_load(&s_draw_inflight) > 0) draw_yield(); + free(old); } int psx_host_overlay_has_draw(void) { @@ -60,6 +89,15 @@ int psx_host_overlay_has_draw(void) { } void psx_host_overlay_draw(int width, int height) { - const PsxHostOverlayDraw *rec = atomic_load_explicit(&s_draw, memory_order_acquire); - if (rec && rec->fn && width > 0 && height > 0) rec->fn(width, height, rec->ctx); + PsxHostOverlayDraw *rec; + if (width <= 0 || height <= 0) return; + atomic_fetch_add(&s_draw_inflight, 1); /* seq_cst, before reading the record */ + rec = atomic_load(&s_draw); + if (rec && rec->fn) { + s_in_draw = 1; + rec->fn(width, height, rec->ctx); + s_in_draw = 0; + } + atomic_fetch_sub(&s_draw_inflight, 1); } + diff --git a/runtime/tests/test_host_overlay.c b/runtime/tests/test_host_overlay.c index 17d6bd6d..cf2ad265 100644 --- a/runtime/tests/test_host_overlay.c +++ b/runtime/tests/test_host_overlay.c @@ -2,6 +2,7 @@ #include "host_overlay.h" #include +#include #include static int g_fail = 0; @@ -50,7 +51,69 @@ static void test_draw(void) { CHECK(draws == 1 && !psx_host_overlay_has_draw()); } +/* Lifetime: once set_draw_cb(NULL) returns, the old callback never runs + * again, so its context can be freed. A reader thread draws continuously; + * the main thread unregisters and immediately marks the context dead. */ +#if !defined(_WIN32) || defined(__MINGW32__) +#include +#include +typedef struct { atomic_int alive; atomic_int calls; atomic_int used_dead; } LifeCtx; +static atomic_int g_stop; +static void life_draw(int w, int h, void* p) { + LifeCtx* c = (LifeCtx*)p; + (void)w; (void)h; + for (volatile int i = 0; i < 2000; ++i) {} /* a frame's worth of work */ + if (!atomic_load(&c->alive)) atomic_fetch_add(&c->used_dead, 1); + atomic_fetch_add(&c->calls, 1); +} +static void* life_reader(void* arg) { + (void)arg; + while (!atomic_load(&g_stop)) psx_host_overlay_draw(64, 64); + return NULL; +} +static void test_draw_lifetime(void) { + pthread_t t; + atomic_store(&g_stop, 0); + pthread_create(&t, NULL, life_reader, NULL); + int dead_uses = 0, total = 0; + for (int round = 0; round < 300; ++round) { + LifeCtx* c = (LifeCtx*)calloc(1, sizeof(*c)); + atomic_store(&c->alive, 1); + psx_host_overlay_set_draw_cb(life_draw, c); + while (atomic_load(&c->calls) < 2) {} + psx_host_overlay_set_draw_cb(NULL, NULL); /* must wait for the reader */ + atomic_store(&c->alive, 0); /* the owner tears down */ + for (volatile int i = 0; i < 20000; ++i) {} + dead_uses += atomic_load(&c->used_dead); + total += atomic_load(&c->calls); + free(c); + } + atomic_store(&g_stop, 1); + pthread_join(t, NULL); + CHECK(total > 0); + CHECK(dead_uses == 0); +} +#else +static void test_draw_lifetime(void) {} +#endif + +/* Changing the callback from inside it must not deadlock. */ +static void self_unregister(int w, int h, void* p) { + (void)w; (void)h; + ++*(int*)p; + psx_host_overlay_set_draw_cb(NULL, NULL); +} +static void test_draw_reentrant(void) { + int n = 0; + psx_host_overlay_set_draw_cb(self_unregister, &n); + psx_host_overlay_draw(64, 64); + psx_host_overlay_draw(64, 64); + CHECK(n == 1 && !psx_host_overlay_has_draw()); +} + int main(void) { + test_draw_lifetime(); + test_draw_reentrant(); test_draw(); test_pause(); printf("host_overlay_test: %s\n", g_fail ? "FAIL" : "ok");