[Patches] [ python-Patches-1003640 ] fix for bugs 976878, 926369, 875404 (pdb bkpt handling)

Wed Aug 25 02:53:54 CEST 2004

Patches item #1003640, was opened at 2004-08-04 16:23
Message generated for change (Comment added) made by isandler
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1003640&group_id=5470

Category: Library (Lib)
Group: None
Status: Open
Resolution: None
Priority: 5
Submitted By: Ilya Sandler (isandler)
Assigned to: Nobody/Anonymous (nobody)
Summary: fix for bugs 976878, 926369, 875404 (pdb bkpt handling)

Initial Comment:
This patch fixes the following bugs:

#976878 PDB: unreliable breakpoints on functions      

#926369 pdb sets breakpoints in the wrong location
#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()..

----------------------------------------------------------------------

>Comment By: Ilya Sandler (isandler)
Date: 2004-08-24 17:53

Message:
Logged In: YES 
user_id=971153

attaching the new patch

----------------------------------------------------------------------

Comment By: Ilya Sandler (isandler)
Date: 2004-08-22 09:32

Message:
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

----------------------------------------------------------------------

Comment By: Ilya Sandler (isandler)
Date: 2004-08-14 20:30

Message:
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).

----------------------------------------------------------------------

Comment By: Johannes Gijsbers (jlgijsbers)
Date: 2004-08-14 03:40

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

----------------------------------------------------------------------

Comment By: Ilya Sandler (isandler)
Date: 2004-08-13 12:24

Message:
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

----------------------------------------------------------------------

Comment By: Johannes Gijsbers (jlgijsbers)
Date: 2004-08-13 08:13

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

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1003640&group_id=5470