functions that delete breakpoints should not insert bp locations (was: Fix execl.exp sporadic failures)

functions that delete breakpoints should not insert bp locations (was: Fix execl.exp sporadic failures)

[Pedro Alves]

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

As I've said in my previous post, thread_db has a call to
remove_thread_event_breakpoints after the inferior having execd,
here:

static ptid_t
thread_db_wait (ptid_t ptid, struct target_waitstatus *ourstatus)
{
  ptid = target_beneath->to_wait (ptid, ourstatus);

  (...)

  if (ourstatus->kind == TARGET_WAITKIND_EXECD)
    {
      remove_thread_event_breakpoints (); <<<<<
      unpush_target (&thread_db_ops);
      using_thread_db = 0;

      return ptid;
    }


breakpoint.c:remove_thread_event_breakpoints is this:

void
remove_thread_event_breakpoints (void)
{
  struct breakpoint *b, *temp;

  ALL_BREAKPOINTS_SAFE (b, temp)
    if (b->type == bp_thread_event)
      delete_breakpoint (b);
}

With the previous patch I just sent in place, which marks
the breakpoints not inserted before reaching this point,
delete_breakpoint (and its callees), will no longer try to
remove the breakpoint locations from the inferior.

The problem with always inserted mode, is that
update_global_location_list, the workhorse that detects duplicate locations,
and if locations should be deleted or inserted, finds that breakpoint
locations tagged as not inserted but which are enabled, and so goes on and
inserts them.  But the addresses in which they are being inserted don't
make any sense in the new exec image, and we again see errors like:

/home/pedro/gdb/execl_hunt/build32/gdb/testsuite/gdb.threads/execl1: error 
while loading shared libraries: libm.so.6:
failed to map segment from shared object: Cannot allocate memory


IMO, having the breakpoints management functions whose job is
to delete a breakpoint insert locations, is a side effect that
is always undesirable.

Here's another example I found while working on non-stop,
which requires always-inserted mode, that is broken due to that
same fact:

void
target_detach (char *args, int from_tty)
{
  /* If we're in breakpoints-always-inserted mode, have to
     remove them before detaching.  */
  remove_breakpoints ();

  (current_target.to_detach) (args, from_tty);
}


static void
thread_db_detach (char *args, int from_tty)
{
  disable_thread_event_reporting ();

  target_beneath->to_detach (args, from_tty);

  /* Should this be done by detach_command?  */
  target_mourn_inferior ();
}

static void
disable_thread_event_reporting (void)
{
  td_thr_events_t events;

  /* Set the process wide mask saying we aren't interested in any
     events anymore.  */
  td_event_emptyset (&events);
  td_ta_set_event_p (thread_agent, &events);

  /* Delete thread event breakpoints, if any.  */
  remove_thread_event_breakpoints (); <<<<<<
  td_create_bp_addr = 0;
  td_death_bp_addr = 0;
}

Notice that first, all breakpoints are removed from the inferior
in target_detach, but if the inferior has thread_db loaded,
the remove_thread_event_breakpoints call, which calls
delete_breakpoint, will end up reinserting the locations into
the inferior, with the effect that the inferior crashes with 
hitting a left over breakpoint right after detaching.


IMO, the best solution to these problems is making sure that
update_global_location_list does not insert breakpoints
locations if being called from a function that is deleting (a)
breakpoint(s).

This also gets rid of the hack of temporarilly disabling
always-inserted mode in update_breakpoints_after_exec, which
was there due to this exact fact.

Tested on x86_64-unknown-linux-gnu {,-m32}, with and without
always-inserted mode.

No regressions, and execl.exp now passes cleanly in all
combinations, always.

OK?

-- 
Pedro Alves

[-- Attachment #2: breakpoint_delete_no_insert.diff --]
[-- Type: text/x-diff, Size: 10309 bytes --]

2008-07-03  Pedro Alves  <pedro@codesourcery.com>

	* breakpoint.c (update_global_location_list): Add boolean
	"inserting" argument.  Only insert locations if caller told it
	too.
	(update_global_location_list_nothrow): Add boolean "inserting"
	argument.  Pass it to update_global_location_list.
	(insert_breakpoints, create_longjmp_breakpoint)
	(create_overlay_event_breakpoint, enable_overlay_breakpoints)
	(create_thread_event_breakpoint, create_solib_event_breakpoint)
	(create_fork_vfork_event_catchpoint, create_exec_event_catchpoint)
	(enable_watchpoints_after_interactive_call_stop)
	(set_momentary_breakpoint, create_breakpoints)
	(break_command_really, watch_command_1)
	(create_ada_exception_breakpoint, update_breakpoint_locations)
	(do_enable_breakpoint, enable_command): Pass true to
	update_global_location_list.
	(bpstat_stop_status, disable_overlay_breakpoints)
	(disable_watchpoints_before_interactive_call_start)
	(delete_breakpoint, disable_breakpoint, disable_command): Pass
	false to update_global_location_list.
	(update_breakpoints_after_exec): Don't temporarily disable
	always-inserted mode.

---
 gdb/breakpoint.c |   71 +++++++++++++++++++++++++------------------------------
 1 file changed, 33 insertions(+), 38 deletions(-)

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2008-07-03 02:06:48.000000000 +0100
+++ src/gdb/breakpoint.c	2008-07-03 02:24:25.000000000 +0100
@@ -191,9 +191,9 @@ static void free_bp_location (struct bp_
 static struct bp_location *
 allocate_bp_location (struct breakpoint *bpt, enum bptype bp_type);
 
-static void update_global_location_list (void);
+static void update_global_location_list (int inserting);
 
-static void update_global_location_list_nothrow (void);
+static void update_global_location_list_nothrow (int inserting);
 
 static int is_hardware_watchpoint (struct breakpoint *bpt);
 
@@ -1264,7 +1264,7 @@ insert_breakpoints (void)
     if (is_hardware_watchpoint (bpt))
       update_watchpoint (bpt, 0 /* don't reparse. */);
 
-  update_global_location_list ();
+  update_global_location_list (1);
 
   if (!always_inserted_mode && target_has_execution)
     /* update_global_location_list does not insert breakpoints
@@ -1441,7 +1441,6 @@ update_breakpoints_after_exec (void)
 {
   struct breakpoint *b;
   struct breakpoint *temp;
-  struct cleanup *cleanup;
 
   /* There used to be a call to mark_breakpoints_out here with the
      following comment:
@@ -1456,13 +1455,6 @@ update_breakpoints_after_exec (void)
      reaching here.  The call has since moved closer to where the each
      target detects an exec.  */
 
-
-  /* The binary we used to debug is now gone, and we're updating
-     breakpoints for the new binary.  Until we're done, we should not
-     try to insert breakpoints.  */
-  cleanup = make_cleanup_restore_integer (&always_inserted_mode);
-  always_inserted_mode = 0;
-
   ALL_BREAKPOINTS_SAFE (b, temp)
   {
     /* Solib breakpoints must be explicitly reset after an exec(). */
@@ -1552,7 +1544,6 @@ update_breakpoints_after_exec (void)
   }
   /* FIXME what about longjmp breakpoints?  Re-create them here?  */
   create_overlay_event_breakpoint ("_ovly_debug_event");
-  do_cleanups (cleanup);
 }
 
 int
@@ -3069,7 +3060,7 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
 	if (b->disposition == disp_disable)
 	  {
 	    b->enable_state = bp_disabled;
-	    update_global_location_list ();
+	    update_global_location_list (0);
 	  }
 	if (b->silent)
 	  bs->print = 0;
@@ -4481,7 +4472,7 @@ create_longjmp_breakpoint (char *func_na
   if ((m = lookup_minimal_symbol_text (func_name, NULL)) == NULL)
     return;
   set_momentary_breakpoint_at_pc (SYMBOL_VALUE_ADDRESS (m), bp_longjmp);
-  update_global_location_list ();
+  update_global_location_list (1);
 }
 
 /* Call this routine when stepping and nexting to enable a breakpoint
@@ -4539,7 +4530,7 @@ create_overlay_event_breakpoint (char *f
       b->enable_state = bp_disabled;
       overlay_events_enabled = 0;
     }
-  update_global_location_list ();
+  update_global_location_list (1);
 }
 
 void
@@ -4551,7 +4542,7 @@ enable_overlay_breakpoints (void)
     if (b->type == bp_overlay_event)
     {
       b->enable_state = bp_enabled;
-      update_global_location_list ();
+      update_global_location_list (1);
       overlay_events_enabled = 1;
     }
 }
@@ -4565,7 +4556,7 @@ disable_overlay_breakpoints (void)
     if (b->type == bp_overlay_event)
     {
       b->enable_state = bp_disabled;
-      update_global_location_list ();
+      update_global_location_list (0);
       overlay_events_enabled = 0;
     }
 }
@@ -4581,7 +4572,7 @@ create_thread_event_breakpoint (CORE_ADD
   /* addr_string has to be used or breakpoint_re_set will delete me.  */
   b->addr_string = xstrprintf ("*0x%s", paddr (b->loc->address));
 
-  update_global_location_list_nothrow ();
+  update_global_location_list_nothrow (1);
 
   return b;
 }
@@ -4627,7 +4618,7 @@ create_solib_event_breakpoint (CORE_ADDR
   struct breakpoint *b;
 
   b = create_internal_breakpoint (address, bp_shlib_event);
-  update_global_location_list_nothrow ();
+  update_global_location_list_nothrow (1);
   return b;
 }
 
@@ -4725,7 +4716,7 @@ create_fork_vfork_event_catchpoint (int 
   b->enable_state = bp_enabled;
   b->disposition = tempflag ? disp_del : disp_donttouch;
   b->forked_inferior_pid = 0;
-  update_global_location_list ();
+  update_global_location_list (1);
 
 
   mention (b);
@@ -4764,7 +4755,7 @@ create_exec_event_catchpoint (int tempfl
   b->addr_string = NULL;
   b->enable_state = bp_enabled;
   b->disposition = tempflag ? disp_del : disp_donttouch;
-  update_global_location_list ();
+  update_global_location_list (1);
 
   mention (b);
 }
@@ -4820,7 +4811,7 @@ disable_watchpoints_before_interactive_c
 	&& breakpoint_enabled (b))
       {
 	b->enable_state = bp_call_disabled;
-	update_global_location_list ();
+	update_global_location_list (0);
       }
   }
 }
