Re: [PATCH:MI] Use observers for breakpoints

[Vladimir Prus <ghost at cs dot msu dot su>]

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