PATCH: Remove dead code, clear breakpoint ignore counts?

PATCH: Remove dead code, clear breakpoint ignore counts?

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 878 bytes --]

I think I stared at this one time too many.

What do you think of the attached?  Would anyone miss this?  There's
no way the user can request to not show hit counts, so, this is dead
code.

Perhaps it would make more sense to clear the ignore counts
in the same place where we clear the hit counts, which is currently
only on run_command_1; or from within clear_breakpoint_hit_counts.  Huh,
we don't clear hit counts when attaching or connecting to a remote
target.

Perhaps we should never clear these automatically then, and provide
a user command to explicitly do so instead?  Too confusing?  Come to
think of it, I guess clearing on every "run" also sounds
confusing to me.

(When we go multi-process, it can be more surprising to clear
hit counts on each "run" invocation.)

My head hurts --- I just wanted to get rid of this 14 year old dead
code.  :-) )

-- 
Pedro Alves

[-- Attachment #2: breakpoint_ignore_counts.diff --]
[-- Type: text/x-diff, Size: 3618 bytes --]

2008-10-14  Pedro Alves  <pedro@codesourcery.com>

	Remove dead code.

	* breakpoint.c (show_breakpoint_hit_counts): Delete.
	(print_one_breakpoint_location): Adjust.
	(breakpoint_clear_ignore_counts): Delete.
	* breakpoint.h (breakpoint_clear_ignore_counts): Remove
	declaration.
	* target.c (generic_mourn_inferior): Don't clear ignore
	counts (never reached).

---
 gdb/breakpoint.c |   18 ++----------------
 gdb/breakpoint.h |    2 --
 gdb/target.c     |    8 --------
 3 files changed, 2 insertions(+), 26 deletions(-)

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2008-10-14 18:41:43.000000000 +0100
+++ src/gdb/breakpoint.c	2008-10-14 18:43:57.000000000 +0100
@@ -315,10 +315,6 @@ static int overlay_events_enabled;
 	     B ? (TMP=B->global_next, 1): 0;	\
 	     B = TMP)
 
-/* True if breakpoint hit counts should be displayed in breakpoint info.  */
-
-int show_breakpoint_hit_counts = 1;
-
 /* Chains of all breakpoints defined.  */
 
 struct breakpoint *breakpoint_chain;
@@ -3812,7 +3808,7 @@ print_one_breakpoint_location (struct br
       ui_out_text (uiout, "\n");
     }
   
-  if (!part_of_multiple && show_breakpoint_hit_counts && b->hit_count)
+  if (!part_of_multiple && b->hit_count)
     {
       /* FIXME should make an annotation for this */
       if (ep_is_catchpoint (b))
@@ -3830,7 +3826,7 @@ print_one_breakpoint_location (struct br
   /* Output the count also if it is zero, but only if this is
      mi. FIXME: Should have a better test for this. */
   if (ui_out_is_mi_like_p (uiout))
-    if (!part_of_multiple && show_breakpoint_hit_counts && b->hit_count == 0)
+    if (!part_of_multiple && b->hit_count == 0)
       ui_out_field_int (uiout, "times", b->hit_count);
 
   if (!part_of_multiple && b->ignore_count)
@@ -7731,16 +7727,6 @@ set_ignore_count (int bptnum, int count,
   error (_("No breakpoint number %d."), bptnum);
 }
 
-/* Clear the ignore counts of all breakpoints.  */
-void
-breakpoint_clear_ignore_counts (void)
-{
-  struct breakpoint *b;
-
-  ALL_BREAKPOINTS (b)
-    b->ignore_count = 0;
-}
-
 /* Command to set ignore-count of breakpoint N to COUNT.  */
 
 static void
Index: src/gdb/breakpoint.h
===================================================================
--- src.orig/gdb/breakpoint.h	2008-10-14 18:41:43.000000000 +0100
+++ src/gdb/breakpoint.h	2008-10-14 18:43:57.000000000 +0100
@@ -713,8 +713,6 @@ extern void delete_breakpoint (struct br
 
 extern void breakpoint_auto_delete (bpstat);
 
-extern void breakpoint_clear_ignore_counts (void);
-
 extern void break_command (char *, int);
 
 extern void hbreak_command_wrapper (char *, int);
Index: src/gdb/target.c
===================================================================
--- src.orig/gdb/target.c	2008-10-14 18:41:43.000000000 +0100
+++ src/gdb/target.c	2008-10-14 18:43:57.000000000 +0100
@@ -2336,7 +2336,6 @@ find_target_beneath (struct target_ops *
 void
 generic_mourn_inferior (void)
 {
-  extern int show_breakpoint_hit_counts;
   ptid_t ptid;
 
   ptid = inferior_ptid;
@@ -2354,13 +2353,6 @@ generic_mourn_inferior (void)
   reopen_exec_file ();
   reinit_frame_cache ();
 
-  /* It is confusing to the user for ignore counts to stick around
-     from previous runs of the inferior.  So clear them.  */
-  /* However, it is more confusing for the ignore counts to disappear when
-     using hit counts.  So don't clear them if we're counting hits.  */
-  if (!show_breakpoint_hit_counts)
-    breakpoint_clear_ignore_counts ();
-
   if (deprecated_detach_hook)
     deprecated_detach_hook ();
 }

Re: PATCH: Remove dead code, clear breakpoint ignore counts?

[Stan Shebs]

Pedro Alves wrote:
> I think I stared at this one time too many.
>
> What do you think of the attached?  Would anyone miss this?  There's
> no way the user can request to not show hit counts, so, this is dead
> code.
>   
Heh, I can't even remember why it seemed like there was any interest in 
conditionalizing; perhaps because the hit counts were a new feature and 
we thought users would want to be able to go back to the old behavior. 
In any case, I think by now there is consensus that hit counts are good 
to display. :-)

Hit counts are going to get a little messy for multi-process, because 
each inferior could have a different hit count, and it seems more useful 
to have a per-inferior hit count than an aggregate over all the 
inferiors to which the breakpoint applies.

Stan

Re: PATCH: Remove dead code, clear breakpoint ignore counts?

[Pedro Alves]

On Tuesday 14 October 2008 19:32:00, Stan Shebs wrote:
> Pedro Alves wrote:
> > I think I stared at this one time too many.
> >
> > What do you think of the attached?  Would anyone miss this?  There's
> > no way the user can request to not show hit counts, so, this is dead
> > code.
> >   
> Heh, I can't even remember why it seemed like there was any interest in 
> conditionalizing; perhaps because the hit counts were a new feature and 
> we thought users would want to be able to go back to the old behavior. 
> In any case, I think by now there is consensus that hit counts are good 
> to display. :-)

Seems like it.  :-)  I guess I wasn't that clear, but the actual code
that initialy bothered me was the clearing the *ignore* counts in
generic_mourn_inferior, or better, the comment there that I must have read a
hundred times already by now, although it's dead code.

Just curious, do people think that it's useful to clear the hit
count automatically at all, considering that we do it on "run" but not
on "attach" or "target remote"?  I can't seem to make up my mind
on it.  It's still logicaly the same breakpoint across runs, so it
could make sense to not do so.

> 
> Hit counts are going to get a little messy for multi-process, because 
> each inferior could have a different hit count, and it seems more useful 
> to have a per-inferior hit count than an aggregate over all the 
> inferiors to which the breakpoint applies.
> 

Right, that would mean storing a hit count per-breakpoint location as well
as per-breakpoint, it seems.  Same for ignore counts.  I'm sure there's
a user out there who would find that useful for breakpoints with multiple
locations in the single-inferior case too.  :-)

