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

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

[Kevin Buettner]

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?

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;
 	  }
     }

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

[Eli Zaretskii]

On Sat, 12 May 2001, Kevin Buettner wrote:

> 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.)

Thanks, I'm happy now.

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

[Jim Blandy]

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;
>  	  }
>      }
> 
> 

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

[Kevin Buettner]

On May 22,  6:26pm, Jim Blandy wrote:

> 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.

Yes, I got your message.  I even made the changes, but got distracted
and hadn't committed them.  Here's my revision:

	* breakpoint.c (breakpoint_address_is_meaningful): New function.
	(check_duplicates): Don't compare non-meaningful addresses.

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/22 23:47:06
@@ -3735,6 +3735,40 @@ set_default_breakpoint (int valid, CORE_
   default_breakpoint_line = line;
 }
 
+/* Return true iff it is meaningful to use the address member of
+   BPT.  For some breakpoint types, the address member is irrelevant
+   and it makes no sense to attempt to compare it to other addresses
+   (or use it for any other purpose either).
+
+   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 */
+
+static int
+breakpoint_address_is_meaningful (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 +3784,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 (! breakpoint_address_is_meaningful (bpt))
     return;
 
   ALL_BREAKPOINTS (b)
@@ -3762,7 +3792,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)
+	&& breakpoint_address_is_meaningful (b))
     {
       /* Have we found a permanent breakpoint?  */
       if (b->enable == permanent)
@@ -3800,7 +3831,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)
+		&& breakpoint_address_is_meaningful (b))
 	      b->duplicate = 1;
 	  }
     }