gh-50571: Check bytecode before accepting a breakpoint in pdb#145121
gh-50571: Check bytecode before accepting a breakpoint in pdb#145121aisk wants to merge 4 commits intopython:mainfrom
Conversation
| fname = db1.canonic(__file__) | ||
| db1.set_break(__file__, 1) | ||
| self.assertEqual(db1.get_all_breaks(), {fname: [1]}) | ||
| db1.set_break(__file__, 51) |
There was a problem hiding this comment.
These change are because the old position is just doc strings which don't have an associate bytecode, so it will cause error. Change them to L51 which have a real statement.
There was a problem hiding this comment.
Let's add some comments here saying this test is sensitive to the test file itself. So people won't get confused if they add something in the header comment and make the test fail. Just explain that the corresponding line numbers have to be associated with byte codes.
gaogaotiantian
left a comment
gaogaotiantian
left a comment
There was a problem hiding this comment.
Overall I think it's a good fix. Also relatively safe. I have a few commonts.
| if lineno is not None: | ||
| linenos.add(lineno) | ||
| for const in code.co_consts: | ||
| if hasattr(const, 'co_lines'): |
There was a problem hiding this comment.
Let's check isinstance(const, CodeType) (from types) instead of co_lines.
| line = linecache.getline(filename, lineno) | ||
| if not line: | ||
| return 'Line %s:%d does not exist' % (filename, lineno) | ||
| source = ''.join(linecache.getlines(filename)) |
There was a problem hiding this comment.
Let's cache this internally. Build a {filename: lineno} cache. Having some overhead for setting breakpoint is fine, but compile + co_lines() is pretty expensive. Let's avoid wasting extra time on it.
| @@ -0,0 +1,3 @@ | |||
| :mod:`bdb` will reports an error when set breakpoint on a line with no | |||
There was a problem hiding this comment.
| :mod:`bdb` will reports an error when set breakpoint on a line with no | |
| :mod:`bdb` will report an error when setting breakpoint on a line with no |
| fname = db1.canonic(__file__) | ||
| db1.set_break(__file__, 1) | ||
| self.assertEqual(db1.get_all_breaks(), {fname: [1]}) | ||
| db1.set_break(__file__, 51) |
There was a problem hiding this comment.
Let's add some comments here saying this test is sensitive to the test file itself. So people won't get confused if they add something in the header comment and make the test fail. Just explain that the corresponding line numbers have to be associated with byte codes.
2 participants
Collect all linenos in a code object recursively, and check if the target breakpoint is in them. If not, report an error, just like set break point on blank lines.