Re: [RFC] Python Finish Breakpoints

[Kevin Pouget <kevin dot pouget at gmail dot com>]

thanks for all your comments, all the obvious problems will be fixed
in the next patch; and I answered inline the other remarks

On Wed, May 11, 2011 at 6:31 AM, Phil Muldoon <pmuldoon@redhat.com> wrote:
> 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.

sounds like a good idea, I don't want to change blindly a code which
was working correctly, so gdbpy_is_finish_bp should do the trick

>> @@ -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?

sure, I didn't know that!

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

I don't know what's the rule to export or not a function, I considered
that `create_breakpoint_sal' fitted my requirements and was
'high-level enough' to be used from Python, but it might be a bit too
naive ...

>> +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. ;)

you're right, there _was_ a reason at the time I changed it, but I
can't see any now, so i'll revert it to its original form

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

no, sorry

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

my implementation is based on (what I understood of) existing files,
namely py-breakpoint.h here; and I understand only now that
 python/python-internal.h plays the role of head for all the python
files. I'll re-organize it, depending on what Tom will say
>
>
>> +/* 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.

thanks, I'll look at that. Does it mean that the other
"PyGILState_Ensure/PyGILState_Release" should disappear ?

>> + ? ? ?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"?

yeah, I wasn't sure how 'out_of_scoped" would sound to a (native)
English hear, "out_of_scope" is the name of the function call when GDB
notices that the frame of the FinishBreakpoint is not anymore in the
callstack, and this flag indicates if the Python script already knows
it or not. `out_of_scope_notification' might be a better naming,
although a bit long

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

no, this time there is a valid reason:

>> -frame_object_to_frame_info (frame_object *frame_obj)
>> +frame_object_to_frame_info (PyObject *obj)

I exported `frame_object_to_frame_info' to to `python-internal.h', but
`frame_object *' is only defined within 'py-frame.c` (that's also the
way -most of- the other python<-->C translators where prototyped)

all the
>> -      FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
>> +      FRAPY_REQUIRE_VALID (self, frame);
follow from that. (FRAPY_REQUIRE_VALID internally uses
frame_object_to_frame_info.)
>
>> -
>> +
>> ? 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.

thanks for all your useful comments,

Kevin