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]

[RFA 2/3] Demote to sw watchpoint only in update_watchpoint


Hi,

watch_command_1 duplicates logic from update_watchpoint to decide
whether the newly created watchpoint should be a software watchpoint or
a hardware watchpoint, and also to error out if trying to set up a read
or access watchpoint without having debug registers available.

I was trying to adapt the logic in both places for masked watchpoints,
but it's hard to anticipate in watch_command_1 what update_watchpoint
will ultimately do without duplicating a good portion of the latter. The
code in watch_command_1 was getting complicated and hard to get right in
all situations for something which would be done again later anyway.

I decided that it was cleaner if watch_command_1 just delegated
everything to update_watchpoint. This patch makes it create a
watchpoint, call update_watchpoint and then delete it if some error is
hit. The only drawback I can think of is that the aborted watchpoint
will "consume" one breakpoint number, and thus GDB will skip one number
when creating the next breakpoint/watchpoint. This doesn't seem
important to me.

Tested without regressions on ppc-linux, ppc64-linux and i386-linux. Ok?
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2011-04-18  Thiago Jung Bauermann  <bauerman@br.ibm.com>

	* breakpoint.c (update_watchpoint): Change between software and
	hardware watchpoint for all kinds of watchpoints, not just
	read/write ones.  Determine b->exact value here instead of
	in watch_command_1.  Error out if there are not enough resources
	for a read or access hardware watchpoint.
	(watch_command_1): Remove logic of checking whether there are
	enough resources available, since update_watchpoint will do that
	work now.  Don't set b->exact here.  Catch exceptions thrown by
	update_watchpoint and delete the watchpoint.
	(can_use_hardware_watchpoint): Remove exact_watchpoints argument.
	Use target_exact_watchpoints instead.
	(delete_breakpoint): Add announce argument to control whether
	observers are notified of the deletion.  Update all callers.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 744057a..2bfdfb0 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -101,7 +101,7 @@ static void clear_command (char *, int);
 
 static void catch_command (char *, int);
 
-static int can_use_hardware_watchpoint (struct value *, int);
+static int can_use_hardware_watchpoint (struct value *);
 
 static void break_command_1 (char *, int, int);
 
@@ -1404,19 +1404,22 @@ update_watchpoint (struct breakpoint *b, int reparse)
 	 an ordinary watchpoint depending on the hardware support
 	 and free hardware slots.  REPARSE is set when the inferior
 	 is started.  */
