[tonghuaroot]

In a free-threading build, gc.set_debug() stored gcstate->debug with a
plain write while gc.get_debug() and the collector read it without
synchronisation. gc.set_debug() takes no lock and, unlike
gc_set_threshold_impl(), does not run under _PyEval_StopTheWorld(), so
ThreadSanitizer reports a write/write race on gcstate->debug.

This accesses the flag with FT_ATOMIC_STORE_INT_RELAXED /
FT_ATOMIC_LOAD_INT_RELAXED in gc_set_debug_impl() / gc_get_debug_impl()
and in the collector reads in Python/gc_free_threading.c. Four of those
collector reads run outside stop-the-world (in delete_garbage(),
handle_legacy_finalizers(), and two stats reads in gc_collect_main()) and
race the write directly; the read in gc_collect_internal() runs under
stop-the-world and is made atomic only for consistency. Relaxed ordering is
correct for an independent int flag, matching how gcstate->enabled and
sys dlopenflags (gh-151644) are already handled.

Python/gc.c (the default GIL build) is left unchanged: it has no concurrent
access, and FT_ATOMIC_* compiles to a plain load/store there anyway.

Verified with a --with-thread-sanitizer --disable-gil build stressing
concurrent gc.set_debug() / gc.get_debug() plus a thread churning cyclic
garbage:

Before: WARNING: ThreadSanitizer: data race ... Write of size 4 ... gc_set_debug (2 reports)
After:  0 data races

A regression test is added under Lib/test/test_free_threading/.

• Issue: Data race on the GC debug flag (gc.set_debug/get_debug) in free-threading builds #153014

[sergey-miryanov]

cc @nascheme as a GC-FT expert

[nascheme]

This looks worth fixing. Some suggested improvements:

• Use threading_helper.run_concurrently()
• Remove the thread that runs GC collections. I don't think that's useful since it stops-the-world.
• I'd use one writer and one reader. It would be enough to trigger the TSAN warnings.
• If gc->debug is read more than once inside a function, read it into a local variable.

[github-actions]

This PR is stale because it has been open for 90 days with no activity.

[tonghuaroot]

Done: gcstate->debug is now read once into a local in the four collector functions, and the test is one gc.set_debug() writer racing one gc.get_debug() reader via run_concurrently() (no collector thread). Verified under --disable-gil --with-thread-sanitizer: TSAN flags the race without the fix and is clean with it. Also merged main to clear the conflict.

I have made the requested changes; please review again

[bedevere-app]

Thanks for making the requested changes!

: please review the changes made to this pull request.

[nascheme]

Hi tonghuaroot, thanks for making the changes requested. This looks good to merge now. After reconsidering the unit test, I think it's better we just remove it (I pushed a change). In terms of CI cost vs what this test buys us, I think it's not worth it. This race is not very serious. Fixing the code so we don't get a TSAN warning is good. Sorry for asking you to fix the test on the first review, normally we do want unit tests when bugs are fixed.

[tonghuaroot]

looks like this PR needs your approval @nascheme

[StanFromIreland]

@nascheme you'll have to approve the PR for the bot to change the label, or change it manually.

[nascheme]

LGTM!