[PATCH] simplify bpstat_check_breakpoint_conditions

[PATCH] simplify bpstat_check_breakpoint_conditions

[Doug Evans]

Hi.

This patch simplifies bpstat_check_breakpoint_conditions a bit.

Regression tested on amd64-linux.
Ok to check in?

[Appended at the end is the same patch with diff's -b option added
to make the actual changes easier to see.]

2013-11-11  Doug Evans  <xdje42@gmail.com>

    	* breakpoint.c (bpstat_check_breakpoint_conditions): Assert
    	bs->stop != 0 on entry.  Update function comment.  Simplify early
    	exit for frame mismatch.  Reindent rest of function.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ffe73fd..36252ee 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5099,8 +5099,8 @@ bpstat_check_watchpoint (bpstat bs)
     }
 }
 
-
-/* Check conditions (condition proper, frame, thread and ignore count)
+/* For breakpoints that are currently marked as telling gdb to stop,
+   check conditions (condition proper, frame, thread and ignore count)
    of breakpoint referred to by BS.  If we should not stop for this
    breakpoint, set BS->stop to 0.  */
 
@@ -5110,6 +5110,10 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
   int thread_id = pid_to_thread_id (ptid);
   const struct bp_location *bl;
   struct breakpoint *b;
+  int value_is_zero = 0;
+  struct expression *cond;
+
+  gdb_assert (bs->stop);
 
   /* BS is built for existing struct breakpoint.  */
   bl = bs->bp_location_at;
@@ -5123,109 +5127,106 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
 
   if (frame_id_p (b->frame_id)
       && !frame_id_eq (b->frame_id, get_stack_frame_id (get_current_frame ())))
-    bs->stop = 0;
-  else if (bs->stop)
     {
-      int value_is_zero = 0;
-      struct expression *cond;
+      bs->stop = 0;
+      return;
+    }
 
-      /* Evaluate Python breakpoints that have a "stop"
-	 method implemented.  */
-      if (b->py_bp_object)
-	bs->stop = gdbpy_should_stop (b->py_bp_object);
+  /* Evaluate Python breakpoints that have a "stop" method implemented.  */
+  if (b->py_bp_object)
+    bs->stop = gdbpy_should_stop (b->py_bp_object);
 
