[RFC] Python Finish Breakpoints
Phil Muldoon
pmuldoon@redhat.com
Wed May 11 10:31:00 GMT 2011
Kevin Pouget <kevin.pouget@gmail.com> writes:
> Any feedback ... ?
Apologies, catching up on email after vacation.
>> I would like to discuss with you guys a new Python interface for
>> breakpoint handling. Based on the `finish' command, I prepared a
>> Python class which allows to catch the return of a given frame.
>> Basically, the motivation behind this class is to allow Python script
>> to wrap inferior function calls:
>>
>> with a code like
>> int do_something(int *a)
>> {
>> *a += 5;
>> sleep(a);
>> return 10;
>> }
>> which may take a few seconds to execute, there was no way to know the
>> updated value of `a' and the return value (`gdb.execute("finish")'
>> could do that, but a Ctrl^C during the `sleep' would have screwed up
>> your results).
The idea looks good.
>> there is one problem behind this function, I had to change the code:
>>
>> +++ b/gdb/infrun.c
>> @@ -5826,7 +5826,7 @@ normal_stop (void)
>> /* Save the function value return registers, if we care.
>> We might be about to restore their previous contents. */
>> - if (inferior_thread ()->control.proceed_to_finish)
>> + /*if (inferior_thread ()->control.proceed_to_finish)*/
>> ...
>> stop_registers = regcache_dup (get_current_regcache ());
>>
>> to correctly set `stop_registers', but I don't really know the
>> implication of this modification ...
I don't think you want to universally modify this condition (I am not
sure of the implications either, maybe Pedro will have some more
in-depth info). Anyway given this case, I would create a function
called something like "gdbpy_is_finish_bp" in python.c and add that to
the condition makeup.
> @@ -279,6 +279,7 @@ SUBDIR_PYTHON_OBS = \
> py-block.o \
> py-bpevent.o \
> py-breakpoint.o \
> + py-finishbreakpoint.o \
> py-cmd.o \
> py-continueevent.o \
> py-event.o \
This is a nit I have personally, but you can put the .o file in the
correct alphabetical order?
> @@ -309,6 +310,7 @@ SUBDIR_PYTHON_SRCS = \
> python/py-block.c \
> python/py-bpevent.c \
> python/py-breakpoint.c \
> + python/py-finishbreakpoint.c \
> python/py-cmd.c \
> python/py-continueevent.c \
> python/py-event.c \
Ditto, see above.
> @@ -2038,6 +2040,10 @@ py-breakpoint.o: $(srcdir)/python/py-breakpoint.c
> $(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-breakpoint.c
> $(POSTCOMPILE)
>
> +py-finishbreakpoint.o: $(srcdir)/python/py-finishbreakpoint.c
> + $(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-finishbreakpoint.c
> + $(POSTCOMPILE)
> +
Ditto.
> py-cmd.o: $(srcdir)/python/py-cmd.c
> $(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-cmd.c
> $(POSTCOMPILE)
> +void
> create_breakpoint_sal (struct gdbarch *gdbarch,
> struct symtabs_and_lines sals, char *addr_string,
> char *cond_string,
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index 7a9c2d4..a003651 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -986,6 +986,16 @@ extern int create_breakpoint (struct gdbarch
> *gdbarch, char *arg,
> int enabled,
> int internal);
>
> +extern void create_breakpoint_sal (struct gdbarch *gdbarch,
> + struct symtabs_and_lines sals,
> + char *addr_string,
> + char *cond_string,
> + enum bptype type, enum bpdisp disposition,
> + int thread, int task, int ignore_count,
> + struct breakpoint_ops *ops, int from_tty,
> + int enabled, int internal,
> + int display_canonical);
I'm not sure we should be exposing this function (create_breakpoint_sal)
on a global scope, though I have no particular issue with it.
> +extern struct value *get_return_value (struct type *func_type,
> + struct type *value_type);
> +
> /* Address at which inferior stopped. */
This patch context is not wide enough to know, but I this function I
think should be placed next to the corresponding print_ function.
> - if (inferior_thread ()->control.proceed_to_finish)
> + /*if (inferior_thread ()->control.proceed_to_finish)*/
> {
> /* This should not be necessary. */
> if (stop_registers)
See above for my comments on this.
> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index 9c33848..db2c411 100644
> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -17,6 +17,8 @@
> You should have received a copy of the GNU General Public License
> along with this program. If not, see <http://www.gnu.org/licenses/>. */
>
> +
> +
Spurious newlines.
> /* This is used to initialize various gdb.bp_* constants. */
> struct pybp_code
> {
> @@ -806,21 +773,25 @@ gdbpy_breakpoint_created (struct breakpoint *bp)
> }
> else
> newbp = PyObject_New (breakpoint_object, &breakpoint_object_type);
> - if (newbp)
> - {
> - newbp->number = bp->number;
> - newbp->bp = bp;
> - newbp->bp->py_bp_object = newbp;
> - Py_INCREF (newbp);
> - ++bppy_live;
> - }
> - else
> - {
> - PyErr_SetString (PyExc_RuntimeError,
> - _("Error while creating breakpoint from GDB."));
> - gdbpy_print_stack ();
> - }
> +
> + if (!newbp)
> + goto fail;
> +
> + newbp->number = bp->number;
> + newbp->bp = bp;
> + newbp->bp->py_bp_object = newbp;
> +
> + Py_INCREF (newbp);
> + ++bppy_live;
> +
> + goto success;
> +
> +fail:
> + PyErr_SetString (PyExc_RuntimeError,
> + _("Error while creating breakpoint from GDB."));
> + gdbpy_print_stack ();
>
> +success:
> PyGILState_Release (state);
> }
I'm not adverse to this change, but the new breakpoint initialization
logic does not seem to need to be rewritten in the context of this
patch? If this is just a change you feel needed to be made, I'd send it
as a separate patch. That's just my opinion, the actual maintainers
might not care. ;)
> -static PyTypeObject breakpoint_object_type =
> +PyTypeObject breakpoint_object_type =
> {
> PyObject_HEAD_INIT (NULL)
> 0, /*ob_size*/
> "gdb.Breakpoint", /*tp_name*/
> sizeof (breakpoint_object), /*tp_basicsize*/
> 0, /*tp_itemsize*/
> - 0, /*tp_dealloc*/
> + 0, /*tp_dealloc*/
Spurious change.
> 0, /*tp_print*/
> 0, /*tp_getattr*/
> 0, /*tp_setattr*/
> @@ -1008,7 +979,7 @@ static PyTypeObject breakpoint_object_type =
> 0, /* tp_dict */
> 0, /* tp_descr_get */
> 0, /* tp_descr_set */
> - 0, /* tp_dictoffset */
> + 0, /* tp_dictoffset */
Ditto (Unless you are correcting indention, which is difficult to see in
a patch context).
> +++ b/gdb/python/py-breakpoint.h
> @@ -0,0 +1,61 @@
> +/* Python interface to breakpoints
> +
> + Copyright (C) 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
> +
> + This file is part of GDB.
> +
> + 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/>. */
> +
> +#ifndef GDB_PY_BREAKPOINT_H
> +#define GDB_PY_BREAKPOINT_H
> +
> +/* Require that BREAKPOINT be a valid breakpoint ID; throw a Python
> + exception if it is invalid. */
> +#define BPPY_REQUIRE_VALID(Breakpoint) \
> + do { \
> + if ((Breakpoint)->bp == NULL) \
> + return PyErr_Format (PyExc_RuntimeError, \
> + _("Breakpoint %d is invalid."), \
> + (Breakpoint)->number); \
> + } while (0)
> +
> +/* Require that BREAKPOINT be a valid breakpoint ID; throw a Python
> + exception if it is invalid. This macro is for use in setter functions. */
> +#define BPPY_SET_REQUIRE_VALID(Breakpoint) \
> + do { \
> + if ((Breakpoint)->bp == NULL) \
> + { \
> + PyErr_Format (PyExc_RuntimeError, _("Breakpoint %d is invalid."), \
> + (Breakpoint)->number); \
> + return -1; \
> + } \
> + } while (0)
> +
> +struct breakpoint_object
> +{
> + PyObject_HEAD
> +
> + /* The breakpoint number according to gdb. */
> + int number;
> +
> + /* The gdb breakpoint object, or NULL if the breakpoint has been
> + deleted. */
> + struct breakpoint *bp;
> +};
> +
> +/* Variables used to pass information between the Breakpoint
> + constructor and the breakpoint-created hook function. */
> +extern breakpoint_object *bppy_pending_object;
> +
> +#endif /* GDB_PY_BREAKPOINT_H */
I'm not sure on whether we should be creating header files for
individual Python objects. Normally, depending on the scope/context of
the exported functions and macros we place them in
python/python-internal.h. I'll defer this change to Tom's wisdom.
> +/* Called when GDB notices that the finish breakpoint BP_OBJ is out of
> + the current callstack. If BP_OBJ has the attribute OUT_OF_SCOPED and
> + its value is FALSE, trigger the method OUT_OF_SCOPE and set the flag to
> + TRUE. */
Two spaces after . in the comment.
> +bpfinishpy_detect_out_scope_cb (struct breakpoint *b, void *args)
> +{
> + struct breakpoint *bp_stopped = (struct breakpoint *) args;
> + PyObject *py_bp = (PyObject *) b->py_bp_object;
> + PyGILState_STATE state;
> +
> + /* Prevent python SEGFAULT because of missing thread state. */
> + state = PyGILState_Ensure();
There is a specialized cleanup function that does this for you:
For example:
cleanup = ensure_python_env (get_current_arch (), current_language);
Make sure you get the arch from the breakpoint if applicable. Then just
call do_cleanups when done. This ensure several internal GDB settings
are saved and restored, as well as the GIL.
> + PyGILState_Release (state);
do_cleanups (cleanup). Also make sure any local failure goto branches
do this too.
> + return;
> +
> + Py_INCREF (&finish_breakpoint_object_type);
> + PyModule_AddObject (gdb_module, "FinishBreakpoint",
> + (PyObject *) &finish_breakpoint_object_type);
> +
> + observer_attach_normal_stop (bpfinishpy_handle_stop);
> +}
> +
> +static PyGetSetDef finish_breakpoint_object_getset[] = {
> + { "out_of_scoped", bpfinishpy_get_outofscoped, bpfinishpy_set_outofscoped,
Sounds weird, should it be "out_of_scope"?
>
> -static struct frame_info *
> -frame_object_to_frame_info (frame_object *frame_obj)
> +struct frame_info *
> +frame_object_to_frame_info (PyObject *obj)
> {
> + frame_object *frame_obj = (frame_object *) obj;
> struct frame_info *frame;
>
> frame = frame_find_by_id (frame_obj->frame_id);
> @@ -103,7 +104,7 @@ frapy_is_valid (PyObject *self, PyObject *args)
> {
> struct frame_info *frame;
>
> - frame = frame_object_to_frame_info ((frame_object *) self);
> + frame = frame_object_to_frame_info (self);
> if (frame == NULL)
> Py_RETURN_FALSE;
>
> @@ -124,7 +125,7 @@ frapy_name (PyObject *self, PyObject *args)
>
> TRY_CATCH (except, RETURN_MASK_ALL)
> {
> - FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + FRAPY_REQUIRE_VALID (self, frame);
>
> find_frame_funname (frame, &name, &lang, NULL);
> }
> @@ -153,7 +154,7 @@ frapy_type (PyObject *self, PyObject *args)
>
> TRY_CATCH (except, RETURN_MASK_ALL)
> {
> - FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + FRAPY_REQUIRE_VALID (self, frame);
>
> type = get_frame_type (frame);
> }
> @@ -174,7 +175,7 @@ frapy_unwind_stop_reason (PyObject *self, PyObject *args)
>
> TRY_CATCH (except, RETURN_MASK_ALL)
> {
> - FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + FRAPY_REQUIRE_VALID (self, frame);
> }
> GDB_PY_HANDLE_EXCEPTION (except);
>
> @@ -195,7 +196,7 @@ frapy_pc (PyObject *self, PyObject *args)
>
> TRY_CATCH (except, RETURN_MASK_ALL)
> {
> - FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + FRAPY_REQUIRE_VALID (self, frame);
>
> pc = get_frame_pc (frame);
> }
> @@ -216,7 +217,7 @@ frapy_block (PyObject *self, PyObject *args)
>
> TRY_CATCH (except, RETURN_MASK_ALL)
> {
> - FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + FRAPY_REQUIRE_VALID (self, frame);
> block = get_frame_block (frame, NULL);
> }
> GDB_PY_HANDLE_EXCEPTION (except);
> @@ -257,7 +258,7 @@ frapy_function (PyObject *self, PyObject *args)
>
> TRY_CATCH (except, RETURN_MASK_ALL)
> {
> - FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + FRAPY_REQUIRE_VALID (self, frame);
>
> sym = find_pc_function (get_frame_address_in_block (frame));
> }
> @@ -319,7 +320,7 @@ frapy_older (PyObject *self, PyObject *args)
>
> TRY_CATCH (except, RETURN_MASK_ALL)
> {
> - FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + FRAPY_REQUIRE_VALID (self, frame);
>
> prev = get_prev_frame (frame);
> if (prev)
> @@ -348,7 +349,7 @@ frapy_newer (PyObject *self, PyObject *args)
>
> TRY_CATCH (except, RETURN_MASK_ALL)
> {
> - FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + FRAPY_REQUIRE_VALID (self, frame);
>
> next = get_next_frame (frame);
> if (next)
> @@ -377,7 +378,7 @@ frapy_find_sal (PyObject *self, PyObject *args)
>
> TRY_CATCH (except, RETURN_MASK_ALL)
> {
> - FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + FRAPY_REQUIRE_VALID (self, frame);
>
> find_frame_sal (frame, &sal);
> sal_obj = symtab_and_line_to_sal_object (sal);
> @@ -433,7 +434,7 @@ frapy_read_var (PyObject *self, PyObject *args)
>
> TRY_CATCH (except, RETURN_MASK_ALL)
> {
> - FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + FRAPY_REQUIRE_VALID (self, frame);
>
> if (!block)
> block = get_frame_block (frame, NULL);
> @@ -461,7 +462,7 @@ frapy_read_var (PyObject *self, PyObject *args)
>
> TRY_CATCH (except, RETURN_MASK_ALL)
> {
> - FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + FRAPY_REQUIRE_VALID (self, frame);
>
> val = read_var_value (var, frame);
> }
> @@ -484,12 +485,11 @@ static PyObject *
> frapy_select (PyObject *self, PyObject *args)
> {
> struct frame_info *fi;
> - frame_object *frame = (frame_object *) self;
> volatile struct gdb_exception except;
>
> TRY_CATCH (except, RETURN_MASK_ALL)
> {
> - FRAPY_REQUIRE_VALID (frame, fi);
> + FRAPY_REQUIRE_VALID (self, fi);
>
> select_frame (fi);
> }
I'm not sure the above is needed for the patch? If it is a cleanup,
somewhat like the case above I would just send it as a desperate patch.
> -
> +
> return 0; /* Break at end. */
> }
Spurious.
Overall I like the idea, but I am unsure of the implementation. I don't
want to unnecessarily bike-shed something before the maintainer have a
had a look at it.
Thanks for you hard work in GDB.
Cheers,
Phil
More information about the Gdb-patches
mailing list