Re: [PATCH] Notify observer of breakpoint auto-disabling

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-08-13 6:24 p.m., Patrick Monnerat via Gdb-patches wrote:
> As observer is currently notified of breakpoint stop before handling its
> auto-disabling after count is reached, the observer is never notified of
> the disabling.
> 
> The problem does not seem to affect gdb alone. However it impacts insight:
> breakpoint GUI window is not properly updated upon auto-disable.
> 
> This patch moves the observer notification after the auto-disabling
> code.
> 
> Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=23336
> 
> * gdb/breakpoint.c (bpstat_stop_status): move observer notification
>   after auto-disabling code.

Note that GDB doesn't use ChangeLogs anymore.

> ---
>  gdb/breakpoint.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 89af44ee4c6..feca224ccf4 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5491,7 +5491,6 @@ bpstat_stop_status (const address_space *aspace,
>  	  if (bs->stop)
>  	    {
>  	      ++(b->hit_count);
> -	      gdb::observers::breakpoint_modified.notify (b);
>  
>  	      /* We will stop here.  */
>  	      if (b->disposition == disp_disable)
> @@ -5501,6 +5500,7 @@ bpstat_stop_status (const address_space *aspace,
>  		    b->enable_state = bp_disabled;
>  		  removed_any = 1;
>  		}
> +	      gdb::observers::breakpoint_modified.notify (b);

I looked at the change a bit more in depth, in particular at the various
observers of breakpoint_modified.  One of them is MI, who prints the
=breakpoint-modified notification.  That suggested that your change
would affect that notification in a similar way that insight is
affected.  And indeed, if I run a program with a breakpoint with "enable
count 1 1", the notification I get without your patch is:

  =breakpoint-modified,bkpt={number="1",type="breakpoint",disp="dis",enabled="n",...
                                                                              ^
And with the patch:

  =breakpoint-modified,bkpt={number="1",type="breakpoint",disp="dis",enabled="y",
                                                                              ^

I think the change is desirable in MI for the same reason it is
desirable in insight.  But this means we should add a test for it.
Maybe gdb.mi/mi-breakpoint-changed.exp would be a good place for it.  I
don't think there's an official way to set a count on a breakpoint using
MI.  However, it seems to work through -break-enable, since
-break-enable is implemented on top of the CLI "enable" command.  So
"-break-enable count 1 1" is equivalent to "enable count 1 1".  It's not
documented but...

There is also the Python breakpoint_modified event.  This script:

  def func(bp):
      print(bp.enabled)

  gdb.events.breakpoint_modified.connect(func)

Prints "True" before your patch and "False" after.  We should have a
test for that too, ideally, I guess in gdb.python/py-breakpoint.exp.

Simon