-
-
Notifications
You must be signed in to change notification settings - Fork 32k
gh-127604: Add C stack dumps to faulthandler
#128159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0ebdc69
0ccf9fb
52945e5
9bdd802
66f2641
0591013
64707a2
8a5b784
4f09358
e1aa619
6b515d3
c69ee9d
f08b1dd
dbb6d25
faf1a3e
ec832aa
524f167
dd08bcb
bda3dcd
a3564a5
f3fcea1
db97dd2
c17457f
d5f7d4b
e79e661
0c84f8a
8198997
0f670f0
079f186
892a085
896abd1
cab079e
1784071
3304e2a
aa97f24
50b4964
533b1db
58b3580
f62dac8
4feaf09
c95369f
95833e7
3e2701d
56127fa
b70bd43
7a070ab
e9c3d7c
195a539
9dd6c3b
ce9c39f
c344ad7
e899792
b136f71
2c381b9
52c0748
bd47026
07a20d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,13 @@ def temporary_filename(): | |
finally: | ||
os_helper.unlink(filename) | ||
|
||
|
||
ADDRESS_EXPR = "0x[0-9a-f]+" | ||
C_STACK_REGEX = [ | ||
r"Current thread's C stack trace \(most recent call first\):", | ||
fr'( Binary file ".+"(, at .*(\+|-){ADDRESS_EXPR})? \[{ADDRESS_EXPR}\])|(<.+>)' | ||
] | ||
|
||
class FaultHandlerTests(unittest.TestCase): | ||
|
||
def get_output(self, code, filename=None, fd=None): | ||
|
@@ -103,6 +110,7 @@ def check_error(self, code, lineno, fatal_error, *, | |
fd=None, know_current_thread=True, | ||
py_fatal_error=False, | ||
garbage_collecting=False, | ||
c_stack=True, | ||
ZeroIntensity marked this conversation as resolved.
Show resolved
Hide resolved
|
||
function='<module>'): | ||
""" | ||
Check that the fault handler for fatal errors is enabled and check the | ||
|
@@ -134,6 +142,8 @@ def check_error(self, code, lineno, fatal_error, *, | |
if garbage_collecting and not all_threads_disabled: | ||
regex.append(' Garbage-collecting') | ||
regex.append(fr' File "<string>", line {lineno} in {function}') | ||
if c_stack: | ||
regex.extend(C_STACK_REGEX) | ||
regex = '\n'.join(regex) | ||
|
||
if other_regex: | ||
|
@@ -950,5 +960,35 @@ def run(self): | |
_, exitcode = self.get_output(code) | ||
self.assertEqual(exitcode, 0) | ||
|
||
def check_c_stack(self, output): | ||
starting_line = output.pop(0) | ||
self.assertRegex(starting_line, C_STACK_REGEX[0]) | ||
self.assertGreater(len(output), 0) | ||
|
||
for line in output: | ||
with self.subTest(line=line): | ||
Comment on lines
+968
to
+969
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is totally unrelated, but many times have I been using a construction of the form:
I'm wondering if it makes sense to have a helper that would be equivalent to the above:
I'm pretty sure we can use this kind of pattern in the test directory quite a lot, and it can also be useful for other libs, but how about we add this kind of helper to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, that could be interesting. What happens when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hum, it doesn't matter? we're computing a product not a zip(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I'm misunderstanding it. Is: for a, b in self.cases(a=iter_a, b=iter_b)`:
print(a, b) supposed to be equivalent to: for a in iter_a:
for b in iter_b:
print(a, b) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. The rationale behind my suggestion is as follows: for a in A:
for b in B:
with self.subTest(a=a, b=b):
pass We're already having 3 levels of indentation. Counting the class indentation and method indentation we're just having 20 whitespaces before wer start writing real code...: class A:
def f(self):
for a in A:
for b in B:
with self.subTest(a=a, b=b):
pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In addition, if we're pretty short on the keywords passed to subTest() if we want to keep everything under 80 chars (so it's a product() + subTest() combo) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I get it--the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One obvious way, probably but this obvious way makes the code really harder to read IMO. Anyway, this is a bit off-topic and I don't know how much we can use it (but I've personally needed more than once to be able to use products for testing cases). Note that importlib's tests already has some "parametrize" decorator as in Pytest, so we could also move it up to test.support instead. |
||
if line != '': # Ignore trailing or leading newlines | ||
self.assertRegex(line, C_STACK_REGEX[1]) | ||
|
||
|
||
def test_dump_c_stack(self): | ||
code = dedent(""" | ||
import faulthandler | ||
faulthandler.dump_c_stack() | ||
""") | ||
output, exitcode = self.get_output(code) | ||
self.assertEqual(exitcode, 0) | ||
self.check_c_stack(output) | ||
|
||
|
||
def test_dump_c_stack_file(self): | ||
import tempfile | ||
|
||
with tempfile.TemporaryFile("w+") as tmp: | ||
faulthandler.dump_c_stack(file=tmp) | ||
tmp.flush() # Just in case | ||
tmp.seek(0) | ||
self.check_c_stack(tmp.read().split("\n")) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we using two blank lines to separate classes in this file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't look like there are any other classes here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yes, most of the file is using 1-blank line. Up to you then. |
||
if __name__ == "__main__": | ||
unittest.main() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Add support for printing the C stack trace on systems that support it via | ||
:func:`faulthandler.dump_c_stack` or via the *c_stack* argument in | ||
:func:`faulthandler.enable`. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,10 +9,10 @@ | |
#include "pycore_sysmodule.h" // _PySys_GetRequiredAttr() | ||
#include "pycore_time.h" // _PyTime_FromSecondsObject() | ||
#include "pycore_traceback.h" // _Py_DumpTracebackThreads | ||
|
||
#ifdef HAVE_UNISTD_H | ||
# include <unistd.h> // _exit() | ||
#endif | ||
|
||
#include <signal.h> // sigaction() | ||
#include <stdlib.h> // abort() | ||
#if defined(HAVE_PTHREAD_SIGMASK) && !defined(HAVE_BROKEN_PTHREAD_SIGMASK) && defined(HAVE_PTHREAD_H) | ||
|
@@ -210,6 +210,25 @@ faulthandler_dump_traceback(int fd, int all_threads, | |
reentrant = 0; | ||
} | ||
|
||
static void | ||
faulthandler_dump_c_stack(int fd) | ||
{ | ||
static volatile int reentrant = 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it is better to use atomics here? AFAIU volatile have different behavior for msvc for different platforms. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be, but regardless, that should be done in a different PR. |
||
|
||
if (reentrant) { | ||
return; | ||
} | ||
|
||
reentrant = 1; | ||
ZeroIntensity marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (fatal_error.c_stack) { | ||
PUTS(fd, "\n"); | ||
_Py_DumpStack(fd); | ||
} | ||
|
||
reentrant = 0; | ||
} | ||
|
||
static PyObject* | ||
faulthandler_dump_traceback_py(PyObject *self, | ||
PyObject *args, PyObject *kwargs) | ||
|
@@ -260,6 +279,33 @@ faulthandler_dump_traceback_py(PyObject *self, | |
Py_RETURN_NONE; | ||
} | ||
|
||
static PyObject * | ||
faulthandler_dump_c_stack_py(PyObject *self, | ||
picnixz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
PyObject *args, PyObject *kwargs) | ||
{ | ||
static char *kwlist[] = {"file", NULL}; | ||
PyObject *file = NULL; | ||
ZeroIntensity marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (!PyArg_ParseTupleAndKeywords(args, kwargs, | ||
"|O:dump_c_stack", kwlist, | ||
&file)) { | ||
return NULL; | ||
} | ||
|
||
int fd = faulthandler_get_fileno(&file); | ||
if (fd < 0) { | ||
return NULL; | ||
} | ||
|
||
_Py_DumpStack(fd); | ||
|
||
if (PyErr_CheckSignals()) { | ||
return NULL; | ||
} | ||
|
||
Py_RETURN_NONE; | ||
} | ||
|
||
static void | ||
faulthandler_disable_fatal_handler(fault_handler_t *handler) | ||
{ | ||
|
@@ -350,6 +396,7 @@ faulthandler_fatal_error(int signum) | |
|
||
faulthandler_dump_traceback(fd, deduce_all_threads(), | ||
fatal_error.interp); | ||
faulthandler_dump_c_stack(fd); | ||
|
||
_Py_DumpExtensionModules(fd, fatal_error.interp); | ||
|
||
|
@@ -425,6 +472,7 @@ faulthandler_exc_handler(struct _EXCEPTION_POINTERS *exc_info) | |
|
||
faulthandler_dump_traceback(fd, deduce_all_threads(), | ||
fatal_error.interp); | ||
faulthandler_dump_c_stack(fd); | ||
|
||
/* call the next exception handler */ | ||
return EXCEPTION_CONTINUE_SEARCH; | ||
|
@@ -519,14 +567,15 @@ faulthandler_enable(void) | |
static PyObject* | ||
faulthandler_py_enable(PyObject *self, PyObject *args, PyObject *kwargs) | ||
{ | ||
static char *kwlist[] = {"file", "all_threads", NULL}; | ||
static char *kwlist[] = {"file", "all_threads", "c_stack", NULL}; | ||
PyObject *file = NULL; | ||
int all_threads = 1; | ||
int fd; | ||
int c_stack = 1; | ||
ZeroIntensity marked this conversation as resolved.
Show resolved
Hide resolved
|
||
PyThreadState *tstate; | ||
|
||
if (!PyArg_ParseTupleAndKeywords(args, kwargs, | ||
"|Op:enable", kwlist, &file, &all_threads)) | ||
"|Opp:enable", kwlist, &file, &all_threads, &c_stack)) | ||
return NULL; | ||
|
||
fd = faulthandler_get_fileno(&file); | ||
|
@@ -543,6 +592,7 @@ faulthandler_py_enable(PyObject *self, PyObject *args, PyObject *kwargs) | |
fatal_error.fd = fd; | ||
fatal_error.all_threads = all_threads; | ||
fatal_error.interp = PyThreadState_GetInterpreter(tstate); | ||
fatal_error.c_stack = c_stack; | ||
|
||
if (faulthandler_enable() < 0) { | ||
return NULL; | ||
|
@@ -1238,6 +1288,10 @@ static PyMethodDef module_methods[] = { | |
PyDoc_STR("dump_traceback($module, /, file=sys.stderr, all_threads=True)\n--\n\n" | ||
"Dump the traceback of the current thread, or of all threads " | ||
"if all_threads is True, into file.")}, | ||
{"dump_c_stack", | ||
_PyCFunction_CAST(faulthandler_dump_c_stack_py), METH_VARARGS|METH_KEYWORDS, | ||
PyDoc_STR("dump_c_stack($module, /, file=sys.stderr)\n--\n\n" | ||
"Dump the C stack of the current thread.")}, | ||
{"dump_traceback_later", | ||
_PyCFunction_CAST(faulthandler_dump_traceback_later), METH_VARARGS|METH_KEYWORDS, | ||
PyDoc_STR("dump_traceback_later($module, /, timeout, repeat=False, file=sys.stderr, exit=False)\n--\n\n" | ||
|
Uh oh!
There was an error while loading. Please reload this page.