Re: Patch: saving breakpoints

[Fernando Nasser <fnasser at redhat dot com>]

Tom Tromey wrote:
> 
> I've wanted Insight to save breakpoint information in sessions for a
> long time now.  Tonight I blew off my other volunteer hacking (writing
> java.awt or working on the automake 1.5 release) to do it.
> 
> First a note about saving breakpoints.  I believe that it makes the
> most sense to save the breakpoint as the user typed it.  If I type
> `b function_name', and then go off and hack, when I come back I expect
> the breakpoint to be on function_name -- not some random source
> location which corresponds only to where `function_name' used to be.
> 
> For breakpoints I set in Insight I expect the location to be the
> file/line number at which I set them in the GUI.
> 
> So, this is what I implemented.  I know this area is a bit
> controversial -- Fernando and I have disagreed about it before.  I
> still think this makes sense though.  I think we all agree that, since
> sources can change in any way, there is no perfect approach to saving
> breakpoints, only heuristics with varying degrees of accuracy.
> 

OK, here is my philosophy.  We have nothing (we don't save anything
about breakpoints).  Tom Tromey offers to do it (actually it does it in
his spare time).  So, this automatically becomes the best way of doing
it (at least at this moment).

I still don't like the approach much, but neither you opinion or mine is
based in real data.  I guess we will only know if we try, so that is why
I suggest the other that we should get you patches in and try it.

If: 1) The experience shows us that may be a better way
AND
2) Someone does find the time to implement another algorithm
then we can always change or improve the heuristic in the future.
But it is time to move ahead and add some new features.  

My vote is that Tom patches should go in.

P.S.: Last time I asked Tom to postpone it because we had some other
priorities and the code he needed to change was undergoing some
changes.  It was just bad timing.


