Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Doc/library/faulthandler.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 9 additions & 1 deletion Lib/test/test_faulthandler.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
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:
Expand All @@ -111,6 +117,8 @@ def check_error(self, code, lineno, fatal_error, *,
if py_fatal_error:
regex.append("Python runtime state: initialized")
regex.append('')
if all_threads_disabled:
regex.append("<Cannot show all threads while the GIL is disabled>")
regex.append(fr'{header} \(most recent call first\):')
if garbage_collecting:
regex.append(' Garbage-collecting')
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Only show the current thread in :mod:`faulthandler` on the :term:`free
threaded <free threading>` build to prevent races.
40 changes: 37 additions & 3 deletions Modules/faulthandler.c
Original file line number Diff line number Diff line change
@@ -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()
Expand Down Expand Up @@ -27,6 +28,8 @@
# include <sys/auxv.h> // 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)
Expand Down Expand Up @@ -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, "<Cannot show all threads while the GIL is disabled>\n");
}
if (tstate != NULL)
_Py_DumpTraceback(fd, tstate);
}
Expand Down Expand Up @@ -266,6 +273,33 @@ 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
if (fatal_error.all_threads == 0) {
return 0;
}
// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gc.collecting field doesn't necessarily imply that all threads are paused:

  • gc.collecting is set before the stop-the-world pause
  • The GC resumes threads while calling finalizers

You could check tstate->interp->stoptheworld.world_stopped and maybe also that tstate matches stoptheworld.requester. (The faulting thread may not be attached to the interpreter when it crashes.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fully convinced that this is worth the extra complexity or the extra chance of crashing during the faulthandler:

  • Most crashes probably won't occur during a stop-the-world pause and for those that do, the stack traces of other threads are even less likely to be relevant (because they're paused).
  • Even during a stop-the-world pause, looping over the linked list of thread states is not safe without a HEAD_LOCK(), which you can't do during a signal handler.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was on the fence about it too. I threw it in there because it seemed simple enough at first, but I think it's better to remove it too.

{
// 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
: FT_IGNORE_ALL_THREADS;
#endif
}

/* Handler for SIGSEGV, SIGFPE, SIGABRT, SIGBUS and SIGILL signals.

Expand Down Expand Up @@ -320,7 +354,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);
Expand Down Expand Up @@ -396,7 +430,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 */
Expand Down
Loading