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

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

[Kevin Buettner]

I decided to poke about in breakpoint.c a bit more to see if there
were any other breakpoint types that needed to be handled specially
in check_duplicates().  Recall that until recently, check_duplicates()
contained some code at the beginning of the function which looked like
this:

      if (address == 0)         /* Watchpoints are uninteresting */
        return;

check_duplicates() was recently changed to allow zero-valued addresses
to be checked for.  The above code was changed to now look like
this:

      /* Watchpoints are uninteresting.  */
      if (bpt->type == bp_watchpoint
          || bpt->type == bp_hardware_watchpoint
          || bpt->type == bp_read_watchpoint
          || bpt->type == bp_access_watchpoint)
        return;

The bad thing about comments is that they often lie about what the
code is doing.  The above comment isn't telling a blatant lie, but
it's also not giving us all of the information that we need in order
to correctly transform the code.

It turns out that there are several other breakpoint types which
were using zero-valued addresses to cause an early return from
check_duplicates().  They are:

    bp_catch_exec
    bp_longjmp_resume
    bp_catch_fork
    bp_catch_vfork

Examination of the code which creates a breakpoint of each of these
types will reveal that the ``sal'' struct will be zeroed out.  (Note
too that all of the watchpoint types will also have a zeroed ``sal''.)
This means that the original code was also returning early for each
of these additional four types in addition to the watchpoint types.

The patch below creates a new function called duplicate_okay() and
uses this function to effect the early return.  I.e, the above code
has again been rewritten as follows:

      if (duplicate_okay (bpt))
	return;

I removed the half-truth telling comment too.  I think the above
statement is reasonably self documenting.

It turns out that duplicate_okay() is also needed later in the
function too for the duplicate removal phase.  To understand why it's
needed, suppose one or more watchpoints already exist.  As noted
earlier, these watchpoints will have zero valued addresses.  (Their
addresses are zero simply because the address field is meaningless,
yet has to have some value.)  Now suppose that the user adds a
breakpoint for address zero.  If we don't exclude those breakpoints
from consideration which have zero-valued addresses simply because the
address field is meaningless, check_duplicates() will end up marking
those breakpoints as duplicates of the newly added breakpoint at zero.

Okay to commit?

	* breakpoint.c (duplicate_okay): New function.
	(check_duplicates): Use duplicate_okay() to determine which
	types of breakpoints may have duplicates.

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 07:26:50
@@ -3735,6 +3735,27 @@ set_default_breakpoint (int valid, CORE_
   default_breakpoint_line = line;
 }
 
+/* Return true for all breakpoint types which are permitted to have
+   addresses which may be a duplicate of some other breakpoint.  E.g,
+   watchpoints will always have zero valued addresses and we don't
+   want check_duplicates() to mark a watchpoint as a duplicate of an
+   actual breakpoint at address zero.  */
+
+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 +3771,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 +3779,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 +3818,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] breakpoint.c: More check_duplicates() changes.

[Eli Zaretskii]

> Date: Sat, 12 May 2001 01:01:25 -0700
> From: Kevin Buettner <kevinb@cygnus.com>
>  
> +/* Return true for all breakpoint types which are permitted to have
> +   addresses which may be a duplicate of some other breakpoint.  E.g,
> +   watchpoints will always have zero valued addresses and we don't
> +   want check_duplicates() to mark a watchpoint as a duplicate of an
> +   actual breakpoint at address zero.  */
> +
> +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;
> +}
> +

Is it perhaps possible to explain, for each of these types, why it is
okay to duplicate it?  The comment only gives one example.

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

[Mark Kettenis]

Kevin Buettner <kevinb@cygnus.com> writes:

> +/* Return true for all breakpoint types which are permitted to have
> +   addresses which may be a duplicate of some other breakpoint.  E.g,
> +   watchpoints will always have zero valued addresses and we don't
> +   want check_duplicates() to mark a watchpoint as a duplicate of an
> +   actual breakpoint at address zero.  */
> +
> +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;
> +}

The GNU coding standards suggest adding () around the returned
expression.  Helps Emacs indenting this bit of code correctly :-).

Mark

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

[Jim Blandy]

Kevin Buettner <kevinb@cygnus.com> writes:
> It turns out that there are several other breakpoint types which
> were using zero-valued addresses to cause an early return from
> check_duplicates().  They are:
> 
>     bp_catch_exec
>     bp_longjmp_resume
>     bp_catch_fork
>     bp_catch_vfork

Wow, I really botched that patch.  I did try to look for this stuff.
:(


> The patch below creates a new function called duplicate_okay() and
> uses this function to effect the early return.  I.e, the above code
> has again been rewritten as follows:
> 
>       if (duplicate_okay (bpt))
> 	return;
> 
> I removed the half-truth telling comment too.  I think the above
> statement is reasonably self documenting.

I think the name `duplicate_okay' is misleading; I mean, it's okay to
have duplicate breakpoints, isn't it?  One has to read the function's
uses carefully to see why that name is appropriate.

I think a better name would be `has_meaningful_address', or something
like that: the function tells us whether the breakpoint structure's
`address' field is meaningful.  If a breakpoint's address isn't
meaningful, then we certainly shouldn't consider it when culling
duplicates.

Beyond that, I approve of this change.