> Anyway, I implemented what I think is most in line with user
> expectation.  I'm willing to discuss it though.
> 
> Second, this patch is not perfect.  However, the flaws are unlikely to
> be noticeable by the casual user, and furthermore they are fixable
> without requiring a major revamp of the breakpoint-saving machinery.
> That is, the fixes to the problems I know of can be done
> incrementally, and I think this patch unambiguously represents forward
> motion (with one possible exception, see the last bullet).
> 
> Some problems:
> 
> * We don't handle the breakpoint language.  This info isn't returned
>   by gdb_get_breakpoint_info, and by the time I realized we might want
>   it I didn't feel like going back and adding it.  Plus at this point
>   I think it would be more appropriate to change
>   gdb_get_breakpoint_info to return something other than a huge list.
> 
> * We don't handle the input radix when setting the breakpoint
>   condition.  To me it seems like there is some real confusion here.
>   It seems to me that the condition and the breakpoint commands should
>   each hold their own language and input radix state, neither of which
>   is done right now.  This change is large enough that I didn't even
>   consider it.
> 
> * (I just noticed this and it is only tangentially related: we should
>   save signal-handling info too.  I don't use this feature much so it
>   is no suprise it slipped my mind.)
> 
> * I added new uses of gdb_cmd.  I know this is wrong.
>   Unfortunately fixing this means writing a a new command in gdbtk-bp.c,
>   since the existing bp commands are still insufficient.
>   This is a real problem.  If we want to require no new gdb_cmd, that
>   is fine, but it will probably be some time before I can work on this
>   again.  In that case I'll just save it in my patch repository until
>   the next time.  The deeper problem here is that as the new Tcl
>   commands are written, they are done in an ad hoc way.  This means
>   all kinds of new functionality require new C code (or a change in
>   the C code and corresponding massive sed-like changes on the Tcl
>   source -- as in this patch).  To me it seems like every time we do
>   this we're increasing the maintenance problem and making the C code
>   harder to follow.  Some automated wrapper solution would be nice.
>   Yeah, I realize this is huge.
> 
> Is this ok to commit?
> 
> 2001-05-11  Tom Tromey  <tromey@redhat.com>
> 
>         * library/session.tcl (session_save): Save breakpoints.
>         (SESSION_serialize_bps): New proc.
>         (SESSION_recreate_bps): New proc.
>         (session_load): Recreate breakpoints.
>         * library/util.tcl (bp_exists): Expect user specification in
>         breakpoint info.
>         * library/srctextwin.itb (SrcTextWin::showBPBalloon): Expect user
>         specification in breakpoint info.
>         * library/gdbevent.itb (BreakpointEvent::_init): Initialize
>         _user_specification.
>         (BreakpointEvent::get): Handle user_specification.
>         * library/gdbevent.ith (BreakpointEvent): Added
>         _user_specification field.
>         * library/bpwin.itb (BpWin::bp_store): Expect user specification
>         and use it when saving.
>         (BpWin::bp_type): Expect user specification.
>         * generic/gdbtk-bp.c (BREAKPOINT_IS_WATCHPOINT): New macro.
>         (gdb_get_breakpoint_info): Added `user specification' to result.
> 
> Tom
> 
> Index: generic/gdbtk-bp.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbtk/generic/gdbtk-bp.c,v
> retrieving revision 1.5
> diff -u -r1.5 gdbtk-bp.c
> --- gdbtk-bp.c  2001/05/11 20:01:57     1.5
> +++ gdbtk-bp.c  2001/05/12 01:27:59
> @@ -61,6 +61,13 @@
>   || (bp)->type == bp_read_watchpoint     \
>   || (bp)->type == bp_access_watchpoint)
> 
> +/* Is this breakpoint a watchpoint?  */
> +#define BREAKPOINT_IS_WATCHPOINT(bp)                                         \
> +((bp)->type == bp_watchpoint                                                 \
> + || (bp)->type == bp_hardware_watchpoint                                     \
> + || (bp)->type == bp_read_watchpoint                                         \
> + || (bp)->type == bp_access_watchpoint)
> +
>  /*
>   * These are routines we need from breakpoint.c.
>   * at some point make these static in breakpoint.c and move GUI code there
> @@ -279,7 +286,7 @@
>   * Tcl Result:
>   *   A list with {file, function, line_number, address, type, enabled?,
>   *                disposition, ignore_count, {list_of_commands},
> - *                condition, thread, hit_count}
> + *                condition, thread, hit_count user_specification}
>   */
>  static int
>  gdb_get_breakpoint_info (ClientData clientData, Tcl_Interp *interp, int objc,
> @@ -355,6 +362,11 @@
>                             Tcl_NewIntObj (b->thread));
>    Tcl_ListObjAppendElement (NULL, result_ptr->obj_ptr,
>                             Tcl_NewIntObj (b->hit_count));
> +
> +  Tcl_ListObjAppendElement (NULL, result_ptr->obj_ptr,
> +                           Tcl_NewStringObj (BREAKPOINT_IS_WATCHPOINT (b)
> +                                             ? b->exp_string
> +                                             : b->addr_string, -1));
> 
>    return TCL_OK;
>  }
> Index: library/bpwin.itb
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbtk/library/bpwin.itb,v
> retrieving revision 1.5
> diff -u -r1.5 bpwin.itb
> --- bpwin.itb   2001/04/19 22:51:02     1.5
> +++ bpwin.itb   2001/05/12 01:28:00
> @@ -298,18 +298,21 @@
>    foreach breakpoint [gdb_get_breakpoint_list] {
>      # This is an lassign
>      foreach {file function line_no address type \
> -              enable_p disp ignore cmds thread hit_count} \
> +              enable_p disp ignore cmds thread hit_count user_spec} \
>        [gdb_get_breakpoint_info $breakpoint] {
>         break
>        }
> 
> -    if {$file != ""} {
> +    if {$user_spec != ""} {
> +      set bp_specifier $user_spec
> +    } elseif {$file != ""} {
>        set filename [file tail $file]
>        set bp_specifier $filename:$line_no
>      } else {
>        set bp_specifier *$address
>      }
> 
> +    # FIXME: doesn't handle watchpoints.
>      if {[string compare $disp "delete"] == 0} {
>        puts $outH "tbreak $bp_specifier"
>      } else {
> @@ -542,7 +545,7 @@
>    #debug "bp_type $i $bpnum"
>    set bpinfo [gdb_get_breakpoint_info $bpnum]
>    lassign $bpinfo file func line pc type enabled disposition \
> -    ignore_count commands cond thread hit_count
> +    ignore_count commands cond thread hit_count user_spec
>    bp_select $i
>    switch $disposition {
>      delete {
> Index: library/gdbevent.itb
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbtk/library/gdbevent.itb,v
> retrieving revision 1.2
> diff -u -r1.2 gdbevent.itb
> --- gdbevent.itb        2001/04/20 17:20:02     1.2
> +++ gdbevent.itb        2001/05/12 01:28:00
> @@ -31,8 +31,9 @@
>      condition    { return $_condition }
>      thread       { return $_thread }
>      hit_count    { return $_hit_count }
> +    user_specification { return $_user_specification }
> 
> -    default { error "unknown event data \"$what\": should be: action|number|file|function|line|address|type|enabled|disposition|ignore_count|commands|condition|thread|hit_count" }
> +    default { error "unknown event data \"$what\": should be: action|number|file|function|line|address|type|enabled|disposition|ignore_count|commands|condition|thread|hit_count|user_specification" }
>    }
>  }
> 
> @@ -53,6 +54,7 @@
>      set _condition    {}
>      set _thread       {}
>      set _hit_count    {}
> +    set _user_specification {}
>    } else {
>      lassign $bpinfo \
>        _file         \
> @@ -66,7 +68,8 @@
>        _commands     \
>        _condition    \
>        _thread       \
> -      _hit_count
> +      _hit_count    \
> +      _user_specification
>    }
>  }
> 
> Index: library/gdbevent.ith
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbtk/library/gdbevent.ith,v
> retrieving revision 1.2
> diff -u -r1.2 gdbevent.ith
> --- gdbevent.ith        2001/04/20 17:20:02     1.2
> +++ gdbevent.ith        2001/05/12 01:28:00
> @@ -40,6 +40,8 @@
>  # condition .... BP condition
>  # thread ....... thread in which BP is set (or -1 for all threads)
>  # hit_count .... number of times BP has been hit
> +# user_specification
> +#             .. text the user initially used to set this breakpoint
>  class BreakpointEvent {
>    inherit GDBEvent
> 
> @@ -71,6 +73,7 @@
>    private variable _condition    {}
>    private variable _thread       {}
>    private variable _hit_count    {}
> +  private variable _user_specification {}
> 
>    private method _init {}
>  }
> Index: library/session.tcl
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbtk/library/session.tcl,v
> retrieving revision 1.6
> diff -u -r1.6 session.tcl
> --- session.tcl 2001/04/18 17:44:00     1.6
> +++ session.tcl 2001/05/12 01:28:01
> @@ -11,6 +11,91 @@
>  # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>  # GNU General Public License for more details.
> 
> +# An internal function used when saving sessions.  Returns a string
> +# that can be used to recreate all pertinent breakpoint state.
> +proc SESSION_serialize_bps {} {
> +  set result {}
> +
> +  foreach bp_num [gdb_get_breakpoint_list] {
> +    lassign [gdb_get_breakpoint_info $bp_num] file function line_number \
> +      address type enabled disposition ignore_count command_list \
> +      condition thread hit_count user_specification
> +
> +    switch -glob -- $type {
> +      "breakpoint" -
> +      "hw breakpoint" {
> +       if {$disposition == "delete"} {
> +         set cmd tbreak
> +       } else {
> +         set cmd break
> +       }
> +
> +       append cmd " "
> +       if {$user_specification != ""} {
> +         append cmd "$user_specification"
> +       } elseif {$file != ""} {
> +         # BpWin::bp_store uses file tail here, but I think that is
> +         # wrong.
> +         append cmd "$file:$line_number"
> +       } else {
> +         append cmd "*$address"
> +       }
> +      }
> +      "watchpoint" -
> +      "hw watchpoint" {
> +       set cmd watch
> +       if {$user_specification != ""} {
> +         append cmd " $user_specification"
> +       } else {
> +         # There's nothing sensible to do.
> +         continue
> +       }
> +      }
> +
> +      "catch*" {
> +       # FIXME: Don't know what to do.
> +       continue
> +      }
> +
> +      default {
> +       # Can't serialize anything other than those listed above.
> +       continue
> +      }
> +    }
> +
> +    lappend result [list $cmd $enabled $condition $command_list]
> +  }
> +
> +  return $result
> +}
> +
> +# An internal function used when loading sessions.  It takes a
> +# breakpoint string and recreates all the breakpoints.
> +proc SESSION_recreate_bps {specs} {
> +  foreach spec $specs {
> +    lassign $spec create enabled condition commands
> +
> +    # Create the breakpoint
> +    gdb_cmd $create
> +
> +    # Below we use `\$bpnum'.  This means we don't have to figure out
> +    # the number of the breakpoint when doing further manipulations.
> +
> +    if {! $enabled} {
> +      gdb_cmd "disable \$bpnum"
> +    }
> +
> +    if {$condition != ""} {
> +      gdb_cmd "cond \$bpnum $condition"
> +    }
> +
> +    if {[llength $commands]} {
> +      lappend commands end
> +      gdb_cmd "commands \$bpnum\n[join $commands \n]"
> +    }
> +  }
> +}
> +
>  #
>  # This procedure decides what makes up a gdb `session'.  Roughly a
>  # session is whatever the user found useful when debugging a certain
> @@ -39,6 +124,9 @@
>    set values(pwd) $gdb_current_directory
>    set values(target) $gdb_target_name
> 
> +  # Breakpoints.
> +  set values(breakpoints) [SESSION_serialize_bps]
> +
>    # Recompute list of recent sessions.  Trim to no more than 5 sessions.
>    set recent [concat [list $name] \
>                 [lremove [pref getd gdb/recent-projects] $name]]
> @@ -86,6 +174,10 @@
>      gdb_clear_file
>      set_exe_name $values(executable)
>      set_exe
> +  }
> +
> +  if {[info exists values(breakpoints)]} {
> +    SESSION_recreate_bps $values(breakpoints)
>    }
> 
>    if {[info exists values(target)]} {
> Index: library/srctextwin.itb
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbtk/library/srctextwin.itb,v
> retrieving revision 1.24
> diff -u -r1.24 srctextwin.itb
> --- srctextwin.itb      2001/04/20 18:47:33     1.24
> +++ srctextwin.itb      2001/05/12 01:28:03
> @@ -2312,7 +2312,7 @@
>    foreach b $bps {
>      set bpinfo [gdb_get_breakpoint_info $b]
>      lassign $bpinfo file func linenum addr type enabled disposition \
> -      ignore_count commands cond thread hit_count
> +      ignore_count commands cond thread hit_count user_specification
>      if {$thread == "-1"} {set thread "all"}
>      set file [lindex [file split $file] end]
>      if {$enabled} {
> Index: library/util.tcl
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbtk/library/util.tcl,v
> retrieving revision 1.6
> diff -u -r1.6 util.tcl
> --- util.tcl    2001/05/03 18:13:21     1.6
> +++ util.tcl    2001/05/12 01:28:04
> @@ -175,7 +175,7 @@
>    foreach bpnum $bps {
>      set bpinfo [gdb_get_breakpoint_info $bpnum]
>      lassign $bpinfo file func line pc type enabled disposition \
> -      ignore_count commands cond thread hit_count
> +      ignore_count commands cond thread hit_count user_specification
>      if {$filename == $file && $function == $func && $addr == $pc} {
>        return $bpnum
>      }

-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9