Skip to content

gh-59013: Make line number of function breakpoint more precise #110582

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Oct 27, 2023
Merged

gh-59013: Make line number of function breakpoint more precise #110582

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5485aee
Make line number of function breakpoint more precise
gaogaotiantian Oct 9, 2023
fb467eb
Check None for lineno
gaogaotiantian Oct 9, 2023
80322d0
📜🤖 Added by blurb_it.
blurb-it[bot] Oct 9, 2023
b6d95e3
Merge branch 'main' into pdb-func-break
gaogaotiantian Oct 12, 2023
3955333
Add generator test and use instr after RESUME
gaogaotiantian Oct 12, 2023
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion Lib/pdb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -880,7 +880,7 @@ def do_break(self, arg, temporary = 0):
#use co_name to identify the bkpt (function names
#could be aliased, but co_name is invariant)
funcname = code.co_name
lineno = code.co_firstlineno
lineno = self._find_first_executable_line(code)
filename = code.co_filename
except:
# last thing to try
Expand Down Expand Up @@ -983,6 +983,16 @@ def checkline(self, filename, lineno):
return 0
return lineno

def _find_first_executable_line(self, code):
""" Try to find the first executable line of the code object.

Return code.co_firstlineno if no executable line is found.
"""
for instr in dis.get_instructions(code):
if instr.opname != 'RESUME' and instr.positions.lineno is not None:
Copy link
Member

@iritkatriel iritkatriel Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might not always work:

>>> def f(): ... yield 42 ... >>> dis.dis(f) 1 0 RETURN_GENERATOR None 2 POP_TOP 1 4 RESUME 0 2 6 LOAD_CONST 1 (42) 8 YIELD_VALUE 1 10 RESUME 1 12 POP_TOP 14 RETURN_CONST 0 (None) None >> 16 CALL_INTRINSIC_1 3 (INTRINSIC_STOPITERATION_ERROR) 18 RERAISE 1 ExceptionTable: 4 to 14 -> 16 [0] lasti >>>
Copy link
Member Author

@gaogaotiantian gaogaotiantian Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True - it's not "worse" than the current solution though.

Actually would it be more reasonable to use the line number of the instruction after RESUME?

Now that I think about it, generators probabaly have more problems with function breakpoints - when I set a breakpoint on a generator function, I'd hope that the breakpoint is hit every time the function is entered right? And pdb is not able to do that now - it stores the line that executed first and break on that line. We could potentially enter the generator on a different line. So the problem is more serious already on generators.
Copy link
Member

@iritkatriel iritkatriel Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually would it be more reasonable to use the line number of the instruction after RESUME?

I think the code object already has that in a field called _co_firsttraceable.
Copy link
Member

@iritkatriel iritkatriel Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think about it, generators probabaly have more problems with function breakpoints

I can believe that. When you call a generator function it creates a generator object and returns it. Then you repeatedly call the generator object (which executes the same code, past the point of the RETURN_GENERATOR).
Copy link
Member Author

@gaogaotiantian gaogaotiantian Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code object already has that in a field called _co_firsttraceable.

Yes and I don't think it was exposed to Python level.

I can believe that. When you call a generator function it creates a generator object and returns it. Then you repeatedly call the generator object (which executes the same code, past the point of the RETURN_GENERATOR).

I caused an assertion error when I was trying to test a little bit more with generators - I'll investigate into it.

From a user's point, what would be the expected behavior if they set a breakpoint to the generator function? Do they want a break when the generator is being created (so actually RETURN_GENERATOR)? That's a valid call. Or do they want a break when the "first time" the generator is executed? Or every time the generator is executed? Those are three different behaviors and the third one has issues with display the line number.
Copy link
Member

@iritkatriel iritkatriel Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect it to be the first time the generator is executed. The next time it will start executing after some yield, and that's not the first line of the function.
Copy link
Member Author

@gaogaotiantian gaogaotiantian Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, we can use the RESUME method I mentioned above (which I think is basically how _co_firsttraceable works). Or do you think we should expose that member to Python?
Copy link
Member

@iritkatriel iritkatriel Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can re-implement it. As long as we have a test that will give us a heads up when it needs to change it should be ok.

@markshannon do you agree?
Copy link
Member Author

@gaogaotiantian gaogaotiantian Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the line searching method to use the instruction after RESUME. Also a generator test case is added.

There is one thing that I realized - if you do break func before the func is defined(evaluated), you'll still get a line number at the function definition - it uses re to find the function.

Is it better to have a consistant wrong answer, or a partially correct one?
return instr.positions.lineno
return code.co_firstlineno

def do_enable(self, arg):
"""enable bpnumber [bpnumber ...]

Expand Down
38 changes: 37 additions & 1 deletion Lib/test/test_pdb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1516,7 +1516,7 @@ def test_next_until_return_at_return_event():
> <doctest test.test_pdb.test_next_until_return_at_return_event[1]>(3)test_function()
-> test_function_2()
(Pdb) break test_function_2
Breakpoint 1 at <doctest test.test_pdb.test_next_until_return_at_return_event[0]>:1
Breakpoint 1 at <doctest test.test_pdb.test_next_until_return_at_return_event[0]>:2
(Pdb) continue
> <doctest test.test_pdb.test_next_until_return_at_return_event[0]>(2)test_function_2()
-> x = 1
Expand Down Expand Up @@ -2350,6 +2350,42 @@ def test_pdb_ambiguous_statements():
(Pdb) continue
"""

def test_pdb_function_break():
"""Testing the line number of break on function

>>> def foo(): pass

>>> def bar():
...
... pass

>>> def boo():
... # comments
... global x
... x = 1

>>> def test_function():
... import pdb; pdb.Pdb(nosigint=True, readrc=False).set_trace()
... pass

>>> with PdbTestInput([ # doctest: +ELLIPSIS +NORMALIZE_WHITESPACE
... 'break foo',
... 'break bar',
... 'break boo',
... 'continue'
... ]):
... test_function()
> <doctest test.test_pdb.test_pdb_function_break[3]>(3)test_function()
-> pass
(Pdb) break foo
Breakpoint ... at <doctest test.test_pdb.test_pdb_function_break[0]>:1
(Pdb) break bar
Breakpoint ... at <doctest test.test_pdb.test_pdb_function_break[1]>:3
(Pdb) break boo
Breakpoint ... at <doctest test.test_pdb.test_pdb_function_break[2]>:4
(Pdb) continue
"""


@support.requires_subprocess()
class PdbTestCase(unittest.TestCase):
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2023-10-09-23-59-04.gh-issue-59013.qPbS-G.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Make line number of function breakpoint more precise in :mod:`pdb`