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

[Doug Evans <dje at google dot 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."
>
>