8000 gh-116750: Add clear_tool_id function to unregister events and callbacks by gaogaotiantian · Pull Request #124568 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-116750: Add clear_tool_id function to unregister events and callbacks #124568

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Oct 1, 2024
Merged

gh-116750: Add clear_tool_id function to unregister events and callbacks #124568

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
76f7716
Add clear_tool_id function to unregister events and callbacks
gaogaotiantian Sep 26, 2024
81e1de0
📜🤖 Added by blurb_it.
blurb-it[bot] Sep 26, 2024
6d1dfa9
Move tool version to code monitoring and force an instrument
gaogaotiantian Sep 26, 2024
529e0df
Initialize tool versions
gaogaotiantian Sep 26, 2024
151e3b3
Add an extra test for use after free
gaogaotiantian Sep 27, 2024
5fb656e
Add comments
gaogaotiantian Sep 27, 2024
fd74a55
Only set tool verions for external tool ids
gaogaotiantian Sep 27, 2024
82b42ec
Revert "Only set tool verions for external tool ids"
gaogaotiantian Sep 27, 2024
9986702
Update comments
gaogaotiantian Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Update comments
  • Loading branch information
@gaogaotiantian
gaogaotiantian committed Sep 30, 2024
commit 9986702852981d2609d11220716e9268189261d8
2 changes: 1 addition & 1 deletion Python/instrumentation.c
8000 View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2074,7 +2074,7 @@ int _PyMonitoring_ClearToolId(int tool_id)

// monitoring_tool_versions[tool_id] is set to latest global version here to
// 1. invalidate local events on all existing code objects
// 2. be ready for the next use_tool_id call so we can only do this in one place
// 2. be ready for the next call to set local events
interp->monitoring_tool_versions[tool_id] = version;
Copy link
Member
@markshannon markshannon Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the version for tool_id will be the only one that matches the global version, which means that tool_id will be the only tool with an up-to-date version number.
Isn't that the opposite of what we want?

Maybe interp->monitoring_tool_versions[tool_id] = 0 and leave the global version untouched?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to force all code object, executing or not, to "refresh" because the local events will be changed, so we need to update global version for the code objects to know.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understood you here. You think it would make sense to do

interp->monitoring_tool_versions[tool_id] = 0;

here so all the code objects checking tool_id will know they are out of date. However, setting it to the latest global version will have the same effect - none of the code object has this version either.

If we set this to 0, we need to set it to the latest global version some time in the future, before we use this tool to instrument code again.

So we can either set it to 0 here and set it to global_version in use_tool_id, or we can do it together here(I can add some. comments here to explain).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I was wrong in the previous comment. The update to global version is critical here because the user can set local events immediately after clear_tool_id. clear_tool_id does not surrender the tool_id. So we have to set the tool id version to the latest global version here for the following local events.


// Set the new global version so all the code objects can refresh the
Expand Down
Loading
0