@@ -4839,7 +4830,7 @@ enable_watchpoints_after_interactive_cal
 	&& (b->enable_state == bp_call_disabled))
       {
 	b->enable_state = bp_enabled;
-	update_global_location_list ();
+	update_global_location_list (1);
       }
   }
 }
@@ -4865,7 +4856,7 @@ set_momentary_breakpoint (struct symtab_
   if (in_thread_list (inferior_ptid))
     b->thread = pid_to_thread_id (inferior_ptid);
 
-  update_global_location_list_nothrow ();
+  update_global_location_list_nothrow (1);
 
   return b;
 }
@@ -5287,7 +5278,7 @@ create_breakpoints (struct symtabs_and_l
 			 thread, ignore_count, ops, from_tty);
     }
 
-  update_global_location_list ();
+  update_global_location_list (1);
 }
 
 /* Parse ARG which is assumed to be a SAL specification possibly
@@ -5608,7 +5599,7 @@ break_command_really (char *arg, char *c
       b->condition_not_parsed = 1;
       b->ops = ops;
 
-      update_global_location_list ();
+      update_global_location_list (1);
       mention (b);
     }
   
@@ -6037,7 +6028,7 @@ watch_command_1 (char *arg, int accessfl
 
   value_free_to_mark (mark);
   mention (b);
-  update_global_location_list ();
+  update_global_location_list (1);
 }
 
 /* Return count of locations need to be watched and can be handled
@@ -6675,7 +6666,7 @@ create_ada_exception_breakpoint (struct 
   b->ops = ops;
 
   mention (b);
-  update_global_location_list ();
+  update_global_location_list (1);
 }
 
 /* Implement the "catch exception" command.  */