-      if ((b->type == bp_watchpoint || b->type == bp_hardware_watchpoint)
-	  && reparse)
+      if (reparse)
 	{
 	  int reg_cnt;
 	  enum bp_loc_type loc_type;
 	  struct bp_location *bl;
 
-	  reg_cnt = can_use_hardware_watchpoint (val_chain, b->exact);
+	  reg_cnt = can_use_hardware_watchpoint (val_chain);
 
 	  if (reg_cnt)
 	    {
 	      int i, target_resources_ok, other_type_used;
 
+	      /* Use an exact watchpoint when there's only one memory region to be
+		 watched, and only one debug register is needed to watch it.  */
+	      b->exact = target_exact_watchpoints && reg_cnt == 1;
+
 	      /* We need to determine how many resources are already
 		 used for all other hardware watchpoints plus this one
 		 to see if we still have enough resources to also fit
@@ -1424,7 +1427,8 @@ update_watchpoint (struct breakpoint *b, int reparse)
 		 hw_watchpoint_used_count call below counts this
 		 watchpoint, make sure that it is marked as a hardware
 		 watchpoint.  */
-	      b->type = bp_hardware_watchpoint;
+	      if (b->type == bp_watchpoint)
+		b->type = bp_hardware_watchpoint;
 
 	      i = hw_watchpoint_used_count (bp_hardware_watchpoint,
 					    &other_type_used);
@@ -1432,8 +1436,22 @@ update_watchpoint (struct breakpoint *b, int reparse)
 	      target_resources_ok = target_can_use_hardware_watchpoint
 		    (bp_hardware_watchpoint, i, other_type_used);
 	      if (target_resources_ok <= 0)
-		b->type = bp_watchpoint;
+		{
+		  if (target_resources_ok == 0
+		      && b->type != bp_hardware_watchpoint)
+		    error (_("Target does not support this type of "
+			     "hardware watchpoint."));
+		  else if (target_resources_ok < 0
+		      && b->type != bp_hardware_watchpoint)
+		    error (_("Target can only support one kind "
+			     "of HW watchpoint at a time."));
+		  else
+		    b->type = bp_watchpoint;
+		}
 	    }
+	  else if (b->type != bp_hardware_watchpoint)
+	    error (_("Expression cannot be implemented with "
+		     "read/access watchpoint."));
 	  else
 	    b->type = bp_watchpoint;
 
@@ -1796,7 +1814,7 @@ breakpoint_program_space_exit (struct program_space *pspace)
   ALL_BREAKPOINTS_SAFE (b, b_temp)
     {
       if (b->pspace == pspace)
-	delete_breakpoint (b);
+	delete_breakpoint (b, 1);
     }
 
   /* Breakpoints set through other program spaces could have locations
@@ -2382,14 +2400,14 @@ update_breakpoints_after_exec (void)
     /* Solib breakpoints must be explicitly reset after an exec().  */
     if (b->type == bp_shlib_event)
       {
-	delete_breakpoint (b);
+	delete_breakpoint (b, 1);
 	continue;
       }
 
     /* JIT breakpoints must be explicitly reset after an exec().  */
     if (b->type == bp_jit_event)
       {
-	delete_breakpoint (b);
+	delete_breakpoint (b, 1);
 	continue;
       }
 
@@ -2399,14 +2417,14 @@ update_breakpoints_after_exec (void)
 	|| b->type == bp_longjmp_master || b->type == bp_std_terminate_master
 	|| b->type == bp_exception_master)
       {
-	delete_breakpoint (b);
+	delete_breakpoint (b, 1);
 	continue;
       }
 
     /* Step-resume breakpoints are meaningless after an exec().  */
     if (b->type == bp_step_resume)
       {
-	delete_breakpoint (b);
+	delete_breakpoint (b, 1);
 	continue;
       }
 
@@ -2415,7 +2433,7 @@ update_breakpoints_after_exec (void)
     if (b->type == bp_longjmp || b->type == bp_longjmp_resume
 	|| b->type == bp_exception || b->type == bp_exception_resume)
       {
-	delete_breakpoint (b);
+	delete_breakpoint (b, 1);
 	continue;
       }
 
@@ -2464,7 +2482,7 @@ update_breakpoints_after_exec (void)
        a.out.  */
     if (b->addr_string == NULL)
       {
-	delete_breakpoint (b);
+	delete_breakpoint (b, 1);
 	continue;
       }
   }
@@ -2736,7 +2754,7 @@ breakpoint_init_inferior (enum inf_context context)
 	   (gdb) tar rem :9999     # remote Windows gdbserver.
 	*/
 
-	delete_breakpoint (b);
+	delete_breakpoint (b, 1);
 	break;
 
       case bp_watchpoint:
@@ -2746,7 +2764,7 @@ breakpoint_init_inferior (enum inf_context context)
 
 	/* Likewise for watchpoints on local expressions.  */
 	if (b->exp_valid_block != NULL)
