[PATCH:MI] Use observers for breakpoints

[PATCH:MI] Use observers for breakpoints

[Nick Roberts]

Following Aleksandar Ristovski's patch for catchpoints in GDB/MI, here's a
patch to demonstrate the use of observers for breakpoints.  This relates to
earlier patches I've submitted which uses event notification to communicate a
change in state rather than using command output directly.  Among other things
this allows the use of CLI commands with MI.

It's not a patch for immediate inclusion as it changes existing behaviour.
However I think it's something we should be aiming towards and would be
suitable for inclusion next time the MI level gets bumped.

Here's some sample output:


b main
&"b main\n"
=breakpoints-changed,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x080486d6",func="main",file="myprog.c",fullname="/home/nickrob/myprog.c",line="140",times="0",original-location="main"}
~"Breakpoint 1 at 0x80486d6: file myprog.c, line 140.\n"
^done
(gdb) 
dis 1
&"dis 1\n"
=breakpoints-changed,bkpt={number="1",type="breakpoint",disp="keep",enabled="n",addr="0x080486d6",func="main",file="myprog.c",fullname="/home/nickrob/myprog.c",line="140",times="0",original-location="main"}
^done
(gdb) 
cond 1 1==1
&"cond 1 1==1\n"
=breakpoints-changed,bkpt={number="1",type="breakpoint",disp="keep",enabled="n",addr="0x080486d6",func="main",file="myprog.c",fullname="/home/nickrob/myprog.c",line="140",cond="1==1",times="0",original-location="main"}
^done
(gdb) 
-break-insert main
=breakpoints-changed,bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="0x080486d6",func="main",file="myprog.c",fullname="/home/nickrob/myprog.c",line="140",times="0",original-location="main"}
^done
(gdb) 
watch i22
&"watch i22\n"
=breakpoints-changed,bkpt={number="3",type="watchpoint",disp="keep",enabled="y",addr="",what="i22",times="0",original-location="i22"}
~"Watchpoint 3: i22\n"
^done
(gdb) 
-break-watch i22
=breakpoints-changed,bkpt={number="4",type="watchpoint",disp="keep",enabled="y",addr="",what="i22",times="0",original-location="i22"}
^done,wpt={number="4",exp="i22"}
(gdb) 
ignore 1 4
&"ignore 1 4\n"
=breakpoints-changed,bkpt={number="1",type="breakpoint",disp="keep",enabled="n",addr="0x080486d6",func="main",file="myprog.c",fullname="/home/nickrob/myprog.c",line="140",cond="1==1",times="0",ignore="4",original-location="main"}
~"Will ignore next 4 crossings of breakpoint 1.\n"
^done
(gdb) 


-- 
Nick                                           http://www.inet.net.nz/~nickrob


2008-06-01  Nick Roberts  <nickrob@snap.net.nz>

	* mi/mi-interp.c (mi_breakpoints_changed): New function.
	(mi_interpreter_init): Register mi_breakpoints_changed as
	breakpoints_changed observer.

	* mi/mi-cmd-break.c (breakpoint_notify, breakpoint_hooks): Delete.
	(mi_cmd_break_insert): Don't use deprecated_set_gdb_event_hooks.

	* breakpoint.c (condition_command, commands_command)
	(commands_from_control_command, mention, delete_breakpoint)
	(set_ignore_count, disable_breakpoint, do_enable_breakpoint): 
	Call observer_notify_breakpoints_changed.
	(gdb_breakpoint_query): Rename to...
	(breakpoint_query): ...this.

	* breakpoint.h: Declare breakpoint_query.

	* gdb.h: Delete extern for gdb_breakpoint_query.

2008-06-01  Nick Roberts  <nickrob@snap.net.nz>

	* observer.texi (GDB Observers): New observer "breakpoints_changed".


Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.322
diff -p -u -r1.322 breakpoint.c
--- breakpoint.c	28 May 2008 14:04:21 -0000	1.322
+++ breakpoint.c	1 Jun 2008 02:17:21 -0000
@@ -621,6 +621,7 @@ condition_command (char *arg, int from_t
 	      }
 	  }
 	breakpoints_changed ();
+	observer_notify_breakpoints_changed (b->number);
 	breakpoint_modify_event (b->number);
 	return;
       }
@@ -660,6 +661,7 @@ commands_command (char *arg, int from_tt
 	free_command_lines (&b->commands);
 	b->commands = l;
 	breakpoints_changed ();
+	observer_notify_breakpoints_changed (b->number);
 	breakpoint_modify_event (b->number);
 	return;
     }
@@ -706,6 +708,7 @@ commands_from_control_command (char *arg
 	   list after it finishes execution.  */
 	b->commands = copy_command_lines (cmd->body_list[0]);
 	breakpoints_changed ();
+	observer_notify_breakpoints_changed (b->number);
 	breakpoint_modify_event (b->number);
 	return simple_control;
       }
@@ -3856,18 +3859,15 @@ do_captured_breakpoint_query (struct ui_
   return GDB_RC_NONE;
 }
 
-enum gdb_rc
-gdb_breakpoint_query (struct ui_out *uiout, int bnum, char **error_message)
+void
+breakpoint_query (int bnum)
 {
   struct captured_breakpoint_query_args args;
   args.bnum = bnum;
   /* For the moment we don't trust print_one_breakpoint() to not throw
      an error. */
-  if (catch_exceptions_with_msg (uiout, do_captured_breakpoint_query, &args,
-				 error_message, RETURN_MASK_ALL) < 0)
-    return GDB_RC_FAIL;
-  else
-    return GDB_RC_OK;
+  catch_exceptions_with_msg (uiout, do_captured_breakpoint_query, &args,
+			     NULL, RETURN_MASK_ALL);
 }
 
 /* Return non-zero if B is user settable (breakpoints, watchpoints,
@@ -4882,6 +4882,7 @@ mention (struct breakpoint *b)
      been done for deprecated_delete_breakpoint_hook and so on.  */
   if (deprecated_create_breakpoint_hook)
     deprecated_create_breakpoint_hook (b);
