[patch] [python] Implement stop_p for gdb.Breakpoint

[patch] [python] Implement stop_p for gdb.Breakpoint

[Phil Muldoon]

This is another submission for the stop_p/eval/condition/whatever-name
that allows Python breakpoints to evaluate arbitrary conditions when a
breakpoint is hit, and accordingly, instruct GDB to stop or continue.
I've decided I would like this patch to be pushed for 7.3, so hence the
resurrection.  Lets make this work and fix what needs to be done for
7.3.

I believe I have implemented all of the requests from the previous patch
discussion.  I eventually renamed eval to stop_p.  Also, recently, Tom
wrote a log_printf Python command that uses this feature. I've included
it here, along with a few alterations Tom had to make to Breakpoint
initialization to make that work.

Here it is. What do you think?

(I've CC'd Tom and Doug directly, purely as a courtesy from the last
discussion.)

I have the ChangeLog entries, but did not include them in this
patch.  I will update them accordingly in the future. 

Tested on x8664 with no regressions.

Cheers

Phil

--

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index c9e149b..04603e0 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -72,6 +72,10 @@
 
 #include "mi/mi-common.h"
 
+#if HAVE_PYTHON
+#include "python/python.h"
+#endif
+
 /* Arguments to pass as context to some catch command handlers.  */
 #define CATCH_PERMANENT ((void *) (uintptr_t) 0)
 #define CATCH_TEMPORARY ((void *) (uintptr_t) 1)
@@ -4217,6 +4221,12 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
       int value_is_zero = 0;
       struct expression *cond;
 
+#if HAVE_PYTHON
+      /* Evaluate Python breakpoints that have a "condition"
+	 method implemented.  */
+      if (b->py_bp_object)
+	bs->stop = gdbpy_stop_p (b->py_bp_object);
+#endif
       if (is_watchpoint (b))
 	cond = b->cond_exp;
       else
diff --git a/gdb/data-directory/Makefile.in b/gdb/data-directory/Makefile.in
index 11cf2e6..96b63b6 100644
--- a/gdb/data-directory/Makefile.in
+++ b/gdb/data-directory/Makefile.in
@@ -56,6 +56,7 @@ PYTHON_FILES = \
 	gdb/types.py \
 	gdb/printing.py \
 	gdb/command/__init__.py \
+	gdb/command/log.py \
 	gdb/command/pretty_printers.py
 
 FLAGS_TO_PASS = \
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index f8b7e2d..20a99f7 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -23082,6 +23082,34 @@ argument defines the class of watchpoint to create, if @var{type} is
 assumed to be a @var{WP_WRITE} class.
 @end defmethod
 
+@defop Operation {gdb.Breakpoint} stop_p (self)
+The @code{gdb.Breakpoint} class can be sub-classed and, in
+particular, you may choose to implement the @code{stop_p} method.
+If this method is defined as a sub-class of @code{gdb.Breakpoint},
+it will be called when the inferior stops at any location of a
+breakpoint which instantiates that sub-class.  If the method returns
+@code{True}, the inferior will be stopped at the location of the
+breakpoint, otherwise the inferior will continue.
+
+If there are multiple breakpoints at the same location with a
+@code{stop_p} method, each one will be called regardless of the
+return status of the previous.  This ensures that all @code{stop_p}
+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.
+
+Example @code{stop_p} implementation:
+
+@smallexample
+class MyBreakpoint (gdb.Breakpoint):
+      def stop_p (self):
+        inf_val = gdb.parse_and_eval("foo")
+        if inf_val == 3:
+          return True
+        return False
+@end smallexample
+@end defop
+
 The available watchpoint types represented by constants are defined in the
 @code{gdb} module:
 
diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
index 43975c2..35d4dfc 100644
--- a/gdb/python/lib/gdb/__init__.py
+++ b/gdb/python/lib/gdb/__init__.py
@@ -14,5 +14,6 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import gdb.command.pretty_printers
+import gdb.command.log
 
 gdb.command.pretty_printers.register_pretty_printer_commands()
diff --git a/gdb/python/lib/gdb/command/log.py b/gdb/python/lib/gdb/command/log.py
new file mode 100644
index 0000000..4b93b8c
--- /dev/null
+++ b/gdb/python/lib/gdb/command/log.py
@@ -0,0 +1,57 @@
+# log-printf command
+# Copyright (C) 2011 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import gdb
+
+class _Logbp(gdb.Breakpoint):
+    "A breakpoint that logs using printf."
+
+    def __init__(self, loc, arg):
+        super(_Logbp, self).__init__(loc)
+        self._cmd = 'printf ' + arg
+
+    def stop_p(self):
+        gdb.execute(self._cmd, from_tty = False)
+        return False
+
+class _Logprintf(gdb.Command):
+    """A variant of printf that logs expressions at a given source location.
+
+Usage: log-printf LINESPEC -- PRINTF-ARGS
+
+LINESPEC is any linespec of the forms accepted by `break'.
+A plain `--' separates the linespec from the remaining arguments,
+which are of the form accepted by the `printf' command.
+
+This command installs a special breakpoint to do its work.
+You can disable the effects of this command by disabling or deleting
+that breakpoint."""
+
+    def __init__(self):
+        super(_Logprintf, self).__init__('log-printf',
+                                         gdb.COMMAND_BREAKPOINTS,
+                                         # Arguable.
+                                         gdb.COMPLETE_SYMBOL)
+
+    def invoke(self, arg, from_tty):
+        dashes = arg.find(' -- ')
+        if dashes == -1:
+            raise gdb.GdbError("could not find ` -- '")
+        linespec = arg[0 : dashes]
+        format = arg[(dashes + 4):]
+        _Logbp(linespec, format)
+
+_Logprintf()
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index bfad002..4551416 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -28,6 +28,8 @@
 #include "observer.h"
 #include "cli/cli-script.h"
 #include "ada-lang.h"
+#include "arch-utils.h"
+#include "language.h"
 
 static PyTypeObject breakpoint_object_type;
 
@@ -586,10 +588,9 @@ bppy_get_ignore_count (PyObject *self, void *closure)
 }
 
 /* Python function to create a new breakpoint.  */
-static PyObject *
-bppy_new (PyTypeObject *subtype, PyObject *args, PyObject *kwargs)
+static int
+bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 {
-  PyObject *result;
   static char *keywords[] = { "spec", "type", "wp_class", "internal", NULL };
   char *spec;
   int type = bp_breakpoint;
@@ -600,19 +601,16 @@ bppy_new (PyTypeObject *subtype, PyObject *args, PyObject *kwargs)
 
   if (! PyArg_ParseTupleAndKeywords (args, kwargs, "s|iiO", keywords,
 				     &spec, &type, &access_type, &internal))
-    return NULL;
+    return -1;
 
   if (internal)
     {
       internal_bp = PyObject_IsTrue (internal);
       if (internal_bp == -1)
-	return NULL;
+	return -1;
     }
 
-  result = subtype->tp_alloc (subtype, 0);
-  if (! result)
-    return NULL;
-  bppy_pending_object = (breakpoint_object *) result;
+  bppy_pending_object = (breakpoint_object *) self;
   bppy_pending_object->number = -1;
   bppy_pending_object->bp = NULL;
   
@@ -649,14 +647,14 @@ bppy_new (PyTypeObject *subtype, PyObject *args, PyObject *kwargs)
     }
   if (except.reason < 0)
     {
-      subtype->tp_free (result);
-      return PyErr_Format (except.reason == RETURN_QUIT
-			   ? PyExc_KeyboardInterrupt : PyExc_RuntimeError,
-			   "%s", except.message);
+      PyErr_Format (except.reason == RETURN_QUIT
+		    ? PyExc_KeyboardInterrupt : PyExc_RuntimeError,
+		    "%s", except.message);
+      return -1;
     }
 
-  BPPY_REQUIRE_VALID ((breakpoint_object *) result);
-  return result;
+  BPPY_SET_REQUIRE_VALID ((breakpoint_object *) self);
+  return 0;
 }
 
 \f
@@ -707,6 +705,47 @@ gdbpy_breakpoints (PyObject *self, PyObject *args)
   return PyList_AsTuple (list);
 }
 
+/* Call the "stop_p" method (if implemented) in the breakpoint class.  If
+   the method returns True, the inferior  will be stopped at the
+   breakpoint.  Otherwise the inferior will be allowed to continue.  */
+
+int
+gdbpy_stop_p (struct breakpoint_object *bp_obj)
+{
+  int should_stop = 1;
+  char *method = "stop_p";
+
+  PyObject *py_bp = (PyObject *) bp_obj;
+  struct breakpoint *b = bp_obj->bp;
+  struct gdbarch *garch = b->gdbarch ? b->gdbarch : get_current_arch ();
+  struct cleanup *cleanup = ensure_python_env (garch, current_language);
+
+  if (PyObject_HasAttrString (py_bp, method))
+    {
+      PyObject *result = PyObject_CallMethod (py_bp, method, NULL);
+
+      if (result)
+	{
+	  int evaluate = PyObject_IsTrue (result);
+
+	  if (evaluate == -1)
+	    gdbpy_print_stack ();
+
+	  /* If the evaluate function returns False that means the
+	     Python breakpoint wants GDB to continue.  */
+	  if (!evaluate)
+	    should_stop = 0;
+
+	  Py_DECREF (result);
+	}
+      else
+	gdbpy_print_stack ();
+    }
+  do_cleanups (cleanup);
+
+  return should_stop;
+}
+
 \f
 
 /* Event callback functions.  */
