This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] PR breakpoints/15697: Remove =breakpoint-modified when hitting dprintf
- From: Doug Evans <dje at google dot com>
- To: Marc Khouzam <marc dot khouzam at ericsson dot com>
- Cc: gdb-patches <gdb-patches at sourceware dot org>
- Date: Mon, 28 Apr 2014 15:06:43 -0700
- Subject: Re: [PATCH] PR breakpoints/15697: Remove =breakpoint-modified when hitting dprintf
- Authentication-results: sourceware.org; auth=none
- References: <1398716623-16991-1-git-send-email-marc dot khouzam at ericsson dot 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. */