-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
gh-102864: Remove access to f_locals in pdb #102865
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
Lib/pdb.py
Outdated
self.frame_locals[frame_idx], | ||
prompt_prefix)) | ||
|
||
def format_stack_entry(self, frame, lineno, f_locals, lprefix=': '): |
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 signature change breaks Liskov with Bdb. You could keep the tuple, no?
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 let myself refactor this in a way that doesn't require this overload. Let me know what you think.
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.
So if we are going to use a proxy, why not use it everywhere? We can get rid of all the f_local
protections and keep all the signatures. The future changes that might involve f_locals
won't step into the same issue anymore either. Do you think that'll be too big a change to the code structure? In theory, the functionality of all the methods should be the same.
If you think that's a reasonable change to make, I can do 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.
Yes, I like that direction. Jury's still out whether we will be able to consider this a bug fix. We'll decide when the code change is complete. This will touch a few places but I don't think this will be too deep of a change. And we can document the proxy to explain the problem, so it will be more visible to future programmers that want to interface with the debugger.
You can also look at whether the change wouldn't be better applied to the base Bdb.
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.
So, I had a draft for the proxy solution. It's a bit more aggressive than the original fix.
The solution introduced two new classes in bdb.py
- FrameProxy
and TracebackProxy
. They are very similar to the original FrameObject
and TracebackObject
, except it won't load f_locals
twice. One noticeable compromise is inspect.getsourcelines()
needs the actual frame object(unless we do a really ugly __unwrap__
hack in FrameProxy
).
As you can tell, it's not completely backward compatible. pdb.py
requires some changes to accomodate, for example, convert tracebacks to the proxy, and get the real frame for the source code.
Like I said, this approach is definitely more aggressive than the original, but it gives a pretty good protection about f_locals
. pdb
won't need to worry about it anymore(actually I changed the code to cache f_locals
to use f_locals
directly as it's more intuitive).
Not sure if this is something we want to pursue. I do have a solution that only has the proxy in pdb
(HEAD~3
I think). We can use that if this is too much.
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.
Ah, I almost forgot the most important thing. There are in fact two competing PEPs on fixing the core issue: PEP 558 and PEP 667. The former is currently out-of-date in terms of the current implementation of CPython. The latter lacks an implementation but I can work on that.
If PEP 667 got accepted, that would have been a better solution to the problem than patching pdb
with frame proxies.
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.
Ah, I should've mentioned the reason I need to put the cache in FrameProxy
and the fact that I tried weakref and it did not work.
If PEP 667 is implemented, we don't need the proxy anymore. This issue would be solved. So at this point, do we want to proceed on this PR? I agree that having the traceback proxy in setup
is better. I can take a look at it and see if it's feasible if we decided to still finish 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.
Let's keep this bugfix open as context for PEP 667 and weakref support in frame objects. I'll convert it to Draft for the time being. Thanks for being active on 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.
Okay let's do that for now!
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.
So, the better solution is definitely waiting for weakref on frames or that f_locals performs well overall. But is this still considered as a bug? If so, should we fix it in 3.10 and 3.11? Maybe do a much better contained fix for all the versions, then move to a better solution?
Lib/pdb.py
Outdated
except KeyboardInterrupt: | ||
pass | ||
|
||
def print_stack_entry(self, frame_lineno, prompt_prefix=line_prefix): | ||
frame, lineno = frame_lineno | ||
def print_stack_entry(self, frame_idx, prompt_prefix=line_prefix): |
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 a breaking change in terms of method signature. That would mean we would only be able to accept this change for 3.12. If this is what you want, you can keep the change, as it makes using the method safer.
It doesn't look like that we will have a well-rounded solution for the issue in 3.12. Should we consider fixing it in a containable way for now? @ambv |
This PR is not needed anymore as PEP 667 is merged. |
Avoid accessing f_locals in pdb which will clear local variable changes.