[PATCH] Fix breakpoint updates for multi-inferior

[PATCH] Fix breakpoint updates for multi-inferior

[Luis Gustavo]

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

Hi,

While working on the target-side condition evaluation patch, i stumbled 
upon the strange situation where GDB had more than a single "inserted" 
location in a list with multiple duplicate locations.

Further investigation showed that the logic for finding the first 
inserted location at a specific address does not work when multiple 
inferiors are being debugged. This code is inside 
update_global_location_list (...).

This is partly because we expect the list of locations to be sorted by 
breakpoint numbers and addresses.

Suppose we're going through locations at address FOO, and we already 
defined the "first" for that set of locations. When a location does not 
match the "first" location of that address, we then assume we've gone 
past the locations at address FOO. This is correct for single inferiors.

Now, consider a multi-inferior scenario and breakpoints with locations 
on multiple inferiors. The code will fail to match two locations due to 
the difference between the locations' program spaces, thus failing to 
mark duplicate locations correctly.

This patch solves this by updating the locations one program space at a 
time, thus preventing multiple insertions of the same location.

This bug shows up when doing multi-inferior debugging in GDBServer. You 
will notice GDB sending multiple insert/remove requests for the same 
address.

OK?

Luis

[-- Attachment #2: 0001-fix_multiprocess_breaks.diff --]
[-- Type: text/x-patch, Size: 6984 bytes --]

2012-01-25  Luis Machado  <lgustavo@codesourcery.com>

	* breakpoint.c (update_global_location_list): Handle *point
	updates one program space at a time.
	* progspace.h (ALL_PROGRAM_SPACES): New helper macro.

Index: gdb/gdb/breakpoint.c
===================================================================
--- gdb.orig/gdb/breakpoint.c	2012-01-25 10:19:35.453821938 -0200
+++ gdb/gdb/breakpoint.c	2012-01-25 11:11:17.721821938 -0200
@@ -10379,6 +10379,7 @@ update_global_location_list (int should_
      built bp_location from the current state of ALL_BREAKPOINTS.  */
   struct bp_location **old_location, **old_locp;
   unsigned old_location_count;
+  struct program_space *cur_pspace;
 
   old_location = bp_location;
   old_location_count = bp_location_count;
@@ -10586,72 +10587,88 @@ update_global_location_list (int should_
 	}
     }
 
-  /* Rescan breakpoints at the same address and section, marking the
-     first one as "first" and any others as "duplicates".  This is so
-     that the bpt instruction is only inserted once.  If we have a
-     permanent breakpoint at the same place as BPT, make that one the
-     official one, and the rest as duplicates.  Permanent breakpoints
-     are sorted first for the same address.
-
-     Do the same for hardware watchpoints, but also considering the
-     watchpoint's type (regular/access/read) and length.  */
-
-  bp_loc_first = NULL;
-  wp_loc_first = NULL;
-  awp_loc_first = NULL;
-  rwp_loc_first = NULL;
-  ALL_BP_LOCATIONS (loc, locp)
+  /* For each program space, go through the list of *points for that specific
+     program space.  This is needed due to assumption (by the code below) that
+     two locations don't match only if they don't have the same address/type,
+     since the location list is sorted first by addresses/breakpoint numbers.
+     The locations also don't match when their program spaces are different
+     (multiprocess).  */
+
+  ALL_PROGRAM_SPACES (cur_pspace)
     {
-      /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always
-	 non-NULL.  */
-      struct bp_location **loc_first_p;
-      b = loc->owner;
-
-      if (!should_be_inserted (loc)
-	  || !breakpoint_address_is_meaningful (b)
-	  /* Don't detect duplicate for tracepoint locations because they are
-	   never duplicated.  See the comments in field `duplicate' of
-	   `struct bp_location'.  */
-	  || is_tracepoint (b))
-	continue;
-
-      /* Permanent breakpoint should always be inserted.  */
-      if (b->enable_state == bp_permanent && ! loc->inserted)
-	internal_error (__FILE__, __LINE__,
-			_("allegedly permanent breakpoint is not "
-			"actually inserted"));
-
-      if (b->type == bp_hardware_watchpoint)
-	loc_first_p = &wp_loc_first;
-      else if (b->type == bp_read_watchpoint)
-	loc_first_p = &rwp_loc_first;
-      else if (b->type == bp_access_watchpoint)
-	loc_first_p = &awp_loc_first;
-      else
-	loc_first_p = &bp_loc_first;
-
-      if (*loc_first_p == NULL
-	  || (overlay_debugging && loc->section != (*loc_first_p)->section)
-	  || !breakpoint_locations_match (loc, *loc_first_p))
+      /* Rescan breakpoints at the same address and section, marking the
+	 first one as "first" and any others as "duplicates".  This is so
+     	 that the bpt instruction is only inserted once.  If we have a
+     	 permanent breakpoint at the same place as BPT, make that one the
+     	 official one, and the rest as duplicates.  Permanent breakpoints
+     	 are sorted first for the same address.
+
+	 Do the same for hardware watchpoints, but also considering the
+	 watchpoint's type (regular/access/read) and length.  */
+
+      bp_loc_first = NULL;
+      wp_loc_first = NULL;
+      awp_loc_first = NULL;
+      rwp_loc_first = NULL;
+
+      ALL_BP_LOCATIONS (loc, locp)
 	{
-	  *loc_first_p = loc;
-	  loc->duplicate = 0;
-	  continue;
-	}
+	  /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always
+	     non-NULL.  */
+	  struct bp_location **loc_first_p;
+	  b = loc->owner;
+
+	  /* Skip this location if it does not belong to the current
+	     program space we are looking for.  */
+	  if (loc->pspace != cur_pspace)
+	    continue;
+
+	  if (!should_be_inserted (loc)
+	      || !breakpoint_address_is_meaningful (b)
+	      /* Don't detect duplicate for tracepoint locations because they
+		 are never duplicated.  See the comments in field `duplicate' of
+	   `	 struct bp_location'.  */
+	      || is_tracepoint (b))
+	    continue;
+
+	  /* Permanent breakpoint should always be inserted.  */
+	  if (b->enable_state == bp_permanent && ! loc->inserted)
+	    internal_error (__FILE__, __LINE__,
+			    _("allegedly permanent breakpoint is not "
+			    "actually inserted"));
+
+	  if (b->type == bp_hardware_watchpoint)
+	    loc_first_p = &wp_loc_first;
+	  else if (b->type == bp_read_watchpoint)
+	    loc_first_p = &rwp_loc_first;
+	  else if (b->type == bp_access_watchpoint)
+	    loc_first_p = &awp_loc_first;
+	  else
+	    loc_first_p = &bp_loc_first;
 
+	  if (*loc_first_p == NULL
+	      || (overlay_debugging && loc->section != (*loc_first_p)->section)
+	      || !breakpoint_locations_match (loc, *loc_first_p))
+	    {
+	      *loc_first_p = loc;
+	      loc->duplicate = 0;
+	      continue;
+	    }
 
-      /* This and the above ensure the invariant that the first location
-	 is not duplicated, and is the inserted one.
-	 All following are marked as duplicated, and are not inserted.  */
-      if (loc->inserted)
-	swap_insertion (loc, *loc_first_p);
-      loc->duplicate = 1;
-
-      if ((*loc_first_p)->owner->enable_state == bp_permanent && loc->inserted
-	  && b->enable_state != bp_permanent)
-	internal_error (__FILE__, __LINE__,
-			_("another breakpoint was inserted on top of "
-			"a permanent breakpoint"));
+	  /* This and the above ensure the invariant that the first location
+	     is not duplicated, and is the inserted one.
+	     All following are marked as duplicated, and are not inserted.  */
+	  if (loc->inserted)
+	    swap_insertion (loc, *loc_first_p);
+	  loc->duplicate = 1;
+
+	  if ((*loc_first_p)->owner->enable_state == bp_permanent
+	      && loc->inserted
+	      && b->enable_state != bp_permanent)
+	    internal_error (__FILE__, __LINE__,
+			    _("another breakpoint was inserted on top of "
+			    "a permanent breakpoint"));
+	}
     }
 
   if (breakpoints_always_inserted_mode () && should_insert
Index: gdb/gdb/progspace.h
===================================================================
--- gdb.orig/gdb/progspace.h	2012-01-25 10:21:24.321821938 -0200
+++ gdb/gdb/progspace.h	2012-01-25 10:41:55.701821938 -0200
@@ -193,6 +193,13 @@ struct program_space
     unsigned num_data;
   };
 