+  observer_notify_breakpoints_changed (b->number);
   breakpoint_create_event (b->number);
 
   if (b->ops != NULL && b->ops->print_mention != NULL)
@@ -7126,6 +7127,7 @@ delete_breakpoint (struct breakpoint *bp
 
   if (deprecated_delete_breakpoint_hook)
     deprecated_delete_breakpoint_hook (bpt);
+  observer_notify_breakpoints_changed (b->number);
   breakpoint_delete_event (bpt->number);
 
   if (breakpoint_chain == bpt)
@@ -7633,6 +7635,7 @@ set_ignore_count (int bptnum, int count,
 			     count, bptnum);
 	}
       breakpoints_changed ();
+      observer_notify_breakpoints_changed (b->number);
       breakpoint_modify_event (b->number);
       return;
     }
@@ -7783,6 +7786,7 @@ disable_breakpoint (struct breakpoint *b
 
   if (deprecated_modify_breakpoint_hook)
     deprecated_modify_breakpoint_hook (bpt);
+  observer_notify_breakpoints_changed (bpt->number);
   breakpoint_modify_event (bpt->number);
 }
 
@@ -7908,6 +7912,7 @@ have been allocated for other watchpoint
   
   if (deprecated_modify_breakpoint_hook)
     deprecated_modify_breakpoint_hook (bpt);
+  observer_notify_breakpoints_changed (bpt->number);
   breakpoint_modify_event (bpt->number);
 }
 
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.72
diff -p -u -r1.72 breakpoint.h
--- breakpoint.h	4 May 2008 19:38:59 -0000	1.72
+++ breakpoint.h	1 Jun 2008 02:17:21 -0000
@@ -865,4 +865,8 @@ void breakpoint_restore_shadows (gdb_byt
 
 extern int breakpoints_always_inserted_mode (void);
 
+/* Print the specified breakpoint on GDB_STDOUT. (Eventually this
+   function will ``print'' the object on ``output''). */
+void breakpoint_query (int bnum);
+
 #endif /* !defined (BREAKPOINT_H) */
Index: gdb.h
===================================================================
RCS file: /cvs/src/src/gdb/gdb.h,v
retrieving revision 1.10
diff -p -u -r1.10 gdb.h
--- gdb.h	1 Feb 2008 16:24:46 -0000	1.10
+++ gdb.h	1 Jun 2008 02:17:21 -0000
@@ -41,12 +41,6 @@ enum gdb_rc {
   GDB_RC_OK = 2
 };
 
-
-/* Print the specified breakpoint on GDB_STDOUT. (Eventually this
-   function will ``print'' the object on ``output''). */
-enum gdb_rc gdb_breakpoint_query (struct ui_out *uiout, int bnum,
-				  char **error_message);
-
 /* Switch thread and print notification. */
 enum gdb_rc gdb_thread_select (struct ui_out *uiout, char *tidstr,
 			       char **error_message);
Index: mi/mi-cmd-break.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmd-break.c,v
retrieving revision 1.19
diff -p -u -r1.19 mi-cmd-break.c
--- mi/mi-cmd-break.c	1 Feb 2008 16:24:46 -0000	1.19
+++ mi/mi-cmd-break.c	1 Jun 2008 02:17:21 -0000
@@ -24,7 +24,6 @@
 #include "breakpoint.h"
 #include "gdb_string.h"
 #include "mi-getopt.h"
-#include "gdb-events.h"
 #include "gdb.h"
 #include "exceptions.h"
 
@@ -33,23 +32,6 @@ enum
     FROM_TTY = 0
   };
 