-	  delete_breakpoint (b);
+	  delete_breakpoint (b, 1);
 	else if (context == inf_starting) 
 	  {
 	    /* Reset val field to force reread of starting value in
@@ -5970,7 +5988,7 @@ delete_longjmp_breakpoint (int thread)
     if (b->type == bp_longjmp || b->type == bp_exception)
       {
 	if (b->thread == thread)
-	  delete_breakpoint (b);
+	  delete_breakpoint (b, 1);
       }
 }
 
@@ -6026,7 +6044,7 @@ delete_std_terminate_breakpoint (void)
 
   ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->type == bp_std_terminate)
-      delete_breakpoint (b);
+      delete_breakpoint (b, 1);
 }
 
 struct breakpoint *
@@ -6054,7 +6072,7 @@ remove_thread_event_breakpoints (void)
   ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->type == bp_thread_event
 	&& b->loc->pspace == current_program_space)
-      delete_breakpoint (b);
+      delete_breakpoint (b, 1);
 }
 
 struct lang_and_radix
@@ -6085,7 +6103,7 @@ remove_jit_event_breakpoints (void)
   ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->type == bp_jit_event
 	&& b->loc->pspace == current_program_space)
-      delete_breakpoint (b);
+      delete_breakpoint (b, 1);
 }
 
 void
@@ -6096,7 +6114,7 @@ remove_solib_event_breakpoints (void)
   ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->type == bp_shlib_event
 	&& b->loc->pspace == current_program_space)
-      delete_breakpoint (b);
+      delete_breakpoint (b, 1);
 }
 
 struct breakpoint *
@@ -8789,6 +8807,7 @@ static void
 watch_command_1 (char *arg, int accessflag, int from_tty,
 		 int just_location, int internal)
 {
+  volatile struct gdb_exception e;
   struct breakpoint *b, *scope_breakpoint = NULL;
   struct expression *exp;
   struct block *exp_valid_block = NULL, *cond_exp_valid_block = NULL;
@@ -8800,9 +8819,7 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
   int toklen;
   char *cond_start = NULL;
   char *cond_end = NULL;
-  int i, other_type_used, target_resources_ok = 0;
   enum bptype bp_type;
-  int reg_cnt = 0;
   int thread = -1;
   int pc = 0;
 
@@ -8932,28 +8949,6 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
   else
     bp_type = bp_hardware_watchpoint;
 
-  reg_cnt = can_use_hardware_watchpoint (val, target_exact_watchpoints);
-  if (reg_cnt == 0 && bp_type != bp_hardware_watchpoint)
-    error (_("Expression cannot be implemented with read/access watchpoint."));
-  if (reg_cnt != 0)
-    {
-      i = hw_watchpoint_used_count (bp_type, &other_type_used);
-      target_resources_ok = 
-	target_can_use_hardware_watchpoint (bp_type, i + reg_cnt,
-					    other_type_used);
-      if (target_resources_ok == 0 && bp_type != bp_hardware_watchpoint)
-	error (_("Target does not support this type of hardware watchpoint."));
-
-      if (target_resources_ok < 0 && bp_type != bp_hardware_watchpoint)
-	error (_("Target can only support one kind "
-		 "of HW watchpoint at a time."));
-    }
-
-  /* Change the type of breakpoint to an ordinary watchpoint if a
-     hardware watchpoint could not be set.  */
-  if (!reg_cnt || target_resources_ok <= 0)
-    bp_type = bp_watchpoint;
-
   frame = block_innermost_frame (exp_valid_block);
 
   /* If the expression is "local", then set up a "watchpoint scope"
@@ -9022,10 +9017,6 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
   b->val_valid = 1;
   b->ops = &watchpoint_breakpoint_ops;
 
-  /* Use an exact watchpoint when there's only one memory region to be
-     watched, and only one debug register is needed to watch it.  */
-  b->exact = target_exact_watchpoints && reg_cnt == 1;
-
   if (cond_start)
     b->cond_string = savestring (cond_start, cond_end - cond_start);
   else
@@ -9053,9 +9044,18 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
   if (!just_location)
     value_free_to_mark (mark);
 
-  /* Finally update the new watchpoint.  This creates the locations
-     that should be inserted.  */
-  update_watchpoint (b, 1);
+  TRY_CATCH (e, RETURN_MASK_ALL)
+    {
+      /* Finally update the new watchpoint.  This creates the locations
+	 that should be inserted.  */
+      update_watchpoint (b, 1);
+    }
+  if (e.reason < 0)
+    {
+      delete_breakpoint (b, 0);
+      throw_exception (e);
+    }
+
   if (internal)
     /* Do not mention breakpoints with a negative number, but do
        notify observers.  */
@@ -9066,14 +9066,10 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
 }
 
 /* Return count of debug registers needed to watch the given expression.
-   If EXACT_WATCHPOINTS is 1, then consider that only the address of
-   the start of the watched region will be monitored (i.e., all accesses
-   will be aligned).  This uses less debug registers on some targets.
-
    If the watchpoint cannot be handled in hardware return zero.  */
 
 static int
