GH-126910: Add GNU backtrace support for unwinding JIT frames#149104
GH-126910: Add GNU backtrace support for unwinding JIT frames#149104diegorusso wants to merge 7 commits intopython:mainfrom
Conversation
diegorusso
requested review from
a team,
AA-Turner,
Fidget-Spinner,
ZeroIntensity,
brandtbucher,
emmatyping,
ericsnowcurrently,
erlend-aasland,
markshannon and
savannahostrowski
as code owners
|
Performance wise, I didn't see any hit. Numbers are in the noise. |
Documentation build overview
41 files changed ·
|
diegorusso
force-pushed
the
jit-backtrace-support
branch
from
abc1072 to
db967dc
Compare
|
Apologies.. I had to force push. |
|
|
||
| #if defined(_Py_JIT) && defined(__linux__) && defined(__ELF__) | ||
| # define PY_HAVE_JIT_GDB_UNWIND | ||
| # if defined(HAVE_EXECINFO_H) && defined(HAVE_BACKTRACE) |
There was a problem hiding this comment.
This needs a separate configure link probe. HAVE_BACKTRACE only tells us that backtrace() is available, but this code also calls libgcc-specific symbols: __register_frame and __deregister_frame.
For example, Linux builds with clang -rtlib=compiler-rt can have execinfo.h / backtrace() while those frame registration symbols are not linkable. In that case thismacro gets enabled and the build fails later at link time.
Please add a configure check for __register_frame __deregister_frame and gate PY_HAVE_JIT_GNU_BACKTRACE_UNWIND on that result.
There was a problem hiding this comment.
For example this can break in:
- Alpine / musl + libexecinfo (no libgcc_s
__register_frame) - clang + compiler-rt without libgcc_s
- LLVM libunwind builds (different FDE table format)
There was a problem hiding this comment.
ok, it makes sense.
| @support.requires_gil_enabled("test requires the GIL enabled") | ||
| @unittest.skipIf(support.is_wasi, "test not supported on WASI") | ||
| @unittest.skipUnless(sys.platform == "linux", "GNU backtrace unwinding test requires Linux") | ||
| class GnuBacktraceUnwindTests(unittest.TestCase): |
There was a problem hiding this comment.
The new test only asserts python_frames > 0 and jit_frames > 0, which a stub unwinder would pass. Two things that would help:
- Tighten to
python_frames >= 10(the recursion depth) so the count actually means something. - Add a test that JIT frames disappear from
backtrace()after the executor is freed: that's the property the deregister code exists to guarantee and nothing covers it today.
| * A NULL result is valid when publication succeeded only through backends | ||
| * with no unregister step, such as perf map output. | ||
| */ | ||
| _PyJitCodeRegistration *_PyJit_RegisterCode(const void *code_addr, |
There was a problem hiding this comment.
_PyJit_RegisterCode returns NULL for three different reasons (perf-only success, calloc failure, all backends failed) and the caller can't tell them apart. Could you rename registered -> any_registered and add a one-liner near the perf branch noting it's intentionally not counted? The deleted comment from jit_record_code about partial-failure being non-fatal would also be nice to restore.
| #if defined(PY_HAVE_JIT_GDB_UNWIND) \ | ||
| || defined(PY_HAVE_JIT_GNU_BACKTRACE_UNWIND) | ||
| struct _PyJitCodeRegistration { | ||
| # if defined(PY_HAVE_JIT_GDB_UNWIND) |
There was a problem hiding this comment.
Do we really need to guard every field of the struct?
3 participants
📚 Documentation preview 📚: https://cpython-previews--149104.org.readthedocs.build/