Fix regression in always-inserted breakpoints mode.

Fix regression in always-inserted breakpoints mode.

[Pedro Alves]

I forward ported to mainline a patch that makes
always-inserted mode work with watchpoint locations too,
and surprisingly, GDB was still removing/inserting watchpoints
at every step/event.  The original patch worked correctly, but
it predated the update_global_location_list rework to qsort
the locations.

I traced it down to the use of bp_location_compare in
update_global_location_list.  All else being equal,
bp_location_compare compares the input pointers.
Often, after a remove/insert sequence, we reach the
bp_location_compare call line shown in the patch below,

       while (locp < bp_location + bp_location_count
	     && bp_location_compare (*locp, old_loc) < 0)
 	locp++;

with `*locp' and `old_loc' having the same address, but
bp_location_compare would still return < 0, due to the
fallback pointer comparision.  This made it so that
locp++ statement was done one time too much.  The
`found_object' check would fail, and GDB would uninsert
a location from the target, that shouldn't be removed.

There's for loop a bit below, that starts iterating on
`locp', and that would misses a few locations, due to the
spurious increment mentioned above.  Here's the loop
in question:

 		  for (loc2p = locp;
		       loc2p < bp_location + bp_location_count
		       && breakpoint_address_match ((*loc2p)->pspace->aspace,
						    (*loc2p)->address,
						    old_loc->pspace->aspace,
						    old_loc->address);
 		       loc2p++)

There's another problem with this for loop.  The
breakpoint_address_match is too strict when debugging
more than one inferior.  Even if breakpoint_address_match
fails due to address space mismatch, there could be other
locations with the same address following loc2p.

Here's a fix for all this.  Tested on x86-64-linux, native
and gdbserver, no regressions.

Jan, could you double check?

-- 
Pedro Alves

2009-11-20  Pedro Alves  <pedro@codesourcery.com>

	* breakpoint.c (update_global_location_list): Fix duplicate
	locations detection.

---
 gdb/breakpoint.c |   51 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 19 deletions(-)

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2009-11-20 00:38:12.000000000 +0000
+++ src/gdb/breakpoint.c	2009-11-20 01:02:18.000000000 +0000
@@ -8249,10 +8249,11 @@ update_global_location_list (int should_
        old_locp++)
     {
       struct bp_location *old_loc = *old_locp;
+      struct bp_location **loc2p;
 
       /* Tells if 'old_loc' is found amoung the new locations.  If not, we
 	 have to free it.  */
-      int found_object;
+      int found_object = 0;
       /* Tells if the location should remain inserted in the target.  */
       int keep_in_target = 0;
       int removed = 0;
@@ -8260,9 +8261,20 @@ update_global_location_list (int should_
       /* Skip LOCP entries which will definitely never be needed.  Stop either
 	 at or being the one matching OLD_LOC.  */
       while (locp < bp_location + bp_location_count
-	     && bp_location_compare (*locp, old_loc) < 0)
+	     && (*locp)->address < old_loc->address)
 	locp++;
-      found_object = locp < bp_location + bp_location_count && *locp == old_loc;
+
+      for (loc2p = locp;
+	   (loc2p < bp_location + bp_location_count
+	    && (*loc2p)->address == old_loc->address);
+	   loc2p++)
+	{
+	  if (*loc2p == old_loc)
+	    {
+	      found_object = 1;
+	      break;
+	    }
+	}
 
       /* If this location is no longer present, and inserted, look if there's
 	 maybe a new location at the same address.  If so, mark that one 
@@ -8288,27 +8300,28 @@ update_global_location_list (int should_
 
 	      if (breakpoint_address_is_meaningful (old_loc->owner))
 		{
-		  struct bp_location **loc2p;
-
 		  for (loc2p = locp;
-		       loc2p < bp_location + bp_location_count
-		       && breakpoint_address_match ((*loc2p)->pspace->aspace,
-						    (*loc2p)->address,
-						    old_loc->pspace->aspace,
-						    old_loc->address);
+		       (loc2p < bp_location + bp_location_count
+			&& (*loc2p)->address == old_loc->address);
 		       loc2p++)
 		    {
 		      struct bp_location *loc2 = *loc2p;
 
-		      /* For the sake of should_be_inserted.
-			 Duplicates check below will fix up this later.  */
-		      loc2->duplicate = 0;
-		      if (loc2 != old_loc && should_be_inserted (loc2))
-			{		  
-			  loc2->inserted = 1;
-			  loc2->target_info = old_loc->target_info;
-			  keep_in_target = 1;
-			  break;
+		      if (breakpoint_address_match (loc2->pspace->aspace,
+						    loc2->address,
+						    old_loc->pspace->aspace,
+						    old_loc->address))
+			{
+			  /* For the sake of should_be_inserted.
+			     Duplicates check below will fix up this later.  */
+			  loc2->duplicate = 0;
+			  if (loc2 != old_loc && should_be_inserted (loc2))
+			    {
+			      loc2->inserted = 1;
+			      loc2->target_info = old_loc->target_info;
+			      keep_in_target = 1;
+			      break;
+			    }
 			}
 		    }
 		}