@@ -793,7 +832,6 @@ gdbpy_initialize_breakpoints (void)
 {
   int i;
 
-  breakpoint_object_type.tp_new = bppy_new;
   if (PyType_Ready (&breakpoint_object_type) < 0)
     return;
 
@@ -899,7 +937,7 @@ static PyTypeObject breakpoint_object_type =
   0,				  /*tp_getattro*/
   0,				  /*tp_setattro*/
   0,				  /*tp_as_buffer*/
-  Py_TPFLAGS_DEFAULT,		  /*tp_flags*/
+  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,  /*tp_flags*/
   "GDB breakpoint object",	  /* tp_doc */
   0,				  /* tp_traverse */
   0,				  /* tp_clear */
@@ -909,5 +947,13 @@ static PyTypeObject breakpoint_object_type =
   0,				  /* tp_iternext */
   breakpoint_object_methods,	  /* tp_methods */
   0,				  /* tp_members */
-  breakpoint_object_getset	  /* tp_getset */
+  breakpoint_object_getset,	  /* tp_getset */
+  0,				  /* tp_base */
+  0,				  /* tp_dict */
+  0,				  /* tp_descr_get */
+  0,				  /* tp_descr_set */
+  0,				  /* tp_dictoffset */
+  bppy_init,			  /* tp_init */
+  0,				  /* tp_alloc */
+  PyType_GenericNew		  /* tp_new */
 };
diff --git a/gdb/python/python.h b/gdb/python/python.h
index 58d97fc..801282e 100644
--- a/gdb/python/python.h
+++ b/gdb/python/python.h
@@ -22,6 +22,8 @@
 
 #include "value.h"
 
+struct breakpoint_object;
+
 extern int gdbpy_global_auto_load;
 
 extern void finish_python_initialization (void);
@@ -41,4 +43,6 @@ void preserve_python_values (struct objfile *objfile, htab_t copied_types);
 
 void load_auto_scripts_for_objfile (struct objfile *objfile);
 
+int gdbpy_stop_p (struct breakpoint_object *bp_obj);
+
 #endif /* GDB_PYTHON_H */
diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
index 6b33284..51ea126 100644
--- a/gdb/testsuite/gdb.python/py-breakpoint.exp
+++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
@@ -198,3 +198,76 @@ gdb_py_test_silent_cmd  "python wp1 = gdb.Breakpoint (\"result\", type=gdb.BP_WA
 gdb_test "info breakpoints" "No breakpoints or watchpoints.*" "Check info breakpoints does not show invisible breakpoints"
 gdb_test "maint info breakpoints" ".*hw watchpoint.*result.*" "Check maint info breakpoints shows invisible breakpoints"
 gdb_test "continue" ".*\[Ww\]atchpoint.*result.*Old value = 0.*New value = 25.*" "Test watchpoint write"
+
+# Breakpoints that have an evaluation function.
+
+# Start with a fresh gdb.
+clean_restart ${testfile}
+
+if ![runto_main] then {
+    fail "Cannot run to main."
+    return 0
+}
+delete_breakpoints
+
+gdb_py_test_multiple "Sub-class a breakpoint" \
+  "python" "" \
+  "class bp_eval (gdb.Breakpoint):" "" \
+  "   inf_i = 0" "" \
+  "   count = 0" "" \
+  "   def stop_p (self):" "" \
+  "      self.count = self.count + 1" "" \
+  "      self.inf_i = gdb.parse_and_eval(\"i\")" "" \
+  "      if self.inf_i == 3:" "" \
+  "        return True" "" \
+  "      return False" "" \
+  "end" ""
+
+gdb_py_test_multiple "Sub-class a second breakpoint" \
+  "python" "" \
+  "class bp_also_eval (gdb.Breakpoint):" "" \
+  "   count = 0" "" \
+  "   def stop_p (self):" "" \
+  "      self.count = self.count + 1" "" \
+  "      if self.count == 9:" "" \
+  "        return True" "" \
+  "      return False" "" \
+  "end" ""
+
+set bp_location2 [gdb_get_line_number "Break at multiply."]
+set end_location [gdb_get_line_number "Break at end."]
+gdb_py_test_silent_cmd  "python eval_bp1 = bp_eval(\"$bp_location2\")" "Set breakpoint" 0
+gdb_py_test_silent_cmd  "python also_eval_bp1 = bp_also_eval(\"$bp_location2\")" "Set breakpoint" 0
+gdb_py_test_silent_cmd  "python never_eval_bp1 = bp_also_eval(\"$end_location\")" "Set breakpoint" 0
+gdb_continue_to_breakpoint "Break at multiply." ".*/$srcfile:$bp_location2.*"
+gdb_test "print i" "3" "Check inferior value matches python accounting"
+gdb_test "python print eval_bp1.inf_i" "3" "Check python accounting matches inferior"
+gdb_test "python print also_eval_bp1.count" "4" \
+    "Check non firing same-location breakpoint eval function was also called at each stop."
+gdb_test "python print eval_bp1.count" "4" \
+    "Check non firing same-location breakpoint eval function was also called at each stop."
+
+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
+gdb_test "python print check_eval.count" "0" \
+    "Test that evaluate function has not been yet executed (ie count = 0)"
+gdb_continue_to_breakpoint "Break at multiply." ".*/$srcfile:$bp_location2.*"
+gdb_test "python print check_eval.count" "1" \
+    "Test that evaluate function is run when location also has normal bp"
+
+gdb_py_test_multiple "Sub-class a watchpoint" \
+  "python" "" \
+  "class wp_eval (gdb.Breakpoint):" "" \
+  "   def stop_p (self):" "" \
+  "      self.result = gdb.parse_and_eval(\"result\")" "" \
+  "      if self.result == 788:" "" \
+  "        return True" "" \
+  "      return False" "" \
+  "end" ""
+
+delete_breakpoints
+gdb_py_test_silent_cmd  "python wp1 = wp_eval (\"result\", type=gdb.BP_WATCHPOINT, wp_class=gdb.WP_WRITE)" "Set watchpoint" 0
+gdb_test "continue" ".*\[Ww\]atchpoint.*result.*Old value =.*New value = 788.*" "Test watchpoint write"
+gdb_test "python print never_eval_bp1.count" "0" \
+    "Check that this unrelated breakpoints eval function was never called."

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Eli Zaretskii]

> From: Phil Muldoon <pmuldoon@redhat.com>
> CC: tromey@redhat.com, dje@google.com
> Date: Wed, 23 Feb 2011 14:56:19 +0000
> 
> +@defop Operation {gdb.Breakpoint} stop_p (self)
                                            ^^^^^^
No parens, just the argument.

Otherwise, the doco part is fine.  Thanks.

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Doug Evans]

On Wed, Feb 23, 2011 at 6:56 AM, Phil Muldoon <pmuldoon@redhat.com> wrote:
>
> This is another submission for the stop_p/eval/condition/whatever-name
> that allows Python breakpoints to evaluate arbitrary conditions when a
> breakpoint is hit, and accordingly, instruct GDB to stop or continue.
> I've decided I would like this patch to be pushed for 7.3, so hence the
> resurrection.  Lets make this work and fix what needs to be done for
> 7.3.
>
> I believe I have implemented all of the requests from the previous patch
> discussion.  I eventually renamed eval to stop_p.  Also, recently, Tom
> wrote a log_printf Python command that uses this feature. I've included
> it here, along with a few alterations Tom had to make to Breakpoint
> initialization to make that work.
>
> Here it is. What do you think?

Hi.
Some nits and comments:

- "consistency is good", so if we go with _p for stop_p we need to go
with _p for all predicates
  - are we prepared for that?
  - are there any existing predicates that don't have _p?
  - does python have an existing convention?
  [I used stop_p at the time for clarity's sake.  But I think these
questions need to be asked.]

- I didn't see any tests for log-printf

- log.py feels misnamed but since the name isn't exported to the user
it's not important enough to find a better name

- can printf get an error (e.g. bad memory access) and would one
necessarily want execution to halt when that happens?
  - I can imagine wanting to just see an error message and have
execution continue
  - OTOH while testing a log-printf I would want execution to stop if
I misspelled something
  - we don't have to add the functionality now, but IWBN to at least
think about if/how we'd provide it

- we probably should document log-printf in the manual

- is the logic for deciding whether to stop correct?
  E.g. if stop_p says "don't stop" and a condition says "stop" will
execution continue?  It looks like it, but maybe I'm misunderstanding
something.

> (I've CC'd Tom and Doug directly, purely as a courtesy from the last
> discussion.)
>
> I have the ChangeLog entries, but did not include them in this
> patch.  I will update them accordingly in the future.
>
> Tested on x8664 with no regressions.
>
> Cheers
>
> Phil
>
> --
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index c9e149b..04603e0 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -72,6 +72,10 @@
>
>  #include "mi/mi-common.h"
>
> +#if HAVE_PYTHON
> +#include "python/python.h"
> +#endif
> +
>  /* Arguments to pass as context to some catch command handlers.  */
>  #define CATCH_PERMANENT ((void *) (uintptr_t) 0)
>  #define CATCH_TEMPORARY ((void *) (uintptr_t) 1)
> @@ -4217,6 +4221,12 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
>       int value_is_zero = 0;
>       struct expression *cond;
>
> +#if HAVE_PYTHON
> +      /* Evaluate Python breakpoints that have a "condition"
> +        method implemented.  */
> +      if (b->py_bp_object)
> +       bs->stop = gdbpy_stop_p (b->py_bp_object);
> +#endif
>       if (is_watchpoint (b))
>        cond = b->cond_exp;
>       else
> diff --git a/gdb/data-directory/Makefile.in b/gdb/data-directory/Makefile.in
> index 11cf2e6..96b63b6 100644
> --- a/gdb/data-directory/Makefile.in
> +++ b/gdb/data-directory/Makefile.in
> @@ -56,6 +56,7 @@ PYTHON_FILES = \
>        gdb/types.py \
>        gdb/printing.py \
>        gdb/command/__init__.py \
> +       gdb/command/log.py \
>        gdb/command/pretty_printers.py
>
>  FLAGS_TO_PASS = \
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index f8b7e2d..20a99f7 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -23082,6 +23082,34 @@ argument defines the class of watchpoint to create, if @var{type} is
>  assumed to be a @var{WP_WRITE} class.
>  @end defmethod
>
> +@defop Operation {gdb.Breakpoint} stop_p (self)
> +The @code{gdb.Breakpoint} class can be sub-classed and, in
> +particular, you may choose to implement the @code{stop_p} method.
> +If this method is defined as a sub-class of @code{gdb.Breakpoint},
> +it will be called when the inferior stops at any location of a
> +breakpoint which instantiates that sub-class.  If the method returns
> +@code{True}, the inferior will be stopped at the location of the
> +breakpoint, otherwise the inferior will continue.
> +
> +If there are multiple breakpoints at the same location with a
> +@code{stop_p} method, each one will be called regardless of the
> +return status of the previous.  This ensures that all @code{stop_p}
> +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.
> +
> +Example @code{stop_p} implementation:
> +
> +@smallexample
> +class MyBreakpoint (gdb.Breakpoint):
> +      def stop_p (self):
> +        inf_val = gdb.parse_and_eval("foo")
> +        if inf_val == 3:
> +          return True
> +        return False
> +@end smallexample
> +@end defop
> +
>  The available watchpoint types represented by constants are defined in the
>  @code{gdb} module:
>
> diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
> index 43975c2..35d4dfc 100644
> --- a/gdb/python/lib/gdb/__init__.py
> +++ b/gdb/python/lib/gdb/__init__.py
> @@ -14,5 +14,6 @@
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>
>  import gdb.command.pretty_printers
> +import gdb.command.log
>
>  gdb.command.pretty_printers.register_pretty_printer_commands()
> diff --git a/gdb/python/lib/gdb/command/log.py b/gdb/python/lib/gdb/command/log.py
> new file mode 100644
> index 0000000..4b93b8c
> --- /dev/null
> +++ b/gdb/python/lib/gdb/command/log.py
> @@ -0,0 +1,57 @@
> +# log-printf command
> +# Copyright (C) 2011 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +import gdb
> +
> +class _Logbp(gdb.Breakpoint):
> +    "A breakpoint that logs using printf."
> +
> +    def __init__(self, loc, arg):
> +        super(_Logbp, self).__init__(loc)
> +        self._cmd = 'printf ' + arg
> +
> +    def stop_p(self):
> +        gdb.execute(self._cmd, from_tty = False)
> +        return False
> +
> +class _Logprintf(gdb.Command):
> +    """A variant of printf that logs expressions at a given source location.
> +
> +Usage: log-printf LINESPEC -- PRINTF-ARGS
> +
> +LINESPEC is any linespec of the forms accepted by `break'.
> +A plain `--' separates the linespec from the remaining arguments,
> +which are of the form accepted by the `printf' command.
> +
> +This command installs a special breakpoint to do its work.
> +You can disable the effects of this command by disabling or deleting
> +that breakpoint."""
> +
> +    def __init__(self):
> +        super(_Logprintf, self).__init__('log-printf',
> +                                         gdb.COMMAND_BREAKPOINTS,
> +                                         # Arguable.
> +                                         gdb.COMPLETE_SYMBOL)
> +
> +    def invoke(self, arg, from_tty):
> +        dashes = arg.find(' -- ')
> +        if dashes == -1:
> +            raise gdb.GdbError("could not find ` -- '")
> +        linespec = arg[0 : dashes]
> +        format = arg[(dashes + 4):]
> +        _Logbp(linespec, format)
> +
> +_Logprintf()
> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index bfad002..4551416 100644
> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -28,6 +28,8 @@
>  #include "observer.h"
>  #include "cli/cli-script.h"
>  #include "ada-lang.h"
> +#include "arch-utils.h"
> +#include "language.h"
>
>  static PyTypeObject breakpoint_object_type;
>
> @@ -586,10 +588,9 @@ bppy_get_ignore_count (PyObject *self, void *closure)
>  }
>
>  /* Python function to create a new breakpoint.  */
> -static PyObject *
> -bppy_new (PyTypeObject *subtype, PyObject *args, PyObject *kwargs)
> +static int
> +bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>  {
> -  PyObject *result;
>   static char *keywords[] = { "spec", "type", "wp_class", "internal", NULL };
>   char *spec;
>   int type = bp_breakpoint;
> @@ -600,19 +601,16 @@ bppy_new (PyTypeObject *subtype, PyObject *args, PyObject *kwargs)
>
>   if (! PyArg_ParseTupleAndKeywords (args, kwargs, "s|iiO", keywords,
>                                     &spec, &type, &access_type, &internal))
> -    return NULL;
> +    return -1;
>
>   if (internal)
>     {
>       internal_bp = PyObject_IsTrue (internal);
>       if (internal_bp == -1)
> -       return NULL;
> +       return -1;
>     }
>
> -  result = subtype->tp_alloc (subtype, 0);
> -  if (! result)
> -    return NULL;
> -  bppy_pending_object = (breakpoint_object *) result;
> +  bppy_pending_object = (breakpoint_object *) self;
>   bppy_pending_object->number = -1;
>   bppy_pending_object->bp = NULL;
>
> @@ -649,14 +647,14 @@ bppy_new (PyTypeObject *subtype, PyObject *args, PyObject *kwargs)
>     }
>   if (except.reason < 0)
>     {
> -      subtype->tp_free (result);
> -      return PyErr_Format (except.reason == RETURN_QUIT
> -                          ? PyExc_KeyboardInterrupt : PyExc_RuntimeError,
> -                          "%s", except.message);
> +      PyErr_Format (except.reason == RETURN_QUIT
> +                   ? PyExc_KeyboardInterrupt : PyExc_RuntimeError,
> +                   "%s", except.message);
> +      return -1;
>     }
>
> -  BPPY_REQUIRE_VALID ((breakpoint_object *) result);
> -  return result;
> +  BPPY_SET_REQUIRE_VALID ((breakpoint_object *) self);
> +  return 0;
>  }
>
>
> @@ -707,6 +705,47 @@ gdbpy_breakpoints (PyObject *self, PyObject *args)
>   return PyList_AsTuple (list);
>  }
>
> +/* Call the "stop_p" method (if implemented) in the breakpoint class.  If
> +   the method returns True, the inferior  will be stopped at the
> +   breakpoint.  Otherwise the inferior will be allowed to continue.  */
> +
> +int
> +gdbpy_stop_p (struct breakpoint_object *bp_obj)
> +{
> +  int should_stop = 1;
> +  char *method = "stop_p";
> +
> +  PyObject *py_bp = (PyObject *) bp_obj;
> +  struct breakpoint *b = bp_obj->bp;
> +  struct gdbarch *garch = b->gdbarch ? b->gdbarch : get_current_arch ();
> +  struct cleanup *cleanup = ensure_python_env (garch, current_language);
> +
> +  if (PyObject_HasAttrString (py_bp, method))
> +    {
> +      PyObject *result = PyObject_CallMethod (py_bp, method, NULL);
> +
> +      if (result)
> +       {
> +         int evaluate = PyObject_IsTrue (result);
> +
> +         if (evaluate == -1)
> +           gdbpy_print_stack ();
> +
> +         /* If the evaluate function returns False that means the
> +            Python breakpoint wants GDB to continue.  */
> +         if (!evaluate)
> +           should_stop = 0;
> +
> +         Py_DECREF (result);
> +       }
> +      else
> +       gdbpy_print_stack ();
> +    }
> +  do_cleanups (cleanup);
> +
> +  return should_stop;
> +}
> +
>
>
>  /* Event callback functions.  */
> @@ -793,7 +832,6 @@ gdbpy_initialize_breakpoints (void)
>  {
>   int i;
>
> -  breakpoint_object_type.tp_new = bppy_new;
>   if (PyType_Ready (&breakpoint_object_type) < 0)
>     return;
>
> @@ -899,7 +937,7 @@ static PyTypeObject breakpoint_object_type =
>   0,                             /*tp_getattro*/
>   0,                             /*tp_setattro*/
>   0,                             /*tp_as_buffer*/
> -  Py_TPFLAGS_DEFAULT,            /*tp_flags*/
> +  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,  /*tp_flags*/
>   "GDB breakpoint object",       /* tp_doc */
>   0,                             /* tp_traverse */
>   0,                             /* tp_clear */
> @@ -909,5 +947,13 @@ static PyTypeObject breakpoint_object_type =
>   0,                             /* tp_iternext */
>   breakpoint_object_methods,     /* tp_methods */
>   0,                             /* tp_members */
> -  breakpoint_object_getset       /* tp_getset */
> +  breakpoint_object_getset,      /* tp_getset */
> +  0,                             /* tp_base */
> +  0,                             /* tp_dict */
> +  0,                             /* tp_descr_get */
> +  0,                             /* tp_descr_set */
> +  0,                             /* tp_dictoffset */
> +  bppy_init,                     /* tp_init */
> +  0,                             /* tp_alloc */
> +  PyType_GenericNew              /* tp_new */
>  };
> diff --git a/gdb/python/python.h b/gdb/python/python.h
> index 58d97fc..801282e 100644
> --- a/gdb/python/python.h
> +++ b/gdb/python/python.h
> @@ -22,6 +22,8 @@
>
>  #include "value.h"
>
> +struct breakpoint_object;
> +
>  extern int gdbpy_global_auto_load;
>
>  extern void finish_python_initialization (void);
> @@ -41,4 +43,6 @@ void preserve_python_values (struct objfile *objfile, htab_t copied_types);
>
>  void load_auto_scripts_for_objfile (struct objfile *objfile);
>
> +int gdbpy_stop_p (struct breakpoint_object *bp_obj);
> +
>  #endif /* GDB_PYTHON_H */
> diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
> index 6b33284..51ea126 100644
> --- a/gdb/testsuite/gdb.python/py-breakpoint.exp
> +++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
> @@ -198,3 +198,76 @@ gdb_py_test_silent_cmd  "python wp1 = gdb.Breakpoint (\"result\", type=gdb.BP_WA
>  gdb_test "info breakpoints" "No breakpoints or watchpoints.*" "Check info breakpoints does not show invisible breakpoints"
>  gdb_test "maint info breakpoints" ".*hw watchpoint.*result.*" "Check maint info breakpoints shows invisible breakpoints"
>  gdb_test "continue" ".*\[Ww\]atchpoint.*result.*Old value = 0.*New value = 25.*" "Test watchpoint write"
> +
> +# Breakpoints that have an evaluation function.
> +
> +# Start with a fresh gdb.
> +clean_restart ${testfile}
> +
> +if ![runto_main] then {
> +    fail "Cannot run to main."
> +    return 0
> +}
> +delete_breakpoints
> +
> +gdb_py_test_multiple "Sub-class a breakpoint" \
> +  "python" "" \
> +  "class bp_eval (gdb.Breakpoint):" "" \
> +  "   inf_i = 0" "" \
> +  "   count = 0" "" \
> +  "   def stop_p (self):" "" \
> +  "      self.count = self.count + 1" "" \
> +  "      self.inf_i = gdb.parse_and_eval(\"i\")" "" \
> +  "      if self.inf_i == 3:" "" \
> +  "        return True" "" \
> +  "      return False" "" \
> +  "end" ""
> +
> +gdb_py_test_multiple "Sub-class a second breakpoint" \
> +  "python" "" \
> +  "class bp_also_eval (gdb.Breakpoint):" "" \
> +  "   count = 0" "" \
> +  "   def stop_p (self):" "" \
> +  "      self.count = self.count + 1" "" \
> +  "      if self.count == 9:" "" \
> +  "        return True" "" \
> +  "      return False" "" \
> +  "end" ""
> +
> +set bp_location2 [gdb_get_line_number "Break at multiply."]
> +set end_location [gdb_get_line_number "Break at end."]
> +gdb_py_test_silent_cmd  "python eval_bp1 = bp_eval(\"$bp_location2\")" "Set breakpoint" 0
> +gdb_py_test_silent_cmd  "python also_eval_bp1 = bp_also_eval(\"$bp_location2\")" "Set breakpoint" 0
> +gdb_py_test_silent_cmd  "python never_eval_bp1 = bp_also_eval(\"$end_location\")" "Set breakpoint" 0
> +gdb_continue_to_breakpoint "Break at multiply." ".*/$srcfile:$bp_location2.*"
> +gdb_test "print i" "3" "Check inferior value matches python accounting"
> +gdb_test "python print eval_bp1.inf_i" "3" "Check python accounting matches inferior"
> +gdb_test "python print also_eval_bp1.count" "4" \
> +    "Check non firing same-location breakpoint eval function was also called at each stop."
> +gdb_test "python print eval_bp1.count" "4" \
> +    "Check non firing same-location breakpoint eval function was also called at each stop."
> +
> +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
> +gdb_test "python print check_eval.count" "0" \
> +    "Test that evaluate function has not been yet executed (ie count = 0)"
> +gdb_continue_to_breakpoint "Break at multiply." ".*/$srcfile:$bp_location2.*"
> +gdb_test "python print check_eval.count" "1" \
> +    "Test that evaluate function is run when location also has normal bp"
> +
> +gdb_py_test_multiple "Sub-class a watchpoint" \
> +  "python" "" \
> +  "class wp_eval (gdb.Breakpoint):" "" \
> +  "   def stop_p (self):" "" \
> +  "      self.result = gdb.parse_and_eval(\"result\")" "" \
> +  "      if self.result == 788:" "" \
> +  "        return True" "" \
> +  "      return False" "" \
> +  "end" ""
> +
> +delete_breakpoints
> +gdb_py_test_silent_cmd  "python wp1 = wp_eval (\"result\", type=gdb.BP_WATCHPOINT, wp_class=gdb.WP_WRITE)" "Set watchpoint" 0
> +gdb_test "continue" ".*\[Ww\]atchpoint.*result.*Old value =.*New value = 788.*" "Test watchpoint write"
> +gdb_test "python print never_eval_bp1.count" "0" \
> +    "Check that this unrelated breakpoints eval function was never called."
>
>

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Phil Muldoon]

Doug Evans <dje@google.com> writes:

>> I believe I have implemented all of the requests from the previous patch
>> discussion.  I eventually renamed eval to stop_p.  Also, recently, Tom
>> wrote a log_printf Python command that uses this feature. I've included
>> it here, along with a few alterations Tom had to make to Breakpoint
>> initialization to make that work.
>>
>> Here it is. What do you think?
>
> Hi.
> Some nits and comments:
>
> - "consistency is good", so if we go with _p for stop_p we need to go
> with _p for all predicates
>   - are we prepared for that?
>   - are there any existing predicates that don't have _p?
>   - does python have an existing convention?
>   [I used stop_p at the time for clarity's sake.  But I think these
> questions need to be asked.]

There are two instances that I can think of where we allow the user
to implement methods that we supply the interface for.  One is the
pretty-printer string, children and hint methods.  The other is the
patch I sent last week for the redesign of parameters.  None of those
use the _p for predicate style.  As far as I can tell (with the express
disclaimer I am don't hack on actual Python code that much), there does
not seem to be a convention.  I'll defer to real Python hackers
here. For my part, I don't have much of an opinion what we call it, or
if we should have a convention; I'll rely on the maintainers being
directive here ;)

> - I didn't see any tests for log-printf
>
> - log.py feels misnamed but since the name isn't exported to the user
> it's not important enough to find a better name
>
> - can printf get an error (e.g. bad memory access) and would one
> necessarily want execution to halt when that happens?
>   - I can imagine wanting to just see an error message and have
> execution continue
>   - OTOH while testing a log-printf I would want execution to stop if
> I misspelled something
>   - we don't have to add the functionality now, but IWBN to at least
> think about if/how we'd provide it
>
> - we probably should document log-printf in the manual

When I edited the original message, I guess I edited out a paragraph I
wanted to leave in! Anyway, I'm not sure we should include log-printf
in the original FSF inclusion, I added it here for a "real world"
case. Just illustrative. I think if we want to include it that is fine,
or Tom can submit it as a separate patch.  I should have probably asked
Tom first ;)  Anyway apologies for the confusion!

> - is the logic for deciding whether to stop correct?
>   E.g. if stop_p says "don't stop" and a condition says "stop" will
> execution continue?  It looks like it, but maybe I'm misunderstanding
> something.

The case of the user having an old-style GDB condition, and an
implementation of a "stop_p" is an odd one. I was inclined to disallow
it, but eventually decided against it.  There will be conflict if stop_p
and condition disagree.  My first thoughts are "stop" should always
trump "don't stop". What do you think?

Cheers

Phil

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Doug Evans]

[apologies for the dup]

On Thu, Feb 24, 2011 at 1:03 AM, Phil Muldoon <pmuldoon@redhat.com> wrote:
> Doug Evans <dje@google.com> writes:
>
> >> I believe I have implemented all of the requests from the previous patch
> >> discussion.  I eventually renamed eval to stop_p.  Also, recently, Tom
> >> wrote a log_printf Python command that uses this feature. I've included
> >> it here, along with a few alterations Tom had to make to Breakpoint
> >> initialization to make that work.
> >>
> >> Here it is. What do you think?
> >
> > Hi.
> > Some nits and comments:
> >
> > - "consistency is good", so if we go with _p for stop_p we need to go
> > with _p for all predicates
> >   - are we prepared for that?
> >   - are there any existing predicates that don't have _p?
> >   - does python have an existing convention?
> >   [I used stop_p at the time for clarity's sake.  But I think these
> > questions need to be asked.]
>
> There are two instances that I can think of where we allow the user
> to implement methods that we supply the interface for.  One is the
> pretty-printer string, children and hint methods.  The other is the
> patch I sent last week for the redesign of parameters.  None of those
> use the _p for predicate style.  As far as I can tell (with the express
> disclaimer I am don't hack on actual Python code that much), there does
> not seem to be a convention.  I'll defer to real Python hackers
> here. For my part, I don't have much of an opinion what we call it, or
> if we should have a convention; I'll rely on the maintainers being
> directive here ;)
>

The only thing I would insist on is consistency (and documentation of it).

> [...] Anyway apologies for the confusion!
>

No worries.

> - is the logic for deciding whether to stop correct?
> >   E.g. if stop_p says "don't stop" and a condition says "stop" will
> > execution continue?  It looks like it, but maybe I'm misunderstanding
> > something.
>
> The case of the user having an old-style GDB condition, and an
> implementation of a "stop_p" is an odd one. I was inclined to disallow
> it, but eventually decided against it.  There will be conflict if stop_p
> and condition disagree.  My first thoughts are "stop" should always
> trump "don't stop". What do you think?


For things like this I like to start slow: "It's easier to relax
restrictions than it is to impose them after the fact."  If it turns out
there's a reasonable use for having both and the first release doesn't
support that, we can easily add the support later (I think!).  But if it
turns out that supporting both causes a lot of
trouble/confusion/wasted-time/whatever, and there's no real need for the
feature anyway, then removing it later will be a lot harder (if not
impossible).

So my vote would be to not support both in the first pass.
It kinda makes intuitive sense too (to me anyway).
i.e. the default implementation of "stop_p" uses the command-line condition,
and if overridden then uses the python-provided addition.
This would mean changes to the u/i, I think, in order to disallow setting a
condition if there is a python stop_p override, and possibly providing a
note in the output of info breakpoints when there is a stop_p override
(question: do these python breakpoints appear in the output of "info
breakpoints?).

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Phil Muldoon]

Doug Evans <dje@google.com> writes:

>> Doug Evans <dje@google.com> writes:
>>
>> >> I believe I have implemented all of the requests from the previous patch
>> >> discussion.  I eventually renamed eval to stop_p.  Also, recently, Tom
>> >> wrote a log_printf Python command that uses this feature. I've included
>> >> it here, along with a few alterations Tom had to make to Breakpoint
>> >> initialization to make that work.
>> >>
>> >> Here it is. What do you think?
>> >
>> > Hi.
>> > Some nits and comments:
>> >
>> > - "consistency is good", so if we go with _p for stop_p we need to go
>> > with _p for all predicates
>> >   - are we prepared for that?
>> >   - are there any existing predicates that don't have _p?
>> >   - does python have an existing convention?
>> >   [I used stop_p at the time for clarity's sake.  But I think these
>> > questions need to be asked.]
>>
>> There are two instances that I can think of where we allow the user
>> to implement methods that we supply the interface for.  One is the
>> pretty-printer string, children and hint methods.  The other is the
>> patch I sent last week for the redesign of parameters.  None of those
>> use the _p for predicate style.  As far as I can tell (with the express
>> disclaimer I am don't hack on actual Python code that much), there does
>> not seem to be a convention.  I'll defer to real Python hackers
>> here. For my part, I don't have much of an opinion what we call it, or
>> if we should have a convention; I'll rely on the maintainers being
>> directive here ;)
>>
>
> The only thing I would insist on is consistency (and documentation of it).

I don't particularly like the stop_p name, or any predicate suffix.
Originally the function was named "eval", but that seemed to stir up
objections.  FWIW, I think given that existing functions don't use the
predicate notation, we should not either, here.  Now, what do we call
the function? ;) I really want to move past this point;  it has been a
productive review in exploring the growing API.  But we still don't have
a convention that everyone agrees on.  I'll defer to the directive
prerogative of the maintainers here


>> - is the logic for deciding whether to stop correct?
>> >   E.g. if stop_p says "don't stop" and a condition says "stop" will
>> > execution continue?  It looks like it, but maybe I'm misunderstanding
>> > something.
>>
>> The case of the user having an old-style GDB condition, and an
>> implementation of a "stop_p" is an odd one. I was inclined to disallow
>> it, but eventually decided against it.  There will be conflict if stop_p
>> and condition disagree.  My first thoughts are "stop" should always
>> trump "don't stop". What do you think?
>
>
> For things like this I like to start slow: "It's easier to relax
> restrictions than it is to impose them after the fact."  If it turns out
> there's a reasonable use for having both and the first release doesn't
> support that, we can easily add the support later (I think!).  But if it
> turns out that supporting both causes a lot of
> trouble/confusion/wasted-time/whatever, and there's no real need for the
> feature anyway, then removing it later will be a lot harder (if not
> impossible).
>
> So my vote would be to not support both in the first pass.

Okay, I will make the condition mutually exclusive.  If one exists in
the CLI then we will not allow the implementation (or rather, bar the
execution) of a function.  I'm not sure on the CLI condition side.  That
would require us to do some grokking of the Python object carried around
in the breakpoint struct.  That would implement some Python specific
code in breakpoint.c. If that is fine, then I'll implement this today.

Cheers,

Phil

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> - "consistency is good", so if we go with _p for stop_p we need to go
Doug> with _p for all predicates
Doug>   - are we prepared for that?
Doug>   - are there any existing predicates that don't have _p?
Doug>   - does python have an existing convention?
Doug>   [I used stop_p at the time for clarity's sake.  But I think these
Doug> questions need to be asked.]

I don't think we should use _p on predicates.  That is not a Python
convention.

I don't think Python actually does have a convention for this kind of
predicate.  I suggest we just pick a name that makes sense.

Doug> - I didn't see any tests for log-printf

I don't think log-printf is ready for submission.  It can either be a
separate patch later, or just not submitted.  I wrote it just to provide
a real-word test of one of the use cases for which this functionality is
intended.

Doug> - is the logic for deciding whether to stop correct?
Doug>   E.g. if stop_p says "don't stop" and a condition says "stop" will
Doug> execution continue?  It looks like it, but maybe I'm misunderstanding
Doug> something.

Phil> The case of the user having an old-style GDB condition, and an
Phil> implementation of a "stop_p" is an odd one. I was inclined to disallow
Phil> it, but eventually decided against it.  There will be conflict if stop_p
Phil> and condition disagree.  My first thoughts are "stop" should always
Phil> trump "don't stop". What do you think?

My view is that the conditions should be separate, and that if either
one indicates "stop", then the breakpoint should stop.

My reason is that the Python method is an implementation detail of some
kind of "stop point" provided by a Python script.  It is not readily
mutable by the user.  On the other hand, the condition is something
explicitly under the user's control.

If we really need to let the Python programmer prevent users from
setting a condition on one of these breakpoints, we can provide a
mechanism for doing that.

Doug> For things like this I like to start slow: "It's easier to relax
Doug> restrictions than it is to impose them after the fact."

Doug> So my vote would be to not support both in the first pass.  It
Doug> kinda makes intuitive sense too (to me anyway).  i.e. the default
Doug> implementation of "stop_p" uses the command-line condition, and if
Doug> overridden then uses the python-provided addition.

These two statements are contradictory.  Or, maybe I didn't understand
one of them.

If we unify stop_p and the user condition now, it will be hard to
separate them later -- because we will have documented that this is how
they work.

I think the most conservative approach is to make it an error for the
user to set a condition on a breakpoint that has a stop_p method, and
vice versa.  That preserves the ability to make a different decision
later.

Tom

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> +#if HAVE_PYTHON
Phil> +#include "python/python.h"
Phil> +#endif

You can include this header without the #if.
It is designed to work properly even if Python support is not enabled.

Phil> +#if HAVE_PYTHON
Phil> +      /* Evaluate Python breakpoints that have a "condition"
Phil> +	 method implemented.  */
Phil> +      if (b->py_bp_object)
Phil> +	bs->stop = gdbpy_stop_p (b->py_bp_object);
Phil> +#endif

I think you should remove the #if here and have a second implementation
of gdbpy_stop_p that just calls internal_error.  The idea here is that
py_bp_object can only be non-NULL if Python support was actually enabled.

Per the other message, just drop the log-printf stuff from the
submission for now.  We can submit it, or not, later on.  (I think we
should still put it in Fedora though.)

The rest of the code bits look good to me.

Tom

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Doug Evans]

On Mon, Mar 7, 2011 at 12:48 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Doug" == Doug Evans <dje@google.com> writes:
>
> Doug> - "consistency is good", so if we go with _p for stop_p we need to go
> Doug> with _p for all predicates
> Doug>   - are we prepared for that?
> Doug>   - are there any existing predicates that don't have _p?
> Doug>   - does python have an existing convention?
> Doug>   [I used stop_p at the time for clarity's sake.  But I think these
> Doug> questions need to be asked.]
>
> I don't think we should use _p on predicates.  That is not a Python
> convention.

"works for me"

> Doug> - is the logic for deciding whether to stop correct?
> Doug>   E.g. if stop_p says "don't stop" and a condition says "stop" will
> Doug> execution continue?  It looks like it, but maybe I'm misunderstanding
> Doug> something.
>
> Phil> The case of the user having an old-style GDB condition, and an
> Phil> implementation of a "stop_p" is an odd one. I was inclined to disallow
> Phil> it, but eventually decided against it.  There will be conflict if stop_p
> Phil> and condition disagree.  My first thoughts are "stop" should always
> Phil> trump "don't stop". What do you think?
>
> My view is that the conditions should be separate, and that if either
> one indicates "stop", then the breakpoint should stop.
>
> My reason is that the Python method is an implementation detail of some
> kind of "stop point" provided by a Python script.  It is not readily
> mutable by the user.  On the other hand, the condition is something
> explicitly under the user's control.

That sounds a bit weird.
The python method is part of an API.
APIs are not implementation details.

> If we really need to let the Python programmer prevent users from
> setting a condition on one of these breakpoints, we can provide a
> mechanism for doing that.

From my point of view it's not about letting the Python programmer
prevent users from ...
It's about us preventing/prohibiting a breakpoint from having both (at
least for now).

> Doug> For things like this I like to start slow: "It's easier to relax
> Doug> restrictions than it is to impose them after the fact."
>
> Doug> So my vote would be to not support both in the first pass.  It
> Doug> kinda makes intuitive sense too (to me anyway).  i.e. the default
> Doug> implementation of "stop_p" uses the command-line condition, and if
> Doug> overridden then uses the python-provided addition.
>
> These two statements are contradictory.  Or, maybe I didn't understand
> one of them.
>
> If we unify stop_p and the user condition now, it will be hard to
> separate them later -- because we will have documented that this is how
> they work.
>
> I think the most conservative approach is to make it an error for the
> user to set a condition on a breakpoint that has a stop_p method, and
> vice versa.  That preserves the ability to make a different decision
> later.

That's what I'd do.  I don't see the contradiction.
[Remember I'm talking about an *intuitive* sense here, not any literal
sense ("literal" as in something I might intend we document).
If my intuitive sense doesn't work for you, you don't have to use it.
:-)  We seem to both agree on the end result.]

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Tom Tromey]

Tom> My reason is that the Python method is an implementation detail of some
Tom> kind of "stop point" provided by a Python script.  It is not readily
Tom> mutable by the user.  On the other hand, the condition is something
Tom> explicitly under the user's control.

Doug> That sounds a bit weird.
Doug> The python method is part of an API.
Doug> APIs are not implementation details.

I think we are using the same words to mean different things.

I was using this from the point of view of writing a gdb extension using
the new feature.  E.g., consider the log-printf code.  The new method is
used by log-printf to do its work.  Here, the method is an
implementation detail of log-printf.

I'm sorry for being unclear.

Tom> I think the most conservative approach is to make it an error for the
Tom> user to set a condition on a breakpoint that has a stop_p method, and
Tom> vice versa.  That preserves the ability to make a different decision
Tom> later.

Doug> That's what I'd do.  I don't see the contradiction.
Doug> [Remember I'm talking about an *intuitive* sense here, not any literal
Doug> sense ("literal" as in something I might intend we document).
Doug> If my intuitive sense doesn't work for you, you don't have to use it.
Doug> :-)  We seem to both agree on the end result.]

I don't really agree, but I think it is less important than getting some
variant of the patch in.

Phil is already implementing this and I think should have a new patch
shortly.

Tom

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Phil Muldoon]

Tom Tromey <tromey@redhat.com> writes:

> Tom> My reason is that the Python method is an implementation detail of some
> Tom> kind of "stop point" provided by a Python script.  It is not readily
> Tom> mutable by the user.  On the other hand, the condition is something
> Tom> explicitly under the user's control.
>
> Doug> That sounds a bit weird.
> Doug> The python method is part of an API.
> Doug> APIs are not implementation details.
>
> I think we are using the same words to mean different things.


Here is the latest patch.  I've added the mutual exclusion to setting
the CLI condition and the Python method.  I renamed the method to
'stop'.  Plus all the other review comments regarding the #HAVE Python
defines . I included some changes from the branch that alter how
breakpoints are initialized (the tests require those changes).

I think I got everything.

WDYT?

Cheers

Phil

--


diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 5bcab87..9736b12 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -72,6 +72,7 @@
 #undef savestring
 
 #include "mi/mi-common.h"
+#include "python/python.h"
 
 /* Arguments to pass as context to some catch command handlers.  */
 #define CATCH_PERMANENT ((void *) (uintptr_t) 0)
@@ -643,6 +644,14 @@ 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);
 	return;
       }
@@ -4070,6 +4079,11 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
       int value_is_zero = 0;
       struct expression *cond;
 
+      /* 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))
 	cond = b->cond_exp;
       else
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index a5eaa72..9ca6c09 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -23099,6 +23099,34 @@ argument defines the class of watchpoint to create, if @var{type} is
 assumed to be a @var{WP_WRITE} class.
 @end defmethod
 
+@defop Operation {gdb.Breakpoint} stop (self)
+The @code{gdb.Breakpoint} class can be sub-classed and, in
+particular, you may choose to implement the @code{stop} method.
+If this method is defined as a sub-class of @code{gdb.Breakpoint},
+it will be called when the inferior stops at any location of a
+breakpoint which instantiates that sub-class.  If the method returns
+@code{True}, the inferior will be stopped at the location of the
+breakpoint, otherwise the inferior will continue.
+
+If there are multiple breakpoints at the same location with a
+@code{stop} method, each one will be called regardless of the
+return status of the previous.  This ensures that all @code{stop}
+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.
+
+Example @code{stop} implementation:
+
+@smallexample
+class MyBreakpoint (gdb.Breakpoint):
+      def stop (self):
+        inf_val = gdb.parse_and_eval("foo")
+        if inf_val == 3:
+          return True
+        return False
+@end smallexample
+@end defop
+
 The available watchpoint types represented by constants are defined in the
 @code{gdb} module:
 
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index bfad002..6e5db8f 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -28,6 +28,8 @@
 #include "observer.h"
 #include "cli/cli-script.h"
 #include "ada-lang.h"
+#include "arch-utils.h"
+#include "language.h"
 
 static PyTypeObject breakpoint_object_type;
 
@@ -38,6 +40,9 @@ static int bppy_live;
    constructor and the breakpoint-created hook function.  */
 static breakpoint_object *bppy_pending_object;
 
+/* Function that is called when a Python condition is evaluated.  */
+char *stop_func = "stop";
+
 struct breakpoint_object
 {
   PyObject_HEAD
@@ -586,10 +591,9 @@ bppy_get_ignore_count (PyObject *self, void *closure)
 }
 
 /* Python function to create a new breakpoint.  */
