Skip to content
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

Fix `up` through generators in postmortem debugger; closes #6251 #11266

Merged
merged 2 commits into from Oct 13, 2018

Conversation

@rhelmot
Copy link
Contributor

commented Aug 16, 2018

By passing the lowest traceback element into the debugger, we require it to use frame.f_back to find older frames. However, f_back is always None for generator frames in the postmortem context. By providing a higher-up traceback element, pdb can traverse down the traceback.tb_next links, which do work correctly across generator calls.

...or at least, that's my understanding of the situation after spending half an hour researching the issue. This fix seems to work for my use cases, but I don't really understand the depths of pdb and can't tell if this may cause adverse affects elsewhere.

(I would also love to write a test for this, but I have no idea if we can or want to drive the debugger automatically for tests?)

@rhelmot

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2018

...any updates on this? Is there something I can provide to help move this along?

@Carreau Carreau added this to the 7.0 milestone Sep 1, 2018

@Carreau

This comment has been minimized.

Copy link
Member

commented Sep 1, 2018

Sorry we missed the pull-request. I will have a look. Marking as 7.0 to make sure to have a look at it.

@Carreau

This comment has been minimized.

Copy link
Member

commented Sep 1, 2018

I believe most of the debugger test are in IPython/core/tests/test_debugger.py are doctests, and ran through a subprocess. Not 10% sure about that though.

@Carreau

This comment has been minimized.

Copy link
Member

commented Sep 1, 2018

Ok I was wrong for above, the only way I can see is to use subprocess and run python -i and check the output. Something similar to IPython/terminal/tests/test_embed.py

I'll see if I can write a scaffold for that.

@Carreau Carreau added the help wanted label Sep 6, 2018

@Carreau

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

Ok, I'm writing some test, but this seem to not be quite right:

In [1]: def f(x):
   ...:     raise Exception

In [2]: gen = (f(x) for x in [0])

In [3]: for x in gen:
   ...:     pass
---------------------------------------------------------------------------
Exception                                 Traceback (most recent call last)
<ipython-input-3-5b2155f9c4b2> in <module>
----> 1 for x in gen:
      2     pass

<ipython-input-2-b7f6c0ea3068> in <genexpr>(.0)
----> 1 gen = (f(x) for x in [0])

<ipython-input-1-a13efb0dffe1> in f(x)
      1 def f(x):
----> 2     raise Exception

Exception:


In [4]: %debug
> <ipython-input-1-a13efb0dffe1>(2)f()
      1 def f(x):
----> 2     raise Exception


ipdb> u
> <ipython-input-2-b7f6c0ea3068>(1)<genexpr>()
----> 1 gen = (f(x) for x in [0])

ipdb> u
> <ipython-input-3-5b2155f9c4b2>(1)<module>()
----> 1 for x in gen:
      2     pass

ipdb> u                                         |
> <ipython-input-1-a13efb0dffe1>(2)f()          |
      1 def f(x):                               |
----> 2     raise Exception                     | These two frames are wrong.
                                                |
ipdb> u                                         |
> <ipython-input-2-b7f6c0ea3068>(1)<genexpr>()  |
----> 1 gen = (f(x) for x in [0])               |
                                                
ipdb> u
*** Oldest frame

It show frames in the wrong place (or re-go through frames it should not)

I'm going to try to polish the tests framework to test that, and we can figure out what to do. I'm going to likely push this thing for 7.1

Fix `up` through generators in postmortem debugger
By passing the lowest traceback element into the debugger, we require it
to use frame.f_back to find older frames. However, f_back is always None
for generator frames. By providing a higher-up traceback element, pdb
can traverse down the traceback.tb_next links, which do work correctly
across generator calls.

Furthermore, pdb will not do the right thing if it is provided with a
frame/traceback pair that do not correspond to each other - it will
duplicate parts of the callstack. Instead, we provide None as a frame,
which will cause the debugger to start at the deepest frame, which is
the desired behavior here.

@rhelmot rhelmot force-pushed the rhelmot:master branch from e8a2f2a to 67eed53 Sep 18, 2018

@rhelmot

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2018

Okay, I read the pdb/bdb source more carefully and force pushed what should be a real fix. Explanation in the commit message.

The takeaway here for future reference is that the only place the frame passed into interaction is used is in Bdb.get_stack, which walks the stack backwards from the frame and forwards from the traceback to construct the full list of stack frames. The frame can be None and this will still work, but if the traceback and frame don't correspond to the same frame, it will duplicate parts of the stack.

@Carreau

This comment has been minimized.

Copy link
Member

commented Sep 19, 2018

Thanks, I'll have a look, thy to commit a test framework (or at least a draft of it).

@Carreau

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

Ok, I just pushed what should be test for this patch.

It's a bit cumbersome to write, as you will see, but let's not optimize until we actually have need to write this often.

@Carreau

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

Once test passes i'll merge.
Many thanks for your patience.

@meeseeksdev[bot] untag help wanted
@meeseeksdev[bot] tag hacktoberfest

@meeseeksdev meeseeksdev bot added Hacktoberfest and removed help wanted labels Oct 12, 2018

@Carreau Carreau force-pushed the rhelmot:master branch from 70f0c1a to 5cc53e8 Oct 12, 2018

@Carreau

This comment has been minimized.

Copy link
Member

commented Oct 13, 2018

Tests are passing, let's get that in, many thanks !

@Carreau Carreau merged commit 16d772f into ipython:master Oct 13, 2018

4 checks passed

codecov/patch 97.56% of diff hit (target 0%)
Details
codecov/project 68.28% (+0.04%) compared to eb8dac3
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.