@@ -7000,7 +6991,7 @@ breakpoint_auto_delete (bpstat bs)
 }
 
 static void
-update_global_location_list (void)
+update_global_location_list (int inserting)
 {
   struct breakpoint *b;
   struct bp_location **next = &bp_location_chain;
@@ -7132,7 +7123,11 @@ update_global_location_list (void)
       check_duplicates (b);
     }
 
-  if (always_inserted_mode && target_has_execution)
+  /* If we get here due to deleting a breakpoint, don't try to insert
+     locations.  This helps cases like: target_detach deleting a
+     breakpoint before detaching causing all other breakpoints to be
+     inserted.  */
+  if (always_inserted_mode && inserting && target_has_execution)
     insert_breakpoint_locations ();
 }
 
@@ -7152,11 +7147,11 @@ breakpoint_retire_moribund (void)
 }
 
 static void
-update_global_location_list_nothrow (void)
+update_global_location_list_nothrow (int inserting)
 {
   struct gdb_exception e;
   TRY_CATCH (e, RETURN_MASK_ERROR)
-    update_global_location_list ();
+    update_global_location_list (inserting);
 }
 
 /* Delete a breakpoint and clean up all traces of it in the data
@@ -7246,7 +7241,7 @@ delete_breakpoint (struct breakpoint *bp
      looks at location's owner.  It might be better
      design to have location completely self-contained,
      but it's not the case now.  */