-      if (is_watchpoint (b))
-	{
-	  struct watchpoint *w = (struct watchpoint *) b;
+  if (is_watchpoint (b))
+    {
+      struct watchpoint *w = (struct watchpoint *) b;
 
-	  cond = w->cond_exp;
-	}
+      cond = w->cond_exp;
+    }
+  else
+    cond = bl->cond;
+
+  if (cond && b->disposition != disp_del_at_next_stop)
+    {
+      int within_current_scope = 1;
+      struct watchpoint * w;
+
+      /* We use value_mark and value_free_to_mark because it could
+	 be a long time before we return to the command level and
+	 call free_all_values.  We can't call free_all_values
+	 because we might be in the middle of evaluating a
+	 function call.  */
+      struct value *mark = value_mark ();
+
+      if (is_watchpoint (b))
+	w = (struct watchpoint *) b;
       else
-	cond = bl->cond;
+	w = NULL;
 
-      if (cond && b->disposition != disp_del_at_next_stop)
+      /* Need to select the frame, with all that implies so that
+	 the conditions will have the right context.  Because we
+	 use the frame, we will not see an inlined function's
+	 variables when we arrive at a breakpoint at the start
+	 of the inlined function; the current frame will be the
+	 call site.  */
+      if (w == NULL || w->cond_exp_valid_block == NULL)
+	select_frame (get_current_frame ());
+      else
 	{
-	  int within_current_scope = 1;
-	  struct watchpoint * w;
-
-	  /* We use value_mark and value_free_to_mark because it could
-	     be a long time before we return to the command level and
-	     call free_all_values.  We can't call free_all_values
-	     because we might be in the middle of evaluating a
-	     function call.  */
-	  struct value *mark = value_mark ();
-
-	  if (is_watchpoint (b))
-	    w = (struct watchpoint *) b;
-	  else
-	    w = NULL;
-
-	  /* Need to select the frame, with all that implies so that
-	     the conditions will have the right context.  Because we
-	     use the frame, we will not see an inlined function's
-	     variables when we arrive at a breakpoint at the start
-	     of the inlined function; the current frame will be the
-	     call site.  */
-	  if (w == NULL || w->cond_exp_valid_block == NULL)
-	    select_frame (get_current_frame ());
-	  else
-	    {
-	      struct frame_info *frame;
-
-	      /* For local watchpoint expressions, which particular
-		 instance of a local is being watched matters, so we
-		 keep track of the frame to evaluate the expression
-		 in.  To evaluate the condition however, it doesn't
-		 really matter which instantiation of the function
-		 where the condition makes sense triggers the
-		 watchpoint.  This allows an expression like "watch
-		 global if q > 10" set in `func', catch writes to
-		 global on all threads that call `func', or catch
-		 writes on all recursive calls of `func' by a single
-		 thread.  We simply always evaluate the condition in
-		 the innermost frame that's executing where it makes
-		 sense to evaluate the condition.  It seems
-		 intuitive.  */
-	      frame = block_innermost_frame (w->cond_exp_valid_block);
-	      if (frame != NULL)
-		select_frame (frame);
-	      else
-		within_current_scope = 0;
-	    }
-	  if (within_current_scope)
-	    value_is_zero
-	      = catch_errors (breakpoint_cond_eval, cond,
-			      "Error in testing breakpoint condition:\n",
-			      RETURN_MASK_ALL);
+	  struct frame_info *frame;
+
+	  /* For local watchpoint expressions, which particular
+	     instance of a local is being watched matters, so we
+	     keep track of the frame to evaluate the expression
+	     in.  To evaluate the condition however, it doesn't
+	     really matter which instantiation of the function
+	     where the condition makes sense triggers the
+	     watchpoint.  This allows an expression like "watch
+	     global if q > 10" set in `func', catch writes to
+	     global on all threads that call `func', or catch
+	     writes on all recursive calls of `func' by a single
+	     thread.  We simply always evaluate the condition in
+	     the innermost frame that's executing where it makes
+	     sense to evaluate the condition.  It seems
+	     intuitive.  */
+	  frame = block_innermost_frame (w->cond_exp_valid_block);
+	  if (frame != NULL)
+	    select_frame (frame);
 	  else
-	    {
-	      warning (_("Watchpoint condition cannot be tested "
-			 "in the current scope"));
-	      /* If we failed to set the right context for this
-		 watchpoint, unconditionally report it.  */
-	      value_is_zero = 0;
-	    }
-	  /* FIXME-someday, should give breakpoint #.  */
-	  value_free_to_mark (mark);
-	}
-
-      if (cond && value_is_zero)
-	{
-	  bs->stop = 0;
+	    within_current_scope = 0;
 	}
-      else if (b->thread != -1 && b->thread != thread_id)
+      if (within_current_scope)
+	value_is_zero
+	  = catch_errors (breakpoint_cond_eval, cond,
+			  "Error in testing breakpoint condition:\n",
+			  RETURN_MASK_ALL);
+      else
 	{
-	  bs->stop = 0;
+	  warning (_("Watchpoint condition cannot be tested "
+		     "in the current scope"));
+	  /* If we failed to set the right context for this
+	     watchpoint, unconditionally report it.  */
+	  value_is_zero = 0;
 	}
-      else if (b->ignore_count > 0)
-	{
-	  b->ignore_count--;
-	  bs->stop = 0;
-	  /* Increase the hit count even though we don't stop.  */
-	  ++(b->hit_count);
-	  observer_notify_breakpoint_modified (b);
-	}	
+      /* FIXME-someday, should give breakpoint #.  */
+      value_free_to_mark (mark);
+    }
+
+  if (cond && value_is_zero)
+    {
+      bs->stop = 0;
+    }
+  else if (b->thread != -1 && b->thread != thread_id)
+    {
+      bs->stop = 0;
     }
+  else if (b->ignore_count > 0)
+    {
+      b->ignore_count--;
+      bs->stop = 0;
+      /* Increase the hit count even though we don't stop.  */
+      ++(b->hit_count);
+      observer_notify_breakpoint_modified (b);
+    }	
 }
 
 
----- with -b added -----

--- breakpoint.c~	2013-11-02 13:42:45.321034724 -0700
+++ breakpoint.c	2013-11-11 19:18:21.525399880 -0800
@@ -5099,8 +5099,8 @@ bpstat_check_watchpoint (bpstat bs)
     }
 }
 
-
-/* Check conditions (condition proper, frame, thread and ignore count)
+/* For breakpoints that are currently marked as telling gdb to stop,
+   check conditions (condition proper, frame, thread and ignore count)
    of breakpoint referred to by BS.  If we should not stop for this
    breakpoint, set BS->stop to 0.  */
 