-- 
Pedro Alves

Re: PATCH: Remove dead code, clear breakpoint ignore counts?

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> Seems like it.  :-) I guess I wasn't that clear, but the actual
Pedro> code that initialy bothered me was the clearing the *ignore*
Pedro> counts in generic_mourn_inferior, or better, the comment there
Pedro> that I must have read a hundred times already by now, although
Pedro> it's dead code.

I say just nuke it.  If it has been dead for 14 years, then nobody
cares.

Pedro> Just curious, do people think that it's useful to clear the hit
Pedro> count automatically at all, considering that we do it on "run" but not
Pedro> on "attach" or "target remote"?

I occasionally use this feature to figure out how I ought to set
ignore counts.  E.g., set a breakpoint, run, "c 99999", wait for the
crash, and then ignore one less than the hit count.

This idiom relies on re-running, so it is not very useful with attach.

I guess it is tough to change behavior that has been deployed for many
years, since it is hard to guess how people are using it.

Pedro> I can't seem to make up my mind on it.  It's still logicaly the
Pedro> same breakpoint across runs, so it could make sense to not do
Pedro> so.

Offhand I could not think of a way I would use the hit count if it
were not auto-cleared.  When would I want to know the accumulated
total of hits across all runs?

Stan> Hit counts are going to get a little messy for multi-process, because 
Stan> each inferior could have a different hit count, and it seems more useful 
Stan> to have a per-inferior hit count than an aggregate over all the 
Stan> inferiors to which the breakpoint applies.

Pedro> Right, that would mean storing a hit count per-breakpoint
Pedro> location as well as per-breakpoint, it seems.  Same for ignore
Pedro> counts.

I would probably just set a breakpoint specific to a particular
inferior if I was worried about this case.

Tom

Re: PATCH: Remove dead code, clear breakpoint ignore counts?

[Pedro Alves]

On Tuesday 14 October 2008 21:03:44, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> Seems like it.  :-) I guess I wasn't that clear, but the actual
> Pedro> code that initialy bothered me was the clearing the *ignore*
> Pedro> counts in generic_mourn_inferior, or better, the comment there
> Pedro> that I must have read a hundred times already by now, although
> Pedro> it's dead code.
> 
> I say just nuke it.  If it has been dead for 14 years, then nobody
> cares.

Yeah.  I've just done so.

> Pedro> Just curious, do people think that it's useful to clear the hit
> Pedro> count automatically at all, considering that we do it on "run" but not
> Pedro> on "attach" or "target remote"?
> 
> I occasionally use this feature to figure out how I ought to set
> ignore counts.  E.g., set a breakpoint, run, "c 99999", wait for the
> crash, and then ignore one less than the hit count.
> 
> This idiom relies on re-running, so it is not very useful with attach.

I guess that comes from the fact that there's no easy way to reset the
hit count of the breakpoint (or of all breakpoints), other than
delete,re-create'ing it (them).

> I guess it is tough to change behavior that has been deployed for many
> years, since it is hard to guess how people are using it.

Yeah.

> Pedro> I can't seem to make up my mind on it.  It's still logicaly the
> Pedro> same breakpoint across runs, so it could make sense to not do
> Pedro> so.
> 
> Offhand I could not think of a way I would use the hit count if it
> were not auto-cleared.  When would I want to know the accumulated
> total of hits across all runs?

I dunno.  I don't actually rely on those counters myself that often.
I was noticing that currently, in a multi-process GDB, if I do "run"
followed by another "run", while the first process is still live (I
don't kill it), all hit counts are being reset.  That seemed wrong.

I guess we'll be getting back to this sooner or later.  I'm happy
for now.  :-)

Thanks!

-- 
Pedro Alves