gh-127604: Add C stack dumps to `faulthandler` #128159

ZeroIntensity · 2024-12-21T18:30:33Z

This only adds it for glibc right now. I think it's possible to implement it for Windows with StackWalk64, but I'd prefer to get a solid working implementation for other systems before I dive into that.

Issue: Add a way of printing a C backtrace to faulthandler #127604

📚 Documentation preview 📚: https://cpython-previews--128159.org.readthedocs.build/en/128159/library/faulthandler.html#dumping-the-c-stack

picnixz

There are some PEP-7 nits but I don't know if this was done to be aligned with the rest of the code so you can freely ignore those nits.

Doc/library/faulthandler.rst

Doc/whatsnew/3.14.rst

Doc/library/faulthandler.rst

Modules/faulthandler.c

Doc/whatsnew/3.14.rst

Modules/faulthandler.c

picnixz · 2024-12-21T18:59:51Z

@ZeroIntensity If you ever use the web UI, don't forget to replace my tabs with spaces... hopefully your IDE does it but I see that I have tabs instead of spaces on my suggestions (I've corrected them but just double-check)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

…thon into c-faulthandler

Doc/library/faulthandler.rst

Modules/faulthandler.c

Doc/library/faulthandler.rst

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

…thon into c-faulthandler

bedevere-bot · 2024-12-21T23:47:13Z

🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit e79e661 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

jakkdl

Nice! I did start playing around with an implementation myself, and looks like we made a couple different implementation decisions:

Instead of adding a dump_c_stack function, I added a kwarg to dump_traceback. I think I prefer your approach here, since a user may want to see the c backtrace but doesn't care about the python traceback. Though that does clash somewhat with it being a kwarg to enable (and register/dump_traceback_later).

I named my kwarg c_backtrace, though I think I prefer c_stack to avoid backtrace/traceback confusion.

I also added a kwarg to register(), and dump_traceback_later could also get it.

I made all functions raise an exception if passed c_stack=True and it wasn't available, and added a global faulthandler.HAS_C_BACKTRACE. I'm not sure what the best approach is here.

Modules/faulthandler.c

Lib/test/test_faulthandler.py

configure.ac

picnixz

Hard to review it on a small screen. Will do a more precise review in 2 weeks when I'm back if the PR is still open.

picnixz · 2025-01-27T09:38:53Z

Modules/faulthandler.c

