-
-
Notifications
You must be signed in to change notification settings - Fork 34k
GH-126910: add test for manual frame unwinding #144137
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
base: main
Are you sure you want to change the base?
Conversation
|
🤖 New build scheduled with the buildbot fleet by @diegorusso for commit 5d53a15 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F144137%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
|
🤖 New build scheduled with the buildbot fleet by @diegorusso for commit c179848 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F144137%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
| jit_enabled = hasattr(sys, "_jit") and sys._jit.is_enabled() | ||
| jit_backend = _testinternalcapi.get_jit_backend() | ||
| ranges = _testinternalcapi.get_jit_code_ranges() if jit_enabled else [] | ||
| jit_code_ranges = len(ranges) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not used anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a leftover of a previous logic to test the existence of JIT frames. Will be removing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you referred only to jit_code_ranges = len(ranges). The rest is needed.
| get_jit_backend(PyObject *self, PyObject *Py_UNUSED(args)) | ||
| { | ||
| #ifdef _Py_JIT | ||
| return PyUnicode_FromString("jit"); | ||
| #elif defined(_Py_TIER2) | ||
| return PyUnicode_FromString("interpreter"); | ||
| #else | ||
| Py_RETURN_NONE; | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's https://docs.python.org/3/library/sys.html#sys._jit which you can use instead of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately that API doesn't differentiate between --enable-eperimental-jit and --enable-experimental-jit=interpreter. In both cases I have:
>>> sys._jit.is_enabled()
True
>>> sys._jit.is_available()
True
This is not useful because it doesn't tell me anything about the backend.
For testing purposes I need to know if the backend is just the interpreter because it means that I don't have any memory allocated for the JIT. This is really a corner case but in our CI we do build --enable-experimental-jit=interpreter as a gatekeeper for the JIT jobs.
| return "python"; | ||
| } | ||
| return "other"; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the module lookup succeeds but filename retrieval fails (len == 0 or len >= MAX_PATH), it falls through without returning anything, This would then check JIT, which is probably fine, but the logic is unclear. Perhaps we can add an explicit return "other"; inside the outer if block after the inner if.
| } | ||
| #else | ||
| static const char * | ||
| classify_address(uintptr_t addr, int jit_enabled, PyInterpreterState *interp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you get a warning here for interp being unused? I would have expected the compiler to complain for platforms without HAVE_DLADDR or WASI 🤔
|
|
||
| uintptr_t *next_fp = (uintptr_t *)frame_pointer[0]; | ||
| // Stop if the frame pointer is extremely low. | ||
| if ((uintptr_t)next_fp < 0x1000) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit (feel free to ignore): extract a global with a name for 0x1000
| self.assertEqual( | ||
| python_frames, | ||
| 1, | ||
| f"unexpected Python frames counted on {self.machine} with env {env}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can see how CI behaves here but when frame pointers aren't expected, asserting exactly 1 Python frame seems brittle. If the unwinder happens to traverse one valid frame before hitting invalid data, this works, but this behavior isn't guaranteed. Consider whether <= 1 perhaps?
pablogsal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I just left a bunch of minor nits. Feel free to land without those
|
Very cool and great work @diegorusso 💯 |
Add a test that unwinds the stack using frame pointers. We test on any platform that frame pointers are enable (the others will be skipped). It tests with and without the JIT.
Output with PYTHON_JIT=0
Output with PYTHON_JIT=1
The core of the test has been taken from @pablogsal repository https://github.com/pablogsal/cpython-unwind