@@ -5110,6 +5110,10 @@ bpstat_check_breakpoint_conditions (bpst
   int thread_id = pid_to_thread_id (ptid);
   const struct bp_location *bl;
   struct breakpoint *b;
+  int value_is_zero = 0;
+  struct expression *cond;
+
+  gdb_assert (bs->stop);
 
   /* BS is built for existing struct breakpoint.  */
   bl = bs->bp_location_at;
@@ -5123,14 +5127,12 @@ bpstat_check_breakpoint_conditions (bpst
 
   if (frame_id_p (b->frame_id)
       && !frame_id_eq (b->frame_id, get_stack_frame_id (get_current_frame ())))
-    bs->stop = 0;
-  else if (bs->stop)
     {
-      int value_is_zero = 0;
-      struct expression *cond;
+      bs->stop = 0;
+      return;
+    }
 
-      /* Evaluate Python breakpoints that have a "stop"
-	 method implemented.  */
+  /* Evaluate Python breakpoints that have a "stop" method implemented.  */
       if (b->py_bp_object)
 	bs->stop = gdbpy_should_stop (b->py_bp_object);
 
@@ -5225,7 +5227,6 @@ bpstat_check_breakpoint_conditions (bpst
 	  ++(b->hit_count);
 	  observer_notify_breakpoint_modified (b);
 	}	
-    }
 }
 
 

Re: [PATCH] simplify bpstat_check_breakpoint_conditions

[Pedro Alves]

On 11/12/2013 04:05 AM, Doug Evans wrote:

> This patch simplifies bpstat_check_breakpoint_conditions a bit.
> 
> Regression tested on amd64-linux.
> Ok to check in?

Looks good to me.

> [Appended at the end is the same patch with diff's -b option added
> to make the actual changes easier to see.]

Thanks, that's quite helpful.

> 
> 2013-11-11  Doug Evans  <xdje42@gmail.com>
> 
>     	* breakpoint.c (bpstat_check_breakpoint_conditions): Assert
>     	bs->stop != 0 on entry.  Update function comment.  Simplify early
>     	exit for frame mismatch.  Reindent rest of function.

-- 
Pedro Alves

[PATCH] Don't evaluate condition for non-matching thread

[Doug Evans]

Pedro Alves <palves@redhat.com> writes:

> On 11/12/2013 04:05 AM, Doug Evans wrote:
>
>> This patch simplifies bpstat_check_breakpoint_conditions a bit.
>> 
>> Regression tested on amd64-linux.
>> Ok to check in?
>
> Looks good to me.

Thanks.
Next patch.

Is there a reason the current code is the way it is?
Evaluating breakpoint conditions is expensive,
and to do so for a thread-specific breakpoint on a non-matching
thread is an awful waste.
I read the thread-specific breakpoints section of the docs
and didn't find anything.

Regression tested on amd64-linux,
but no claim is made that I'm sure there isn't some gotcha
for why things are the way they are (including "Oops, but it's in the
wild now and reason X means we can't change it." :-)).

Thank goodness the ignore_count check is done after the thread-id check
so there's no change w.r.t. ignore counts.

[I realize conditions can have side-effects, to, e.g., collect data,
but the user has made the breakpoint thread-specific already.]

If you think this warrants a NEWS entry I can certainly add one.

Ok to check in?

2013-11-12  Doug Evans  <xdje42@gmail.com>

	* breakpoint.c (bpstat_check_breakpoint_conditions): For thread
	specific breakpoints, don't evaluate breakpoint condition if
	different thread.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 36252ee..ebb8336 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5132,6 +5132,14 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
       return;
     }
 
+  /* If this is a thread-specific breakpoint, don't waste cpu evaluating the
+     condition if this isn't the specified thread.  */
+  if (b->thread != -1 && b->thread != thread_id)
+    {
+      bs->stop = 0;
+      return;
+    }
+
   /* Evaluate Python breakpoints that have a "stop" method implemented.  */
   if (b->py_bp_object)
     bs->stop = gdbpy_should_stop (b->py_bp_object);
@@ -5215,10 +5223,6 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
     {
       bs->stop = 0;
     }
-  else if (b->thread != -1 && b->thread != thread_id)
-    {
-      bs->stop = 0;
-    }
   else if (b->ignore_count > 0)
     {
       b->ignore_count--;

Re: [PATCH] Don't evaluate condition for non-matching thread

[Pedro Alves]

On 11/13/2013 03:01 AM, Doug Evans wrote:

> Is there a reason the current code is the way it is?

I can't think of any.  I'm actually surprised we do this.

We'd get the same effect if/when we push the thread check
down to the target, and I wouldn't imagine this "always eval
condition first" blocking such new feature.

> [I realize conditions can have side-effects, to, e.g., collect data,
> but the user has made the breakpoint thread-specific already.]

Yeah.  There are several other ways to get to same result, so
it's not a real issue, IMO.

> If you think this warrants a NEWS entry I can certainly add one.

I don't think so.

> Ok to check in?

OK.

Thanks,
-- 
Pedro Alves