-can_use_hardware_watchpoint (struct value *v, int exact_watchpoints)
+can_use_hardware_watchpoint (struct value *v)
 {
   int found_memory_cnt = 0;
   struct value *head = v;
@@ -9129,7 +9125,7 @@ can_use_hardware_watchpoint (struct value *v, int exact_watchpoints)
 		  int len;
 		  int num_regs;
 
-		  len = (exact_watchpoints
+		  len = (target_exact_watchpoints
 			 && is_scalar_type_recursive (vtype))?
 		    1 : TYPE_LENGTH (value_type (v));
 
@@ -9245,9 +9241,9 @@ until_break_command_continuation (void *arg)
 {
   struct until_break_command_continuation_args *a = arg;
 
-  delete_breakpoint (a->breakpoint);
+  delete_breakpoint (a->breakpoint, 1);
   if (a->breakpoint2)
-    delete_breakpoint (a->breakpoint2);
+    delete_breakpoint (a->breakpoint2, 1);
   delete_longjmp_breakpoint (a->thread_num);
 }
 
@@ -9976,7 +9972,7 @@ clear_command (char *arg, int from_tty)
     {
       if (from_tty)
 	printf_unfiltered ("%d ", b->number);
-      delete_breakpoint (b);
+      delete_breakpoint (b, 1);
     }
   if (from_tty)
     putchar_unfiltered ('\n');
@@ -9995,12 +9991,12 @@ breakpoint_auto_delete (bpstat bs)
     if (bs->breakpoint_at
 	&& bs->breakpoint_at->disposition == disp_del
 	&& bs->stop)
-      delete_breakpoint (bs->breakpoint_at);
+      delete_breakpoint (bs->breakpoint_at, 1);
 
   ALL_BREAKPOINTS_SAFE (b, b_tmp)
   {
     if (b->disposition == disp_del_at_next_stop)
-      delete_breakpoint (b);
+      delete_breakpoint (b, 1);
   }
 }
 
@@ -10441,10 +10437,11 @@ bpstat_remove_breakpoint_callback (struct thread_info *th, void *data)
 }
 
 /* Delete a breakpoint and clean up all traces of it in the data
-   structures.  */
+   structures.  If ANNOUNCE is 1, then notifies observers that
+   the breakpoint was deleted.  */
 
 void
-delete_breakpoint (struct breakpoint *bpt)
+delete_breakpoint (struct breakpoint *bpt, int announce)
 {
   struct breakpoint *b;
 
@@ -10487,7 +10484,8 @@ delete_breakpoint (struct breakpoint *bpt)
       bpt->related_breakpoint = bpt;
     }
 
-  observer_notify_breakpoint_deleted (bpt->number);
+  if (announce)
+    observer_notify_breakpoint_deleted (bpt->number);
 
   if (breakpoint_chain == bpt)
     breakpoint_chain = bpt->next;
@@ -10544,7 +10542,7 @@ delete_breakpoint (struct breakpoint *bpt)
 static void
 do_delete_breakpoint_cleanup (void *b)
 {
-  delete_breakpoint (b);
+  delete_breakpoint (b, 1);
 }
 
 struct cleanup *
@@ -10559,7 +10557,7 @@ make_cleanup_delete_breakpoint (struct breakpoint *b)
 static void
 do_delete_breakpoint (struct breakpoint *b, void *ignore)
 {
-  delete_breakpoint (b);
+  delete_breakpoint (b, 1);
 }
 
 void