-  update_global_location_list ();
+  update_global_location_list (0);
 
 
   /* On the chance that someone will soon try again to delete this same
@@ -7456,7 +7451,7 @@ update_breakpoint_locations (struct brea
       }
   }
 
-  update_global_location_list ();
+  update_global_location_list (1);
 }
 
 
@@ -7842,7 +7837,7 @@ disable_breakpoint (struct breakpoint *b
 
   bpt->enable_state = bp_disabled;
 
-  update_global_location_list ();
+  update_global_location_list (0);
 
   if (deprecated_modify_breakpoint_hook)
     deprecated_modify_breakpoint_hook (bpt);
@@ -7881,7 +7876,7 @@ disable_command (char *args, int from_tt
       struct bp_location *loc = find_location_by_number (args);
       if (loc)
 	loc->enabled = 0;
-      update_global_location_list ();
+      update_global_location_list (0);
     }
   else
     map_breakpoint_numbers (args, disable_breakpoint);
@@ -7966,7 +7961,7 @@ have been allocated for other watchpoint
   if (bpt->enable_state != bp_permanent)
     bpt->enable_state = bp_enabled;
   bpt->disposition = disposition;
-  update_global_location_list ();
+  update_global_location_list (1);
   breakpoints_changed ();
   
   if (deprecated_modify_breakpoint_hook)
@@ -8017,7 +8012,7 @@ enable_command (char *args, int from_tty
       struct bp_location *loc = find_location_by_number (args);
       if (loc)
 	loc->enabled = 1;
-      update_global_location_list ();
+      update_global_location_list (1);
     }
   else
     map_breakpoint_numbers (args, enable_breakpoint);

Re: functions that delete breakpoints should not insert bp locations (was: Fix execl.exp sporadic failures)

[Vladimir Prus]

Pedro Alves wrote:

> static void
> -update_global_location_list (void)
> +update_global_location_list (int inserting)

There's no comment for the new parameter. Also, the parameter tells
the function to do, or not do something, so "-ing" sounds awkward.
How about 'should_insert' or 'suppress_insert'?

> {
> struct breakpoint *b;
> struct bp_location **next = &bp_location_chain;
> @@ -7132,7 +7123,11 @@ update_global_location_list (void)
> check_duplicates (b);
> }
> 
> -  if (always_inserted_mode && target_has_execution)
> +  /* If we get here due to deleting a breakpoint, don't try to insert
> +     locations.  

This does not tell how 'inserting' is related to 'deleting a breakpoint'.

> This helps cases like: target_detach deleting a 
> +     breakpoint before detaching causing all other breakpoints to be
> +     inserted.  */


