[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