From 95424ce937cfd6eebcd6f4c3a55c06fd3a380683 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Thu, 2 Jan 2025 14:30:41 -0500 Subject: [PATCH 01/10] Only show the current thread while the GIL is disabled. --- Modules/faulthandler.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/Modules/faulthandler.c b/Modules/faulthandler.c index b62362f277797e..93ace0d7a7d2cc 100644 --- a/Modules/faulthandler.c +++ b/Modules/faulthandler.c @@ -1,4 +1,5 @@ #include "Python.h" +#include "pycore_ceval.h" // _PyEval_IsGILEnabled #include "pycore_initconfig.h" // _PyStatus_ERR #include "pycore_pyerrors.h" // _Py_DumpExtensionModules #include "pycore_pystate.h" // _PyThreadState_GET() @@ -27,6 +28,8 @@ # include // getauxval() #endif +/* Sentinel to ignore all_threads on free-threading */ +#define FT_IGNORE_ALL_THREADS 2 /* Allocate at maximum 100 MiB of the stack to raise the stack overflow */ #define STACK_OVERFLOW_MAX_SIZE (100 * 1024 * 1024) @@ -201,10 +204,14 @@ faulthandler_dump_traceback(int fd, int all_threads, PyGILState_GetThisThreadState(). */ PyThreadState *tstate = PyGILState_GetThisThreadState(); - if (all_threads) { + if (all_threads == 1) { (void)_Py_DumpTracebackThreads(fd, NULL, tstate); } else { + if (all_threads == FT_IGNORE_ALL_THREADS) + { + PUTS(fd, "\n"); + } if (tstate != NULL) _Py_DumpTraceback(fd, tstate); } @@ -266,6 +273,18 @@ faulthandler_disable_fatal_handler(fault_handler_t *handler) #endif } +static int +deduce_all_threads(void) +{ +#ifndef Py_GIL_DISABLED + return fatal_error.all_threads; +#else + /* In theory, it's safe to dump all threads if the GIL is enabled */ + return _PyEval_IsGILEnabled(_PyThreadState_GET()) + ? fatal_error.all_threads + : FT_IGNORE_ALL_THREADS; +#endif +} /* Handler for SIGSEGV, SIGFPE, SIGABRT, SIGBUS and SIGILL signals. @@ -320,7 +339,7 @@ faulthandler_fatal_error(int signum) PUTS(fd, "\n\n"); } - faulthandler_dump_traceback(fd, fatal_error.all_threads, + faulthandler_dump_traceback(fd, deduce_all_threads(), fatal_error.interp); _Py_DumpExtensionModules(fd, fatal_error.interp); @@ -396,7 +415,7 @@ faulthandler_exc_handler(struct _EXCEPTION_POINTERS *exc_info) } } - faulthandler_dump_traceback(fd, fatal_error.all_threads, + faulthandler_dump_traceback(fd, deduce_all_threads(), fatal_error.interp); /* call the next exception handler */ From 8d35bb63a83d136009ba2fb33fc1d1639c05826f Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Thu, 2 Jan 2025 14:34:29 -0500 Subject: [PATCH 02/10] Change the message. --- Modules/faulthandler.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/faulthandler.c b/Modules/faulthandler.c index 93ace0d7a7d2cc..bf0ad911e8238c 100644 --- a/Modules/faulthandler.c +++ b/Modules/faulthandler.c @@ -210,7 +210,7 @@ faulthandler_dump_traceback(int fd, int all_threads, else { if (all_threads == FT_IGNORE_ALL_THREADS) { - PUTS(fd, "\n"); + PUTS(fd, "\n"); } if (tstate != NULL) _Py_DumpTraceback(fd, tstate); From aadadde0972f0252d18204ec8ee1b5152894ae01 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Thu, 2 Jan 2025 14:53:31 -0500 Subject: [PATCH 03/10] Fix tests. --- Lib/test/test_faulthandler.py | 4 +++- Modules/faulthandler.c | 11 ++++++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_faulthandler.py b/Lib/test/test_faulthandler.py index 60815be96e14eb..ceb0ed4fa30770 100644 --- a/Lib/test/test_faulthandler.py +++ b/Lib/test/test_faulthandler.py @@ -100,7 +100,7 @@ def check_error(self, code, lineno, fatal_error, *, Raise an error if the output doesn't match the expected format. """ - if all_threads: + if all_threads and sys._is_gil_enabled(): if know_current_thread: header = 'Current thread 0x[0-9a-f]+' else: @@ -111,6 +111,8 @@ def check_error(self, code, lineno, fatal_error, *, if py_fatal_error: regex.append("Python runtime state: initialized") regex.append('') + if not sys._is_gil_enabled(): + regex.append("") regex.append(fr'{header} \(most recent call first\):') if garbage_collecting: regex.append(' Garbage-collecting') diff --git a/Modules/faulthandler.c b/Modules/faulthandler.c index bf0ad911e8238c..530ed51155acf0 100644 --- a/Modules/faulthandler.c +++ b/Modules/faulthandler.c @@ -279,8 +279,17 @@ deduce_all_threads(void) #ifndef Py_GIL_DISABLED return fatal_error.all_threads; #else + if (fatal_error.all_threads == 0) { + return 0; + } + // We can't use _PyThreadState_GET + PyThreadState *tstate = PyGILState_GetThisThreadState(); + if (tstate == NULL) + { + return 0; + } /* In theory, it's safe to dump all threads if the GIL is enabled */ - return _PyEval_IsGILEnabled(_PyThreadState_GET()) + return _PyEval_IsGILEnabled(tstate) ? fatal_error.all_threads : FT_IGNORE_ALL_THREADS; #endif From 170aa2a7619d15a52a053e44476463396fa4f411 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Thu, 2 Jan 2025 15:17:47 -0500 Subject: [PATCH 04/10] Fix tests. --- Lib/test/test_faulthandler.py | 10 ++++++++-- Modules/faulthandler.c | 8 +++++++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_faulthandler.py b/Lib/test/test_faulthandler.py index ceb0ed4fa30770..e3f6b280dbedcf 100644 --- a/Lib/test/test_faulthandler.py +++ b/Lib/test/test_faulthandler.py @@ -100,7 +100,13 @@ def check_error(self, code, lineno, fatal_error, *, Raise an error if the output doesn't match the expected format. """ - if all_threads and sys._is_gil_enabled(): + all_threads_disabled = ( + (not py_fatal_error) + and all_threads + and (not sys._is_gil_enabled()) + and (not garbage_collecting) + ) + if all_threads and not all_threads_disabled: if know_current_thread: header = 'Current thread 0x[0-9a-f]+' else: @@ -111,7 +117,7 @@ def check_error(self, code, lineno, fatal_error, *, if py_fatal_error: regex.append("Python runtime state: initialized") regex.append('') - if not sys._is_gil_enabled(): + if all_threads_disabled: regex.append("") regex.append(fr'{header} \(most recent call first\):') if garbage_collecting: diff --git a/Modules/faulthandler.c b/Modules/faulthandler.c index 530ed51155acf0..f403237606f302 100644 --- a/Modules/faulthandler.c +++ b/Modules/faulthandler.c @@ -282,12 +282,18 @@ deduce_all_threads(void) if (fatal_error.all_threads == 0) { return 0; } - // We can't use _PyThreadState_GET + // We can't use _PyThreadState_GET, so use the stored GILstate one PyThreadState *tstate = PyGILState_GetThisThreadState(); if (tstate == NULL) { return 0; } + if (tstate->interp->gc.collecting) + { + // Yay! All threads are paused, it's safe to access them. + return 1; + } + /* In theory, it's safe to dump all threads if the GIL is enabled */ return _PyEval_IsGILEnabled(tstate) ? fatal_error.all_threads From ed232334d9b04f9c695e0bd061317da14b2d6da3 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Thu, 2 Jan 2025 15:19:13 -0500 Subject: [PATCH 05/10] Add documentation note. --- Doc/library/faulthandler.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Doc/library/faulthandler.rst b/Doc/library/faulthandler.rst index 4067d7912b88b2..ba808694dc94cc 100644 --- a/Doc/library/faulthandler.rst +++ b/Doc/library/faulthandler.rst @@ -91,6 +91,10 @@ Fault handler state The dump now mentions if a garbage collector collection is running if *all_threads* is true. + .. versionchanged:: next + Only the current thread is dumped if the :term:`GIL` is disabled and + other threads are running. + .. function:: disable() Disable the fault handler: uninstall the signal handlers installed by From 5622f420d4e79d80e1fcbe6e18081870c07baa00 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Thu, 2 Jan 2025 15:20:21 -0500 Subject: [PATCH 06/10] Add blurb --- .../next/Library/2025-01-02-15-20-17.gh-issue-128400.UMiG4f.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2025-01-02-15-20-17.gh-issue-128400.UMiG4f.rst diff --git a/Misc/NEWS.d/next/Library/2025-01-02-15-20-17.gh-issue-128400.UMiG4f.rst b/Misc/NEWS.d/next/Library/2025-01-02-15-20-17.gh-issue-128400.UMiG4f.rst new file mode 100644 index 00000000000000..f9d5f84224c8dc --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-01-02-15-20-17.gh-issue-128400.UMiG4f.rst @@ -0,0 +1,2 @@ +Only show the current thread in :mod:`faulthandler` on the :term:`free +threaded ` build to prevent races. From 60895774fa74d7e1d04c96cd16fcaf2cf8399641 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Thu, 2 Jan 2025 16:19:05 -0500 Subject: [PATCH 07/10] Remove garbage collector shenanigans. --- Lib/test/test_faulthandler.py | 3 +-- Modules/faulthandler.c | 5 ----- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/Lib/test/test_faulthandler.py b/Lib/test/test_faulthandler.py index e3f6b280dbedcf..a695ac672013af 100644 --- a/Lib/test/test_faulthandler.py +++ b/Lib/test/test_faulthandler.py @@ -104,7 +104,6 @@ def check_error(self, code, lineno, fatal_error, *, (not py_fatal_error) and all_threads and (not sys._is_gil_enabled()) - and (not garbage_collecting) ) if all_threads and not all_threads_disabled: if know_current_thread: @@ -120,7 +119,7 @@ def check_error(self, code, lineno, fatal_error, *, if all_threads_disabled: regex.append("") regex.append(fr'{header} \(most recent call first\):') - if garbage_collecting: + if garbage_collecting and not all_threads_disabled: regex.append(' Garbage-collecting') regex.append(fr' File "", line {lineno} in {function}') regex = '\n'.join(regex) diff --git a/Modules/faulthandler.c b/Modules/faulthandler.c index f403237606f302..6def4376b89d68 100644 --- a/Modules/faulthandler.c +++ b/Modules/faulthandler.c @@ -288,11 +288,6 @@ deduce_all_threads(void) { return 0; } - if (tstate->interp->gc.collecting) - { - // Yay! All threads are paused, it's safe to access them. - return 1; - } /* In theory, it's safe to dump all threads if the GIL is enabled */ return _PyEval_IsGILEnabled(tstate) From b710acbb8ff764b8e98ba09fec4b1ad200fd3297 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Thu, 2 Jan 2025 16:19:57 -0500 Subject: [PATCH 08/10] Be more clear in the docs. --- Doc/library/faulthandler.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Doc/library/faulthandler.rst b/Doc/library/faulthandler.rst index ba808694dc94cc..b81da4af3cff58 100644 --- a/Doc/library/faulthandler.rst +++ b/Doc/library/faulthandler.rst @@ -92,8 +92,8 @@ Fault handler state if *all_threads* is true. .. versionchanged:: next - Only the current thread is dumped if the :term:`GIL` is disabled and - other threads are running. + Only the current thread is dumped if the :term:`GIL` is disabled to + prevent the risk of data races. .. function:: disable() From b1677f3aa9b6224b009d3717d9a973a7fd1da84e Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Thu, 2 Jan 2025 17:41:58 -0500 Subject: [PATCH 09/10] Update faulthandler.c Co-authored-by: Sam Gross --- Modules/faulthandler.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Modules/faulthandler.c b/Modules/faulthandler.c index 6def4376b89d68..e8b79703cbf937 100644 --- a/Modules/faulthandler.c +++ b/Modules/faulthandler.c @@ -208,8 +208,7 @@ faulthandler_dump_traceback(int fd, int all_threads, (void)_Py_DumpTracebackThreads(fd, NULL, tstate); } else { - if (all_threads == FT_IGNORE_ALL_THREADS) - { + if (all_threads == FT_IGNORE_ALL_THREADS) { PUTS(fd, "\n"); } if (tstate != NULL) From 11bb63809e87443825541b5726bad8d589eb38d1 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Thu, 2 Jan 2025 17:42:03 -0500 Subject: [PATCH 10/10] Update faulthandler.c Co-authored-by: Sam Gross --- Modules/faulthandler.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Modules/faulthandler.c b/Modules/faulthandler.c index e8b79703cbf937..36d34230fa9e6d 100644 --- a/Modules/faulthandler.c +++ b/Modules/faulthandler.c @@ -283,8 +283,7 @@ deduce_all_threads(void) } // We can't use _PyThreadState_GET, so use the stored GILstate one PyThreadState *tstate = PyGILState_GetThisThreadState(); - if (tstate == NULL) - { + if (tstate == NULL) { return 0; }