gh-148925: Add PyUnstable_CollectCallStack and PyUnstable_PrintCallStack#148930
gh-148925: Add PyUnstable_CollectCallStack and PyUnstable_PrintCallStack#148930danielsn wants to merge 1 commit intopython:mainfrom
Conversation
danielsn
requested review from
a team,
AA-Turner,
ZeroIntensity,
emmatyping and
erlend-aasland
as code owners
|
All commit authors signed the Contributor License Agreement. |
|
|
||
| /* Buffer size for the filename and name fields in PyUnstable_FrameInfo: | ||
| up to 500 content bytes plus '\0' (1). */ | ||
| #define Py_UNSTABLE_FRAMEINFO_STRSIZE 501 |
There was a problem hiding this comment.
I used 500 because that was what the print to fd version used, https://github.com/danielsn/cpython/blob/main/Python/traceback.c#L56C9-L56C26 but this seems larger than we'd need in practice.
| the respective string was longer than Py_UNSTABLE_FRAMEINFO_STRSIZE-1 | ||
| bytes and was truncated. */ | ||
| typedef struct { | ||
| int lineno; |
There was a problem hiding this comment.
The code object has other fields (e.g. column), would these be worth collecting as well?
|
Will try to review this week. Heads up that we are very close to beta freeze so we would need to land this this week or next week at the latest |
| if (r > 0) { | ||
| PyUnstable_PrintCallStack(fd, &fi, 1, 0); | ||
| depth++; | ||
| } | ||
| else if (r < 0) { | ||
| PUTS(fd, " <invalid frame>\n"); | ||
| break; | ||
| return; | ||
| } |
There was a problem hiding this comment.
I was wondering if we needed to do anything in case r == 0 (which currently falls through) and it seems like collect_frame returns 0 if frame->owner == FRAME_OWNED_BY_INTERPRETER.
This is probably highly unlikely (but not impossible because this may be executed on crash in a signal handler), but should the r == 0 case also increment depth to make sure we don't get stuck in an infinite loop if we're unlucky and collect_frame keeps returning 0?
As I said, very unlikely, but also very cheap to avoid.
| current Python frame. | ||
|
|
||
| Filenames and function names are ASCII-encoded (non-ASCII characters are | ||
| backslash-escaped) and truncated to 500 characters; if truncated, the |
There was a problem hiding this comment.
| backslash-escaped) and truncated to 500 characters; if truncated, the | |
| backslash-escaped) and truncated to 500 bytes; if truncated, the |
| int lasti = _PyFrame_SafeGetLasti(frame); | ||
| out->lineno = (lasti >= 0) ? _PyCode_SafeAddr2Line(code, lasti) : -1; |
There was a problem hiding this comment.
In what case(s) can lasti be less than 0? Would it indicate corrupted memory? If so, shouldn't we instead early-return/stop walking the stack to avoid potentially touching memory we don't own and completely crash while potentially in a crash handler? Or am I missing something?
There was a problem hiding this comment.
I'm not sure the cost/benefit pencils out here.
This is a meaningful amount of new code to carry: a new public-ish struct with fixed buffers, a new format_ascii encoder that largely duplicates _Py_DumpASCII, a new collect/print pair, plus a refactor of dump_traceback. The safety story is subtle enough that it requires a maintainer-contract banner just to keep future contributors from breaking it (no malloc, no refcount, no GIL, no non-AS-safe libc, etc.), and even with that discipline in place the contract is admittedly best-effort as CollectCallStack itself documents that it "may even rash itself."
A third-party extension can already do all of this today: include the CPython headers, build against Py_BUILD_CORE-style internals if needed, and walk frames using the existing PyUnstable_InterpreterFrame_* primitives. That's always possible. But landing it here flips the direction of the maintenance burden instead of profiler vendors maintaining their own stack-walker against each CPython version (which they already do for everything else they care about), we take on maintaining it for them, with the additional constraint that the AS-safety invariants must be preserved by every future contributor touching traceback.c. That's a real cost, not a free PyUnstable_* escape hatch.
It's also worth noting that this only helps a fairly narrow class of tools: in-process profilers and crash handlers running inside the target interpreter. The majority of production Python profilers are out-of-process. Those tools get nothing from this API; they need stable struct layouts and offsets they can read remotely, not an in-process C function. So we'd be adding a non-trivial maintenance surface to CPython to serve a subset of in-process tooling, while the dominant profiler architecture is unaffected.
That said, I want to think about this more before taking a firm position, and I'd like to hear other core devs weigh in.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
It was proposed to make PyUnstable_DumpTraceback() public: see PR gh-148145. IMO it would already be a first step forward. I prefer to wait until that's done to consider adding another similar-but-different API. |
4 participants
#145559 proposes a signal-safe mechanism to print backtraces. This is useful for human debugging, but not ideal for tools because it requires parsing human-readable text. For example, a crash-tracker may wish to export and analyze structured backtraces. Similarly, a profiler can benefit from the ability to generate structure stack-traces in pure C, without needing to reason about the safety of reentering the python interpreter.
📚 Documentation preview 📚: https://cpython-previews--148930.org.readthedocs.build/