Patch: saving breakpoints

[Tom Tromey <tromey at redhat dot com>]

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.

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
     }