+/* Iterate over all program spaces.  */
+
+#define ALL_PROGRAM_SPACES(PSPACE)	      \
+	for (PSPACE = program_spaces;	      \
+	     PSPACE;			      \
+	     PSPACE = PSPACE->next)
+
 /* The object file that the main symbol table was loaded from (e.g. the
    argument to the "symbol-file" or "file" command).  */
 

Re: [PATCH] Fix breakpoint updates for multi-inferior

[Luis Gustavo]

On 01/25/2012 06:07 PM, Luis Gustavo wrote:
> Hi,
>
> While working on the target-side condition evaluation patch, i stumbled
> upon the strange situation where GDB had more than a single "inserted"
> location in a list with multiple duplicate locations.
>
> Further investigation showed that the logic for finding the first
> inserted location at a specific address does not work when multiple
> inferiors are being debugged. This code is inside
> update_global_location_list (...).
>
> This is partly because we expect the list of locations to be sorted by
> breakpoint numbers and addresses.
>
> Suppose we're going through locations at address FOO, and we already
> defined the "first" for that set of locations. When a location does not
> match the "first" location of that address, we then assume we've gone
> past the locations at address FOO. This is correct for single inferiors.
>
> Now, consider a multi-inferior scenario and breakpoints with locations
> on multiple inferiors. The code will fail to match two locations due to
> the difference between the locations' program spaces, thus failing to
> mark duplicate locations correctly.
>
> This patch solves this by updating the locations one program space at a
> time, thus preventing multiple insertions of the same location.
>
> This bug shows up when doing multi-inferior debugging in GDBServer. You
> will notice GDB sending multiple insert/remove requests for the same
> address.
>
> OK?
>
> Luis

