Skip to content

gh-132532: Add CHECK_PERIODIC instruction and use it for CALLs instead of the uop version. #132533

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
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Apr 14, 2025

@iritkatriel
Copy link
Member Author

@gaogaotiantian this PR adds an instruction (CHECK_PERIODIC) after each call. So now pdb.set_trace() sees that as the next instruction, and thinks it's breaking one line earlier. How do you suggest we fix this? Skip the CHECK_PERIODIC instruction (where would the happen?)

@gaogaotiantian
Copy link
Member

The test_pdb failure is not a real failure. We should just fix(remove) the test. It was a corner case where the instruction after CALL (POP_TOP) does not have a line number associated with it - and we should not stop there because missing line number would frustrate pdb. However, if the following instruction is CHECK_PERIODIC and it has a line number, we should just stop there.

@iritkatriel
Copy link
Member Author

CHECK_PERIODIC has the same line number as the CALL of set_trace(). So it will look like we stopped just before the set_trace call. I don't think that's what we want.

@gaogaotiantian
Copy link
Member

That is what we want. The existing test that failed is an exception, which stopped at the next line. We put a lot of effort to make breakpoint() stop at breakpoint(), so this is a desired behavior.

@@ -4714,7 +4714,8 @@ def _read():
# handlers, which in this case will invoke alarm_interrupt().
signal.alarm(1)
try:
self.assertRaises(ZeroDivisionError, wio.write, large_data)
with self.assertRaises(ZeroDivisionError):
wio.write(large_data)
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 needed to make this change because otherwise the exception is not swallowed (seems that it shows up after the self.assertRaises call has returned).

@markshannon
Copy link
Member

It looks like the specialize_method_descriptor needs to account for the CHECK_PERIODIC before the POP_TOP, and presumably CALL_LIST_APPEND will need to skip the two following instructions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants