[PATCH, doc RFA] Allow CLI and Python conditions to be set on same breakpoint

[Doug Evans <xdje42 at gmail dot com>]

Hi.

Way back when this was added I wanted to prevent both CLI and Python
conditions from being set on the same breakpoint because it wasn't
clear there wouldn't be any problems allowing both.
[And it's easier to relax restrictions than to impose them after the fact.]

Enough time has passed and I can't think of a reason to keep the
current restriction.

This patch removes the restriction.
I couldn't find a doc change that's necessary.
I added something to the Python section to say setting both is OK.

Regression tested on amd64-linux.

Ok to check in?

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

	* breakpoint.c (condition_command): Allow CLI condition and Python
	condition to be set on same breakpoint.
	(bpstat_check_breakpoint_conditions): Handle both CLI and Python
	conditions being present.
	* python/py-breakpoint.c (local_setattro): Delete.
	(breakpoint_object_type): Update.

	doc/
	* gdb.texinfo (Breakpoints In Python): Mention that a CLI condition
	and Python condition are allowed on the same breakpoint.

	testsuite/
	* gdb.python/py-breakpoint.c: Verify able to set CLI condition and
	Python condition on same breakpoint.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 5bfed9d..4baac57 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1048,14 +1048,6 @@ condition_command (char *arg, int from_tty)
   ALL_BREAKPOINTS (b)
     if (b->number == bnum)
       {
-	/* Check if this breakpoint has a Python object assigned to
-	   it, and if it has a definition of the "stop"
-	   method.  This method and conditions entered into GDB from
-	   the CLI are mutually exclusive.  */
-	if (b->py_bp_object
-	    && gdbpy_breakpoint_has_py_cond (b->py_bp_object))
-	  error (_("Cannot set a condition where a Python 'stop' "
-		   "method has been defined in the breakpoint."));
 	set_breakpoint_condition (b, p, from_tty);
 
 	if (is_breakpoint (b))
@@ -5112,8 +5104,12 @@ 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;
+  struct expression *cli_cond;
+  /* Non-zero if there's a CLI condition *and* it evaluates to true.  */
+  int cli_cond_is_true = 0;
+  int have_py_cond = 0;
+  /* Non-zero if there's a Python condition *and* it evaluates to true.  */
+  int py_cond_is_true = 0;
 
   gdb_assert (bs->stop);
 
@@ -5142,20 +5138,33 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
       return;
     }
 
+  /* The default is to stop when the breakpoint is reached.
+     However, if the breakpoint has any condition (either from the CLI, or
+     from Python, or both), then only stop if at least one of the conditions
+     is TRUE.  */
+
   /* Evaluate Python breakpoints that have a "stop" method implemented.  */
   if (b->py_bp_object)
-    bs->stop = gdbpy_should_stop (b->py_bp_object);
+    {
+      /* We have to call this even if there is no "stop" method in order
+	 to call the FinishBreakpoint pre,post stop hooks.  */
+      int py_should_stop = gdbpy_should_stop (b->py_bp_object);
+
+      have_py_cond = gdbpy_breakpoint_has_py_cond (b->py_bp_object);
+      if (have_py_cond)
+	py_cond_is_true = py_should_stop;
+    }
 
   if (is_watchpoint (b))
     {
       struct watchpoint *w = (struct watchpoint *) b;
 
-      cond = w->cond_exp;
+      cli_cond = w->cond_exp;
     }
   else
-    cond = bl->cond;
+    cli_cond = bl->cond;
 
-  if (cond && b->disposition != disp_del_at_next_stop)
+  if (cli_cond && b->disposition != disp_del_at_next_stop)
     {
       int within_current_scope = 1;
       struct watchpoint * w;
@@ -5205,25 +5214,35 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
 	    within_current_scope = 0;
 	}
       if (within_current_scope)
-	value_is_zero
-	  = catch_errors (breakpoint_cond_eval, cond,
+	{
+	  int cond_result =
+	    catch_errors (breakpoint_cond_eval, cli_cond,
 			  "Error in testing breakpoint condition:\n",
 			  RETURN_MASK_ALL);
+
+	  /* See breakpoint_cond_eval for why it returns the inverted CLI
+	     condition.  */
+	  cli_cond_is_true = !cond_result;
+	}
       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;
+	  cli_cond_is_true = 1;
 	}
       /* FIXME-someday, should give breakpoint #.  */
       value_free_to_mark (mark);
     }
 
-  if (cond && value_is_zero)
+  /* Stop if any condition says so.  */
+  if (cli_cond || have_py_cond)
     {
-      bs->stop = 0;
+      /* bs->stop is already set, so we only have to handle the negative
+	 result: Only continue if all conditions are false.  */
+      if (!cli_cond_is_true && !py_cond_is_true)
+	bs->stop = 0;
     }
   else if (b->ignore_count > 0)
     {
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 84acd5c..c872960 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -27196,6 +27196,12 @@ methods have a chance to execute at that location.  In this scenario
 if one of the methods returns @code{True} but the others return
 @code{False}, the inferior will still be stopped.
 
+A breakpoint may have both a normal breakpoint condition
+(@pxref{Conditions, ,Break Conditions}) and a Python
+@code{gdb.Breakpoint.stop} condition.
+Both will be evaluated and if either return @code{True} then the
+inferior will be stopped, otherwise the inferior will continue.
+
 You should not alter the execution state of the inferior (i.e.@:, step,
 next, etc.), alter the current frame context (i.e.@:, change the current
 active frame), or alter, add or delete any breakpoint.  As a general
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index e83471e..6bc96e2 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -933,37 +933,6 @@ gdbpy_initialize_breakpoints (void)
 
 
 
-/* Helper function that overrides this Python object's
-   PyObject_GenericSetAttr to allow extra validation of the attribute
-   being set.  */
-
-static int 
-local_setattro (PyObject *self, PyObject *name, PyObject *v)
-{
-  breakpoint_object *obj = (breakpoint_object *) self;  
-  char *attr = python_string_to_host_string (name);
-  
-  if (attr == NULL)
-    return -1;
-  
-  /* If the attribute trying to be set is the "stop" method,
-     but we already have a condition set in the CLI, disallow this
-     operation.  */
-  if (strcmp (attr, stop_func) == 0 && obj->bp->cond_string)
-    {
-      xfree (attr);
-      PyErr_SetString (PyExc_RuntimeError, 
-		       _("Cannot set 'stop' method.  There is an " \
-			 "existing GDB condition attached to the " \
-			 "breakpoint."));
-      return -1;
-    }
-  
-  xfree (attr);
-  
-  return PyObject_GenericSetAttr ((PyObject *)self, name, v);  
-}
-
 static PyGetSetDef breakpoint_object_getset[] = {
   { "enabled", bppy_get_enabled, bppy_set_enabled,
     "Boolean telling whether the breakpoint is enabled.", NULL },
@@ -1034,7 +1003,7 @@ PyTypeObject breakpoint_object_type =
   0,				  /*tp_call*/
   0,				  /*tp_str*/
   0,				  /*tp_getattro*/
-  (setattrofunc)local_setattro,   /*tp_setattro */
+  0,				  /*tp_setattro */
   0,				  /*tp_as_buffer*/
   Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,  /*tp_flags*/
   "GDB breakpoint object",	  /* tp_doc */
diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
index e2a169b..03c4f92 100644
--- a/gdb/testsuite/gdb.python/py-breakpoint.exp
+++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
@@ -257,25 +257,39 @@ gdb_test "python print (also_eval_bp1.count)" "4" \
 gdb_test "python print (eval_bp1.count)" "4" \
     "Check non firing same-location breakpoint eval function was also called at each stop."
 
+# Test mixed CLI/Python breakpoint conditions.
+
 delete_breakpoints
 set cond_bp [gdb_get_line_number "Break at multiply."]
 gdb_py_test_silent_cmd  "python eval_bp1 = bp_eval(\"$cond_bp\")" "Set breakpoint" 0
 set test_cond {cond $bpnum}
-gdb_test "$test_cond \"foo==3\"" "Cannot set a condition where a Python.*" \
-    "Check you cannot add a CLI condition to a Python breakpoint that" \
+gdb_test_no_output "$test_cond \"foo==3\"" \
+    "Check you can add a CLI condition to a Python breakpoint that" \
     "has defined stop"
+
+if ![runto_main] then {
+    fail "Cannot run to main."
+    return 0
+}
+delete_breakpoints
 gdb_py_test_silent_cmd  "python eval_bp2 = basic(\"$cond_bp\")" "Set breakpoint" 0
-gdb_py_test_silent_cmd  "python eval_bp2.condition = \"1==1\"" "Set a condition" 0
+gdb_py_test_silent_cmd  "python eval_bp2.condition = \"i==3\"" "Set a condition" 0
 gdb_py_test_multiple "Construct an eval function" \
   "python" "" \
   "def stop_func ():" "" \
-  "   return True" "" \
+  "   return gdb.parse_and_eval(\"i\") == 1" "" \
   "end" ""
+gdb_py_test_silent_cmd "python eval_bp2.stop = stop_func" \
+    "Assign stop function to a breakpoint that has a condition" 1
+gdb_continue_to_breakpoint "Break at multiply." ".*/$srcfile:$bp_location2.*"
+gdb_test "print i" "1" "Check python condition triggered"
+gdb_continue_to_breakpoint "Break at multiply." ".*/$srcfile:$bp_location2.*"
+gdb_test "print i" "3" "Check cli condition triggered"
 
-gdb_test "python eval_bp2.stop = stop_func"  \
-    "RuntimeError: Cannot set 'stop' method.*" \
-    "Assign stop function to a breakpoint that has a condition"
-
+if ![runto_main] then {
+    fail "Cannot run to main."
+    return 0
+}
 delete_breakpoints
 gdb_breakpoint [gdb_get_line_number "Break at multiply."]
 gdb_py_test_silent_cmd  "python check_eval = bp_eval(\"$bp_location2\")" "Set breakpoint" 0