Ping?

Re: [PATCH] Fix breakpoint updates for multi-inferior

[Pedro Alves]

On 01/25/2012 08:07 PM, Luis Gustavo wrote:
> Hi,
> 
> While working on the target-side condition evaluation patch, i stumbled upon the strange situation where GDB had more than a single "inserted" location in a list with multiple duplicate locations.
> 
> Further investigation showed that the logic for finding the first inserted location at a specific address does not work when multiple inferiors are being debugged. This code is inside update_global_location_list (...).
> 
> This is partly because we expect the list of locations to be sorted by breakpoint numbers and addresses.
> 
> Suppose we're going through locations at address FOO, and we already defined the "first" for that set of locations. When a location does not match the "first" location of that address, we then assume we've gone past the locations at address FOO. This is correct for single inferiors.
> 
> Now, consider a multi-inferior scenario and breakpoints with locations on multiple inferiors. The code will fail to match two locations due to the difference between the locations' program spaces, thus failing to mark duplicate locations correctly.
> 
> This patch solves this by updating the locations one program space at a time, thus preventing multiple insertions of the same location.
> 
> This bug shows up when doing multi-inferior debugging in GDBServer. You will notice GDB sending multiple insert/remove requests for the same address.
> 
> OK?

So if I understood correctly, we may end up with a location list like:

#1 PSPACE1 ADDR1
#2 PSPACE2 ADDR1
#3 PSPACE1 ADDR1

and then we fail to detect that locations #1 and #3 are duplicate, because
locations #1 -> #2 don't match, and locations #2 -> #3 don't match, even though
locations #1 and #3 match, and should be considered duplicate.

Your change makes gdb loop over all locations once for each program space.
How about we sort by program space in addition to address in the first place, so
that we'd have:

#1 PSPACE1 ADDR1
#2 PSPACE1 ADDR1
#3 PSPACE2 ADDR1

and things would then still work correctly with just one pass?

