Re: [RFC] Python Finish Breakpoints

[Phil Muldoon <pmuldoon at redhat dot com>]

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