-static PyObject *
-bppy_new (PyTypeObject *subtype, PyObject *args, PyObject *kwargs)
+static int
+bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 {
-  PyObject *result;
   static char *keywords[] = { "spec", "type", "wp_class", "internal", NULL };
   char *spec;
   int type = bp_breakpoint;
@@ -600,19 +604,16 @@ bppy_new (PyTypeObject *subtype, PyObject *args, PyObject *kwargs)
 
   if (! PyArg_ParseTupleAndKeywords (args, kwargs, "s|iiO", keywords,
 				     &spec, &type, &access_type, &internal))
-    return NULL;
+    return -1;
 
   if (internal)
     {
       internal_bp = PyObject_IsTrue (internal);
       if (internal_bp == -1)
-	return NULL;
+	return -1;
     }
 
-  result = subtype->tp_alloc (subtype, 0);
-  if (! result)
-    return NULL;
-  bppy_pending_object = (breakpoint_object *) result;
+  bppy_pending_object = (breakpoint_object *) self;
   bppy_pending_object->number = -1;
   bppy_pending_object->bp = NULL;
   
@@ -649,14 +650,14 @@ bppy_new (PyTypeObject *subtype, PyObject *args, PyObject *kwargs)
     }
   if (except.reason < 0)
     {
-      subtype->tp_free (result);
-      return PyErr_Format (except.reason == RETURN_QUIT
-			   ? PyExc_KeyboardInterrupt : PyExc_RuntimeError,
-			   "%s", except.message);
+      PyErr_Format (except.reason == RETURN_QUIT
+		    ? PyExc_KeyboardInterrupt : PyExc_RuntimeError,
+		    "%s", except.message);
+      return -1;
     }
 
-  BPPY_REQUIRE_VALID ((breakpoint_object *) result);
-  return result;
+  BPPY_SET_REQUIRE_VALID ((breakpoint_object *) self);
+  return 0;
 }
 
 \f
@@ -707,6 +708,69 @@ gdbpy_breakpoints (PyObject *self, PyObject *args)
   return PyList_AsTuple (list);
 }
 
+/* Call the "stop" method (if implemented) in the breakpoint
+   class.  If the method returns True, the inferior  will be
+   stopped at the breakpoint.  Otherwise the inferior will be
+   allowed to continue.  */
+
+int
+gdbpy_should_stop (struct breakpoint_object *bp_obj)
+{
+  int stop = 1;
+
+  PyObject *py_bp = (PyObject *) bp_obj;
+  struct breakpoint *b = bp_obj->bp;
+  struct gdbarch *garch = b->gdbarch ? b->gdbarch : get_current_arch ();
+  struct cleanup *cleanup = ensure_python_env (garch, current_language);
+
+  if (PyObject_HasAttrString (py_bp, stop_func))
+    {
+      PyObject *result = PyObject_CallMethod (py_bp, stop_func, NULL);
+
+      if (result)
+	{
+	  int evaluate = PyObject_IsTrue (result);
+
+	  if (evaluate == -1)
+	    gdbpy_print_stack ();
+
+	  /* If the "stop" function returns False that means
+	     the Python breakpoint wants GDB to continue.  */
+	  if (! evaluate)
+	    stop = 0;
+
+	  Py_DECREF (result);
+	}
+      else
+	gdbpy_print_stack ();
+    }
+  do_cleanups (cleanup);
+
+  return stop;
+}
+
+/* Checks if the  "stop" method exists in this breakpoint.
+   Used by condition_command to ensure mutual exclusion of breakpoint
+   conditions.  */
+
+int
+gdbpy_breakpoint_has_py_cond (struct breakpoint_object *bp_obj)
+{
+  int has_func = 0;
+  PyObject *py_bp = (PyObject *) bp_obj;
+  struct gdbarch *garch = bp_obj->bp->gdbarch ? bp_obj->bp->gdbarch :
+    get_current_arch ();
+  struct cleanup *cleanup = ensure_python_env (garch, current_language);
+  
+  if (py_bp == NULL)
+    return 0;
+
+  has_func = PyObject_HasAttrString (py_bp, stop_func);
+  do_cleanups (cleanup);
+
+  return has_func;
+}
+
 \f
 
 /* Event callback functions.  */
@@ -793,7 +857,6 @@ gdbpy_initialize_breakpoints (void)
 {
   int i;
 
-  breakpoint_object_type.tp_new = bppy_new;
   if (PyType_Ready (&breakpoint_object_type) < 0)
     return;
 
@@ -828,6 +891,36 @@ gdbpy_initialize_breakpoints (void)
 
 \f
 
+/* 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 },
@@ -897,9 +990,9 @@ static PyTypeObject breakpoint_object_type =
   0,				  /*tp_call*/
   0,				  /*tp_str*/
   0,				  /*tp_getattro*/
-  0,				  /*tp_setattro*/
+  (setattrofunc)local_setattro,   /*tp_setattro */
   0,				  /*tp_as_buffer*/
-  Py_TPFLAGS_DEFAULT,		  /*tp_flags*/
+  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,  /*tp_flags*/
   "GDB breakpoint object",	  /* tp_doc */
   0,				  /* tp_traverse */
   0,				  /* tp_clear */
@@ -909,5 +1002,13 @@ static PyTypeObject breakpoint_object_type =
   0,				  /* tp_iternext */
   breakpoint_object_methods,	  /* tp_methods */
   0,				  /* tp_members */
-  breakpoint_object_getset	  /* tp_getset */
+  breakpoint_object_getset,	  /* tp_getset */
+  0,				  /* tp_base */
+  0,				  /* tp_dict */
+  0,				  /* tp_descr_get */
+  0,				  /* tp_descr_set */
+  0,				  /* tp_dictoffset */
+  bppy_init,			  /* tp_init */
+  0,				  /* tp_alloc */
+  PyType_GenericNew		  /* tp_new */
 };
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 3b10d8c..2d7213a 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -30,6 +30,7 @@
 #include "exceptions.h"
 #include "event-loop.h"
 #include "serial.h"
+#include "python.h"
 
 #include <ctype.h>
 
@@ -864,6 +864,22 @@ source_python_script (FILE *stream, const char *file)
 	       _("Python scripting is not supported in this copy of GDB."));
 }
 
+int
+gdbpy_should_stop (struct breakpoint_object *bp_obj)
+{
+  internal_error (__FILE__, __LINE__,
+		  _("gdbpy_should_stop called when Python scripting is  " \
+		    "not supported."));
+}
+
+int
+gdbpy_breakpoint_has_py_cond (struct breakpoint_object *bp_obj)
+{
+  internal_error (__FILE__, __LINE__,
+		  _("gdbpy_breakpoint_has_py_cond called when Python " \
+		    "scripting is not supported."));
+}
+
 #endif /* HAVE_PYTHON */
 
 \f
diff --git a/gdb/python/python.h b/gdb/python/python.h
index 58d97fc..ce0eb35 100644
--- a/gdb/python/python.h
+++ b/gdb/python/python.h
@@ -22,6 +22,8 @@
 
 #include "value.h"
 
+struct breakpoint_object;
+
 extern int gdbpy_global_auto_load;
 
 extern void finish_python_initialization (void);
@@ -41,4 +43,8 @@ void preserve_python_values (struct objfile *objfile, htab_t copied_types);
 
 void load_auto_scripts_for_objfile (struct objfile *objfile);
 
+int gdbpy_should_stop (struct breakpoint_object *bp_obj);
+
+int gdbpy_breakpoint_has_py_cond (struct breakpoint_object *bp_obj);
+
 #endif /* GDB_PYTHON_H */
diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
index 8f58181..f0a83f1 100644
--- a/gdb/testsuite/gdb.python/py-breakpoint.exp
+++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
@@ -195,3 +195,101 @@ gdb_py_test_silent_cmd  "python wp1 = gdb.Breakpoint (\"result\", type=gdb.BP_WA
 gdb_test "info breakpoints" "No breakpoints or watchpoints.*" "Check info breakpoints does not show invisible breakpoints"
 gdb_test "maint info breakpoints" ".*watchpoint.*result.*" "Check maint info breakpoints shows invisible breakpoints"
 gdb_test "continue" ".*\[Ww\]atchpoint.*result.*Old value = 0.*New value = 25.*" "Test watchpoint write"