-- 
Pedro Alves

Re: [PATCH] Fix breakpoint updates for multi-inferior

[Luis Gustavo]

On 02/08/2012 01:16 PM, Pedro Alves wrote:
> On 01/25/2012 08:07 PM, Luis Gustavo wrote:
>> Hi,
>>
>> While working on the target-side condition evaluation patch, i stumbled upon the strange situation where GDB had more than a single "inserted" location in a list with multiple duplicate locations.
>>
>> Further investigation showed that the logic for finding the first inserted location at a specific address does not work when multiple inferiors are being debugged. This code is inside update_global_location_list (...).
>>
>> This is partly because we expect the list of locations to be sorted by breakpoint numbers and addresses.
>>
>> Suppose we're going through locations at address FOO, and we already defined the "first" for that set of locations. When a location does not match the "first" location of that address, we then assume we've gone past the locations at address FOO. This is correct for single inferiors.
>>
>> Now, consider a multi-inferior scenario and breakpoints with locations on multiple inferiors. The code will fail to match two locations due to the difference between the locations' program spaces, thus failing to mark duplicate locations correctly.
>>
>> This patch solves this by updating the locations one program space at a time, thus preventing multiple insertions of the same location.
>>
>> This bug shows up when doing multi-inferior debugging in GDBServer. You will notice GDB sending multiple insert/remove requests for the same address.
>>
>> OK?
>
> So if I understood correctly, we may end up with a location list like:
>
> #1 PSPACE1 ADDR1
> #2 PSPACE2 ADDR1
> #3 PSPACE1 ADDR1
>
> and then we fail to detect that locations #1 and #3 are duplicate, because
> locations #1 ->  #2 don't match, and locations #2 ->  #3 don't match, even though
> locations #1 and #3 match, and should be considered duplicate.
>
> Your change makes gdb loop over all locations once for each program space.
> How about we sort by program space in addition to address in the first place, so
> that we'd have:
>
> #1 PSPACE1 ADDR1
> #2 PSPACE1 ADDR1
> #3 PSPACE2 ADDR1
>
> and things would then still work correctly with just one pass?
>

breakpoint.c:bp_location_compare (...)'s comment about keeping a stable 
user-visible ordering of breakpoints made me consider that solution 
inappropriate.

Maybe i'm missing something?

Luis

Re: [PATCH] Fix breakpoint updates for multi-inferior

[Pedro Alves]

On 02/08/2012 03:27 PM, Luis Gustavo wrote:

>> Your change makes gdb loop over all locations once for each program space.
>> How about we sort by program space in addition to address in the first place, so
>> that we'd have:
>>
>> #1 PSPACE1 ADDR1
>> #2 PSPACE1 ADDR1
>> #3 PSPACE2 ADDR1
>>
>> and things would then still work correctly with just one pass?
>>
> 
> breakpoint.c:bp_location_compare (...)'s comment about keeping a stable user-visible ordering of breakpoints made me consider that solution inappropriate.
> 
> Maybe i'm missing something?

We're already sorting by address first, so I'm not really sure what is
it that's user-visible that we're trying to preserve.  Jan?  Even if that
is still necessary, would it be ok to sort by address, then pspace, and only
after by bkpt number?

-- 
Pedro Alves

[patch] update_global_location_list my comment fix [Re: [PATCH] Fix breakpoint updates for multi-inferior]

[Jan Kratochvil]

On Wed, 08 Feb 2012 16:47:23 +0100, Pedro Alves wrote:
> We're already sorting by address first, so I'm not really sure what is
> it that's user-visible that we're trying to preserve.  Jan?

I no longer remember if

(a) I was wrongly expecting "duplicate"-marked locations are somehow visible
    in "info breakpoints".
or
(b) <the new patch comment below>.

I will check the comment change in, I hope everyone agrees with the reason.


> Even if that is still necessary, would it be ok to sort by address, then
> pspace, and only after by bkpt number?

I agree with Pedro, update_global_location_list was introduced as GDB
acceleration as the breakpoints performance became no longer bearable.