I'd suggest to remove this comment, and add a comment before function saying this:

/* If SUPPRESS_INSERT is true, do not insert any breakpoint locations into target,
   only remove already-inserted locations that no longer should be inserted.
   This behaviour is useful during tear-down -- when the target no longer is
   capable of inserting breakpoints, and we're removing internal GDB breakpoints,
   we don't want to try inserting other breakpoints into already-gone target.  */

As we've already discussed, I think the ideal solution is to mark the target is
'going down' and suppress insertion of breakpoints if the target is in that state.
Your patch, however, should be almost as good, so I don't have any objections.
Thanks for addressing this issue!

- Volodya

Re: functions that delete breakpoints should not insert bp locations (was: Fix execl.exp sporadic failures)

[Pedro Alves]

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

A Thursday 03 July 2008 09:48:13, Vladimir Prus wrote:
> Pedro Alves wrote:
> > static void
> > -update_global_location_list (void)
> > +update_global_location_list (int inserting)
>
> There's no comment for the new parameter. Also, the parameter tells
> the function to do, or not do something, so "-ing" sounds awkward.
> How about 'should_insert' or 'suppress_insert'?

You're quite right.  I changed it to should_inserted, to avoid
going through all callers and reverse the logic.  And added a
comment similar to what you suggested.

Thanks!

-- 
Pedro Alves

