Re: [PATCH] PR breakpoints/15697: Remove =breakpoint-modified when hitting dprintf

[Doug Evans <dje@google.com>]

On Mon, Apr 28, 2014 at 1:23 PM, Marc Khouzam <marc.khouzam@ericsson.com> wrote:
> GDB currently sends a =breakpoint-modified for every dprintf hit.
> This can cause performance degradation at the frontend.  The below
> patch prevents GDB from sending this event for dprintf when the
> event is triggered by the dprintf being hit.  This means the event
> is not sent when the hit-count is incremented or the ignore-count
> is decremented due to a hit.
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=15697
>
> This was discussed last year:
> http://sourceware.org/ml/gdb-patches/2013-03/msg00260.html
>
> No regressions.
>
> Ok?
>
> Thanks
>
> Marc
>
> ---
>
> DPrintfs can be hit very often since they don't interrupt
> the execution.  Having a =breakpoint-modified event at
> every hit can cause performance degradation at the
> frontend.  The only value in this event is to indicate that
> the hit-count has changed.  For the hit-count value however,
> it is sufficient for a frontend to get the latest value upon
> request and not through an asynchronous event.
>
> We also remove =breakpoint-modified when hitting a dprintf
> and decrementing the ignore count.  If the ignore count is
> set to a high number, the =breakpoint-modified could also
> cause performance degradation as it will be sent at every
> dprintf hit which decrements the ignore count.
>
> 2014-04-28  Marc Khouzam  <marc.khouzam@ericsson.com>
>
>         PR breakpoints/15697
>         * breakpoint.c (bpstat_check_breakpoint_conditions):
>         Don't call observer_notify_breakpoint_modified for dprintf.
>         * breakpoint.c (bpstat_stop_status): Ditto.

Nit. No need to write "* breakpoint.c" twice.

> ---
>  gdb/breakpoint.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index f422998..25615eb 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5378,7 +5378,8 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
>        bs->stop = 0;
>        /* Increase the hit count even though we don't stop.  */
>        ++(b->hit_count);
> -      observer_notify_breakpoint_modified (b);
> +      if (b->type != bp_dprintf)
> +       observer_notify_breakpoint_modified (b);
>      }
>  }
>
> @@ -5515,7 +5516,8 @@ bpstat_stop_status (struct address_space *aspace,
>           if (bs->stop)
>             {
>               ++(b->hit_count);
> -             observer_notify_breakpoint_modified (b);
> +             if (b->type != bp_dprintf)
> +               observer_notify_breakpoint_modified (b);
>
>               /* We will stop here.  */
>               if (b->disposition == disp_disable)
> --
> 1.7.9.5
>

Hi.
I've read the mentioned thread and I agree with your analysis.
The patch is ok with me, but give it a few days for others to comment.

Adding a comment explaining why the test is present might be useful,
but I'm ok with skipping it.
E.g., something like
  /* Don't send notifications for dprintf, there can be a lot and aren't useful
     to frontends.  */