+
+# Breakpoints that have an evaluation function.
+
+# Start with a fresh gdb.
+clean_restart ${testfile}
+
+if ![runto_main] then {
+    fail "Cannot run to main."
+    return 0
+}
+delete_breakpoints
+
+gdb_py_test_multiple "Sub-class a breakpoint" \
+  "python" "" \
+  "class bp_eval (gdb.Breakpoint):" "" \
+  "   inf_i = 0" "" \
+  "   count = 0" "" \
+  "   def stop (self):" "" \
+  "      self.count = self.count + 1" "" \
+  "      self.inf_i = gdb.parse_and_eval(\"i\")" "" \
+  "      if self.inf_i == 3:" "" \
+  "        return True" "" \
+  "      return False" "" \
+  "end" ""
+
+gdb_py_test_multiple "Sub-class a second breakpoint" \
+  "python" "" \
+  "class bp_also_eval (gdb.Breakpoint):" "" \
+  "   count = 0" "" \
+  "   def stop (self):" "" \
+  "      self.count = self.count + 1" "" \
+  "      if self.count == 9:" "" \
+  "        return True" "" \
+  "      return False" "" \
+  "end" ""
+
+gdb_py_test_multiple "Sub-class a third breakpoint" \
+  "python" "" \
+  "class basic (gdb.Breakpoint):" "" \
+  "   count = 0" "" \
+  "end" ""
+
+set bp_location2 [gdb_get_line_number "Break at multiply."]
+set end_location [gdb_get_line_number "Break at end."]
+gdb_py_test_silent_cmd  "python eval_bp1 = bp_eval(\"$bp_location2\")" "Set breakpoint" 0
+gdb_py_test_silent_cmd  "python also_eval_bp1 = bp_also_eval(\"$bp_location2\")" "Set breakpoint" 0
+gdb_py_test_silent_cmd  "python never_eval_bp1 = bp_also_eval(\"$end_location\")" "Set breakpoint" 0
+gdb_continue_to_breakpoint "Break at multiply." ".*/$srcfile:$bp_location2.*"
+gdb_test "print i" "3" "Check inferior value matches python accounting"
+gdb_test "python print eval_bp1.inf_i" "3" "Check python accounting matches inferior"
+gdb_test "python print also_eval_bp1.count" "4" \
+    "Check non firing same-location breakpoint eval function was also called at each stop."
+gdb_test "python print eval_bp1.count" "4" \
+    "Check non firing same-location breakpoint eval function was also called at each stop."
+
+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" \
+    "has defined stop"
+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_multiple "Construct an eval function" \
+  "python" "" \
+  "def stop_func ():" "" \
+  "   return True" "" \
+  "end" ""
+
+gdb_test "python eval_bp2.stop = stop_func"  \
+    "RuntimeError: Cannot set 'stop' method.*" \
+    "Assign stop function to a breakpoint that has a condition"
+
+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
+gdb_test "python print check_eval.count" "0" \
+    "Test that evaluate function has not been yet executed (ie count = 0)"
+gdb_continue_to_breakpoint "Break at multiply." ".*/$srcfile:$bp_location2.*"
+gdb_test "python print check_eval.count" "1" \
+    "Test that evaluate function is run when location also has normal bp"
+
+gdb_py_test_multiple "Sub-class a watchpoint" \
+  "python" "" \
+  "class wp_eval (gdb.Breakpoint):" "" \
+  "   def stop (self):" "" \
+  "      self.result = gdb.parse_and_eval(\"result\")" "" \
+  "      if self.result == 788:" "" \
+  "        return True" "" \
+  "      return False" "" \
+  "end" ""
+
+delete_breakpoints
+gdb_py_test_silent_cmd  "python wp1 = wp_eval (\"result\", type=gdb.BP_WATCHPOINT, wp_class=gdb.WP_WRITE)" "Set watchpoint" 0
+gdb_test "continue" ".*\[Ww\]atchpoint.*result.*Old value =.*New value = 788.*" "Test watchpoint write"
+gdb_test "python print never_eval_bp1.count" "0" \
+    "Check that this unrelated breakpoints eval function was never called."

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> Here is the latest patch.  I've added the mutual exclusion to setting
Phil> the CLI condition and the Python method.  I renamed the method to
Phil> 'stop'.  Plus all the other review comments regarding the #HAVE Python
Phil> defines . I included some changes from the branch that alter how
Phil> breakpoints are initialized (the tests require those changes).

Thanks.

Phil> +	  error (_("Cannot set a condition where a Python 'stop' " \

No backslash.

Phil> +If this method is defined as a sub-class of @code{gdb.Breakpoint},
Phil> +it will be called when the inferior stops at any location of a
Phil> +breakpoint which instantiates that sub-class.  If the method returns
Phil> +@code{True}, the inferior will be stopped at the location of the
Phil> +breakpoint, otherwise the inferior will continue.

The repeated use of "stops" here makes it read a little strangely.  I
think it should use "reaches" first, like "... when the inferior reaches
any location of a ...".

Phil> +/* Function that is called when a Python condition is evaluated.  */
Phil> +char *stop_func = "stop";

`static const'

Phil> diff --git a/gdb/python/python.c b/gdb/python/python.c
Phil> index 3b10d8c..2d7213a 100644
Phil> --- a/gdb/python/python.c
Phil> +++ b/gdb/python/python.c
Phil> @@ -30,6 +30,7 @@
Phil>  #include "exceptions.h"
Phil>  #include "event-loop.h"
Phil>  #include "serial.h"
Phil> +#include "python.h"

You should remove the other inclusion of python.h.

Ok with the above changes.  Thanks again.

Tom

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> Here is the latest patch.  I've added the mutual exclusion to setting
Phil> the CLI condition and the Python method.  I renamed the method to
Phil> 'stop'.  Plus all the other review comments regarding the #HAVE Python
Phil> defines . I included some changes from the branch that alter how
Phil> breakpoints are initialized (the tests require those changes).

I forgot to mention -- I think this should have a NEWS entry.

Tom

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Doug Evans]

On Fri, Mar 11, 2011 at 9:43 AM, Tom Tromey <tromey@redhat.com> wrote:
> Phil> +/* Function that is called when a Python condition is evaluated.  */
> Phil> +char *stop_func = "stop";
>
> `static const'

Better, static const char stop_func[] = "stop";

Or static const char* const stop_func.

[Assuming the code that uses this doesn't cough, sigh.]

Plus blank line between function comment and definition please.
[Your very good at catching all other style issues ...]

+/* 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)

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Phil Muldoon]

Doug Evans <dje@google.com> writes:

> On Fri, Mar 11, 2011 at 9:43 AM, Tom Tromey <tromey@redhat.com> wrote:
>> Phil> +/* Function that is called when a Python condition is evaluated.  */
>> Phil> +char *stop_func = "stop";
>>
>> `static const'
>
> Better, static const char stop_func[] = "stop";
>
> Or static const char* const stop_func.


I remember a bug about this in 2.5 that did accept const definitions
but would later die.  (And a patch was checked in.  Can't remember by
who though).  Anyway, Python 2.6 rejects all of the above with:

../../archer/gdb/python/py-breakpoint.c:733:7: error: passing argument 2 of  PyObject_CallMethodâ discards qualifiers from pointer target type
/usr/include/python2.7/abstract.h:340:29: note: expected char * but argument is of type const char *

Cheers

Phil

> [Assuming the code that uses this doesn't cough, sigh.]
>
> Plus blank line between function comment and definition please.
> [Your very good at catching all other style issues ...]
>
> +/* 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)

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> I remember a bug about this in 2.5 that did accept const definitions
Phil> but would later die.  (And a patch was checked in.  Can't remember by
Phil> who though).  Anyway, Python 2.6 rejects all of the above with:

Thanks, I never remember that.  It is ok without that const.

Could you file a bug against Python, like Jan suggested?

Tom

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Phil Muldoon]

Tom Tromey <tromey@redhat.com> writes:

>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
>
> Phil> I remember a bug about this in 2.5 that did accept const definitions
> Phil> but would later die.  (And a patch was checked in.  Can't remember by
> Phil> who though).  Anyway, Python 2.6 rejects all of the above with:
>
> Thanks, I never remember that.  It is ok without that const.
>
> Could you file a bug against Python, like Jan suggested?
>

Sure.  And I have committed the code and doc patches per previous
approval.  Thanks.

Should I also write something on how Parameters now have callback functions too?

Eli, Here is a NEWS patch.  

Cheers,

Phil
--

diff --git a/gdb/NEWS b/gdb/NEWS
index fb36383..826bad3 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -38,6 +38,10 @@
 
 * Python scripting
 
+  ** Breakpoints can now be sub-classed in Python, and in particular
+     you may implement a 'stop' function that is executed each time
+     the inferior reaches that breakpoint.   
+
   ** New function gdb.lookup_global_symbol looks up a global symbol.
 
   ** GDB values in Python are now callable if the value represents a

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> Should I also write something on how Parameters now have callback
Phil> functions too?

You mean for NEWS?  Yes, that would be good.  I'd like all Python API
additions to get a little blurb there.

Tom

Re: [patch] [python] Implement stop_p for gdb.Breakpoint

[Eli Zaretskii]

> From: Phil Muldoon <pmuldoon@redhat.com>
> Cc: Doug Evans <dje@google.com>, gdb-patches@sourceware.org
> Date: Mon, 14 Mar 2011 17:38:56 +0000
> 
> Eli, Here is a NEWS patch.  

It is fine, thanks.