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

Simon Marchi via Gdb-patches
2021-08-14T04:08:28Z

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