-/* Output a single breakpoint. */
-
-static void
-breakpoint_notify (int b)
-{
-  gdb_breakpoint_query (uiout, b, NULL);
-}
-
-
-struct gdb_events breakpoint_hooks =
-{
-  breakpoint_notify,
-  breakpoint_notify,
-  breakpoint_notify,
-};
-
-
 enum bp_type
   {
     REG_BP,
@@ -71,7 +53,6 @@ mi_cmd_break_insert (char *command, char
   char *condition = NULL;
   int pending = 0;
   struct gdb_exception e;
-  struct gdb_events *old_hooks;
   enum opt
     {
       HARDWARE_OPT, TEMP_OPT /*, REGEXP_OPT */ , CONDITION_OPT,
@@ -131,8 +112,6 @@ mi_cmd_break_insert (char *command, char
     error (_("mi_cmd_break_insert: Garbage following <location>"));
   address = argv[optind];
 
-  /* Now we have what we need, let's insert the breakpoint! */
-  old_hooks = deprecated_set_gdb_event_hooks (&breakpoint_hooks);
   /* Make sure we restore hooks even if exception is thrown.  */
   TRY_CATCH (e, RETURN_MASK_ALL)
     {
@@ -164,7 +143,7 @@ mi_cmd_break_insert (char *command, char
 			  _("mi_cmd_break_insert: Bad switch."));
 	}
     }
-  deprecated_set_gdb_event_hooks (old_hooks);
+
   if (e.reason < 0)
     throw_exception (e);
 
Index: mi/mi-interp.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-interp.c,v
retrieving revision 1.30
diff -p -u -r1.30 mi-interp.c
--- mi/mi-interp.c	3 May 2008 15:10:42 -0000	1.30
+++ mi/mi-interp.c	1 Jun 2008 02:17:22 -0000
@@ -68,6 +68,7 @@ static void mi_remove_notify_hooks (void
 
 static void mi_new_thread (struct thread_info *t);
 static void mi_thread_exit (struct thread_info *t);
+static void mi_breakpoints_changed (int bnum);
 
 static void *
 mi_interpreter_init (int top_level)
@@ -92,6 +93,7 @@ mi_interpreter_init (int top_level)
     {
       observer_attach_new_thread (mi_new_thread);
       observer_attach_thread_exit (mi_thread_exit);
+      observer_attach_breakpoints_changed (mi_breakpoints_changed);
     }
 
   return mi;
@@ -331,6 +333,27 @@ mi_thread_exit (struct thread_info *t)
   gdb_flush (mi->event_channel);
 }
 
+static void
+mi_breakpoints_changed (int bnum)
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+  struct interp *interp_to_use;
+  struct ui_out *old_uiout, *temp_uiout;
+  int version;
+
+  fprintf_unfiltered (mi->event_channel, "breakpoints-changed");
+  interp_to_use = top_level_interpreter ();
+  old_uiout = uiout;
+  temp_uiout = interp_ui_out (interp_to_use);
+  version = mi_version (temp_uiout);
+  temp_uiout = mi_out_new (version);
+  uiout = temp_uiout;
+  breakpoint_query (bnum);
+  mi_out_put (uiout, mi->event_channel);
+  uiout = old_uiout;
+  gdb_flush (mi->event_channel);
+}
+
 extern initialize_file_ftype _initialize_mi_interp; /* -Wmissing-prototypes */
 
 void
Index: doc/observer.texi
===================================================================
RCS file: /cvs/src/src/gdb/doc/observer.texi,v
retrieving revision 1.15
diff -p -u -r1.15 observer.texi
--- doc/observer.texi	3 May 2008 15:10:42 -0000	1.15
+++ doc/observer.texi	1 Jun 2008 02:17:22 -0000
@@ -137,3 +137,7 @@ The thread specified by @var{t} has been
 The thread specified by @var{t} has exited.
 @end deftypefun
 
+@deftypefun void breakpoints_changed (int @var{bnum})
+The thread specified by @var{t} has exited.
+@end deftypefun
+

Re: [PATCH:MI] Use observers for breakpoints

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Sun, 1 Jun 2008 14:46:04 +1200
> Cc: ghost@cs.msu.su
> 
> +@deftypefun void breakpoints_changed (int @var{bnum})
> +The thread specified by @var{t} has exited.

There's no argument @var{t} in the function's signature.

Re: [PATCH:MI] Use observers for breakpoints

[Nick Roberts]

 > > +@deftypefun void breakpoints_changed (int @var{bnum})
 > > +The thread specified by @var{t} has exited.
 > 
 > There's no argument @var{t} in the function's signature.

OK, thanks.  Too much cut and paste.  That should be:

+@deftypefun void breakpoints_changed (int @var{bnum})
+The breakpoint specified by @var{bnum} has changed or is a new breakpoint.


-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH:MI] Use observers for breakpoints

[Joel Brobecker]

> Following Aleksandar Ristovski's patch for catchpoints in GDB/MI,
> here's a patch to demonstrate the use of observers for breakpoints.
> This relates to earlier patches I've submitted which uses event
> notification to communicate a change in state rather than using
> command output directly.  Among other things this allows the use of
> CLI commands with MI.

I missed the earlier patch, but I think that the idea is sound.
I suggest you change the observer profile to take a breakpoint
rather than a breakpoint number, to avoid having going from
the breakpoint number back to the breakpoint itself should an
observer need it.

I'm wondering if it might be useful to create two specific events
for breakpoint created and deleted. Particularly for the "deleted"
event, where you end up outputing the entire description of a breakpoint
that is obsolete.

Just as an aside, I don't know how others would feel about that,
but I wouldn't mind seeing annotate.c:breakpoints_changed being
renamed to annotate_breakpoints_changed.

-- 
Joel

Re: [PATCH:MI] Use observers for breakpoints

[Nick Roberts]

 > > Following Aleksandar Ristovski's patch for catchpoints in GDB/MI,
 > > here's a patch to demonstrate the use of observers for breakpoints.
 > > This relates to earlier patches I've submitted which uses event
 > > notification to communicate a change in state rather than using
 > > command output directly.  Among other things this allows the use of
 > > CLI commands with MI.
 > 
 > I missed the earlier patch, but I think that the idea is sound.

Here's one reference:

http://sourceware.org/ml/gdb-patches/2008-04/msg00377.html

Like Daniel says, the frame-changed notification would probably fire too often,
but the thread-changed one still seems a good idea and I have submitted a patch
to do this with annotations too:

http://sourceware.org/ml/gdb-patches/2008-05/msg00611.html


 > I suggest you change the observer profile to take a breakpoint
 > rather than a breakpoint number, to avoid having going from
 > the breakpoint number back to the breakpoint itself should an
 > observer need it.

My observer just uses a function called breakpoint_query (based on
gdb_breakpoint_query) which only needs the breakpoint number.  I
think this is an internal detail which could easily be changed should
the need arise.


 > I'm wondering if it might be useful to create two specific events
 > for breakpoint created and deleted. Particularly for the "deleted"
 > event, where you end up outputing the entire description of a breakpoint
 > that is obsolete.

Sure, it's just a sketch.  There are three types of event:

breakpoint_create_event
breakpoint_modify_event
breakpoint_delete_event

I don't know if there's much to be gained in differentiating between
creating and modifying a breakpoint but it would certainly make sense
to have two observers, breakpoints_changed and breakpoints_deleted, say.

If this patch goes in we could start dismantling the events mechanism.  They're
only used in a few other places, e.g, tracepoints, and presumably observers
could be used there too.


 > Just as an aside, I don't know how others would feel about that,
 > but I wouldn't mind seeing annotate.c:breakpoints_changed being
 > renamed to annotate_breakpoints_changed.

This function was moved from breakpoint.c.  A couple of years ago I submitted
a patch to remove this and some other annotations, just keeping the level
three annotations.  If we are going to keep it, I suggest calling it
annotate_breakpoints_invalid after the name of the associated annotation
and to be consistent with annotate_frames_invalid.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH:MI] Use observers for breakpoints

[Joel Brobecker]

>  > I suggest you change the observer profile to take a breakpoint
>  > rather than a breakpoint number, to avoid having going from
>  > the breakpoint number back to the breakpoint itself should an
>  > observer need it.
> 
> My observer just uses a function called breakpoint_query (based on
> gdb_breakpoint_query) which only needs the breakpoint number.  I
> think this is an internal detail which could easily be changed should
> the need arise.