@@ -10610,7 +10608,7 @@ delete_command (char *arg, int from_tty)
 		&& b->type != bp_std_terminate_master
 		&& b->type != bp_exception_master
 		&& b->number >= 0)
-	      delete_breakpoint (b);
+	      delete_breakpoint (b, 1);
 	  }
 	}
     }
@@ -11073,7 +11071,7 @@ breakpoint_re_set_one (void *bint)
       if (b->addr_string == NULL)
 	{
 	  /* Anything without a string can't be re-set.  */
-	  delete_breakpoint (b);
+	  delete_breakpoint (b, 1);
 	  return 0;
 	}
 
@@ -11127,7 +11125,7 @@ breakpoint_re_set_one (void *bint)
     case bp_longjmp_master:
     case bp_std_terminate_master:
     case bp_exception_master:
-      delete_breakpoint (b);
+      delete_breakpoint (b, 1);
       break;
 
       /* This breakpoint is special, it's set up when the inferior
@@ -12114,7 +12112,7 @@ delete_trace_command (char *arg, int from_tty)
 	  {
 	    if (is_tracepoint (b)
 		&& b->number >= 0)
-	      delete_breakpoint (b);
+	      delete_breakpoint (b, 1);
 	  }
 	}
     }
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 7a9c2d4..bf827e5 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -953,7 +953,7 @@ extern void breakpoint_init_inferior (enum inf_context);
 
 extern struct cleanup *make_cleanup_delete_breakpoint (struct breakpoint *);
 
-extern void delete_breakpoint (struct breakpoint *);
+extern void delete_breakpoint (struct breakpoint *, int);
 
 extern void breakpoint_auto_delete (bpstat);
 
diff --git a/gdb/elfread.c b/gdb/elfread.c
index 2d589a4..277a093 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1036,7 +1036,7 @@ elf_gnu_ifunc_resolver_return_stop (struct breakpoint *b)
 	case bp_gnu_ifunc_resolver:
 	  break;
 	case bp_gnu_ifunc_resolver_return:
-	  delete_breakpoint (b);
+	  delete_breakpoint (b, 1);
 	  break;
 	default:
 	  internal_error (__FILE__, __LINE__,
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 3dc13e3..f0e9d96 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1501,7 +1501,7 @@ finish_command_continuation (void *arg)
      that the *stopped notification includes the return value.  */
   if (bs != NULL && tp->control.proceed_to_finish)
     observer_notify_normal_stop (bs, 1 /* print frame */);
-  delete_breakpoint (a->breakpoint);
+  delete_breakpoint (a->breakpoint, 1);
   delete_longjmp_breakpoint (inferior_thread ()->num);
 }
 
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 0c21bfc..4450e60 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -302,7 +302,7 @@ bppy_delete_breakpoint (PyObject *self, PyObject *args)
 
   BPPY_REQUIRE_VALID (self_bp);
 
-  delete_breakpoint (self_bp->bp);
+  delete_breakpoint (self_bp->bp, 1);
 
   Py_RETURN_NONE;
 }
diff --git a/gdb/thread.c b/gdb/thread.c
index 6ad1807..929c335 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -87,7 +87,7 @@ delete_step_resume_breakpoint (struct thread_info *tp)
 {
   if (tp && tp->control.step_resume_breakpoint)
     {
-      delete_breakpoint (tp->control.step_resume_breakpoint);
+      delete_breakpoint (tp->control.step_resume_breakpoint, 1);
       tp->control.step_resume_breakpoint = NULL;
     }
 }
@@ -97,7 +97,7 @@ delete_exception_resume_breakpoint (struct thread_info *tp)
 {
   if (tp && tp->control.exception_resume_breakpoint)
     {
-      delete_breakpoint (tp->control.exception_resume_breakpoint);
+      delete_breakpoint (tp->control.exception_resume_breakpoint, 1);
       tp->control.exception_resume_breakpoint = NULL;
     }
 }



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