[-- Attachment #2: breakpoint_delete_no_insert.diff --]
[-- Type: text/x-diff, Size: 10944 bytes --]

2008-07-03  Pedro Alves  <pedro@codesourcery.com>

	* breakpoint.c (update_global_location_list): Add boolean
	"should_insert" argument.  Only insert locations if caller told it
	too.
	(update_global_location_list_nothrow): Add boolean "should_insert"
	argument.  Pass it to update_global_location_list.
	(insert_breakpoints, create_longjmp_breakpoint)
	(create_overlay_event_breakpoint, enable_overlay_breakpoints)
	(create_thread_event_breakpoint, create_solib_event_breakpoint)
	(create_fork_vfork_event_catchpoint, create_exec_event_catchpoint)
	(enable_watchpoints_after_interactive_call_stop)
	(set_momentary_breakpoint, create_breakpoints)
	(break_command_really, watch_command_1)
	(create_ada_exception_breakpoint, update_breakpoint_locations)
	(do_enable_breakpoint, enable_command): Pass true to
	update_global_location_list.
	(bpstat_stop_status, disable_overlay_breakpoints)
	(disable_watchpoints_before_interactive_call_start)
	(delete_breakpoint, disable_breakpoint, disable_command): Pass
	false to update_global_location_list.
	(update_breakpoints_after_exec): Don't temporarily disable
	always-inserted mode.

---
 gdb/breakpoint.c |   82 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 44 insertions(+), 38 deletions(-)

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2008-07-03 02:06:48.000000000 +0100
+++ src/gdb/breakpoint.c	2008-07-03 11:43:44.000000000 +0100
@@ -191,9 +191,9 @@ static void free_bp_location (struct bp_
 static struct bp_location *
 allocate_bp_location (struct breakpoint *bpt, enum bptype bp_type);
 
-static void update_global_location_list (void);
+static void update_global_location_list (int);
 
-static void update_global_location_list_nothrow (void);
+static void update_global_location_list_nothrow (int);
 
 static int is_hardware_watchpoint (struct breakpoint *bpt);
 
@@ -1264,7 +1264,7 @@ insert_breakpoints (void)
     if (is_hardware_watchpoint (bpt))
       update_watchpoint (bpt, 0 /* don't reparse. */);
 
-  update_global_location_list ();
+  update_global_location_list (1);
 
   if (!always_inserted_mode && target_has_execution)
     /* update_global_location_list does not insert breakpoints
@@ -1441,7 +1441,6 @@ update_breakpoints_after_exec (void)
 {
   struct breakpoint *b;
   struct breakpoint *temp;
-  struct cleanup *cleanup;
 
   /* There used to be a call to mark_breakpoints_out here with the
      following comment:
@@ -1456,13 +1455,6 @@ update_breakpoints_after_exec (void)
      reaching here.  The call has since moved closer to where the each
      target detects an exec.  */
 
-
-  /* The binary we used to debug is now gone, and we're updating
-     breakpoints for the new binary.  Until we're done, we should not
-     try to insert breakpoints.  */
-  cleanup = make_cleanup_restore_integer (&always_inserted_mode);
-  always_inserted_mode = 0;
-
   ALL_BREAKPOINTS_SAFE (b, temp)
   {
     /* Solib breakpoints must be explicitly reset after an exec(). */
@@ -1552,7 +1544,6 @@ update_breakpoints_after_exec (void)
   }
   /* FIXME what about longjmp breakpoints?  Re-create them here?  */
   create_overlay_event_breakpoint ("_ovly_debug_event");
-  do_cleanups (cleanup);
 }
 
 int
@@ -3069,7 +3060,7 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
 	if (b->disposition == disp_disable)
 	  {
 	    b->enable_state = bp_disabled;
-	    update_global_location_list ();
+	    update_global_location_list (0);
 	  }
 	if (b->silent)
 	  bs->print = 0;
@@ -4481,7 +4472,7 @@ create_longjmp_breakpoint (char *func_na
   if ((m = lookup_minimal_symbol_text (func_name, NULL)) == NULL)
     return;
   set_momentary_breakpoint_at_pc (SYMBOL_VALUE_ADDRESS (m), bp_longjmp);
-  update_global_location_list ();
+  update_global_location_list (1);
 }
 
 /* Call this routine when stepping and nexting to enable a breakpoint
@@ -4539,7 +4530,7 @@ create_overlay_event_breakpoint (char *f
       b->enable_state = bp_disabled;
       overlay_events_enabled = 0;
     }
-  update_global_location_list ();
+  update_global_location_list (1);
 }
 
 void
@@ -4551,7 +4542,7 @@ enable_overlay_breakpoints (void)
     if (b->type == bp_overlay_event)
     {
       b->enable_state = bp_enabled;
-      update_global_location_list ();
+      update_global_location_list (1);
       overlay_events_enabled = 1;
     }
 }
@@ -4565,7 +4556,7 @@ disable_overlay_breakpoints (void)
     if (b->type == bp_overlay_event)
     {
       b->enable_state = bp_disabled;
-      update_global_location_list ();
+      update_global_location_list (0);
       overlay_events_enabled = 0;
     }
 }
@@ -4581,7 +4572,7 @@ create_thread_event_breakpoint (CORE_ADD
   /* addr_string has to be used or breakpoint_re_set will delete me.  */
   b->addr_string = xstrprintf ("*0x%s", paddr (b->loc->address));
 
-  update_global_location_list_nothrow ();
+  update_global_location_list_nothrow (1);
 
   return b;
 }
@@ -4627,7 +4618,7 @@ create_solib_event_breakpoint (CORE_ADDR
   struct breakpoint *b;
 
   b = create_internal_breakpoint (address, bp_shlib_event);
-  update_global_location_list_nothrow ();
+  update_global_location_list_nothrow (1);
   return b;
 }
 
@@ -4725,7 +4716,7 @@ create_fork_vfork_event_catchpoint (int 
   b->enable_state = bp_enabled;
   b->disposition = tempflag ? disp_del : disp_donttouch;
   b->forked_inferior_pid = 0;
-  update_global_location_list ();
+  update_global_location_list (1);
 
 
   mention (b);
@@ -4764,7 +4755,7 @@ create_exec_event_catchpoint (int tempfl
   b->addr_string = NULL;
   b->enable_state = bp_enabled;
   b->disposition = tempflag ? disp_del : disp_donttouch;
-  update_global_location_list ();
+  update_global_location_list (1);
 
   mention (b);
 }
@@ -4820,7 +4811,7 @@ disable_watchpoints_before_interactive_c
 	&& breakpoint_enabled (b))
       {
 	b->enable_state = bp_call_disabled;
-	update_global_location_list ();
+	update_global_location_list (0);
       }
   }
 }
@@ -4839,7 +4830,7 @@ enable_watchpoints_after_interactive_cal
 	&& (b->enable_state == bp_call_disabled))
       {
 	b->enable_state = bp_enabled;
-	update_global_location_list ();
+	update_global_location_list (1);
       }
   }
 }
@@ -4865,7 +4856,7 @@ set_momentary_breakpoint (struct symtab_
   if (in_thread_list (inferior_ptid))
     b->thread = pid_to_thread_id (inferior_ptid);
 
-  update_global_location_list_nothrow ();
+  update_global_location_list_nothrow (1);
 
   return b;
 }
@@ -5287,7 +5278,7 @@ create_breakpoints (struct symtabs_and_l
 			 thread, ignore_count, ops, from_tty);
     }
 
-  update_global_location_list ();
+  update_global_location_list (1);
 }
 
 /* Parse ARG which is assumed to be a SAL specification possibly
@@ -5608,7 +5599,7 @@ break_command_really (char *arg, char *c
       b->condition_not_parsed = 1;
       b->ops = ops;
 
-      update_global_location_list ();
+      update_global_location_list (1);
       mention (b);
     }
   
@@ -6037,7 +6028,7 @@ watch_command_1 (char *arg, int accessfl
 
   value_free_to_mark (mark);
   mention (b);
-  update_global_location_list ();
+  update_global_location_list (1);
 }
 
 /* Return count of locations need to be watched and can be handled
@@ -6675,7 +6666,7 @@ create_ada_exception_breakpoint (struct 
   b->ops = ops;
 
   mention (b);
-  update_global_location_list ();
+  update_global_location_list (1);
 }
 
 /* Implement the "catch exception" command.  */
@@ -6999,8 +6990,23 @@ breakpoint_auto_delete (bpstat bs)
   }
 }
 
+/* If SHOULD_INSERT is true, do not insert any breakpoint locations
+   into the inferior, only remove already-inserted locations that no
+   longer should be inserted.  Functions that delete a breakpoint or
+   breakpoints should pass false, so that deleting a breakpoint
+   doesn't have the side effect of inserting the locations of other
+   breakpoints that are marked not-inserted, but should_be_inserted
+   returns true on them.
+
+   This behaviour is useful is situations close to tear-down -- e.g.,
+   after an exec, while the target still has execution, but breakpoint
+   shadows of the previous executable image should *NOT* be restored
+   to the new image; or before detaching, where the target still has
+   execution and wants to delete breakpoints from GDB's lists, and all
+   breakpoints had already been removed from the inferior.  */
+
 static void
-update_global_location_list (void)
+update_global_location_list (int should_insert)
 {
   struct breakpoint *b;
   struct bp_location **next = &bp_location_chain;
@@ -7132,7 +7138,7 @@ update_global_location_list (void)
       check_duplicates (b);
     }
 
-  if (always_inserted_mode && target_has_execution)
+  if (always_inserted_mode && should_insert && target_has_execution)
     insert_breakpoint_locations ();
 }
 
@@ -7152,11 +7158,11 @@ breakpoint_retire_moribund (void)
 }
 
 static void
