GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — GH-126910: Add GNU backtrace support for unwinding JIT frames by diegorusso · Pull Request #149104 · python/cpython Conversation
|
|
Performance wise, I didn't see any hit. Numbers are in the noise. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Documentation build overview
25 files changed ·
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
abc1072 to
db967dc
Compare
|
Apologies.. I had to force push. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
|
||
| #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.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
For example this can break in:
__register_frame)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
ok, it makes sense.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
link probe implemented here: ef9ea52
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| @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 an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The new test only asserts python_frames > 0 and jit_frames > 0, which a stub unwinder would pass. Two things that would help:
python_frames >= 10 (the recursion depth) so the count actually means something.backtrace() after the executor is freed: that's the property the deregister code exists to guarantee and nothing covers it today.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Addressed here: 01df239
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| * 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 an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
_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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Addressed here: cf16da0
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| #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.
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to guard every field of the struct?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The current version is valid, but the per-field guards are probably more defensive than necessary. I added them to keep the registration object matching the enabled backends, but since the struct is private and the fields are opaque handles, we can simplify the struct and keep the feature guards around the actual register/unregister code.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I reviewed this and tested it locally again and it works in all cases I can find.
Great work @diegorusso 💪
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
The buildbot failure will be fixed by: #149362 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Looks like this fails on ppc64le: https://buildbot.python.org/#/builders/451/builds/6989 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Also Raspbian failures went from to (even with #149362 applied) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
No, the GNU backtrace works on that platform. The one failing is We need to update @pablogsal PR (#149362) to include the shape for ppc64. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
ppc64le is fixed now; but on Raspbian it is GNU backtrace that's failing -- see #149409 (comment) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.