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

[Pedro Alves <pedro at codesourcery dot com>]

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

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);