From 12915685610999d7e2c93b296862753425d79978 Mon Sep 17 00:00:00 2001 From: maurycy <5383+maurycy@users.noreply.github.com> Date: Sat, 15 Aug 2026 10:48:34 +0200 Subject: [PATCH 1/2] update_seq --- Include/internal/pycore_interp_structs.h | 1 + ...-08-15-10-20-40.gh-issue-155811.knP-YB.rst | 3 +++ Modules/_remote_debugging/gc_stats.c | 24 +++++++++++++++++++ Python/gc.c | 9 +++++-- Python/gc_free_threading.c | 6 +++++ 5 files changed, 41 insertions(+), 2 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2026-08-15-10-20-40.gh-issue-155811.knP-YB.rst diff --git a/Include/internal/pycore_interp_structs.h b/Include/internal/pycore_interp_structs.h index 0623adce693d465..e370f52082939f5 100644 --- a/Include/internal/pycore_interp_structs.h +++ b/Include/internal/pycore_interp_structs.h @@ -219,6 +219,7 @@ struct gc_old_stats_buffer { struct gc_stats { struct gc_young_stats_buffer young; struct gc_old_stats_buffer old[2]; + uint32_t update_seq; }; struct _gc_runtime_state { diff --git a/Misc/NEWS.d/next/Library/2026-08-15-10-20-40.gh-issue-155811.knP-YB.rst b/Misc/NEWS.d/next/Library/2026-08-15-10-20-40.gh-issue-155811.knP-YB.rst new file mode 100644 index 000000000000000..2032fd74380db8b --- /dev/null +++ b/Misc/NEWS.d/next/Library/2026-08-15-10-20-40.gh-issue-155811.knP-YB.rst @@ -0,0 +1,3 @@ +Add a sequence counter to GC statistics to prevent :mod:`!_remote_debugging` +returning inconsistent snapshots caused by non-atomic reads. Patch by Maurycy +Pawłowski-Wieroński. diff --git a/Modules/_remote_debugging/gc_stats.c b/Modules/_remote_debugging/gc_stats.c index d5d05edb8ecf5ee..e325b8bbc6bdb4d 100644 --- a/Modules/_remote_debugging/gc_stats.c +++ b/Modules/_remote_debugging/gc_stats.c @@ -103,6 +103,16 @@ get_gc_stats_from_interpreter_state(RuntimeOffsets *offsets, } struct gc_stats stats; + uintptr_t sequence_address = gc_stats_addr + + offsetof(struct gc_stats, update_seq); + uint32_t before; + if (_Py_RemoteDebug_ReadRemoteMemory(&offsets->handle, + sequence_address, + sizeof(before), &before) < 0) { + set_exception_cause(offsets, PyExc_RuntimeError, + "Failed to read GC update sequence"); + return -1; + } if (_Py_RemoteDebug_ReadRemoteMemory(&offsets->handle, gc_stats_addr, sizeof(stats), @@ -111,6 +121,20 @@ get_gc_stats_from_interpreter_state(RuntimeOffsets *offsets, return -1; } + uint32_t after; + if (_Py_RemoteDebug_ReadRemoteMemory(&offsets->handle, + sequence_address, + sizeof(after), &after) < 0) { + set_exception_cause(offsets, PyExc_RuntimeError, + "Failed to read GC update sequence"); + return -1; + } + if (before != after || before != stats.update_seq || (after & 1)) { + PyErr_SetString(PyExc_RuntimeError, + "GC stats changed while being read; retry later"); + return -1; + } + if (read_gc_stats(&stats, iid, ctx->result, ctx->gc_stats_info_type) < 0) { set_exception_cause(offsets, PyExc_RuntimeError, "Failed to populate GC stats result"); diff --git a/Python/gc.c b/Python/gc.c index 201c621bcc3cb9b..cb09349b4d73383 100644 --- a/Python/gc.c +++ b/Python/gc.c @@ -1399,6 +1399,12 @@ gc_get_prev_stats(GCState *gcstate, int gen) static void add_stats(GCState *gcstate, int gen, struct gc_generation_stats *stats) { + struct gc_stats *generation_stats = gcstate->generation_stats; + uint32_t seq = _Py_atomic_load_uint32(&generation_stats->update_seq); + assert((seq & 1) == 0); + _Py_atomic_store_uint32(&generation_stats->update_seq, seq + 1); + _Py_atomic_fence_seq_cst(); + struct gc_generation_stats *prev_stats = gc_get_prev_stats(gcstate, gen); struct gc_generation_stats *cur_stats = gc_get_stats(gcstate, gen); @@ -1412,9 +1418,8 @@ add_stats(GCState *gcstate, int gen, struct gc_generation_stats *stats) cur_stats->duration += stats->duration; cur_stats->heap_size = stats->heap_size; - /* Publish ts_stop last so remote readers do not select a partially - updated stats record as the latest collection. */ cur_stats->ts_stop = stats->ts_stop; + _Py_atomic_store_uint32(&generation_stats->update_seq, seq + 2); } /* This is the main function. Read this to understand how the diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c index 99f1a1eb47e3ddc..2272a55c8f8319b 100644 --- a/Python/gc_free_threading.c +++ b/Python/gc_free_threading.c @@ -2283,6 +2283,11 @@ gc_collect_main(PyThreadState *tstate, int generation, _PyGC_Reason reason) /* Update stats. */ PyMutex_Lock(&gcstate->stats_mutex); + struct gc_stats *generation_stats = gcstate->generation_stats; + uint32_t seq = _Py_atomic_load_uint32(&generation_stats->update_seq); + assert((seq & 1) == 0); + _Py_atomic_store_uint32(&generation_stats->update_seq, seq + 1); + _Py_atomic_fence_seq_cst(); struct gc_generation_stats *stats = get_stats(gcstate, generation); stats->ts_start = start; stats->ts_stop = stop; @@ -2291,6 +2296,7 @@ gc_collect_main(PyThreadState *tstate, int generation, _PyGC_Reason reason) stats->uncollectable += n; stats->duration += duration; stats->candidates += state.candidates; + _Py_atomic_store_uint32(&generation_stats->update_seq, seq + 2); PyMutex_Unlock(&gcstate->stats_mutex); GC_STAT_ADD(generation, objects_collected, m); From 0cb2510028c2c87c271ceb2efecfcecfa1986c80 Mon Sep 17 00:00:00 2001 From: maurycy <5383+maurycy@users.noreply.github.com> Date: Sat, 15 Aug 2026 13:37:14 +0200 Subject: [PATCH 2/2] no need for XCHGL, MOVL is enough? --- Python/gc.c | 7 ++++--- Python/gc_free_threading.c | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/Python/gc.c b/Python/gc.c index cb09349b4d73383..bb20dae5a6543fa 100644 --- a/Python/gc.c +++ b/Python/gc.c @@ -1400,9 +1400,10 @@ static void add_stats(GCState *gcstate, int gen, struct gc_generation_stats *stats) { struct gc_stats *generation_stats = gcstate->generation_stats; - uint32_t seq = _Py_atomic_load_uint32(&generation_stats->update_seq); + uint32_t seq = _Py_atomic_load_uint32_relaxed(&generation_stats->update_seq); assert((seq & 1) == 0); - _Py_atomic_store_uint32(&generation_stats->update_seq, seq + 1); + /* Odd seq tells the reader that an update is in progress. */ + _Py_atomic_store_uint32_relaxed(&generation_stats->update_seq, seq + 1); _Py_atomic_fence_seq_cst(); struct gc_generation_stats *prev_stats = gc_get_prev_stats(gcstate, gen); @@ -1419,7 +1420,7 @@ add_stats(GCState *gcstate, int gen, struct gc_generation_stats *stats) cur_stats->duration += stats->duration; cur_stats->heap_size = stats->heap_size; cur_stats->ts_stop = stats->ts_stop; - _Py_atomic_store_uint32(&generation_stats->update_seq, seq + 2); + _Py_atomic_store_uint32_release(&generation_stats->update_seq, seq + 2); } /* This is the main function. Read this to understand how the diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c index 2272a55c8f8319b..5690aeb22495ac1 100644 --- a/Python/gc_free_threading.c +++ b/Python/gc_free_threading.c @@ -2284,9 +2284,10 @@ gc_collect_main(PyThreadState *tstate, int generation, _PyGC_Reason reason) /* Update stats. */ PyMutex_Lock(&gcstate->stats_mutex); struct gc_stats *generation_stats = gcstate->generation_stats; - uint32_t seq = _Py_atomic_load_uint32(&generation_stats->update_seq); + uint32_t seq = _Py_atomic_load_uint32_relaxed(&generation_stats->update_seq); assert((seq & 1) == 0); - _Py_atomic_store_uint32(&generation_stats->update_seq, seq + 1); + /* Odd seq tells the reader that an update is in progress. */ + _Py_atomic_store_uint32_relaxed(&generation_stats->update_seq, seq + 1); _Py_atomic_fence_seq_cst(); struct gc_generation_stats *stats = get_stats(gcstate, generation); stats->ts_start = start; @@ -2296,7 +2297,7 @@ gc_collect_main(PyThreadState *tstate, int generation, _PyGC_Reason reason) stats->uncollectable += n; stats->duration += duration; stats->candidates += state.candidates; - _Py_atomic_store_uint32(&generation_stats->update_seq, seq + 2); + _Py_atomic_store_uint32_release(&generation_stats->update_seq, seq + 2); PyMutex_Unlock(&gcstate->stats_mutex); GC_STAT_ADD(generation, objects_collected, m);