While update_global_location_list is far from perfect (it should be
incremental) we should not regress performance when it is enough to do it just
in a bit different way as Pedro suggests.


Thanks,
Jan


gdb/
2012-02-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* breakpoint.c (bp_location_compare): Fix comment.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -10589,8 +10589,9 @@ bp_location_compare (const void *ap, const void *bp)
   if (a_perm != b_perm)
     return (a_perm < b_perm) - (a_perm > b_perm);
 
-  /* Make the user-visible order stable across GDB runs.  Locations of
-     the same breakpoint can be sorted in arbitrary order.  */
+  /* Make the internal GDB representation stable across GDB runs
+     where A and B memory inside GDB can differ.  Breakpoint locations of
+     the same type at the same address can be sorted in arbitrary order.  */
 
   if (a->owner->number != b->owner->number)
     return (a->owner->number > b->owner->number)

Re: [patch] update_global_location_list my comment fix [Re: [PATCH] Fix breakpoint updates for multi-inferior]

[Luis Gustavo]

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

On 02/08/2012 03:27 PM, Jan Kratochvil wrote:
> On Wed, 08 Feb 2012 16:47:23 +0100, Pedro Alves wrote:
>> We're already sorting by address first, so I'm not really sure what is
>> it that's user-visible that we're trying to preserve.  Jan?
>
> I no longer remember if
>
> (a) I was wrongly expecting "duplicate"-marked locations are somehow visible
>      in "info breakpoints".
> or
> (b)<the new patch comment below>.
>
> I will check the comment change in, I hope everyone agrees with the reason.
>
>
>> Even if that is still necessary, would it be ok to sort by address, then
>> pspace, and only after by bkpt number?
>
> I agree with Pedro, update_global_location_list was introduced as GDB
> acceleration as the breakpoints performance became no longer bearable.
>
> While update_global_location_list is far from perfect (it should be
> incremental) we should not regress performance when it is enough to do it just
> in a bit different way as Pedro suggests.

I agree. Here's a new patch that touches the bp_location_compare 
function instead.

Luis

