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

[Doug Evans <dje@google.com>]

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."
>
>