Re: [RFC] Python Finish Breakpoints

[Phil Muldoon <pmuldoon@redhat.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