-update_global_location_list_nothrow (void)
+update_global_location_list_nothrow (int inserting)
 {
   struct gdb_exception e;
   TRY_CATCH (e, RETURN_MASK_ERROR)
-    update_global_location_list ();
+    update_global_location_list (inserting);
 }
 
 /* Delete a breakpoint and clean up all traces of it in the data
@@ -7246,7 +7252,7 @@ delete_breakpoint (struct breakpoint *bp
      looks at location's owner.  It might be better
      design to have location completely self-contained,
      but it's not the case now.  */
-  update_global_location_list ();
+  update_global_location_list (0);
 
 
   /* On the chance that someone will soon try again to delete this same
@@ -7456,7 +7462,7 @@ update_breakpoint_locations (struct brea
       }
   }
 
-  update_global_location_list ();
+  update_global_location_list (1);
 }
 
 
@@ -7842,7 +7848,7 @@ disable_breakpoint (struct breakpoint *b
 
   bpt->enable_state = bp_disabled;
 
-  update_global_location_list ();
+  update_global_location_list (0);
 
   if (deprecated_modify_breakpoint_hook)
     deprecated_modify_breakpoint_hook (bpt);
@@ -7881,7 +7887,7 @@ disable_command (char *args, int from_tt
       struct bp_location *loc = find_location_by_number (args);
       if (loc)
 	loc->enabled = 0;
-      update_global_location_list ();
+      update_global_location_list (0);
     }
   else
     map_breakpoint_numbers (args, disable_breakpoint);
@@ -7966,7 +7972,7 @@ have been allocated for other watchpoint
   if (bpt->enable_state != bp_permanent)
     bpt->enable_state = bp_enabled;
   bpt->disposition = disposition;
-  update_global_location_list ();
+  update_global_location_list (1);
   breakpoints_changed ();
   
   if (deprecated_modify_breakpoint_hook)
@@ -8017,7 +8023,7 @@ enable_command (char *args, int from_tty
       struct bp_location *loc = find_location_by_number (args);
       if (loc)
 	loc->enabled = 1;
-      update_global_location_list ();
+      update_global_location_list (1);
     }
   else
     map_breakpoint_numbers (args, enable_breakpoint);

Re: functions that delete breakpoints should not insert bp locations (was: Fix execl.exp sporadic failures)

[Daniel Jacobowitz]

On Thu, Jul 03, 2008 at 11:45:53AM +0100, Pedro Alves wrote:
> 2008-07-03  Pedro Alves  <pedro@codesourcery.com>
> 
> 	* breakpoint.c (update_global_location_list): Add boolean
> 	"should_insert" argument.  Only insert locations if caller told it
> 	too.
> 	(update_global_location_list_nothrow): Add boolean "should_insert"
> 	argument.  Pass it to update_global_location_list.
> 	(insert_breakpoints, create_longjmp_breakpoint)
> 	(create_overlay_event_breakpoint, enable_overlay_breakpoints)
> 	(create_thread_event_breakpoint, create_solib_event_breakpoint)
> 	(create_fork_vfork_event_catchpoint, create_exec_event_catchpoint)
> 	(enable_watchpoints_after_interactive_call_stop)
> 	(set_momentary_breakpoint, create_breakpoints)
> 	(break_command_really, watch_command_1)
> 	(create_ada_exception_breakpoint, update_breakpoint_locations)
> 	(do_enable_breakpoint, enable_command): Pass true to
> 	update_global_location_list.
> 	(bpstat_stop_status, disable_overlay_breakpoints)
> 	(disable_watchpoints_before_interactive_call_start)
> 	(delete_breakpoint, disable_breakpoint, disable_command): Pass
> 	false to update_global_location_list.
> 	(update_breakpoints_after_exec): Don't temporarily disable
> 	always-inserted mode.

Thanks, this is OK.

-- 
Daniel Jacobowitz
CodeSourcery