I would still prefer it if you passed the breakpoint rather than
the breakpoint number. I'd rather not have to change the interface
later if the breakpoint ended up being needed.

> breakpoint_create_event
> breakpoint_modify_event
> breakpoint_delete_event
> 
> I don't know if there's much to be gained in differentiating between
> creating and modifying a breakpoint but it would certainly make sense
> to have two observers, breakpoints_changed and breakpoints_deleted, say.

We can indeed start with 2 for now.

>  > Just as an aside, I don't know how others would feel about that,
>  > but I wouldn't mind seeing annotate.c:breakpoints_changed being
>  > renamed to annotate_breakpoints_changed.
> 
> This function was moved from breakpoint.c.  A couple of years ago I
> submitted a patch to remove this and some other annotations, just
> keeping the level three annotations.  If we are going to keep it, I
> suggest calling it annotate_breakpoints_invalid after the name of the
> associated annotation and to be consistent with
> annotate_frames_invalid.

Sounds good to me.

-- 
Joel

Re: [PATCH:MI] Use observers for breakpoints

[Nick Roberts]

 > I would still prefer it if you passed the breakpoint rather than
 > the breakpoint number. I'd rather not have to change the interface
 > later if the breakpoint ended up being needed.

Sure, it's quite easy to do.

 > > breakpoint_create_event
 > > breakpoint_modify_event
 > > breakpoint_delete_event
 > > 
 > > I don't know if there's much to be gained in differentiating between
 > > creating and modifying a breakpoint but it would certainly make sense
 > > to have two observers, breakpoints_changed and breakpoints_deleted, say.
 > 
 > We can indeed start with 2 for now.

OK, I'll do that.

I'll submit another patch but it won't be anytime in the near future because I
have to look more carefully at the output of each type of breakpoint command.

As it breaks existing behaviour, the MI level would need to be bumped.  It
would make sense to do this at the same time as other similar pending changes,
e.g., no-stop mode, argument parsing etc

Perhaps we could aim for a release in which to do this, e.g. 7.0, in about
a year's time?

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH:MI] Use observers for breakpoints

[Vladimir Prus]

On Sunday 01 June 2008 06:46:04 Nick Roberts wrote:

> (gdb)
> -break-watch i22
> =breakpoints-changed,bkpt={number="4",type="watchpoint",disp="keep",e
>nabled="y",addr="",what="i22",times="0",original-location="i22"}
> ^done,wpt={number="4",exp="i22"}

Is this an output we actually want? ;-)
With one breakpoint mentioned twice?

> 2008-06-01  Nick Roberts  <nickrob@snap.net.nz>
> 
>         * mi/mi-interp.c (mi_breakpoints_changed): New function.
>         (mi_interpreter_init): Register mi_breakpoints_changed as
>         breakpoints_changed observer.
> 
>         * mi/mi-cmd-break.c (breakpoint_notify, breakpoint_hooks): Delete.
>         (mi_cmd_break_insert): Don't use deprecated_set_gdb_event_hooks.
> 
>         * breakpoint.c (condition_command, commands_command)
>         (commands_from_control_command, mention, delete_breakpoint)
>         (set_ignore_count, disable_breakpoint, do_enable_breakpoint): 
>         Call observer_notify_breakpoints_changed.

I wonder if you missed this bit in bpstat_stop_status:

	if (b->disposition == disp_disable)
	  {
	    b->enable_state = bp_disabled;
	    update_global_location_list ();
	  }

Probably, breakpoint_re_set_one should call the observer too.

In fact, it's probably would be nice to add call to the observer to
update_global_location_list. This function is called whenever we change any property 
of a  breakpoint that affects what should be inserted in the target. The only
issue is that right now update_global_location_list does not know which breakpoint
has changed -- we probably can add a parameter.

This won't catch all cases -- there are properties of breakpoints that are not 
reflected in the target -- like condition, commands and ignore count. But seems like
your patch has it covered already.

Is there any way to make -break-insert report the new breakpoint directly, as opposed to
via notification?


