Skip to content

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

Closed
wants to merge 8 commits into from

Conversation

gaogaotiantian
Copy link
Member

@gaogaotiantian gaogaotiantian commented Mar 21, 2023

Avoid accessing f_locals in pdb which will clear local variable changes.

@arhadthedev arhadthedev added the stdlib Python modules in the Lib dir label Mar 22, 2023
@ambv ambv changed the title gh-102864 Remove access to f_locals in pdb gh-102864: Remove access to f_locals in pdb Mar 22, 2023
Lib/pdb.py Outdated
self.frame_locals[frame_idx],
prompt_prefix))

def format_stack_entry(self, frame, lineno, f_locals, lprefix=': '):
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

@gaogaotiantian gaogaotiantian Mar 23, 2023

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.

Copy link
Contributor

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.

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 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.

Copy link
Contributor

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!

Copy link
Member Author

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!

Copy link
Member Author

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):
Copy link
Contributor

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.

@ambv ambv marked this pull request as draft March 23, 2023 17:23
@gaogaotiantian
Copy link
Member Author

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

@gaogaotiantian
Copy link
Member Author

This PR is not needed anymore as PEP 667 is merged.

@gaogaotiantian gaogaotiantian deleted the fix-flocals-usage branch May 25, 2024 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review DO-NOT-MERGE stdlib Python modules in the Lib dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants