public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [patch 1/3] Clear stale specific locs, not whole bpts
@ 2010-05-03 20:02 Jan Kratochvil
  2010-05-17 21:25 ` [patch 1/3] Clear stale specific locs, not whole bpts [rediff] Jan Kratochvil
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Kratochvil @ 2010-05-03 20:02 UTC (permalink / raw)
  To: gdb-patches

Hi,

while I do not have a reproducer of a bug I believe this is a right cleanup as
currently after update_breakpoint_locations thread_info->stop_bpstat may
contain stale bp_location references.  Just the current code never
dereferences such stale reference.

As the new handle_inferior_event code can execute multiple actions where the
preceding ones can call update_breakpoint_locations I have found this fixups
as required for this patchset.


Thanks,
Jan


gdb/
2010-05-03  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Clear stale specific bp_location from former whole breakpoint.
	* breakpoint.c (delete_breakpoint): Move the stale referencing clear
	code ...
	(free_bp_location): ... here.  Rename there the called function to
	bpstat_remove_bp_location_callback.
	(bpstat_remove_breakpoint_callback): Rename to ...
	(bpstat_remove_bp_location_callback): ... here, change DATA resolution
	to struct bp_location.  Change the called function to
	bpstat_remove_bp_location.  Create new declaration for the function.
	(bpstat_remove_breakpoint): Rename to ...
	(bpstat_remove_bp_location): ..., change the parameter to loc, adjust
	code for the new parameter type.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -206,6 +206,9 @@ static void update_global_location_list (int);
 
 static void update_global_location_list_nothrow (int);
 
+static int bpstat_remove_bp_location_callback (struct thread_info *th,
+					       void *data);
+
 static int is_hardware_watchpoint (const struct breakpoint *bpt);
 
 static int is_watchpoint (const struct breakpoint *bpt);
@@ -5377,6 +5380,18 @@ allocate_bp_location (struct breakpoint *bpt)
 
 static void free_bp_location (struct bp_location *loc)
 {
+  /* Be sure no bpstat's are pointing at it after it's been freed.  */
+  /* FIXME, how can we find all bpstat's?
+     We just check stop_bpstat for now.  Note that we cannot just
+     remove bpstats pointing at bpt from the stop_bpstat list
+     entirely, as breakpoint commands are associated with the bpstat;
+     if we remove it here, then the later call to
+         bpstat_do_actions (&stop_bpstat);
+     in event-top.c won't do anything, and temporary breakpoints
+     with commands won't work.  */
+
+  iterate_over_threads (bpstat_remove_bp_location_callback, loc);
+
   if (loc->cond)
     xfree (loc->cond);
 
@@ -9208,13 +9223,13 @@ update_global_location_list_nothrow (int inserting)
     update_global_location_list (inserting);
 }
 
-/* Clear BPT from a BPS.  */
+/* Clear LOC from a BPS.  */
 static void
-bpstat_remove_breakpoint (bpstat bps, struct breakpoint *bpt)
+bpstat_remove_bp_location (bpstat bps, struct bp_location *loc)
 {
   bpstat bs;
   for (bs = bps; bs; bs = bs->next)
-    if (bs->breakpoint_at && bs->breakpoint_at->owner == bpt)
+    if (bs->breakpoint_at == loc)
       {
 	bs->breakpoint_at = NULL;
 	bs->old_val = NULL;
@@ -9224,10 +9239,11 @@ bpstat_remove_breakpoint (bpstat bps, struct breakpoint *bpt)
 
 /* Callback for iterate_over_threads.  */
 static int
-bpstat_remove_breakpoint_callback (struct thread_info *th, void *data)
+bpstat_remove_bp_location_callback (struct thread_info *th, void *data)
 {
-  struct breakpoint *bpt = data;
-  bpstat_remove_breakpoint (th->stop_bpstat, bpt);
+  struct bp_location *loc = data;
+
+  bpstat_remove_bp_location (th->stop_bpstat, loc);
   return 0;
 }
 
@@ -9291,18 +9307,6 @@ delete_breakpoint (struct breakpoint *bpt)
   xfree (bpt->exec_pathname);
   clean_up_filters (&bpt->syscalls_to_be_caught);
 
-  /* Be sure no bpstat's are pointing at it after it's been freed.  */
-  /* FIXME, how can we find all bpstat's?
-     We just check stop_bpstat for now.  Note that we cannot just
-     remove bpstats pointing at bpt from the stop_bpstat list
-     entirely, as breakpoint commands are associated with the bpstat;
-     if we remove it here, then the later call to
-         bpstat_do_actions (&stop_bpstat);
-     in event-top.c won't do anything, and temporary breakpoints
-     with commands won't work.  */
-
-  iterate_over_threads (bpstat_remove_breakpoint_callback, bpt);
-
   /* Now that breakpoint is removed from breakpoint
      list, update the global location list.  This
      will remove locations that used to belong to

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [patch 1/3] Clear stale specific locs, not whole bpts [rediff]
  2010-05-03 20:02 [patch 1/3] Clear stale specific locs, not whole bpts Jan Kratochvil
@ 2010-05-17 21:25 ` Jan Kratochvil
  2010-05-24 12:18   ` Pedro Alves
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Kratochvil @ 2010-05-17 21:25 UTC (permalink / raw)
  To: gdb-patches

Hi,

while I do not have a reproducer of a bug I believe this is a right cleanup as
currently after update_breakpoint_locations thread_info->stop_bpstat may
contain stale bp_location references.  Just the current code never
dereferences such stale reference.

As the new handle_inferior_event code can execute multiple actions where the
preceding ones can call update_breakpoint_locations I have found this fixups
as required for this patchset.


Thanks,
Jan


gdb/
2010-05-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Clear stale specific bp_location from former whole breakpoint.
	* breakpoint.c (delete_breakpoint): Move the stale referencing clear
	code ...
	(free_bp_location): ... here.  Rename there the called function to
	bpstat_remove_bp_location_callback.
	(bpstat_remove_breakpoint_callback): Rename to ...
	(bpstat_remove_bp_location_callback): ... here, change DATA resolution
	to struct bp_location.  Change the called function to
	bpstat_remove_bp_location.  Create new declaration for the function.
	(bpstat_remove_breakpoint): Rename to ...
	(bpstat_remove_bp_location): ..., change the parameter to loc, adjust
	code for the new parameter type.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -206,6 +206,9 @@ static void update_global_location_list (int);
 
 static void update_global_location_list_nothrow (int);
 
+static int bpstat_remove_bp_location_callback (struct thread_info *th,
+					       void *data);
+
 static int is_hardware_watchpoint (const struct breakpoint *bpt);
 
 static int is_watchpoint (const struct breakpoint *bpt);
@@ -5371,6 +5374,18 @@ allocate_bp_location (struct breakpoint *bpt)
 
 static void free_bp_location (struct bp_location *loc)
 {
+  /* Be sure no bpstat's are pointing at it after it's been freed.  */
+  /* FIXME, how can we find all bpstat's?
+     We just check stop_bpstat for now.  Note that we cannot just
+     remove bpstats pointing at bpt from the stop_bpstat list
+     entirely, as breakpoint commands are associated with the bpstat;
+     if we remove it here, then the later call to
+         bpstat_do_actions (&stop_bpstat);
+     in event-top.c won't do anything, and temporary breakpoints
+     with commands won't work.  */
+
+  iterate_over_threads (bpstat_remove_bp_location_callback, loc);
+
   if (loc->cond)
     xfree (loc->cond);
 
@@ -9219,14 +9234,14 @@ update_global_location_list_nothrow (int inserting)
     update_global_location_list (inserting);
 }
 
-/* Clear BPT from a BPS.  */
+/* Clear LOC from a BPS.  */
 static void
-bpstat_remove_breakpoint (bpstat bps, struct breakpoint *bpt)
+bpstat_remove_bp_location (bpstat bps, struct bp_location *loc)
 {
   bpstat bs;
 
   for (bs = bps; bs; bs = bs->next)
-    if (bs->breakpoint_at && bs->breakpoint_at->owner == bpt)
+    if (bs->breakpoint_at == loc)
       {
 	bs->breakpoint_at = NULL;
 	bs->old_val = NULL;
@@ -9236,11 +9251,11 @@ bpstat_remove_breakpoint (bpstat bps, struct breakpoint *bpt)
 
 /* Callback for iterate_over_threads.  */
 static int
-bpstat_remove_breakpoint_callback (struct thread_info *th, void *data)
+bpstat_remove_bp_location_callback (struct thread_info *th, void *data)
 {
-  struct breakpoint *bpt = data;
+  struct bp_location *loc = data;
 
-  bpstat_remove_breakpoint (th->stop_bpstat, bpt);
+  bpstat_remove_bp_location (th->stop_bpstat, loc);
   return 0;
 }
 
@@ -9303,18 +9318,6 @@ delete_breakpoint (struct breakpoint *bpt)
   xfree (bpt->exec_pathname);
   clean_up_filters (&bpt->syscalls_to_be_caught);
 
-  /* Be sure no bpstat's are pointing at it after it's been freed.  */
-  /* FIXME, how can we find all bpstat's?
-     We just check stop_bpstat for now.  Note that we cannot just
-     remove bpstats pointing at bpt from the stop_bpstat list
-     entirely, as breakpoint commands are associated with the bpstat;
-     if we remove it here, then the later call to
-         bpstat_do_actions (&stop_bpstat);
-     in event-top.c won't do anything, and temporary breakpoints
-     with commands won't work.  */
-
-  iterate_over_threads (bpstat_remove_breakpoint_callback, bpt);
-
   /* Now that breakpoint is removed from breakpoint
      list, update the global location list.  This
      will remove locations that used to belong to

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch 1/3] Clear stale specific locs, not whole bpts [rediff]
  2010-05-17 21:25 ` [patch 1/3] Clear stale specific locs, not whole bpts [rediff] Jan Kratochvil
@ 2010-05-24 12:18   ` Pedro Alves
  2010-06-04 19:19     ` Jan Kratochvil
  0 siblings, 1 reply; 15+ messages in thread
From: Pedro Alves @ 2010-05-24 12:18 UTC (permalink / raw)
  To: gdb-patches; +Cc: Jan Kratochvil

On Monday 17 May 2010 22:22:43, Jan Kratochvil wrote:
>  static void free_bp_location (struct bp_location *loc)
>  {
> +  /* Be sure no bpstat's are pointing at it after it's been freed.  */
> +  /* FIXME, how can we find all bpstat's?
> +     We just check stop_bpstat for now.  Note that we cannot just
> +     remove bpstats pointing at bpt from the stop_bpstat list
> +     entirely, as breakpoint commands are associated with the bpstat;
> +     if we remove it here, then the later call to
> +         bpstat_do_actions (&stop_bpstat);
> +     in event-top.c won't do anything, and temporary breakpoints
> +     with commands won't work.  */
> +
> +  iterate_over_threads (bpstat_remove_bp_location_callback, loc);
> +

I think this isn't the best place for this, considering moribund
breakpoint locations --- it's too late.  When you delete a
breakpoint in non-stop/always-inserted modes, you'll delete
the breakpoint, but deleting the bp_location itself is deferred.
This means that in that case, the thread's bpstat chains may still
contain references to bp_locations that are now in the moribund list
(those will be still valid objects, not xfree'd yet), but their owner
breakpoint is gone, meaning bpstat walks can still dereference those
now invalid bp_location->owner pointers.

Instead, can you move this to within in the `if (!found_object)' branch
of update_global_location_list?  That is, move it close to where we
detected we had unlinked the bp_location from the global
location list?

-- 
Pedro Alves

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch 1/3] Clear stale specific locs, not whole bpts [rediff]
  2010-05-24 12:18   ` Pedro Alves
@ 2010-06-04 19:19     ` Jan Kratochvil
  2010-06-07 11:50       ` Pedro Alves
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Kratochvil @ 2010-06-04 19:19 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On Sun, 23 May 2010 23:40:17 +0200, Pedro Alves wrote:
> On Monday 17 May 2010 22:22:43, Jan Kratochvil wrote:
> >  static void free_bp_location (struct bp_location *loc)
> >  {
> > +  iterate_over_threads (bpstat_remove_bp_location_callback, loc);
> 
> I think this isn't the best place for this, considering moribund
> breakpoint locations --- it's too late.  When you delete a
> breakpoint in non-stop/always-inserted modes, you'll delete
> the breakpoint, but deleting the bp_location itself is deferred.
> This means that in that case, the thread's bpstat chains may still
> contain references to bp_locations that are now in the moribund list
> (those will be still valid objects, not xfree'd yet), but their owner
> breakpoint is gone, meaning bpstat walks can still dereference those
> now invalid bp_location->owner pointers.
      ^^^^^^^
Not "invalid" but they are explicitly NULL:
static void
update_global_location_list (int should_insert)
              old_loc->owner = NULL;
              VEC_safe_push (bp_location_p, moribund_locations, old_loc);

And the code expects such state with valid bp_location but NULL its ->owner.
struct bpstat_what
bpstat_what (bpstat bs)
      if (bs->breakpoint_at->owner == NULL)
        bs_class = bp_nostop;

Other bs->breakpoint_at->owner dereferencing code either checks it is NULL or
such code cannot meet bpstat referencing moribund bp_location.


> Instead, can you move this to within in the `if (!found_object)' branch
> of update_global_location_list?  That is, move it close to where we
> detected we had unlinked the bp_location from the global
> location list?

While I can do the simple move in next mail do you still request it?

I try to follow the paradigm to always remove references exactly at the point
one of the two peers dies.  That makes the code both simple and foolproof for
future modifications even by such programmers like me.

Therefore if something is referencing bp_location then the reference should
die at the point bp_location dies and that's all.


Thanks,
Jan


2010-06-04  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* breakpoint.h (owner): Extend the comment.

--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -244,7 +244,8 @@ struct bp_location
 
   /* Each breakpoint location must belong to exactly one higher-level
      breakpoint.  This and the DUPLICATE flag are more straightforward
-     than reference counting.  */
+     than reference counting.  This pointer is NULL iff this bp_location is in
+     (and therefore only in) moribund_locations.  */
   struct breakpoint *owner;
 
   /* Conditional.  Break only if this expression's value is nonzero.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch 1/3] Clear stale specific locs, not whole bpts [rediff]
  2010-06-04 19:19     ` Jan Kratochvil
@ 2010-06-07 11:50       ` Pedro Alves
  2010-06-07 13:40         ` Jan Kratochvil
  0 siblings, 1 reply; 15+ messages in thread
From: Pedro Alves @ 2010-06-07 11:50 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: gdb-patches

On Friday 04 June 2010 20:18:55, Jan Kratochvil wrote:

> While I can do the simple move in next mail do you still request it?

I'm not going to request it.  We'll probably end up with refcounting
these things at some point anyway.  Your patch is okay.

> 2010-06-04  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* breakpoint.h (owner): Extend the comment.

Okay too.  Thanks.

-- 
Pedro Alves

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch 1/3] Clear stale specific locs, not whole bpts [rediff]
  2010-06-07 11:50       ` Pedro Alves
@ 2010-06-07 13:40         ` Jan Kratochvil
  2010-06-10 14:09           ` Frederic Riss
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Kratochvil @ 2010-06-07 13:40 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On Mon, 07 Jun 2010 13:50:31 +0200, Pedro Alves wrote:
> On Friday 04 June 2010 20:18:55, Jan Kratochvil wrote:
> 
> > While I can do the simple move in next mail do you still request it?
> 
> I'm not going to request it.  We'll probably end up with refcounting
> these things at some point anyway.  Your patch is okay.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2010-06/msg00058.html


> > 2010-06-04  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	* breakpoint.h (owner): Extend the comment.
> 
> Okay too.  Thanks.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2010-06/msg00059.html


Thanks,
Jan

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch 1/3] Clear stale specific locs, not whole bpts [rediff]
  2010-06-07 13:40         ` Jan Kratochvil
@ 2010-06-10 14:09           ` Frederic Riss
  2010-06-10 20:39             ` Jan Kratochvil
  0 siblings, 1 reply; 15+ messages in thread
From: Frederic Riss @ 2010-06-10 14:09 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: Pedro Alves, gdb-patches

Hi !

On 7 June 2010 15:40, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> Checked-in:
>        http://sourceware.org/ml/gdb-cvs/2010-06/msg00058.html

Seems that this commit is partially broken. In non-stop mode, after
deleteing a step breakpoint, GDB call print_it_typical () on a bpstat
that references that moribund breakpoint. Looking at the
print_it_typical () code, you'll quickly see that this means crash.
It's very easy to reproduce:

------ foo.c ---------------------------
int foo () {
	return 0;
}

int main (int argc, char *argv[])
{
	return foo ();
}
------ foo.c ---------------------------

gcc -g foo.c && gdb a.out -ex 'set non-stop' -ex start -ex n

Cheers,
Fred

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch 1/3] Clear stale specific locs, not whole bpts [rediff]
  2010-06-10 14:09           ` Frederic Riss
@ 2010-06-10 20:39             ` Jan Kratochvil
  2010-06-11  7:58               ` Frederic Riss
                                 ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Jan Kratochvil @ 2010-06-10 20:39 UTC (permalink / raw)
  To: Frederic Riss; +Cc: Pedro Alves, gdb-patches

On Thu, 10 Jun 2010 16:08:46 +0200, Frederic Riss wrote:
> Seems that this commit is partially broken. In non-stop mode, after
> deleteing a step breakpoint, GDB call print_it_typical () on a bpstat
> that references that moribund breakpoint.

:-(  I was wrong and not precise enough checking for my:

On Fri, 04 Jun 2010 21:18:55 +0200, Jan Kratochvil wrote:
# Other bs->breakpoint_at->owner dereferencing code either checks it is NULL
# or such code cannot meet bpstat referencing moribund bp_location.


Is this patch OK?  I understand the former Pedro A.'s suggested way would not
have this regression.

No regressions on {x86_64,x86_64-m32,i686}-fedora13-linux-gnu.


Sorry,
Jan


gdb/
2010-06-10  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* breakpoint.c (breakpoint_restore_shadows): New OWNER comment.
	(should_be_inserted): Return zero also on NULL OWNER.
	(breakpoint_program_space_exit): New OWNER comment.
	(insert_breakpoint_locations): Extend comment for OWNER.
	(remove_breakpoint_1, remove_breakpoint): Assert on OWNER.
	(breakpoint_init_inferior, breakpoint_here_p, breakpoint_thread_match):
	New OWNER comment.
	(print_it_typical): Return PRINT_UNKNOWN on NULL OWNER.
	(watchpoint_check): New assert on BREAKPOINT_AT and OWNER.
	(bpstat_check_location): New assert on OWNER.
	(bpstat_check_watchpoint, bpstat_check_breakpoint_conditions): Move BL
	and B initializations to the code block.  New assert on them.
	(print_one_breakpoint_location): New OWNER comment.
	(watchpoint_locations_match): Assert on OWNER.
	(breakpoint_locations_match): Move HW_POINT1 and HW_POINT2
	initializations to the code block.  New assert on OWNER.
	(set_breakpoint_location_function): New assert on OWNER.
	(disable_breakpoints_in_shlibs, disable_breakpoints_in_unloaded_shlib)
	(bp_location_compare, update_global_location_list)
	(update_global_location_list): New OWNER comment.

gdb/testsuite/
2010-06-10  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/moribund-step.exp: New.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1112,6 +1112,7 @@ breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len)
     int bp_size = 0;
     int bptoffset = 0;
 
+    /* bp_location array has B->OWNER always non-NULL.  */
     if (b->owner->type == bp_none)
       warning (_("reading through apparently deleted breakpoint #%d?"),
               b->owner->number);
@@ -1571,7 +1572,7 @@ in which its expression is valid.\n"),
 static int
 should_be_inserted (struct bp_location *bpt)
 {
-  if (!breakpoint_enabled (bpt->owner))
+  if (bpt->owner == NULL || !breakpoint_enabled (bpt->owner))
     return 0;
 
   if (bpt->owner->disposition == disp_del_at_next_stop)
@@ -1874,6 +1875,7 @@ breakpoint_program_space_exit (struct program_space *pspace)
 
       if (loc->pspace == pspace)
 	{
+	  /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL.  */
 	  if (loc->owner->loc == loc)
 	    loc->owner->loc = loc->next;
 	  else
@@ -1943,7 +1945,8 @@ insert_breakpoint_locations (void)
 	continue;
 
       /* There is no point inserting thread-specific breakpoints if the
-	 thread no longer exists.  */
+	 thread no longer exists.  ALL_BP_LOCATIONS bp_location has B->OWNER
+	 always non-NULL.  */
       if (b->owner->thread != -1
 	  && !valid_thread_id (b->owner->thread))
 	continue;
@@ -2394,6 +2397,9 @@ remove_breakpoint_1 (struct bp_location *b, insertion_state_t is)
 {
   int val;
 
+  /* B is never in moribund_locations by our callers.  */
+  gdb_assert (b->owner != NULL);
+
   if (b->owner->enable_state == bp_permanent)
     /* Permanent breakpoints cannot be inserted or removed.  */
     return 0;
@@ -2509,6 +2515,9 @@ remove_breakpoint (struct bp_location *b, insertion_state_t is)
   int ret;
   struct cleanup *old_chain;
 
+  /* B is never in moribund_locations by our callers.  */
+  gdb_assert (b->owner != NULL);
+
   if (b->owner->enable_state == bp_permanent)
     /* Permanent breakpoints cannot be inserted or removed.  */
     return 0;
@@ -2566,6 +2575,7 @@ breakpoint_init_inferior (enum inf_context context)
 
   ALL_BP_LOCATIONS (bpt, bptp_tmp)
   {
+    /* ALL_BP_LOCATIONS bp_location has BPT->OWNER always non-NULL.  */
     if (bpt->pspace == pspace
 	&& bpt->owner->enable_state != bp_permanent)
       bpt->inserted = 0;
@@ -2663,6 +2673,7 @@ breakpoint_here_p (struct address_space *aspace, CORE_ADDR pc)
 	  && bpt->loc_type != bp_loc_hardware_breakpoint)
 	continue;
 
+      /* ALL_BP_LOCATIONS bp_location has BPT->OWNER always non-NULL.  */
       if ((breakpoint_enabled (bpt->owner)
 	   || bpt->owner->enable_state == bp_permanent)
 	  && breakpoint_address_match (bpt->pspace->aspace, bpt->address,
@@ -2827,6 +2838,7 @@ breakpoint_thread_match (struct address_space *aspace, CORE_ADDR pc,
 	  && bpt->loc_type != bp_loc_hardware_breakpoint)
 	continue;
 
+      /* ALL_BP_LOCATIONS bp_location has BPT->OWNER always non-NULL.  */
       if (!breakpoint_enabled (bpt->owner)
 	  && bpt->owner->enable_state != bp_permanent)
 	continue;
@@ -3193,6 +3205,11 @@ print_it_typical (bpstat bs)
   if (bs->breakpoint_at == NULL)
     return PRINT_UNKNOWN;
   bl = bs->breakpoint_at;
+
+  /* bl->owner can be NULL if it was a momentary breakpoint
+     which has since been placed into moribund_locations.  */
+  if (bl->owner == NULL)
+    return PRINT_UNKNOWN;
   b = bl->owner;
 
   stb = ui_out_stream_new (uiout);
@@ -3563,6 +3580,9 @@ watchpoint_check (void *p)
   struct frame_info *fr;
   int within_current_scope;
 
+  /* BS is built for existing struct breakpoint.  */
+  gdb_assert (bs->breakpoint_at != NULL);
+  gdb_assert (bs->breakpoint_at->owner != NULL);
   b = bs->breakpoint_at->owner;
 
   /* If this is a local watchpoint, we only want to check if the
@@ -3691,6 +3711,9 @@ bpstat_check_location (const struct bp_location *bl,
 {
   struct breakpoint *b = bl->owner;
 
+  /* BL is from existing struct breakpoint.  */
+  gdb_assert (b != NULL);
+
   /* By definition, the inferior does not report stops at
      tracepoints.  */
   if (is_tracepoint (b))
@@ -3746,8 +3769,14 @@ bpstat_check_location (const struct bp_location *bl,
 static void
 bpstat_check_watchpoint (bpstat bs)
 {
-  const struct bp_location *bl = bs->breakpoint_at;
-  struct breakpoint *b = bl->owner;
+  const struct bp_location *bl;
+  struct breakpoint *b;
+
+  /* BS is built for existing struct breakpoint.  */
+  bl = bs->breakpoint_at;
+  gdb_assert (bl != NULL);
+  b = bl->owner;
+  gdb_assert (b != NULL);
 
   if (is_watchpoint (b))
     {
@@ -3899,8 +3928,14 @@ static void
 bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
 {
   int thread_id = pid_to_thread_id (ptid);
-  const struct bp_location *bl = bs->breakpoint_at;
-  struct breakpoint *b = bl->owner;
+  const struct bp_location *bl;
+  struct breakpoint *b;
+
+  /* BS is built for existing struct breakpoint.  */
+  bl = bs->breakpoint_at;
+  gdb_assert (bl != NULL);
+  b = bl->owner;
+  gdb_assert (b != NULL);
 
   if (frame_id_p (b->frame_id)
       && !frame_id_eq (b->frame_id, get_stack_frame_id (get_current_frame ())))
@@ -4678,6 +4713,8 @@ print_one_breakpoint_location (struct breakpoint *b,
 	  || (!gdbarch_has_global_breakpoints (target_gdbarch)
 	      && (number_of_program_spaces () > 1
 		  || number_of_inferiors () > 1)
+	      /* LOC is for existing B, it cannot be in moribund_locations and
+		 thus having NULL OWNER.  */
 	      && loc->owner->type != bp_catchpoint)))
     {
       struct inferior *inf;
@@ -5214,6 +5250,10 @@ breakpoint_address_is_meaningful (struct breakpoint *bpt)
 static int
 watchpoint_locations_match (struct bp_location *loc1, struct bp_location *loc2)
 {
+  /* Both of them must not be in moribund_locations.  */
+  gdb_assert (loc1->owner != NULL);
+  gdb_assert (loc2->owner != NULL);
+
   /* Note that this checks the owner's type, not the location's.  In
      case the target does not support read watchpoints, but does
      support access watchpoints, we'll have bp_read_watchpoint
@@ -5247,8 +5287,14 @@ breakpoint_address_match (struct address_space *aspace1, CORE_ADDR addr1,
 static int
 breakpoint_locations_match (struct bp_location *loc1, struct bp_location *loc2)
 {
-  int hw_point1 = is_hardware_watchpoint (loc1->owner);
-  int hw_point2 = is_hardware_watchpoint (loc2->owner);
+  int hw_point1, hw_point2;
+
+  /* Both of them must not be in moribund_locations.  */
+  gdb_assert (loc1->owner != NULL);
+  gdb_assert (loc2->owner != NULL);
+
+  hw_point1 = is_hardware_watchpoint (loc1->owner);
+  hw_point2 = is_hardware_watchpoint (loc2->owner);
 
   if (hw_point1 != hw_point2)
     return 0;
@@ -5445,6 +5491,8 @@ set_raw_breakpoint_without_location (struct gdbarch *gdbarch,
 static void
 set_breakpoint_location_function (struct bp_location *loc)
 {
+  gdb_assert (loc->owner != NULL);
+
   if (loc->owner->type == bp_breakpoint
       || loc->owner->type == bp_hardware_breakpoint
       || is_tracepoint (loc->owner))
@@ -5727,7 +5775,9 @@ disable_breakpoints_in_shlibs (void)
 
   ALL_BP_LOCATIONS (loc, locp_tmp)
   {
+    /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL.  */
     struct breakpoint *b = loc->owner;
+
     /* We apply the check to all breakpoints, including disabled
        for those with loc->duplicate set.  This is so that when breakpoint
        becomes enabled, or the duplicate is removed, gdb will try to insert
@@ -5770,6 +5820,7 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
 
   ALL_BP_LOCATIONS (loc, locp_tmp)
   {
+    /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL.  */
     struct breakpoint *b = loc->owner;
 
     if ((loc->loc_type == bp_loc_hardware_breakpoint
@@ -8848,6 +8899,7 @@ bp_location_compare (const void *ap, const void *bp)
 {
   struct bp_location *a = *(void **) ap;
   struct bp_location *b = *(void **) bp;
+  /* A and B come from existing breakpoints having non-NULL OWNER.  */
   int a_perm = a->owner->enable_state == bp_permanent;
   int b_perm = b->owner->enable_state == bp_permanent;
 
@@ -9025,6 +9077,7 @@ update_global_location_list (int should_insert)
 		 See if there's another location at the same address, in which 
 		 case we don't need to remove this one from the target.  */
 
+	      /* OLD_LOC comes from existing struct breakpoint.  */
 	      if (breakpoint_address_is_meaningful (old_loc->owner))
 		{
 		  for (loc2p = locp;
@@ -9157,6 +9210,7 @@ update_global_location_list (int should_insert)
   rwp_loc_first = NULL;
   ALL_BP_LOCATIONS (loc, locp)
     {
+      /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL.  */
       struct breakpoint *b = loc->owner;
       struct bp_location **loc_first_p;
 
--- /dev/null
+++ b/gdb/testsuite/gdb.base/moribund-step.exp
@@ -0,0 +1,28 @@
+# Copyright (C) 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set testfile moribund-step
+if { [prepare_for_testing ${testfile}.exp ${testfile} start.c] } {
+    return -1
+}
+
+gdb_test_no_output "set non-stop on"
+
+if ![runto_main] {
+    return
+}
+
+# GDB could crash on printing breakpoint hit on a moribund bp_location.
+gdb_test "step"

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch 1/3] Clear stale specific locs, not whole bpts [rediff]
  2010-06-10 20:39             ` Jan Kratochvil
@ 2010-06-11  7:58               ` Frederic Riss
  2010-06-11 13:00                 ` Jan Kratochvil
  2010-06-11 10:50               ` Pedro Alves
  2010-06-11 16:24               ` Ulrich Weigand
  2 siblings, 1 reply; 15+ messages in thread
From: Frederic Riss @ 2010-06-11  7:58 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: Pedro Alves, gdb-patches

On 10 June 2010 22:39, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> :-(  I was wrong and not precise enough checking for my:
>
> On Fri, 04 Jun 2010 21:18:55 +0200, Jan Kratochvil wrote:
> # Other bs->breakpoint_at->owner dereferencing code either checks it is NULL
> # or such code cannot meet bpstat referencing moribund bp_location.
>
> Is this patch OK?  I understand the former Pedro A.'s suggested way would not
> have this regression.

I didn't review the patch, but I tested it and I can confirm that it
fixes the issue and does not regress for me.

Fred

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch 1/3] Clear stale specific locs, not whole bpts [rediff]
  2010-06-10 20:39             ` Jan Kratochvil
  2010-06-11  7:58               ` Frederic Riss
@ 2010-06-11 10:50               ` Pedro Alves
  2010-06-11 16:24               ` Ulrich Weigand
  2 siblings, 0 replies; 15+ messages in thread
From: Pedro Alves @ 2010-06-11 10:50 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: Frederic Riss, gdb-patches

On Thursday 10 June 2010 21:39:13, Jan Kratochvil wrote:
> On Thu, 10 Jun 2010 16:08:46 +0200, Frederic Riss wrote:
> > Seems that this commit is partially broken. In non-stop mode, after
> > deleteing a step breakpoint, GDB call print_it_typical () on a bpstat
> > that references that moribund breakpoint.
> 
> :-(  I was wrong and not precise enough checking for my:
> 
> On Fri, 04 Jun 2010 21:18:55 +0200, Jan Kratochvil wrote:
> # Other bs->breakpoint_at->owner dereferencing code either checks it is NULL
> # or such code cannot meet bpstat referencing moribund bp_location.
> 
> 
> Is this patch OK?  I understand the former Pedro A.'s suggested way would not
> have this regression.

Okay.

> 
> No regressions on {x86_64,x86_64-m32,i686}-fedora13-linux-gnu.
> 
> 
> Sorry,
> Jan
> 
> 
> gdb/
> 2010-06-10  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* breakpoint.c (breakpoint_restore_shadows): New OWNER comment.
> 	(should_be_inserted): Return zero also on NULL OWNER.
> 	(breakpoint_program_space_exit): New OWNER comment.
> 	(insert_breakpoint_locations): Extend comment for OWNER.
> 	(remove_breakpoint_1, remove_breakpoint): Assert on OWNER.
> 	(breakpoint_init_inferior, breakpoint_here_p, breakpoint_thread_match):
> 	New OWNER comment.
> 	(print_it_typical): Return PRINT_UNKNOWN on NULL OWNER.
> 	(watchpoint_check): New assert on BREAKPOINT_AT and OWNER.
> 	(bpstat_check_location): New assert on OWNER.
> 	(bpstat_check_watchpoint, bpstat_check_breakpoint_conditions): Move BL
> 	and B initializations to the code block.  New assert on them.
> 	(print_one_breakpoint_location): New OWNER comment.
> 	(watchpoint_locations_match): Assert on OWNER.
> 	(breakpoint_locations_match): Move HW_POINT1 and HW_POINT2
> 	initializations to the code block.  New assert on OWNER.
> 	(set_breakpoint_location_function): New assert on OWNER.
> 	(disable_breakpoints_in_shlibs, disable_breakpoints_in_unloaded_shlib)
> 	(bp_location_compare, update_global_location_list)
> 	(update_global_location_list): New OWNER comment.
> 
> gdb/testsuite/
> 2010-06-10  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.base/moribund-step.exp: New.
> 
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -1112,6 +1112,7 @@ breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len)
>      int bp_size = 0;
>      int bptoffset = 0;
>  
> +    /* bp_location array has B->OWNER always non-NULL.  */
>      if (b->owner->type == bp_none)
>        warning (_("reading through apparently deleted breakpoint #%d?"),
>                b->owner->number);
> @@ -1571,7 +1572,7 @@ in which its expression is valid.\n"),
>  static int
>  should_be_inserted (struct bp_location *bpt)
>  {
> -  if (!breakpoint_enabled (bpt->owner))
> +  if (bpt->owner == NULL || !breakpoint_enabled (bpt->owner))
>      return 0;
>  
>    if (bpt->owner->disposition == disp_del_at_next_stop)
> @@ -1874,6 +1875,7 @@ breakpoint_program_space_exit (struct program_space *pspace)
>  
>        if (loc->pspace == pspace)
>  	{
> +	  /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL.  */
>  	  if (loc->owner->loc == loc)
>  	    loc->owner->loc = loc->next;
>  	  else
> @@ -1943,7 +1945,8 @@ insert_breakpoint_locations (void)
>  	continue;
>  
>        /* There is no point inserting thread-specific breakpoints if the
> -	 thread no longer exists.  */
> +	 thread no longer exists.  ALL_BP_LOCATIONS bp_location has B->OWNER
> +	 always non-NULL.  */
>        if (b->owner->thread != -1
>  	  && !valid_thread_id (b->owner->thread))
>  	continue;
> @@ -2394,6 +2397,9 @@ remove_breakpoint_1 (struct bp_location *b, insertion_state_t is)
>  {
>    int val;
>  
> +  /* B is never in moribund_locations by our callers.  */
> +  gdb_assert (b->owner != NULL);
> +
>    if (b->owner->enable_state == bp_permanent)
>      /* Permanent breakpoints cannot be inserted or removed.  */
>      return 0;
> @@ -2509,6 +2515,9 @@ remove_breakpoint (struct bp_location *b, insertion_state_t is)
>    int ret;
>    struct cleanup *old_chain;
>  
> +  /* B is never in moribund_locations by our callers.  */
> +  gdb_assert (b->owner != NULL);
> +
>    if (b->owner->enable_state == bp_permanent)
>      /* Permanent breakpoints cannot be inserted or removed.  */
>      return 0;
> @@ -2566,6 +2575,7 @@ breakpoint_init_inferior (enum inf_context context)
>  
>    ALL_BP_LOCATIONS (bpt, bptp_tmp)
>    {
> +    /* ALL_BP_LOCATIONS bp_location has BPT->OWNER always non-NULL.  */
>      if (bpt->pspace == pspace
>  	&& bpt->owner->enable_state != bp_permanent)
>        bpt->inserted = 0;
> @@ -2663,6 +2673,7 @@ breakpoint_here_p (struct address_space *aspace, CORE_ADDR pc)
>  	  && bpt->loc_type != bp_loc_hardware_breakpoint)
>  	continue;
>  
> +      /* ALL_BP_LOCATIONS bp_location has BPT->OWNER always non-NULL.  */
>        if ((breakpoint_enabled (bpt->owner)
>  	   || bpt->owner->enable_state == bp_permanent)
>  	  && breakpoint_address_match (bpt->pspace->aspace, bpt->address,
> @@ -2827,6 +2838,7 @@ breakpoint_thread_match (struct address_space *aspace, CORE_ADDR pc,
>  	  && bpt->loc_type != bp_loc_hardware_breakpoint)
>  	continue;
>  
> +      /* ALL_BP_LOCATIONS bp_location has BPT->OWNER always non-NULL.  */
>        if (!breakpoint_enabled (bpt->owner)
>  	  && bpt->owner->enable_state != bp_permanent)
>  	continue;
> @@ -3193,6 +3205,11 @@ print_it_typical (bpstat bs)
>    if (bs->breakpoint_at == NULL)
>      return PRINT_UNKNOWN;
>    bl = bs->breakpoint_at;
> +
> +  /* bl->owner can be NULL if it was a momentary breakpoint
> +     which has since been placed into moribund_locations.  */
> +  if (bl->owner == NULL)
> +    return PRINT_UNKNOWN;
>    b = bl->owner;
>  
>    stb = ui_out_stream_new (uiout);
> @@ -3563,6 +3580,9 @@ watchpoint_check (void *p)
>    struct frame_info *fr;
>    int within_current_scope;
>  
> +  /* BS is built for existing struct breakpoint.  */
> +  gdb_assert (bs->breakpoint_at != NULL);
> +  gdb_assert (bs->breakpoint_at->owner != NULL);
>    b = bs->breakpoint_at->owner;
>  
>    /* If this is a local watchpoint, we only want to check if the
> @@ -3691,6 +3711,9 @@ bpstat_check_location (const struct bp_location *bl,
>  {
>    struct breakpoint *b = bl->owner;
>  
> +  /* BL is from existing struct breakpoint.  */
> +  gdb_assert (b != NULL);
> +
>    /* By definition, the inferior does not report stops at
>       tracepoints.  */
>    if (is_tracepoint (b))
> @@ -3746,8 +3769,14 @@ bpstat_check_location (const struct bp_location *bl,
>  static void
>  bpstat_check_watchpoint (bpstat bs)
>  {
> -  const struct bp_location *bl = bs->breakpoint_at;
> -  struct breakpoint *b = bl->owner;
> +  const struct bp_location *bl;
> +  struct breakpoint *b;
> +
> +  /* BS is built for existing struct breakpoint.  */
> +  bl = bs->breakpoint_at;
> +  gdb_assert (bl != NULL);
> +  b = bl->owner;
> +  gdb_assert (b != NULL);
>  
>    if (is_watchpoint (b))
>      {
> @@ -3899,8 +3928,14 @@ static void
>  bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
>  {
>    int thread_id = pid_to_thread_id (ptid);
> -  const struct bp_location *bl = bs->breakpoint_at;
> -  struct breakpoint *b = bl->owner;
> +  const struct bp_location *bl;
> +  struct breakpoint *b;
> +
> +  /* BS is built for existing struct breakpoint.  */
> +  bl = bs->breakpoint_at;
> +  gdb_assert (bl != NULL);
> +  b = bl->owner;
> +  gdb_assert (b != NULL);
>  
>    if (frame_id_p (b->frame_id)
>        && !frame_id_eq (b->frame_id, get_stack_frame_id (get_current_frame ())))
> @@ -4678,6 +4713,8 @@ print_one_breakpoint_location (struct breakpoint *b,
>  	  || (!gdbarch_has_global_breakpoints (target_gdbarch)
>  	      && (number_of_program_spaces () > 1
>  		  || number_of_inferiors () > 1)
> +	      /* LOC is for existing B, it cannot be in moribund_locations and
> +		 thus having NULL OWNER.  */
>  	      && loc->owner->type != bp_catchpoint)))
>      {
>        struct inferior *inf;
> @@ -5214,6 +5250,10 @@ breakpoint_address_is_meaningful (struct breakpoint *bpt)
>  static int
>  watchpoint_locations_match (struct bp_location *loc1, struct bp_location *loc2)
>  {
> +  /* Both of them must not be in moribund_locations.  */
> +  gdb_assert (loc1->owner != NULL);
> +  gdb_assert (loc2->owner != NULL);
> +
>    /* Note that this checks the owner's type, not the location's.  In
>       case the target does not support read watchpoints, but does
>       support access watchpoints, we'll have bp_read_watchpoint
> @@ -5247,8 +5287,14 @@ breakpoint_address_match (struct address_space *aspace1, CORE_ADDR addr1,
>  static int
>  breakpoint_locations_match (struct bp_location *loc1, struct bp_location *loc2)
>  {
> -  int hw_point1 = is_hardware_watchpoint (loc1->owner);
> -  int hw_point2 = is_hardware_watchpoint (loc2->owner);
> +  int hw_point1, hw_point2;
> +
> +  /* Both of them must not be in moribund_locations.  */
> +  gdb_assert (loc1->owner != NULL);
> +  gdb_assert (loc2->owner != NULL);
> +
> +  hw_point1 = is_hardware_watchpoint (loc1->owner);
> +  hw_point2 = is_hardware_watchpoint (loc2->owner);
>  
>    if (hw_point1 != hw_point2)
>      return 0;
> @@ -5445,6 +5491,8 @@ set_raw_breakpoint_without_location (struct gdbarch *gdbarch,
>  static void
>  set_breakpoint_location_function (struct bp_location *loc)
>  {
> +  gdb_assert (loc->owner != NULL);
> +
>    if (loc->owner->type == bp_breakpoint
>        || loc->owner->type == bp_hardware_breakpoint
>        || is_tracepoint (loc->owner))
> @@ -5727,7 +5775,9 @@ disable_breakpoints_in_shlibs (void)
>  
>    ALL_BP_LOCATIONS (loc, locp_tmp)
>    {
> +    /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL.  */
>      struct breakpoint *b = loc->owner;
> +
>      /* We apply the check to all breakpoints, including disabled
>         for those with loc->duplicate set.  This is so that when breakpoint
>         becomes enabled, or the duplicate is removed, gdb will try to insert
> @@ -5770,6 +5820,7 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
>  
>    ALL_BP_LOCATIONS (loc, locp_tmp)
>    {
> +    /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL.  */
>      struct breakpoint *b = loc->owner;
>  
>      if ((loc->loc_type == bp_loc_hardware_breakpoint
> @@ -8848,6 +8899,7 @@ bp_location_compare (const void *ap, const void *bp)
>  {
>    struct bp_location *a = *(void **) ap;
>    struct bp_location *b = *(void **) bp;
> +  /* A and B come from existing breakpoints having non-NULL OWNER.  */
>    int a_perm = a->owner->enable_state == bp_permanent;
>    int b_perm = b->owner->enable_state == bp_permanent;
>  
> @@ -9025,6 +9077,7 @@ update_global_location_list (int should_insert)
>  		 See if there's another location at the same address, in which 
>  		 case we don't need to remove this one from the target.  */
>  
> +	      /* OLD_LOC comes from existing struct breakpoint.  */
>  	      if (breakpoint_address_is_meaningful (old_loc->owner))
>  		{
>  		  for (loc2p = locp;
> @@ -9157,6 +9210,7 @@ update_global_location_list (int should_insert)
>    rwp_loc_first = NULL;
>    ALL_BP_LOCATIONS (loc, locp)
>      {
> +      /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL.  */
>        struct breakpoint *b = loc->owner;
>        struct bp_location **loc_first_p;
>  
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/moribund-step.exp
> @@ -0,0 +1,28 @@
> +# Copyright (C) 2010 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +set testfile moribund-step
> +if { [prepare_for_testing ${testfile}.exp ${testfile} start.c] } {
> +    return -1
> +}
> +
> +gdb_test_no_output "set non-stop on"
> +
> +if ![runto_main] {
> +    return
> +}
> +
> +# GDB could crash on printing breakpoint hit on a moribund bp_location.
> +gdb_test "step"
> 


-- 
Pedro Alves

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch 1/3] Clear stale specific locs, not whole bpts [rediff]
  2010-06-11  7:58               ` Frederic Riss
@ 2010-06-11 13:00                 ` Jan Kratochvil
  0 siblings, 0 replies; 15+ messages in thread
From: Jan Kratochvil @ 2010-06-11 13:00 UTC (permalink / raw)
  To: Frederic Riss, Pedro Alves; +Cc: gdb-patches

On Fri, 11 Jun 2010 09:58:27 +0200, Frederic Riss wrote:
> I didn't review the patch, but I tested it and I can confirm that it
> fixes the issue and does not regress for me.

Thanks for the test.


On Fri, 11 Jun 2010 12:50:03 +0200, Pedro Alves wrote:
> On Thursday 10 June 2010 21:39:13, Jan Kratochvil wrote:
> > Is this patch OK?  I understand the former Pedro A.'s suggested way would not
> > have this regression.
> 
> Okay.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2010-06/msg00084.html


Thanks,
Jan

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch 1/3] Clear stale specific locs, not whole bpts [rediff]
  2010-06-10 20:39             ` Jan Kratochvil
  2010-06-11  7:58               ` Frederic Riss
  2010-06-11 10:50               ` Pedro Alves
@ 2010-06-11 16:24               ` Ulrich Weigand
  2010-06-11 18:34                 ` Jan Kratochvil
  2 siblings, 1 reply; 15+ messages in thread
From: Ulrich Weigand @ 2010-06-11 16:24 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: Frederic Riss, Pedro Alves, gdb-patches

Jan Kratochvil wrote:

> gdb/testsuite/
> 2010-06-10  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.base/moribund-step.exp: New.

> +gdb_test_no_output "set non-stop on"
> +
> +if ![runto_main] {
> +    return
> +}

This fails on platforms that do not support non-stop mode:

(gdb) set non-stop on^M
(gdb) PASS: gdb.base/moribund-step.exp: set non-stop on
delete breakpoints^M
(gdb) info breakpoints^M
No breakpoints or watchpoints.^M
(gdb) break main^M
Breakpoint 1 at 0x18c: file /home/uweigand/fsf/gdb-head/gdb/testsuite/gdb.base/start.c, line 34.^M
(gdb) run ^M
The target does not support running in non-stop mode.^M
(gdb) FAIL: gdb.base/moribund-step.exp: running to main in runto


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch 1/3] Clear stale specific locs, not whole bpts [rediff]
  2010-06-11 16:24               ` Ulrich Weigand
@ 2010-06-11 18:34                 ` Jan Kratochvil
  2010-06-11 21:51                   ` Ulrich Weigand
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Kratochvil @ 2010-06-11 18:34 UTC (permalink / raw)
  To: Ulrich Weigand; +Cc: Frederic Riss, Pedro Alves, gdb-patches

On Fri, 11 Jun 2010 18:24:40 +0200, Ulrich Weigand wrote:
> This fails on platforms that do not support non-stop mode:
> 
> (gdb) set non-stop on^M
> (gdb) PASS: gdb.base/moribund-step.exp: set non-stop on
> delete breakpoints^M
> (gdb) info breakpoints^M
> No breakpoints or watchpoints.^M
> (gdb) break main^M
> Breakpoint 1 at 0x18c: file /home/uweigand/fsf/gdb-head/gdb/testsuite/gdb.base/start.c, line 34.^M
> (gdb) run ^M
> The target does not support running in non-stop mode.^M
> (gdb) FAIL: gdb.base/moribund-step.exp: running to main in runto

I see now lib/mi-support.exp catches:
        -re "\\^error,msg=\"The target does not support running in non-stop mode.\"" {
            unsupported "Non-stop mode not supported"
            return -1
        }
while CLI lib/gdb.exp so far has not.

OK to check-in?

Tested on x86 opensolaris.


Thanks,
Jan


2010-06-11  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* lib/gdb.exp (gdb_run_cmd): Return on $gdb_prompt.
	(runto): Catch "The target does not support running in non-stop mode.".

--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -290,6 +290,9 @@ proc gdb_run_cmd {args} {
 	    exp_continue
 	}
 	-notransfer -re "Starting program: \[^\r\n\]*" {}
+	-notransfer -re "$gdb_prompt $" {
+	    # There is no more input expected.
+	}
     }
 }
 
@@ -416,6 +419,10 @@ proc runto { function args } {
 	-re "Breakpoint \[0-9\]*, \[0-9xa-f\]* in .*$gdb_prompt $" { 
 	    return 1
 	}
+	-re "The target does not support running in non-stop mode.\r\n$gdb_prompt $" {
+	    unsupported "Non-stop mode not supported"
+	    return 0
+	}
 	-re "$gdb_prompt $" { 
 	    fail "running to $function in runto"
 	    return 0

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch 1/3] Clear stale specific locs, not whole bpts [rediff]
  2010-06-11 18:34                 ` Jan Kratochvil
@ 2010-06-11 21:51                   ` Ulrich Weigand
  2010-06-11 21:59                     ` Jan Kratochvil
  0 siblings, 1 reply; 15+ messages in thread
From: Ulrich Weigand @ 2010-06-11 21:51 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: Frederic Riss, Pedro Alves, gdb-patches

Jan Kratochvil wrote:

> I see now lib/mi-support.exp catches:
>         -re "\\^error,msg=\"The target does not support running in non-stop mode.\"" {
>             unsupported "Non-stop mode not supported"
>             return -1
>         }
> while CLI lib/gdb.exp so far has not.

Good point.  You patch fixes the problem for me, thanks!

> 2010-06-11  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* lib/gdb.exp (gdb_run_cmd): Return on $gdb_prompt.
> 	(runto): Catch "The target does not support running in non-stop mode.".

OK.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch 1/3] Clear stale specific locs, not whole bpts [rediff]
  2010-06-11 21:51                   ` Ulrich Weigand
@ 2010-06-11 21:59                     ` Jan Kratochvil
  0 siblings, 0 replies; 15+ messages in thread
From: Jan Kratochvil @ 2010-06-11 21:59 UTC (permalink / raw)
  To: Ulrich Weigand; +Cc: Frederic Riss, Pedro Alves, gdb-patches

On Fri, 11 Jun 2010 23:51:44 +0200, Ulrich Weigand wrote:
> Jan Kratochvil wrote:
> > 2010-06-11  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	* lib/gdb.exp (gdb_run_cmd): Return on $gdb_prompt.
> > 	(runto): Catch "The target does not support running in non-stop mode.".
> 
> OK.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2010-06/msg00093.html


Thanks,
Jan

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2010-06-11 21:59 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-05-03 20:02 [patch 1/3] Clear stale specific locs, not whole bpts Jan Kratochvil
2010-05-17 21:25 ` [patch 1/3] Clear stale specific locs, not whole bpts [rediff] Jan Kratochvil
2010-05-24 12:18   ` Pedro Alves
2010-06-04 19:19     ` Jan Kratochvil
2010-06-07 11:50       ` Pedro Alves
2010-06-07 13:40         ` Jan Kratochvil
2010-06-10 14:09           ` Frederic Riss
2010-06-10 20:39             ` Jan Kratochvil
2010-06-11  7:58               ` Frederic Riss
2010-06-11 13:00                 ` Jan Kratochvil
2010-06-11 10:50               ` Pedro Alves
2010-06-11 16:24               ` Ulrich Weigand
2010-06-11 18:34                 ` Jan Kratochvil
2010-06-11 21:51                   ` Ulrich Weigand
2010-06-11 21:59                     ` Jan Kratochvil

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).