-
Notifications
You must be signed in to change notification settings - Fork 422
Avoid holding frames captured by another thread #823
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
Open
godlygeek
wants to merge
3
commits into
bloomberg:main
Choose a base branch
from
godlygeek:fix_GetFrame_stack_mismatch
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rather than having callers of the Python stack tracker work directly at the level of pushing and popping individual frames, allow them to express higher level goals that the stack tracker will achieve on their behalf. This currently just reorganizes some code, but lays the groundwork for future changes that rely on the stack tracker knowing why a particular frame is being pushed or popped. Signed-off-by: Matt Wozniski <[email protected]>
Previously `PythonStackTracker::handleGreenletSwitch` cleared the shadow stack itself, rather than reusing the `PythonStackTracker::clear` method. The approach used by `handleGreenletSwitch` should be a little bit faster, so adopt it into `clear`. Also, switch `clear` to be private rather than public, since it's currently only used internally to the class. Signed-off-by: Matt Wozniski <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #823 +/- ##
==========================================
+ Coverage 92.01% 92.17% +0.15%
==========================================
Files 95 95
Lines 11940 11952 +12
Branches 413 413
==========================================
+ Hits 10987 11017 +30
+ Misses 953 935 -18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
17f418e
to
8ae2869
Compare
When tracing starts in one thread, we use `PyThreadState_GetFrame` to capture the current Python call stack in every other thread, so that we can associate allocations made later in pre-existing threads with the correct call stack. Unfortunately, our approach doesn't play nicely with trace functions, like those installed by Coverage. The stack we get by walking backwards from `PyThreadState_GetFrame` sees call frames that a profile function never sees (because `PyThreadState_GetFrame` gets frames that correspond to calls to trace functions, but profile functions don't get called for calls into trace functions). Because of this, we wind up referring to frames even after they've been popped off the call stack. Because we're holding borrowed references, this results in a use-after-free, rather than just incorrect stacks. We could hold owned references instead of borrowed references. That fixes the problem of use-after-free, though we'd still report incorrect stacks. Unfortunately, it's not easy to do, as there are times when we need to drop those references while the GIL isn't held and we're not able to decrement the reference count. We can't easily trim frames off the `PyThreadState_GetFrame` to make it match the stack that profile functions see, either. That would require us to be able to recognize frames that correspond to calls to trace functions. That's easy if the thread never changes its trace function, but it's impossible in the general case where the trace function installs a different trace function. The approach taken by this commit is that, instead of referencing the frame objects captured when tracking started, we instead copy information out of them and reference that information when allocations occur. Later, when our profile function runs in that thread, we switch to our normal shadow stack approach, using `PyEval_GetFrame` to populate it from a place where we know there can be no trace function calls on the stack. This sidesteps the use-after-free problem, but it still means that we'll misreport stacks until the first time our profile function gets called in any thread that already existed when the tracking session started. Notably, because the profile function doesn't get called when changing from one line to another within a Python function, we'll report incorrect line numbers for allocations made in background threads until the first Python function call or return on that thread. Signed-off-by: Matt Wozniski <[email protected]>
8ae2869
to
1e2ec15
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #820
When tracing starts in one thread, we use
PyThreadState_GetFrame
tocapture the current Python call stack in every other thread, so that we
can associate allocations made later in pre-existing threads with the
correct call stack.
Unfortunately, our approach doesn't play nicely with trace functions,
like those installed by Coverage. The stack we get by walking backwards
from
PyThreadState_GetFrame
sees call frames that a profile functionnever sees (because
PyThreadState_GetFrame
gets frames that correspondto calls to trace functions, but profile functions don't get called for
calls into trace functions). Because of this, we wind up referring to
frames even after they've been popped off the call stack. Because we're
holding borrowed references, this results in a use-after-free, rather
than just incorrect stacks.
We could hold owned references instead of borrowed references. That
fixes the problem of use-after-free, though we'd still report incorrect
stacks. Unfortunately, it's not easy to do, as there are times when we
need to drop those references while the GIL isn't held and we're not
able to decrement the reference count.
We can't easily trim frames off the
PyThreadState_GetFrame
to make itmatch the stack that profile functions see, either. That would require
us to be able to recognize frames that correspond to calls to trace
functions. That's easy if the thread never changes its trace function,
but it's impossible in the general case where the trace function
installs a different trace function.
The approach taken by this commit is that, instead of referencing the
frame objects captured when tracking started, we instead copy
information out of them and reference that information when allocations
occur. Later, when our profile function runs in that thread, we switch
to our normal shadow stack approach, using
PyEval_GetFrame
to populateit from a place where we know there can be no trace function calls on
the stack. This sidesteps the use-after-free problem, but it still means
that we'll misreport stacks until the first time our profile function
gets called in any thread that already existed when the tracking session
started. Notably, because the profile function doesn't get called when
changing from one line to another within a Python function, we'll report
incorrect line numbers for allocations made in background threads until
the first Python function call or return on that thread.