Re: [PATCH RFA #2] breakpoint.c: More check_duplicates() changes

[Jim Blandy <jimb@zwingli.cygnus.com>]

Kevin Buettner <kevinb@cygnus.com> writes:
> The patch below supercedes the one that I recently posted in
> 
>     http://sources.redhat.com/ml/gdb-patches/2001-05/msg00267.html
> 
> It addresses the concerns that Eli Zaretskii and Mark Kettenis had
> about the previous patch.  (Eli wanted a better explanation regarding
> why it was okay to duplicate each of the types listed in the new
> function duplicate_okay.  Mark wanted parens around the return
> expression.)
> 
> Okay to apply?

Did you get my message about the name `duplicate_okay'?  I think it's
misleading --- duplicates of any sort of eventpoint are `okay'.  I
think it should be called `address_meaningful', and the sense of its
return value reversed, since that's really what it's telling us.  If
an eventpoint's address isn't meaningful, then it shouldn't be
included in the search for breakpoints at a particular address.


> Index: breakpoint.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/breakpoint.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 breakpoint.c
> --- breakpoint.c	2001/05/12 04:08:23	1.37
> +++ breakpoint.c	2001/05/12 22:23:25
> @@ -3735,6 +3735,43 @@ set_default_breakpoint (int valid, CORE_
>    default_breakpoint_line = line;
>  }
>  
> +/* Return true if the type of BPT is one whose address is permitted to
> +   be a duplicate of some other breakpoint.
> +
> +   More specifically, each of the following breakpoint types will always
> +   have a zero valued address and we don't want check_duplicates() to mark
> +   breakpoints of any of these types to be a duplicate of an actual
> +   breakpoint at address zero:
> +
> +      bp_watchpoint
> +      bp_hardware_watchpoint
> +      bp_read_watchpoint
> +      bp_access_watchpoint
> +      bp_catch_exec
> +      bp_longjmp_resume
> +      bp_catch_fork
> +      bp_catch_vork
> +
> +  Another way to look at it is that the address field is irrelevant
> +  for each of these breakpoint types and duplicate_okay() is simply
> +  a predicate for determining whether it is meaningful to use the
> +  address field for comparison purposes.  */
> +
> +static int
> +duplicate_okay (struct breakpoint *bpt)
> +{
> +  enum bptype type = bpt->type;
> +
> +  return (type == bp_watchpoint
> +	  || type == bp_hardware_watchpoint
> +	  || type == bp_read_watchpoint
> +	  || type == bp_access_watchpoint
> +	  || type == bp_catch_exec
> +	  || type == bp_longjmp_resume
> +	  || type == bp_catch_fork
> +	  || type == bp_catch_vfork);
> +}
> +
>  /* Rescan breakpoints at the same address and section as BPT,
>     marking the first one as "first" and any others as "duplicates".
>     This is so that the bpt instruction is only inserted once.
> @@ -3750,11 +3787,7 @@ check_duplicates (struct breakpoint *bpt
>    CORE_ADDR address = bpt->address;
>    asection *section = bpt->section;
>  
> -  /* Watchpoints are uninteresting.  */
> -  if (bpt->type == bp_watchpoint
> -      || bpt->type == bp_hardware_watchpoint
> -      || bpt->type == bp_read_watchpoint
> -      || bpt->type == bp_access_watchpoint)
> +  if (duplicate_okay (bpt))
>      return;
>  
>    ALL_BREAKPOINTS (b)
> @@ -3762,7 +3795,8 @@ check_duplicates (struct breakpoint *bpt
>  	&& b->enable != shlib_disabled
>  	&& b->enable != call_disabled
>  	&& b->address == address
> -	&& (overlay_debugging == 0 || b->section == section))
> +	&& (overlay_debugging == 0 || b->section == section)
> +	&& !duplicate_okay (b))
>      {
>        /* Have we found a permanent breakpoint?  */
>        if (b->enable == permanent)
> @@ -3800,7 +3834,8 @@ check_duplicates (struct breakpoint *bpt
>  		&& b->enable != shlib_disabled
>  		&& b->enable != call_disabled
>  		&& b->address == address
> -		&& (overlay_debugging == 0 || b->section == section))
> +		&& (overlay_debugging == 0 || b->section == section)
> +		&& !duplicate_okay (b))
>  	      b->duplicate = 1;
>  	  }
>      }
> 
>