> Index: mi/mi-cmd-break.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/mi/mi-cmd-break.c,v
> retrieving revision 1.19
> diff -p -u -r1.19 mi-cmd-break.c
> --- mi/mi-cmd-break.c   1 Feb 2008 16:24:46 -0000       1.19
> +++ mi/mi-cmd-break.c   1 Jun 2008 02:17:21 -0000
> @@ -24,7 +24,6 @@
>  #include "breakpoint.h"
>  #include "gdb_string.h"
>  #include "mi-getopt.h"
> -#include "gdb-events.h"
>  #include "gdb.h"
>  #include "exceptions.h"
>  
> @@ -33,23 +32,6 @@ enum
>      FROM_TTY = 0
>    };
>  
> -/* Output a single breakpoint. */
> -
> -static void
> -breakpoint_notify (int b)
> -{
> -  gdb_breakpoint_query (uiout, b, NULL);
> -}
> -
> -
> -struct gdb_events breakpoint_hooks =
> -{
> -  breakpoint_notify,
> -  breakpoint_notify,
> -  breakpoint_notify,
> -};
> -
> -
>  enum bp_type
>    {
>      REG_BP,
> @@ -71,7 +53,6 @@ mi_cmd_break_insert (char *command, char
>    char *condition = NULL;
>    int pending = 0;
>    struct gdb_exception e;
> -  struct gdb_events *old_hooks;
>    enum opt
>      {
>        HARDWARE_OPT, TEMP_OPT /*, REGEXP_OPT */ , CONDITION_OPT,
> @@ -131,8 +112,6 @@ mi_cmd_break_insert (char *command, char
>      error (_("mi_cmd_break_insert: Garbage following <location>"));
>    address = argv[optind];
>  
> -  /* Now we have what we need, let's insert the breakpoint! */
> -  old_hooks = deprecated_set_gdb_event_hooks (&breakpoint_hooks);
>    /* Make sure we restore hooks even if exception is thrown.  */
>    TRY_CATCH (e, RETURN_MASK_ALL)
>      {
> @@ -164,7 +143,7 @@ mi_cmd_break_insert (char *command, char
>                           _("mi_cmd_break_insert: Bad switch."));
>         }
>      }
> -  deprecated_set_gdb_event_hooks (old_hooks);
> +
>    if (e.reason < 0)
>      throw_exception (e);
>  
> Index: mi/mi-interp.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/mi/mi-interp.c,v
> retrieving revision 1.30
> diff -p -u -r1.30 mi-interp.c
> --- mi/mi-interp.c      3 May 2008 15:10:42 -0000       1.30
> +++ mi/mi-interp.c      1 Jun 2008 02:17:22 -0000
> @@ -68,6 +68,7 @@ static void mi_remove_notify_hooks (void
>  
>  static void mi_new_thread (struct thread_info *t);
>  static void mi_thread_exit (struct thread_info *t);
> +static void mi_breakpoints_changed (int bnum);
>  
>  static void *
>  mi_interpreter_init (int top_level)
> @@ -92,6 +93,7 @@ mi_interpreter_init (int top_level)
>      {
>        observer_attach_new_thread (mi_new_thread);
>        observer_attach_thread_exit (mi_thread_exit);
> +      observer_attach_breakpoints_changed (mi_breakpoints_changed);
>      }
>  
>    return mi;
> @@ -331,6 +333,27 @@ mi_thread_exit (struct thread_info *t)
>    gdb_flush (mi->event_channel);
>  }
>  
> +static void
> +mi_breakpoints_changed (int bnum)
> +{
> +  struct mi_interp *mi = top_level_interpreter_data ();
> +  struct interp *interp_to_use;
> +  struct ui_out *old_uiout, *temp_uiout;
> +  int version;
> +
> +  fprintf_unfiltered (mi->event_channel, "breakpoints-changed");
> +  interp_to_use = top_level_interpreter ();
> +  old_uiout = uiout;
> +  temp_uiout = interp_ui_out (interp_to_use);
> +  version = mi_version (temp_uiout);
> +  temp_uiout = mi_out_new (version);
> +  uiout = temp_uiout;
> +  breakpoint_query (bnum);

Shall we have a helper function to do this creation of temporary uiout?

I think that except for compatibility issues due to -break-insert no longer
reporting the breakpoint that is created as part of ^done response, this
patch is good.

- Volodya

Re: [PATCH:MI] Use observers for breakpoints

[Nick Roberts]

Vladimir Prus writes:
 > On Sunday 01 June 2008 06:46:04 Nick Roberts wrote:
 > 
 > > (gdb)
 > > -break-watch i22
 > > =breakpoints-changed,bkpt={number="4",type="watchpoint",disp="keep",e
 > >nabled="y",addr="",what="i22",times="0",original-location="i22"}
 > > ^done,wpt={number="4",exp="i22"}
 >
 > Is this an output we actually want? ;-)
 > With one breakpoint mentioned twice?

I don't know why watchpoints are currently reported in a different way to
normal breakpoints or why type="watchpoint" isn't adequate and a special
field "wpt" is needed, but I would suggest removing the latter output.


 > I wonder if you missed this bit in bpstat_stop_status:
 > 
 > 	if (b->disposition == disp_disable)
 > 	  {
 > 	    b->enable_state = bp_disabled;
 > 	    update_global_location_list ();
 > 	  }

Could be.  At the moment, as I seaid to Joel, it's just a sketch.  I just used
the locations in breakpoint.c where the event functions were called.  If new
locations have been created since they were added then I will have missed them.

How does the breakpoint list change at this point?


 > Probably, breakpoint_re_set_one should call the observer too.
 > 
 > In fact, it's probably would be nice to add call to the observer to
 > update_global_location_list. This function is called whenever we change any
 > property of a breakpoint that affects what should be inserted in the
 > target. The only issue is that right now update_global_location_list does
 > not know which breakpoint has changed -- we probably can add a parameter.
 > 
 > This won't catch all cases -- there are properties of breakpoints that are
 > not reflected in the target -- like condition, commands and ignore
 > count. But seems like your patch has it covered already.

So is update_global_location_list just updating the target, i.e. putting new
breakpoints in, old ones back etc?  If so, perhaps changes in the breakpoint
list have already been reflected elsewhere and no observer calls need to be
made here.


 > Is there any way to make -break-insert report the new breakpoint directly,
 > as opposed to via notification?

That's what it currently does, isn't it?  I'm trying to move away from that
so that GDB just reports changes no matter how they are made, e.g., MI command
or CLI command.


 > ...
 > > +static void
 > > +mi_breakpoints_changed (int bnum)
 > > +{
 > > +  struct mi_interp *mi = top_level_interpreter_data ();
 > > +  struct interp *interp_to_use;
 > > +  struct ui_out *old_uiout, *temp_uiout;
 > > +  int version;
 > > +
 > > +  fprintf_unfiltered (mi->event_channel, "breakpoints-changed");
 > > +  interp_to_use = top_level_interpreter ();
 > > +  old_uiout = uiout;
 > > +  temp_uiout = interp_ui_out (interp_to_use);
 > > +  version = mi_version (temp_uiout);
 > > +  temp_uiout = mi_out_new (version);
 > > +  uiout = temp_uiout;
 > > +  breakpoint_query (bnum);
 > 
 > Shall we have a helper function to do this creation of temporary uiout?

The thread-changed observer in my earlier patch uses a very similar function
so there probably could be some refactoring.

 
 > I think that except for compatibility issues due to -break-insert no longer
 > reporting the breakpoint that is created as part of ^done response, this
 > patch is good.

The only way I can see around compatibility issues would be to add a new level
for MI.  This change could be lumped with others that are being proposed,
e.g, the no stop mode.  Then the logic could be separated according to MI
level and notice given that at some stage the old level would be removed.

It's a lot of work and I must admit I'm not really sure that I have the stamina
to go through with all of it.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH:MI] Use observers for breakpoints

[Vladimir Prus]

On Wednesday 04 June 2008 12:33:58 you wrote:
> Vladimir Prus writes:
>  > On Sunday 01 June 2008 06:46:04 Nick Roberts wrote:
>  > 
>  > > (gdb)
>  > > -break-watch i22
>  > > =breakpoints-changed,bkpt={number="4",type="watchpoint",disp="keep",e
>  > >nabled="y",addr="",what="i22",times="0",original-location="i22"}
>  > > ^done,wpt={number="4",exp="i22"}
>  >
>  > Is this an output we actually want? ;-)
>  > With one breakpoint mentioned twice?
> 
> I don't know why watchpoints are currently reported in a different way to
> normal breakpoints or why type="watchpoint" isn't adequate and a special
> field "wpt" is needed, but I would suggest removing the latter output.

Good. You did not mention this initially, so I wondered if you consider
this OK. The 'wpt' comes from the mention function.

>  > I wonder if you missed this bit in bpstat_stop_status:
>  > 
>  > 	if (b->disposition == disp_disable)
>  > 	  {
>  > 	    b->enable_state = bp_disabled;
>  > 	    update_global_location_list ();
>  > 	  }
> 
> Could be.  At the moment, as I seaid to Joel, it's just a sketch.  I just used
> the locations in breakpoint.c where the event functions were called.  If new
> locations have been created since they were added then I will have missed them.
> 
> How does the breakpoint list change at this point?

One breakpoint becomes disabled.

>  > Probably, breakpoint_re_set_one should call the observer too.
>  > 
>  > In fact, it's probably would be nice to add call to the observer to
>  > update_global_location_list. This function is called whenever we change any
>  > property of a breakpoint that affects what should be inserted in the
>  > target. The only issue is that right now update_global_location_list does
>  > not know which breakpoint has changed -- we probably can add a parameter.
>  > 
>  > This won't catch all cases -- there are properties of breakpoints that are
>  > not reflected in the target -- like condition, commands and ignore
>  > count. But seems like your patch has it covered already.
> 
> So is update_global_location_list just updating the target, i.e. putting new
> breakpoints in, old ones back etc?  If so, perhaps changes in the breakpoint
> list have already been reflected elsewhere and no observer calls need to be
> made here.

Depends on your design. I think it makes perfect sense to report changes in properties
that affect inserted breakpoints in update_global_location_list, and report changes
in other properties in the corresponding functions (condition_command, etc).

>  > Is there any way to make -break-insert report the new breakpoint directly,
>  > as opposed to via notification?
> 
> That's what it currently does, isn't it?  I'm trying to move away from that
> so that GDB just reports changes no matter how they are made, e.g., MI command
> or CLI command.

I think that for -break-insert, saying "^done" and then saying, "ah, btw, some
breakpoints were inserted", is a bit indirect. For example, in KDevelop, there's
a class to represent breakpoint. When a breakpoint is created, and -break-insert
is sent, and breakpoint is inserted, the 'id' field of that class is set to breakpoint
number that gdb reports, and at this point we know that the breakpoint is correctly
inserted. If -break-insert no longer reports the breakpoint directly, I'd have to
somehow match the output =breakpoints-changes to the previous -break-insert. If I
don't, then =breakpoints-changed will actually add new breakpoint to the breakpoint
table, without marking the breakpoint been inserted via -break-insert as inserted.

One can argue that such matching is not very hard to do. But if so, then it's
not hard to do for GDB, either, and -break-insert can still report the breakpoint
that was inserted.

I'm also not sure that emitting =breakpoints-changes in response to -break-condition,
or -break-disable, is a good idea. It does not add any new information.

Presumably, we can suppress the breakpoint observer during executing of MI commands
that directly create/modify breakpoint, and leave the observer enabled for other
commands -- where we don't mean no breakpoint changes but might get them.

>  > ...
>  > > +static void
>  > > +mi_breakpoints_changed (int bnum)
>  > > +{
>  > > +  struct mi_interp *mi = top_level_interpreter_data ();
>  > > +  struct interp *interp_to_use;
>  > > +  struct ui_out *old_uiout, *temp_uiout;
>  > > +  int version;
>  > > +
>  > > +  fprintf_unfiltered (mi->event_channel, "breakpoints-changed");
>  > > +  interp_to_use = top_level_interpreter ();
>  > > +  old_uiout = uiout;
>  > > +  temp_uiout = interp_ui_out (interp_to_use);
>  > > +  version = mi_version (temp_uiout);
>  > > +  temp_uiout = mi_out_new (version);
>  > > +  uiout = temp_uiout;
>  > > +  breakpoint_query (bnum);
>  > 
>  > Shall we have a helper function to do this creation of temporary uiout?
> 
> The thread-changed observer in my earlier patch uses a very similar function
> so there probably could be some refactoring.

OK.

>  > I think that except for compatibility issues due to -break-insert no longer
>  > reporting the breakpoint that is created as part of ^done response, this
>  > patch is good.
> 
> The only way I can see around compatibility issues would be to add a new level
> for MI.  

Or make -break-insert output the new breakpoint directly, by temporary suppressing
the observer.


> This change could be lumped with others that are being proposed, 
> e.g, the no stop mode.  

Just to clarify -- the non stop mode is optional, and so does not introduce any backward
compatibility issues.

> Then the logic could be separated according to MI 
> level and notice given that at some stage the old level would be removed.
> 
> It's a lot of work and I must admit I'm not really sure that I have the stamina
> to go through with all of it.

It seems to me that this patch, with minor adjustments, can be checked in soonish.

- Volodya

Re: [PATCH:MI] Use observers for breakpoints

[Nick Roberts]

 > >  > > (gdb)
 > >  > > -break-watch i22
 > >  > > =breakpoints-changed,bkpt={number="4",type="watchpoint",disp="keep",e
 > >  > >nabled="y",addr="",what="i22",times="0",original-location="i22"}
 > >  > > ^done,wpt={number="4",exp="i22"}
 > >  >
 > >  > Is this an output we actually want? ;-)
 > >  > With one breakpoint mentioned twice?
 > > 
 > > I don't know why watchpoints are currently reported in a different way to
 > > normal breakpoints or why type="watchpoint" isn't adequate and a special
 > > field "wpt" is needed, but I would suggest removing the latter output.
 > 
 > Good. You did not mention this initially, so I wondered if you consider
 > this OK. The 'wpt' comes from the mention function.

I'm just trying to find agreement the MI should use event notification for
change of state and not directly MI command output.  I should have used RFC in
the subject header.  I will submit a proper patch later.  Note that removing
the "wpt" field requires a new MI level.


 > ...
 > > So is update_global_location_list just updating the target, i.e. putting
 > > new breakpoints in, old ones back etc?  If so, perhaps changes in the
 > > breakpoint list have already been reflected elsewhere and no observer
 > > calls need to be made here.
 > 
 > Depends on your design. I think it makes perfect sense to report changes in
 > properties that affect inserted breakpoints in update_global_location_list,
 > and report changes in other properties in the corresponding functions
 > (condition_command, etc).

When I submit a patch you'll probably have to explain to me where breakpoints
might change.


 > >  > Is there any way to make -break-insert report the new breakpoint
 > >  > directly, as opposed to via notification?
 > > 
 > > That's what it currently does, isn't it?  I'm trying to move away from
 > > that so that GDB just reports changes no matter how they are made, e.g.,
 > > MI command or CLI command.
 > 
 > I think that for -break-insert, saying "^done" and then saying, "ah, btw,
 > some breakpoints were inserted", is a bit indirect. For example, in
 > KDevelop, there's a class to represent breakpoint. When a breakpoint is
 > created, and -break-insert is sent, and breakpoint is inserted, the 'id'
 > field of that class is set to breakpoint number that gdb reports, and at
 > this point we know that the breakpoint is correctly inserted. If
 > -break-insert no longer reports the breakpoint directly, I'd have to somehow
 > match the output =breakpoints-changes to the previous -break-insert. If I
 > don't, then =breakpoints-changed will actually add new breakpoint to the
 > breakpoint table, without marking the breakpoint been inserted via
 > -break-insert as inserted.
 > 
 > One can argue that such matching is not very hard to do. But if so, then
 > it's not hard to do for GDB, either, and -break-insert can still report the
 > breakpoint that was inserted.

The event notification reports the breakpoint number so what is there to
match?.  I don't see why it should be reported twice, except perhaps not to
break existing behaviour.  In that case, I think it would be better to
condition on the MI level, so current level outputs as now and mi=3 outputs
notifications only.


 > I'm also not sure that emitting =breakpoints-changes in response to
 > -break-condition, or -break-disable, is a good idea. It does not add any new
 > information.

In a frontend that uses the console, e.g., Eclipse, if the user enters a
command like "disable 1", how does it know that this breakpoint has been
disabled?


 > Presumably, we can suppress the breakpoint observer during executing of MI
 > commands that directly create/modify breakpoint,

That seems to defeat the object.

 >                                                    and leave the observer
 > enabled for other commands -- where we don't mean no breakpoint changes but
 > might get them.

Where would that happen?

 > >  > I think that except for compatibility issues due to -break-insert no
 > >  > longer reporting the breakpoint that is created as part of ^done
 > >  > response, this patch is good.
 > > 
 > > The only way I can see around compatibility issues would be to add a new
 > > level for MI.
 > 
 > Or make -break-insert output the new breakpoint directly, by temporary
 > suppressing the observer.

I guess it could output the breakpoint directly in addition to the notification.
Current frontends should ignore such notifications (if they don't they will
presumably break with the ones already added).


 > > This change could be lumped with others that are being proposed, 
 > > e.g, the no stop mode.  
 > 
 > Just to clarify -- the non stop mode is optional, and so does not introduce
 > any backward compatibility issues.

I'm thinking of changes to accommodate non stop mode, e.g., no token in stopped
output (I realise that this has already been made).  Note these examples are
still present in the GDB/MI Program Execution node of th manual:

     (gdb)
     111-exec-continue
     111^running

     (gdb)
     000-exec-run
     000^running

changing ^running to *running?, removing the (gdb) `prompt'?


 > > Then the logic could be separated according to MI level and notice given
 > > that at some stage the old level would be removed.
 > > 
 > > It's a lot of work and I must admit I'm not really sure that I have the
 > > stamina to go through with all of it.
 > 
 > It seems to me that this patch, with minor adjustments, can be checked in
 > soonish.

I'll see what I can do but a lot of questions seem to remain.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH:MI] Use observers for breakpoints

[Vladimir Prus]

On Monday 09 June 2008 14:46:47 you wrote:
>  > >  > > (gdb)
>  > >  > > -break-watch i22
>  > >  > > =breakpoints-changed,bkpt={number="4",type="watchpoint",disp="keep",e
>  > >  > >nabled="y",addr="",what="i22",times="0",original-location="i22"}
>  > >  > > ^done,wpt={number="4",exp="i22"}
>  > >  >
>  > >  > Is this an output we actually want? ;-)
>  > >  > With one breakpoint mentioned twice?
>  > > 
>  > > I don't know why watchpoints are currently reported in a different way to
>  > > normal breakpoints or why type="watchpoint" isn't adequate and a special
>  > > field "wpt" is needed, but I would suggest removing the latter output.
>  > 
>  > Good. You did not mention this initially, so I wondered if you consider
>  > this OK. The 'wpt' comes from the mention function.
> 
> I'm just trying to find agreement the MI should use event notification for
> change of state and not directly MI command output.  

I think you're mixing two unrelated issues together. One is that MI client should
be notified if breakpoints are changed, no matter what command changed breakpoints.
There's no argument that it's necessary. The one issues if whether -break-insert
should report newly created breakpoint directly, of via notification.

> I should have used RFC in 
> the subject header.  I will submit a proper patch later.  Note that removing
> the "wpt" field requires a new MI level.

I don't think so -- there are no breakpoint change notification in current GDB,
so no backward compatibility issue with removing this bit from =breakpoint-changed.

>  > ...
>  > > So is update_global_location_list just updating the target, i.e. putting
>  > > new breakpoints in, old ones back etc?  If so, perhaps changes in the
>  > > breakpoint list have already been reflected elsewhere and no observer
>  > > calls need to be made here.
>  > 
>  > Depends on your design. I think it makes perfect sense to report changes in
>  > properties that affect inserted breakpoints in update_global_location_list,
>  > and report changes in other properties in the corresponding functions
>  > (condition_command, etc).
> 
> When I submit a patch you'll probably have to explain to me where breakpoints
> might change.

Well, if you prefer to first write a patch, and then adjust it, fine :-)

>  > >  > Is there any way to make -break-insert report the new breakpoint
>  > >  > directly, as opposed to via notification?
>  > > 
>  > > That's what it currently does, isn't it?  I'm trying to move away from
>  > > that so that GDB just reports changes no matter how they are made, e.g.,
>  > > MI command or CLI command.
>  > 
>  > I think that for -break-insert, saying "^done" and then saying, "ah, btw,
>  > some breakpoints were inserted", is a bit indirect. For example, in
>  > KDevelop, there's a class to represent breakpoint. When a breakpoint is
>  > created, and -break-insert is sent, and breakpoint is inserted, the 'id'
>  > field of that class is set to breakpoint number that gdb reports, and at
>  > this point we know that the breakpoint is correctly inserted. If
>  > -break-insert no longer reports the breakpoint directly, I'd have to somehow
>  > match the output =breakpoints-changes to the previous -break-insert. If I
>  > don't, then =breakpoints-changed will actually add new breakpoint to the
>  > breakpoint table, without marking the breakpoint been inserted via
>  > -break-insert as inserted.
>  > 
>  > One can argue that such matching is not very hard to do. But if so, then
>  > it's not hard to do for GDB, either, and -break-insert can still report the
>  > breakpoint that was inserted.
> 
> The event notification reports the breakpoint number so what is there to
> match?.  

Frontend has one breakpoint in its breakpoint table, having id of -1, because
the breakpoint is not inserted yet. Now, notification arrives, saying that 
there's new breakpoint with id of 7. Should the frontend add a new breakpoint
to its breakpoint table, or should it update the breakpoint already in its
table with the properties reported by GDB? The frontend can decide by
knowing that the last command was -break-insert, but GDB can do it easily, too. 

> I don't see why it should be reported twice, except perhaps not to 
> break existing behaviour.  In that case, I think it would be better to
> condition on the MI level, so current level outputs as now and mi=3 outputs
> notifications only.

Of course, we can use mi level to do this, but should we? Current frontends
work fine, except that they miss breakpoint changes caused by CLI commands.
We want to fix that, and to that end, we want to introduce breakpoint change
notifications. Why force the frontend to modify the logic for the commands that
already work fine -- like -break-insert, or -break-disable?

>  > I'm also not sure that emitting =breakpoints-changes in response to
>  > -break-condition, or -break-disable, is a good idea. It does not add any new
>  > information.
> 
> In a frontend that uses the console, e.g., Eclipse, if the user enters a
> command like "disable 1", how does it know that this breakpoint has been
> disabled?

I did not say anything about 'disable 1', I'm talking specifically about MI
commands which indended purpose is to modify a breakpoint.

>  > Presumably, we can suppress the breakpoint observer during executing of MI
>  > commands that directly create/modify breakpoint,
> 
> That seems to defeat the object.
>  >                                                    and leave the observer
>  > enabled for other commands -- where we don't mean no breakpoint changes but
>  > might get them.
> 
> Where would that happen?

"that" been what? The "other commands" are all CLI commands, as well as MI commands
that don't change breakpoints as their primary purpose but might change them
accidentally -- for example, run commands update hit count of breakpoints and
might auto-delete breakpoints. The logic for temporary disabling notification
belongs, probably, to individual breakpoint commands.

>  > >  > I think that except for compatibility issues due to -break-insert no
>  > >  > longer reporting the breakpoint that is created as part of ^done
>  > >  > response, this patch is good.
>  > > 
>  > > The only way I can see around compatibility issues would be to add a new
>  > > level for MI.
>  > 
>  > Or make -break-insert output the new breakpoint directly, by temporary
>  > suppressing the observer.
> 
> I guess it could output the breakpoint directly in addition to the notification.
> Current frontends should ignore such notifications (if they don't they will
> presumably break with the ones already added).

I think we can assume the frontends ignore notifications they don't understand.

>  > > This change could be lumped with others that are being proposed, 
>  > > e.g, the no stop mode.  
>  > 
>  > Just to clarify -- the non stop mode is optional, and so does not introduce
>  > any backward compatibility issues.
> 
> I'm thinking of changes to accommodate non stop mode, e.g., no token in stopped
> output (I realise that this has already been made).  Note these examples are
> still present in the GDB/MI Program Execution node of th manual:
> 
>      (gdb)
>      111-exec-continue
>      111^running
> 
>      (gdb)
>      000-exec-run
>      000^running
> 
> changing ^running to *running?, 

No such change is planned. *running will be output in addition to ^running. While
I'll surely kill ^running if we bump MI level, removing ^running does not appear
to be very important in itself.

> removing the (gdb) `prompt'? 

So far, my non-stop patches don't remove it.

>  > > Then the logic could be separated according to MI level and notice given
>  > > that at some stage the old level would be removed.
>  > > 
>  > > It's a lot of work and I must admit I'm not really sure that I have the
>  > > stamina to go through with all of it.
>  > 
>  > It seems to me that this patch, with minor adjustments, can be checked in
>  > soonish.
> 
> I'll see what I can do but a lot of questions seem to remain.

I'm sure we can sort those questions.

- Volodya

 

Re: [PATCH:MI] Use observers for breakpoints

[Nick Roberts]

 > > I'll see what I can do but a lot of questions seem to remain.
 > 
 > I'm sure we can sort those questions.

Well I'm talking about removing events and therefore the direct command output
from the -break-* commands while you appear to be talking about suppressing the
observers for those commands.  I think this will have the disadvantage that it
will make the code more complicated.

Anyway let me go away and submit something more concrete.

-- 
Nick                                           http://www.inet.net.nz/~nickrob