fix for bugs 976878, 926369, 875404 (pdb bkpt handling)

[isandler]

BPO	1003640
Files	pdb.patch2-v1 pdb.patch2-v2

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2004-08-30.13:31:46.000>
created_at = <Date 2004-08-04.23:23:12.000>
labels = ['library']
title = 'fix for bugs 976878, 926369, 875404 (pdb bkpt handling)'
updated_at = <Date 2004-08-30.13:31:46.000>
user = 'https://bugs.python.org/isandler'

bugs.python.org fields:

activity = <Date 2004-08-30.13:31:46.000>
actor = 'jlgijsbers'
assignee = 'none'
closed = True
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2004-08-04.23:23:12.000>
creator = 'isandler'
dependencies = []
files = ['6137', '6138']
hgrepos = []
issue_num = 1003640
keywords = ['patch']
message_count = 8.0
messages = ['46529', '46530', '46531', '46532', '46533', '46534', '46535', '46536']
nosy_count = 2.0
nosy_names = ['isandler', 'jlgijsbers']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue1003640'
versions = []

[isandler]

This patch fixes the following bugs:

bpo-976878 PDB: unreliable breakpoints on functions

bpo-926369 pdb sets breakpoints in the wrong location
bpo-875404 global stmt causes breakpoints to be ignored

Discussion:

all 3 bugs are caused by faults in pdb.checkline()
logic when it tries to determine the first executable
line of a function body.

checkline() did its job by doing basic parsing of the
function body. The parsing was missing quite a few
cases where checkline() returned either a wrong or
non-executable line of code...

The problem only happened when a breakpoint was set via
function name:
"b some_func"

Solution:

Instead of attempting to fix checkline() the patch
instead changes the breakpoint logic as follows

1. When a breakpoint is set via a func tionname
   1a) the bkpt gets the lineno of the def statement
   thus eliminating the parsing logic in checkline()
   1b) a new funcname attribute is attached to the
   breakpoint

2. bdb.break_here() is changed to detect and handle 2
   special cases
   2a) def statement is executed...No breakpoint is needed
   2b) a first executable line of a function with such
   a breakpoint is reached. Break is neded

Overall line count in pdb+bdb has actually gone down
slightly and I think this solution is cleaner than
attempting to expand parsing in checkline()..

[jlgijsbers]

Logged In: YES
user_id=469548

The patch works for me (and any patch that fixes three bugs
should really be commited <wink>), but I have some suggestions:

• The checkline() docstring needs to be updated.
• Try not to use multi-line comments after a statement
  (e.g.: 'return False #it's not a func call, but rather', etc.).
• For consistency, Breakpoint.breakingline should probably
  be called func_firstlineno.
• Moving the 'bp.breakingline=frame.f_lineno' statement into
  an else: branch of the 'if bp.breakingline and
  (bp.breakingline != frame.f_lineno)' statement would
  probably make the logic clearer.

[isandler]

Logged In: YES
user_id=971153

• The checkline() docstring needs to be updated.
• Try not to use multi-line comments after a statement
  (e.g.: 'return False #it's not a func call, but rather', etc.).

Ok, I can resubmit a patch for these two

• For consistency, Breakpoint.breakingline should probably
  be called func_firstlineno.

Well, I am not sure: func_firstlineno causes a wrong
association with
co_firstlineno (while these are almost never the same).
Furthermore,
the name ".breakingline" reflects the purpose: "a line
number where to break", while, "func_firstlineno" is totatly
neutral..

• Moving the 'bp.breakingline=frame.f_lineno' statement into
  an else: branch of the 'if bp.breakingline and
  (bp.breakingline != frame.f_lineno)' statement would
  probably make the logic clearer.

Are you suggesting to replace:

                if bp.breakingline and (bp.breakingline !=
frame.f_lineno):
                    return False 
                bp.breakingline=frame.f_lineno

with:

               if bp.breakingline and (bp.breakingline !=
frame.f_lineno):
                    return False 
               else: 
                    bp.breakingline=frame.f_lineno

Why do you think it's better? It's definitely more verbose, has
an extra indentation level and looking through the code I
see a lot of code which looks like:

if  (....):
    return
foo
bar
without the else: branch

Would the following be better?

if not bp.breakingline:
      #the function is entered for the 1st time
      bp.breakingline=frame.f_lineno
if  bp.breakingline != frame.f_lineno:
       return False 

Thus making it explicit that there are really 2 decisions
being made:

1. do we need to set the breakingline
2. do we need to break here

I don't have any strong feelings regarding these 2 issues,
just want to make sure that I understand the problem

[jlgijsbers]

Logged In: YES
user_id=469548

"Well, I am not sure: func_firstlineno causes a wrong
association with co_firstlineno (while these are almost
never the same)."

Ah, I misread the patch there. How about
func_first_executable_lineno (or something like that)? Or am
I off-base again?

"Would the following be better?"

if not bp.breakingline:
      #the function is entered for the 1st time
      bp.breakingline=frame.f_lineno
if  bp.breakingline != frame.f_lineno:
       return False 
"

Yes. I was just having a bit of a hard time following your
patch. I think this is clearer.

[isandler]

Logged In: YES
user_id=971153

Ok, I'll submit an updated patch hopefully by the end of the
next week (I will be out of town for the next few days).

[isandler]

Logged In: YES
user_id=971153

I am attaching a new version of the patch.

In addition to issues raised earlier the v1 version of patch
had a couple of other problems:

1. funcname based breakpoints got a wrong hit statistics
   (hit counter was incremented on every effective() call,
   which happened for every line of a function with a funcname
   bkpt)

2. If a user set a bkpt via line number at def statement
   (this is probably quite rare, but still possible) , then pdb
   would stop at every line of that function when the function
   is called

To fix these I had to move most of funcname handling logic
from break_here() into a separate function
(bdb.checkfuncname) and call it from bdb.effective()

Changes in the new version include:

1. Fix the issues with statistics and bkpts at def stmt

2. Fix a doc line of checkline()

3. Rename .breakingline into .func_first_executable_line

4. Comment fixes

[isandler]

Logged In: YES
user_id=971153

attaching the new patch

[jlgijsbers]

Logged In: YES
user_id=469548

Checked in as rev 1.45 of bdb.py and rev 1.69 pdb.py with
some modifications to comments and docstrings. Thanks for
the patch!