Re: Fix regression in always-inserted breakpoints mode.

[Jan Kratochvil]

On Fri, 20 Nov 2009 03:11:18 +0100, Pedro Alves wrote:
> There's for loop a bit below, that starts iterating on
> `locp', and that would misses a few locations,

I agree I brought in such regression, thanks for catching it.

I will check-in the [obv] patch below after you check it in.


> There's another problem with this for loop.  The
> breakpoint_address_match is too strict when debugging
> more than one inferior.  Even if breakpoint_address_match
> fails due to address space mismatch, there could be other
> locations with the same address following loc2p.

This is unrelated to the regression from me.


Sorry,
Jan


gdb/
2009-11-20  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* breakpoint.c (bp_location_compare): Change parameter a to ap and b to
	bp.  New variables a and b.
	(bp_location_compare_for_qsort): Remove.
	(update_global_location_list): Use now bp_location_compare.

--- ./gdb/breakpoint.c-1	2009-11-20 12:22:12.000000000 +0100
+++ ./gdb/breakpoint.c	2009-11-20 12:27:44.000000000 +0100
@@ -8073,15 +8073,17 @@ breakpoint_auto_delete (bpstat bs)
   }
 }
 
-/* A comparison function for bp_location A and B being interfaced to qsort.
+/* A comparison function for bp_location AP and BP being interfaced to qsort.
    Sort elements primarily by their ADDRESS (no matter what does
    breakpoint_address_is_meaningful say for its OWNER), secondarily by ordering
    first bp_permanent OWNERed elements and terciarily just ensuring the array
    is sorted stable way despite qsort being an instable algorithm.  */
 
 static int
-bp_location_compare (struct bp_location *a, struct bp_location *b)
+bp_location_compare (const void *ap, const void *bp)
 {
+  struct bp_location *a = *(void **) ap;
+  struct bp_location *b = *(void **) bp;
   int a_perm = a->owner->enable_state == bp_permanent;
   int b_perm = b->owner->enable_state == bp_permanent;
 
@@ -8102,17 +8104,6 @@ bp_location_compare (struct bp_location 
   return (a > b) - (a < b);
 }
 
-/* Interface bp_location_compare as the COMPAR parameter of qsort function.  */
-
-static int
-bp_location_compare_for_qsort (const void *ap, const void *bp)
-{
-  struct bp_location *a = *(void **) ap;
-  struct bp_location *b = *(void **) bp;
-
-  return bp_location_compare (a, b);
-}
-
 /* Set bp_location_placed_address_before_address_max and
    bp_location_shadow_len_after_address_max according to the current content of
    the bp_location array.  */
@@ -8196,7 +8187,7 @@ update_global_location_list (int should_
     for (loc = b->loc; loc; loc = loc->next)
       *locp++ = loc;
   qsort (bp_location, bp_location_count, sizeof (*bp_location),
-	 bp_location_compare_for_qsort);
+	 bp_location_compare);
 
   bp_location_target_extensions_update ();
 

Re: Fix regression in always-inserted breakpoints mode.

[Pedro Alves]

On Friday 20 November 2009 11:38:21, Jan Kratochvil wrote:
> On Fri, 20 Nov 2009 03:11:18 +0100, Pedro Alves wrote:
> > There's for loop a bit below, that starts iterating on
> > `locp', and that would misses a few locations,
> 
> I agree I brought in such regression, thanks for catching it.
> 
> I will check-in the [obv] patch below after you check it in.

Good idea.  I've checked mine in.  Go ahead.

> > There's another problem with this for loop.  The
> > breakpoint_address_match is too strict when debugging
> > more than one inferior.  Even if breakpoint_address_match
> > fails due to address space mismatch, there could be other
> > locations with the same address following loc2p.
> 
> This is unrelated to the regression from me.

We were always looping over all locations previously,
and breakpoint_address_match wasn't a part of the for
loop conditional, so it is.  But that's okay, it doesn't
matter, regressions happen.  I was just looking for an extra
pair of eyes that knows this new code.

-- 
Pedro Alves

Re: Fix regression in always-inserted breakpoints mode.

[Jan Kratochvil]

On Fri, 20 Nov 2009 17:00:37 +0100, Pedro Alves wrote:
> On Friday 20 November 2009 11:38:21, Jan Kratochvil wrote:
> > I will check-in the [obv] patch below after you check it in.
> 
> Good idea.  I've checked mine in.  Go ahead.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2009-11/msg00179.html


> > > There's another problem with this for loop.  The
> > > breakpoint_address_match is too strict when debugging
> > > more than one inferior.  Even if breakpoint_address_match
> > > fails due to address space mismatch, there could be other
> > > locations with the same address following loc2p.
> > 
> > This is unrelated to the regression from me.
> 
> We were always looping over all locations previously,
> and breakpoint_address_match wasn't a part of the for
> loop conditional, so it is.

OK, I admit this was also a regression by my patch.

The rebase on the multi-inferior patch was done just as an "obvious"
pre-checkin update...


Sorry,
Jan