This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]