-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
gh-111744: Support opcode events in bdb #111834
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
Conversation
Hi @brandtbucher , I know you've been busy recently. When you have some time, could you take a look at this? This is the prerequisite for a |
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.
Thanks a ton for your patience (I really dropped the ball on reviewing this)! I'd love to get your thoughts on a couple of things before merging:
Lib/bdb.py
Outdated
def _set_trace_opcodes(self, trace_opcodes): | ||
if trace_opcodes != self.trace_opcodes: | ||
self.trace_opcodes = trace_opcodes | ||
frame = self.__curframe |
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.
And just to jog my memory: self.__curframe
is needed because we only want to change events at or above the frame where set_trace
was called (these are the same frames where f_trace_lines
was set), but we may be much deeper at this point. Right?
But why is it double-underscored again? Do we need the name-mangling? I see pdb.Pdb
has a curframe
attribute, so maybe it's to avoid confusion with that?
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.
Yes the reason for the double-underscore is to avoid the conflict with pdb.Pdb.curframe
. And yes, we only want to set trace_opcodes
in the "user frames", which means frames above (including) the caller frame into the pdb. So we need to record the entering frame.
The curframe
attribute of pdb
means the current frame being debugged. Do you prefer to change the the name __curframe
to something like entering_frame
?
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.
Yeah, that feels a lot less cryptic and scary. :) Especially considering we already have stopframe
, returnframe
, and botframe
.
Co-authored-by: Brandt Bucher <[email protected]>
Also, I played with this a bit while reviewing, and it's pretty neat. I know time's running out: do you think the rest of the |
Also, what's the relationship between this PR and GH-103050? |
#103050 was the bigger ambition to directly support instruction level debugging back then. It was not easy to push something that big at that time when I did not have some basic reputation of "this person knows what he's doing". That one was kind of abandoned and the most critical part (supporting opcode event in general) was adopted into this PR. I will try to pursue it again in 3.14, alongwith some other more ambitious targets, but that's after the language summit :) And to your question, I will finish the breakpoint change by the end of tomorrow and see if we can make it in 3.13. |
Let me know if there's anything I can do to help, besides being available for review. I think it's a great change. |
I had the prototype working once it should not be too difficult. Although this might break some tests - now that it stopped after the instruction after |
I'm ready to land this once |
I used |
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.
Sorry, spotted one more thing:
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
This is the first step to solve #111744 . After some discussion in discord, most of the developers like the idea to have
breakpoint()
break immediately, rather than on next event. In order to achieve that, we need to use the opcode events.This PR is just to lay the ground for opcode events. In general, the users can use opcode events now with a new set of API -
set_stepinstr()
anduser_opcode()
. The documentation is not updated on purpose - if we prefer to keep it internal now and just use it as an infra to support newbreakpoint()
, we can keep quiet about it. If we decide to make it available to public, I can write the docs.breakpoint
does not enter pdb until the "next event" happens #111744