[-- Attachment #2: 0001-fix_multiprocess_breaks.diff --]
[-- Type: text/x-patch, Size: 937 bytes --]

2012-02-08  Luis Machado  <lgustavo@codesourcery.com>

	* breakpoint.c (bp_location_compare): Sort by pspace before sorting by
	number.

Index: gdb/gdb/breakpoint.c
===================================================================
--- gdb.orig/gdb/breakpoint.c	2012-02-08 15:55:24.535075001 -0200
+++ gdb/gdb/breakpoint.c	2012-02-08 15:57:26.107075004 -0200
@@ -10589,6 +10589,13 @@ bp_location_compare (const void *ap, con
   if (a_perm != b_perm)
     return (a_perm < b_perm) - (a_perm > b_perm);
 
+  /* Sort by pspace.  This effectively sorts locations by inferior in
+     a multi-inferior environment.  */
+
+  if (a->pspace != b->pspace)
+    return (a->pspace > b->pspace)
+	   - (a->pspace < b->pspace);
+
   /* Make the internal GDB representation stable across GDB runs
      where A and B memory inside GDB can differ.  Breakpoint locations of
      the same type at the same address can be sorted in arbitrary order.  */

Re: [patch] update_global_location_list my comment fix [Re: [PATCH] Fix breakpoint updates for multi-inferior]

[Joel Brobecker]

I don't know this area, so cannot formally review, but a minor comment:

> +  if (a->pspace != b->pspace)
> +    return (a->pspace > b->pspace)
> +	   - (a->pspace < b->pspace);

The GNU Coding Standards asks us to use an extra pair of parentheses
in order to help code formatters, even if it is strictly not necessary
here, thus:

        return ((a->pspace > b->pspace)
                - ((a->pspace < b->pspace)));

But going beyond this, ISTM that you can simply put the entire
expression on one line and be done with it:

  if (a->pspace != b->pspace)
    return (a->pspace > b->pspace) - (a->pspace < b->pspace);

-- 
Joel

Re: [patch] update_global_location_list my comment fix [Re: [PATCH] Fix breakpoint updates for multi-inferior]

[Luis Gustavo]

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

On 02/08/2012 09:32 PM, Joel Brobecker wrote:
> I don't know this area, so cannot formally review, but a minor comment:
>
>> +  if (a->pspace != b->pspace)
>> +    return (a->pspace>  b->pspace)
>> +	   - (a->pspace<  b->pspace);
>
> The GNU Coding Standards asks us to use an extra pair of parentheses
> in order to help code formatters, even if it is strictly not necessary
> here, thus:
>
>          return ((a->pspace>  b->pspace)
>                  - ((a->pspace<  b->pspace)));
>
> But going beyond this, ISTM that you can simply put the entire
> expression on one line and be done with it:
>
>    if (a->pspace != b->pspace)
>      return (a->pspace>  b->pspace) - (a->pspace<  b->pspace);
>

I went with putting everything on one line.

Luis

[-- Attachment #2: 0001-fix_multiprocess_breaks.diff --]
[-- Type: text/x-patch, Size: 932 bytes --]

2012-02-08  Luis Machado  <lgustavo@codesourcery.com>

	* breakpoint.c (bp_location_compare): Sort by pspace before sorting by
	number.

Index: gdb/gdb/breakpoint.c
===================================================================
--- gdb.orig/gdb/breakpoint.c	2012-02-08 19:29:41.887074999 -0200
+++ gdb/gdb/breakpoint.c	2012-02-08 21:38:10.935074996 -0200
@@ -10589,6 +10589,12 @@ bp_location_compare (const void *ap, con
   if (a_perm != b_perm)
     return (a_perm < b_perm) - (a_perm > b_perm);
 
+  /* Sort by pspace.  This effectively sorts locations by inferior in
+     a multi-inferior environment.  */
+
+  if (a->pspace != b->pspace)
+    return (a->pspace > b->pspace) - (a->pspace < b->pspace);
+
   /* Make the internal GDB representation stable across GDB runs
      where A and B memory inside GDB can differ.  Breakpoint locations of
      the same type at the same address can be sorted in arbitrary order.  */

[commit] [patch] update_global_location_list my comment fix [Re: [PATCH] Fix breakpoint updates for multi-inferior]

[Jan Kratochvil]

http://sourceware.org/ml/gdb-cvs/2012-02/msg00048.html

--- src/gdb/ChangeLog	2012/02/08 19:54:35	1.13816
+++ src/gdb/ChangeLog	2012/02/09 08:20:03	1.13817
@@ -1,3 +1,7 @@
+2012-02-09  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* breakpoint.c (bp_location_compare): Fix comment.  Reindent the code.
+
 2012-02-08  Joel Brobecker  <brobecker@adacore.com>
 
 	* language.h (symbol_name_cmp_ftype): Renames
--- src/gdb/breakpoint.c	2012/02/02 20:19:12	1.650
+++ src/gdb/breakpoint.c	2012/02/09 08:20:03	1.651
@@ -10589,12 +10589,13 @@
   if (a_perm != b_perm)
     return (a_perm < b_perm) - (a_perm > b_perm);
 
-  /* Make the user-visible order stable across GDB runs.  Locations of
-     the same breakpoint can be sorted in arbitrary order.  */
+  /* Make the internal GDB representation stable across GDB runs
+     where A and B memory inside GDB can differ.  Breakpoint locations of
+     the same type at the same address can be sorted in arbitrary order.  */
 
   if (a->owner->number != b->owner->number)
-    return (a->owner->number > b->owner->number)
-           - (a->owner->number < b->owner->number);
+    return ((a->owner->number > b->owner->number)
+	    - (a->owner->number < b->owner->number));
 
   return (a > b) - (a < b);
 }

Re: [patch] update_global_location_list my comment fix [Re: [PATCH] Fix breakpoint updates for multi-inferior]

[Jan Kratochvil]

On Thu, 09 Feb 2012 00:39:42 +0100, Luis Gustavo wrote:
> +  /* Sort by pspace.  This effectively sorts locations by inferior in
> +     a multi-inferior environment.  */
> +
> +  if (a->pspace != b->pspace)
> +    return (a->pspace > b->pspace) - (a->pspace < b->pspace);

This does not follow the comment I made:

  /* Make the internal GDB representation stable across GDB runs
     where A and B memory inside GDB can differ.

I would prefer there (and it will not fit on a single line :-) ):

       return (a->pspace->num > b->pspace->num) - (a->pspace->num < b->pspace->num);

NUM should be the same on each GDB run for the same inferior / command file.


Thanks,
Jan

Re: [patch] update_global_location_list my comment fix [Re: [PATCH] Fix breakpoint updates for multi-inferior]

[Luis Gustavo]

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

On 02/09/2012 06:22 AM, Jan Kratochvil wrote:
> On Thu, 09 Feb 2012 00:39:42 +0100, Luis Gustavo wrote:
>> +  /* Sort by pspace.  This effectively sorts locations by inferior in
>> +     a multi-inferior environment.  */
>> +
>> +  if (a->pspace != b->pspace)
>> +    return (a->pspace>  b->pspace) - (a->pspace<  b->pspace);
>
> This does not follow the comment I made:
>
>    /* Make the internal GDB representation stable across GDB runs
>       where A and B memory inside GDB can differ.
>
> I would prefer there (and it will not fit on a single line :-) ):
>
>         return (a->pspace->num>  b->pspace->num) - (a->pspace->num<  b->pspace->num);
>
> NUM should be the same on each GDB run for the same inferior / command file.

I would still like to keep an appropriate comment regarding 
multi-inferiors next to the pspace comparison, like the following... 
maybe it would've been best to keep the change above your new comment. 
Here's a new version.

What do you think?



[-- Attachment #2: 0001-fix_multiprocess_breaks.diff --]
[-- Type: text/x-patch, Size: 1024 bytes --]

2012-02-09  Luis Machado  <lgustavo@codesourcery.com>

	* breakpoint.c (bp_location_compare): Sort by pspace before sorting by
	number.

Index: gdb/gdb/breakpoint.c
===================================================================
--- gdb.orig/gdb/breakpoint.c	2012-02-09 08:46:30.495074997 -0200
+++ gdb/gdb/breakpoint.c	2012-02-09 09:02:14.227075000 -0200
@@ -10593,6 +10593,14 @@ bp_location_compare (const void *ap, con
      where A and B memory inside GDB can differ.  Breakpoint locations of
      the same type at the same address can be sorted in arbitrary order.  */
 
+  /* Sort locations at the same address by their pspace number, keeping
+     locations of the same inferior (in a multi-inferior environment)
+     grouped.  */
+
+  if (a->pspace->num != b->pspace->num)
+    return ((a->pspace->num > b->pspace->num)
+	    - (a->pspace->num < b->pspace->num));
+
   if (a->owner->number != b->owner->number)
     return ((a->owner->number > b->owner->number)
 	    - (a->owner->number < b->owner->number));

Re: [patch] update_global_location_list my comment fix [Re: [PATCH] Fix breakpoint updates for multi-inferior]

[Pedro Alves]

On 02/09/2012 11:04 AM, Luis Gustavo wrote:
> On 02/09/2012 06:22 AM, Jan Kratochvil wrote:
>> On Thu, 09 Feb 2012 00:39:42 +0100, Luis Gustavo wrote:
>>> +  /* Sort by pspace.  This effectively sorts locations by inferior in
>>> +     a multi-inferior environment.  */
>>> +
>>> +  if (a->pspace != b->pspace)
>>> +    return (a->pspace>  b->pspace) - (a->pspace<  b->pspace);
>>
>> This does not follow the comment I made:
>>
>>    /* Make the internal GDB representation stable across GDB runs
>>       where A and B memory inside GDB can differ.
>>
>> I would prefer there (and it will not fit on a single line :-) ):
>>
>>         return (a->pspace->num>  b->pspace->num) - (a->pspace->num<  b->pspace->num);
>>
>> NUM should be the same on each GDB run for the same inferior / command file.
> 
> I would still like to keep an appropriate comment regarding multi-inferiors next to the pspace comparison, like the following... maybe it would've been best to keep the change above your new comment. Here's a new version.
> 
> What do you think?

We need to sort by pspace even before that, before:

  /* Sort permanent breakpoints first.  */
  if (a_perm != b_perm)
    return (a_perm < b_perm) - (a_perm > b_perm);

So that you don't get:

 #1 PSPACE1 ADDR1 PERM
 #2 PSPACE2 ADDR1 PERM
 #3 PSPACE1 ADDR1
 #4 PSPACE2 ADDR1

But instead:

 #1 PSPACE1 ADDR1 PERM
 #2 PSPACE1 ADDR1
 #3 PSPACE2 ADDR1 PERM
 #4 PSPACE2 ADDR1

-- 
Pedro Alves

Re: [patch] update_global_location_list my comment fix [Re: [PATCH] Fix breakpoint updates for multi-inferior]

[Luis Gustavo]

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

On 02/09/2012 09:31 AM, Pedro Alves wrote:
> On 02/09/2012 11:04 AM, Luis Gustavo wrote:
>> On 02/09/2012 06:22 AM, Jan Kratochvil wrote:
>>> On Thu, 09 Feb 2012 00:39:42 +0100, Luis Gustavo wrote:
>>>> +  /* Sort by pspace.  This effectively sorts locations by inferior in
>>>> +     a multi-inferior environment.  */
>>>> +
>>>> +  if (a->pspace != b->pspace)
>>>> +    return (a->pspace>   b->pspace) - (a->pspace<   b->pspace);
>>>
>>> This does not follow the comment I made:
>>>
>>>     /* Make the internal GDB representation stable across GDB runs
>>>        where A and B memory inside GDB can differ.
>>>
>>> I would prefer there (and it will not fit on a single line :-) ):
>>>
>>>          return (a->pspace->num>   b->pspace->num) - (a->pspace->num<   b->pspace->num);
>>>
>>> NUM should be the same on each GDB run for the same inferior / command file.
>>
>> I would still like to keep an appropriate comment regarding multi-inferiors next to the pspace comparison, like the following... maybe it would've been best to keep the change above your new comment. Here's a new version.
>>
>> What do you think?
>
> We need to sort by pspace even before that, before:
>
>    /* Sort permanent breakpoints first.  */
>    if (a_perm != b_perm)
>      return (a_perm<  b_perm) - (a_perm>  b_perm);
>
> So that you don't get:
>
>   #1 PSPACE1 ADDR1 PERM
>   #2 PSPACE2 ADDR1 PERM
>   #3 PSPACE1 ADDR1
>   #4 PSPACE2 ADDR1
>
> But instead:
>
>   #1 PSPACE1 ADDR1 PERM
>   #2 PSPACE1 ADDR1
>   #3 PSPACE2 ADDR1 PERM
>   #4 PSPACE2 ADDR1
>

I've checked the following in...

Luis

[-- Attachment #2: 0001-fix_multiprocess_breaks.diff --]
[-- Type: text/x-patch, Size: 947 bytes --]

2012-02-24  Luis Machado  <lgustavo@codesourcery.com>

	* breakpoint.c (bp_location_compare): Sort by pspace before sorting by
	number.

Index: gdb/gdb/breakpoint.c
===================================================================
--- gdb.orig/gdb/breakpoint.c	2012-02-24 12:44:27.582553984 -0200
+++ gdb/gdb/breakpoint.c	2012-02-24 12:44:59.210553984 -0200
@@ -10609,6 +10609,14 @@ bp_location_compare (const void *ap, con
   if (a->address != b->address)
     return (a->address > b->address) - (a->address < b->address);
 
+  /* Sort locations at the same address by their pspace number, keeping
+     locations of the same inferior (in a multi-inferior environment)
+     grouped.  */
+
+  if (a->pspace->num != b->pspace->num)
+    return ((a->pspace->num > b->pspace->num)
+	    - (a->pspace->num < b->pspace->num));
+
   /* Sort permanent breakpoints first.  */
   if (a_perm != b_perm)
     return (a_perm < b_perm) - (a_perm > b_perm);