+{
+    static volatile int reentrant = 0;
+
+    if (reentrant)


Some missing PEP-7 here (on mobile so not sure if this is consistent with the rest of the file)

Yeah, I'm just keeping consistency.

PEP 7 should be respected for new code.

picnixz · 2025-01-27T09:39:38Z

Modules/faulthandler.c

@@ -264,6 +283,32 @@ faulthandler_dump_traceback_py(PyObject *self,
    Py_RETURN_NONE;
 }

+static PyObject *
+faulthandler_dump_c_stack_py(PyObject *self,


Why not using clinic?

We could, but faulthandler doesn't have anything in AC yet. I'd be happy to do it in a follow-up, but for now lets keep it as-is.

picnixz · 2025-01-27T09:48:14Z

Python/traceback.c

+    }
+    for (int i = 0; i < frames; ++i) {
+        char entry_str[TRACEBACK_ENTRY_MAX_SIZE];
+        snprintf(entry_str, TRACEBACK_ENTRY_MAX_SIZE, "  %s\n", strings[i]);


Not sure if we do I in general, but you can suppress the return value of snprintf. I have been suppressing unused return values when used in a void function or if it's an inline call, but only for some function (ideally I'd like to do it everywhere but since we don't have a compiler warning because of this for now, I don't do it for old code that I'm not modifying).

OTOH, isn't the return value the number of characters that were written to the buffer? (excluding the null byte) why can't you use it instead of recomputing strlen?

WDYM suppress it, like a (void) cast? (Speaking of this though, we should use PyOS_snprintf.)

OTOH, isn't the return value the number of characters that were written to the buffer?

I vaguely remember looking this up when I wrote it. I think it only returns the full length if the passed string was NULL, and then otherwise returns the number of missing characters if the string wasn't big enough. From IBM's docs: "The return value is the number of characters (not counting the terminating null character) that would have been written if n had been large enough."

picnixz · 2025-01-27T09:51:26Z

Python/traceback.c

+    for (int i = 0; i < frames; ++i) {
+        char entry_str[TRACEBACK_ENTRY_MAX_SIZE];
+        snprintf(entry_str, TRACEBACK_ENTRY_MAX_SIZE, "  %s\n", strings[i]);
+        size_t length = strlen(entry_str) + 1;


I missed it previously but we would also replace the last characters for formatted strings that fitted perfectly right?

Yeah, but I'm not sure it's worth the extra effort to add a case for it.

picnixz · 2025-01-27T09:53:44Z

Python/traceback.c

+        char entry_str[TRACEBACK_ENTRY_MAX_SIZE];
+        snprintf(entry_str, TRACEBACK_ENTRY_MAX_SIZE, "  %s\n", strings[i]);
+        size_t length = strlen(entry_str) + 1;
+        if (length == TRACEBACK_ENTRY_MAX_SIZE) {


We could extract this in a function as this could perhaps improve readability although you're not forced to

ZeroIntensity added 9 commits December 21, 2024 12:27

Initial implementation for glibc

0ebdc69

Make it cross-platform buildable.

0ccf9fb

Call it on Windows.

52945e5

Add a whatsnew entry

9bdd802

Add dump_c_stack

66f2641

Reverse the loop.

0591013

Add documentation entries.

64707a2

Make the whatsnew entry more specific.

8a5b784

Add tests.

4f09358

ZeroIntensity requested review from gpshead and picnixz December 21, 2024 18:30

ZeroIntensity requested review from erlend-aasland and corona10 as code owners December 21, 2024 18:30

bedevere-app bot mentioned this pull request Dec 21, 2024

Add a way of printing a C backtrace to faulthandler #127604

Open

bedevere-app bot added the awaiting review label Dec 21, 2024

ZeroIntensity added 7 commits December 21, 2024 13:32

Add blurb from whatsnew.

e1aa619

Fix failing Sphinx build.

6b515d3

Fix PyArg_ParseTupleAndKeywords format string.

c69ee9d

Fix Sphinx warnings.

f08b1dd

Add ignore to test_inspect

dbb6d25

Fix name.

faf1a3e

Ignore the reentrant variable in the C analyzer.

ec832aa

picnixz reviewed Dec 21, 2024

View reviewed changes

ZeroIntensity requested a review from ericsnowcurrently as a code owner December 21, 2024 18:58

ZeroIntensity and others added 5 commits December 21, 2024 14:03

Apply suggestions from code review

524f167

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Use manpage references.

dd08bcb

Add issue number to whatsnew.

bda3dcd

Merge branch 'c-faulthandler' of https://github.com/ZeroIntensity/cpy…

a3564a5

…thon into c-faulthandler

Rename macros.

f3fcea1

Reduce number of printed calls.

db97dd2

picnixz reviewed Dec 21, 2024

View reviewed changes

Doc/library/faulthandler.rst Outdated Show resolved Hide resolved

Modules/faulthandler.c Outdated Show resolved Hide resolved

Modules/faulthandler.c Outdated Show resolved Hide resolved

Doc/library/faulthandler.rst Outdated Show resolved Hide resolved

ZeroIntensity and others added 3 commits December 21, 2024 14:50

Apply suggestions from code review

c17457f

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Address code review.

d5f7d4b

Merge branch 'c-faulthandler' of https://github.com/ZeroIntensity/cpy…

e79e661

…thon into c-faulthandler

ZeroIntensity added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 21, 2024

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 21, 2024

jakkdl suggested changes Dec 22, 2024

View reviewed changes

Modules/faulthandler.c Outdated Show resolved Hide resolved

Modules/faulthandler.c Show resolved Hide resolved

Lib/test/test_faulthandler.py Show resolved Hide resolved

configure.ac Outdated Show resolved Hide resolved

bedevere-app bot added awaiting core review and removed awaiting review labels Dec 22, 2024

ZeroIntensity added 3 commits December 22, 2024 10:36

Explicitly check for backtrace() and backtrace_symbols()

0c84f8a

Handle no frames returned.

8198997

Move the function that calls backtrace() to within the core.

0f670f0

ZeroIntensity requested review from picnixz and vstinner January 25, 2025 03:41

ZeroIntensity added 3 commits January 26, 2025 14:31

Merge into main.

079f186

Fix test case.

892a085

Run make regen-configure

896abd1

picnixz reviewed Jan 27, 2025

View reviewed changes

ZeroIntensity added 2 commits January 27, 2025 10:08

Extract behavior in own function.

cab079e

PEP-7

1784071

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-127604: Add C stack dumps to `faulthandler` #128159

gh-127604: Add C stack dumps to `faulthandler` #128159

ZeroIntensity commented Dec 21, 2024 •

edited

Loading

picnixz left a comment

picnixz commented Dec 21, 2024 •

edited

Loading

bedevere-bot commented Dec 21, 2024

jakkdl left a comment •

edited

Loading

picnixz left a comment

picnixz Jan 27, 2025

ZeroIntensity Jan 27, 2025

vstinner Jan 27, 2025

picnixz Jan 27, 2025

ZeroIntensity Jan 27, 2025

picnixz Jan 27, 2025

ZeroIntensity Jan 27, 2025

picnixz Jan 27, 2025

ZeroIntensity Jan 27, 2025

picnixz Jan 27, 2025

gh-127604: Add C stack dumps to faulthandler #128159

Are you sure you want to change the base?

gh-127604: Add C stack dumps to faulthandler #128159

Conversation

ZeroIntensity commented Dec 21, 2024 • edited Loading

picnixz left a comment

Choose a reason for hiding this comment

picnixz commented Dec 21, 2024 • edited Loading

bedevere-bot commented Dec 21, 2024

jakkdl left a comment • edited Loading

Choose a reason for hiding this comment

picnixz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gh-127604: Add C stack dumps to `faulthandler` #128159

gh-127604: Add C stack dumps to `faulthandler` #128159

ZeroIntensity commented Dec 21, 2024 •

edited

Loading

picnixz commented Dec 21, 2024 •

edited

Loading

jakkdl left a comment •

edited

Loading