From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gdb-patches-return-80969-listarch-gdb-patches=sources.redhat.com@sourceware.org>
Received: (qmail 29908 invoked by alias); 11 May 2011 11:29:32 -0000
Received: (qmail 29861 invoked by uid 22791); 11 May 2011 11:29:27 -0000
X-SWARE-Spam-Status: No, hits=-1.7 required=5.0	tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,RFC_ABUSE_POST,TW_EG,TW_WB,TW_YB,T_FILL_THIS_FORM_SHORT,T_TO_NO_BRKTS_FREEMAIL
X-Spam-Check-By: sourceware.org
Received: from mail-vx0-f169.google.com (HELO mail-vx0-f169.google.com) (209.85.220.169)    by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 11 May 2011 11:29:10 +0000
Received: by vxk20 with SMTP id 20so343459vxk.0        for <multiple recipients>; Wed, 11 May 2011 04:29:09 -0700 (PDT)
Received: by 10.52.74.99 with SMTP id s3mr1196039vdv.108.1305113349062; Wed, 11 May 2011 04:29:09 -0700 (PDT)
MIME-Version: 1.0
Received: by 10.220.61.6 with HTTP; Wed, 11 May 2011 04:28:49 -0700 (PDT)
In-Reply-To: <m3pqnpil4u.fsf@redhat.com>
References: <BANLkTim+YH64zkWvdz2_kVUUj=AJ7v4LKw@mail.gmail.com> <BANLkTi=gtHzWzXOJzB+a19=jkfk1hGwQBg@mail.gmail.com> <BANLkTikVdqbMqjguTV8ct0TWiBDhHGYtLg@mail.gmail.com> <BANLkTik6ct3VG=qVmPrqgOPnFvFQuoPp7A@mail.gmail.com> <m3pqnpil4u.fsf@redhat.com>
From: Kevin Pouget <kevin.pouget@gmail.com>
Date: Wed, 11 May 2011 11:29:00 -0000
Message-ID: <BANLkTik8N7X+w-HZvuXXRPGHMQA75Muu=g@mail.gmail.com>
Subject: Re: [RFC] Python Finish Breakpoints
To: pmuldoon@redhat.com
Cc: gdb@sourceware.org, gdb-patches@sourceware.org
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
X-IsSubscribed: yes
Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Id: <gdb-patches.sourceware.org>
List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org>
List-Archive: <http://sourceware.org/ml/gdb-patches/>
List-Post: <mailto:gdb-patches@sourceware.org>
List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs>
Sender: gdb-patches-owner@sourceware.org
X-SW-Source: 2011-05/txt/msg00280.txt.bz2

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)
>>> {
>>> =A0 *a +=3D 5;
>>> =A0 sleep(a);
>>> =A0 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)
>>> =A0 /* Save the function value return registers, if we care.
>>> =A0 =A0 =A0We might be about to restore their previous contents. =A0*/
>>> - =A0if (inferior_thread ()->control.proceed_to_finish)
>>> + =A0/*if (inferior_thread ()->control.proceed_to_finish)*/
>>> =A0 =A0...
>>> =A0 =A0stop_registers =3D 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). =A0Anyway 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 =3D \
>> =A0 =A0 =A0 =A0py-block.o \
>> =A0 =A0 =A0 =A0py-bpevent.o \
>> =A0 =A0 =A0 =A0py-breakpoint.o \
>> + =A0 =A0 =A0 py-finishbreakpoint.o \
>> =A0 =A0 =A0 =A0py-cmd.o \
>> =A0 =A0 =A0 =A0py-continueevent.o \
>> =A0 =A0 =A0 =A0py-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 =3D \
>> =A0 =A0 =A0 =A0python/py-block.c \
>> =A0 =A0 =A0 =A0python/py-bpevent.c \
>> =A0 =A0 =A0 =A0python/py-breakpoint.c \
>> + =A0 =A0 =A0 python/py-finishbreakpoint.c \
>> =A0 =A0 =A0 =A0python/py-cmd.c \
>> =A0 =A0 =A0 =A0python/py-continueevent.c \
>> =A0 =A0 =A0 =A0python/py-event.c \
>
> Ditto, see above.
>
>> @@ -2038,6 +2040,10 @@ py-breakpoint.o: $(srcdir)/python/py-breakpoint.c
>> =A0 =A0 =A0 =A0$(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-breakpoin=
t.c
>> =A0 =A0 =A0 =A0$(POSTCOMPILE)
>>
>> +py-finishbreakpoint.o: $(srcdir)/python/py-finishbreakpoint.c
>> + =A0 =A0 =A0 $(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-finishbrea=
kpoint.c
>> + =A0 =A0 =A0 $(POSTCOMPILE)
>> +
>
> Ditto.
>
>> =A0py-cmd.o: $(srcdir)/python/py-cmd.c
>> =A0 =A0 =A0 =A0$(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-cmd.c
>> =A0 =A0 =A0 =A0$(POSTCOMPILE)
>
>
>> +void
>> =A0create_breakpoint_sal (struct gdbarch *gdbarch,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct symtabs_and_lines sal=
s, char *addr_string,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 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,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int enabled,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int internal);
>>
>> +extern void create_breakpoint_sal (struct gdbarch *gdbarch,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 st=
ruct symtabs_and_lines sals,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ch=
ar *addr_string,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ch=
ar *cond_string,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 en=
um bptype type, enum bpdisp disposition,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 in=
t thread, int task, int ignore_count,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 st=
ruct breakpoint_ops *ops, int from_tty,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 in=
t enabled, int internal,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 in=
t 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,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 struct type *value_type);
>> +
>> =A0/* Address at which inferior stopped. =A0*/
>
>
> This patch context is not wide enough to know, but I this function I
> think should be placed next to the corresponding print_ function.
>
>> - =A0if (inferior_thread ()->control.proceed_to_finish)
>> + =A0/*if (inferior_thread ()->control.proceed_to_finish)*/
>> =A0 =A0 {
>> =A0 =A0 =A0 /* This should not be necessary. =A0*/
>> =A0 =A0 =A0 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 @@
>> =A0 =A0You should have received a copy of the GNU General Public License
>> =A0 =A0along with this program. =A0If not, see <http://www.gnu.org/licen=
ses/>. =A0*/
>>
>> +
>> +
>
> Spurious newlines.
>
>> =A0/* This is used to initialize various gdb.bp_* constants. =A0*/
>> =A0struct pybp_code
>> =A0{
>> @@ -806,21 +773,25 @@ gdbpy_breakpoint_created (struct breakpoint *bp)
>> =A0 =A0 }
>> =A0 else
>> =A0 =A0 newbp =3D PyObject_New (breakpoint_object, &breakpoint_object_ty=
pe);
>> - =A0if (newbp)
>> - =A0 =A0{
>> - =A0 =A0 =A0newbp->number =3D bp->number;
>> - =A0 =A0 =A0newbp->bp =3D bp;
>> - =A0 =A0 =A0newbp->bp->py_bp_object =3D newbp;
>> - =A0 =A0 =A0Py_INCREF (newbp);
>> - =A0 =A0 =A0++bppy_live;
>> - =A0 =A0}
>> - =A0else
>> - =A0 =A0{
>> - =A0 =A0 =A0PyErr_SetString (PyExc_RuntimeError,
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0_("Error while creating bre=
akpoint from GDB."));
>> - =A0 =A0 =A0gdbpy_print_stack ();
>> - =A0 =A0}
>> +
>> + =A0if (!newbp)
>> + =A0 =A0goto fail;
>> +
>> + =A0newbp->number =3D bp->number;
>> + =A0newbp->bp =3D bp;
>> + =A0newbp->bp->py_bp_object =3D newbp;
>> +
>> + =A0Py_INCREF (newbp);
>> + =A0++bppy_live;
>> +
>> + =A0goto success;
>> +
>> +fail:
>> + =A0PyErr_SetString (PyExc_RuntimeError,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 _("Error while creating breakpoint=
 from GDB."));
>> + =A0gdbpy_print_stack ();
>>
>> +success:
>> =A0 PyGILState_Release (state);
>> =A0}
>
>
> 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? =A0If this is just a change you feel needed to be made, I'd send it
> as a separate patch. =A0That'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 =3D
>> +PyTypeObject breakpoint_object_type =3D
>> =A0{
>> =A0 PyObject_HEAD_INIT (NULL)
>> =A0 0, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /*ob_size=
*/
>> =A0 "gdb.Breakpoint", =A0 =A0 =A0 =A0 =A0 =A0 =A0/*tp_name*/
>> =A0 sizeof (breakpoint_object), =A0 =A0/*tp_basicsize*/
>> =A0 0, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /*tp_item=
size*/
>> - =A00, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /*tp_dea=
lloc*/
>> + =A00, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/*tp_=
dealloc*/
>
> Spurious change.
>
>> =A0 0, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /*tp_prin=
t*/
>> =A0 0, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /*tp_geta=
ttr*/
>> =A0 0, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /*tp_seta=
ttr*/
>> @@ -1008,7 +979,7 @@ static PyTypeObject breakpoint_object_type =3D
>> =A0 0, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* tp_dic=
t */
>> =A0 0, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* tp_des=
cr_get */
>> =A0 0, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* tp_des=
cr_set */
>> - =A00, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* tp_di=
ctoffset */
>> + =A00, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* 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
>> +
>> + =A0 Copyright (C) 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
>> +
>> + =A0 This file is part of GDB.
>> +
>> + =A0 This program is free software; you can redistribute it and/or modi=
fy
>> + =A0 it under the terms of the GNU General Public License as published =
by
>> + =A0 the Free Software Foundation; either version 3 of the License, or
>> + =A0 (at your option) any later version.
>> +
>> + =A0 This program is distributed in the hope that it will be useful,
>> + =A0 but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + =A0 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. =A0See the
>> + =A0 GNU General Public License for more details.
>> +
>> + =A0 You should have received a copy of the GNU General Public License
>> + =A0 along with this program. =A0If not, see <http://www.gnu.org/licens=
es/>. =A0*/
>> +
>> +#ifndef GDB_PY_BREAKPOINT_H
>> +#define GDB_PY_BREAKPOINT_H
>> +
>> +/* Require that BREAKPOINT be a valid breakpoint ID; throw a Python
>> + =A0 exception if it is invalid. =A0*/
>> +#define BPPY_REQUIRE_VALID(Breakpoint) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
>> + =A0 =A0do { =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
>> + =A0 =A0 =A0if ((Breakpoint)->bp =3D=3D NULL) =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 \
>> + =A0 =A0 =A0 =A0return PyErr_Format (PyExc_RuntimeError, =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 _("Breakpoint =
%d is invalid."), =A0 =A0 =A0 =A0 =A0 =A0\
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (Breakpoint)->=
number); =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 \
>> + =A0 =A0} while (0)
>> +
>> +/* Require that BREAKPOINT be a valid breakpoint ID; throw a Python
>> + =A0 exception if it is invalid. =A0This macro is for use in setter fun=
ctions. =A0*/
>> +#define BPPY_SET_REQUIRE_VALID(Breakpoint) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
>> + =A0 =A0do { =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
>> + =A0 =A0 =A0if ((Breakpoint)->bp =3D=3D NULL) =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 \
>> + =A0 =A0 =A0 =A0{ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 \
>> + =A0 =A0 =A0 =A0 =A0PyErr_Format (PyExc_RuntimeError, _("Breakpoint %d =
is invalid."), \
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(Breakpoint)->number); =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
>> + =A0 =A0 =A0 =A0 =A0return -1; =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
>> + =A0 =A0 =A0 =A0} =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 \
>> + =A0 =A0} while (0)
>> +
>> +struct breakpoint_object
>> +{
>> + =A0PyObject_HEAD
>> +
>> + =A0/* The breakpoint number according to gdb. =A0*/
>> + =A0int number;
>> +
>> + =A0/* The gdb breakpoint object, or NULL if the breakpoint has been
>> + =A0 =A0 deleted. =A0*/
>> + =A0struct breakpoint *bp;
>> +};
>> +
>> +/* Variables used to pass information between the Breakpoint
>> + =A0 constructor and the breakpoint-created hook function. =A0*/
>> +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. =A0Normally, depending on the scope/context of
> the exported functions and macros we place them in
> python/python-internal.h. =A0I'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
>> + =A0 the current callstack. If BP_OBJ has the attribute OUT_OF_SCOPED a=
nd
>> + =A0 its value is FALSE, trigger the method OUT_OF_SCOPE and set the fl=
ag to
>> + =A0 TRUE. =A0*/
>
> Two spaces after . in the comment.
>
>> +bpfinishpy_detect_out_scope_cb (struct breakpoint *b, void *args)
>> +{
>> + =A0struct breakpoint *bp_stopped =3D (struct breakpoint *) args;
>> + =A0PyObject *py_bp =3D (PyObject *) b->py_bp_object;
>> + =A0PyGILState_STATE state;
>> +
>> + =A0/* Prevent python SEGFAULT because of missing thread state. =A0*/
>> + =A0state =3D PyGILState_Ensure();
>
> There is a specialized cleanup function that does this for you:
>
> For example:
>
> =A0cleanup =3D ensure_python_env (get_current_arch (), current_language);
>
> Make sure you get the arch from the breakpoint if applicable. =A0Then just
> call do_cleanups when done. =A0This ensure several internal GDB settings
> are saved and restored, as well as the GIL.
>
>> + =A0PyGILState_Release (state);
>
> do_cleanups (cleanup). =A0Also 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 ?

>> + =A0 =A0 =A0return;
>> +
>> + =A0Py_INCREF (&finish_breakpoint_object_type);
>> + =A0PyModule_AddObject (gdb_module, "FinishBreakpoint",
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(PyObject *) &finish_breakp=
oint_object_type);
>> +
>> + =A0observer_attach_normal_stop (bpfinishpy_handle_stop);
>> +}
>> +
>> +static PyGetSetDef finish_breakpoint_object_getset[] =3D {
>> + =A0{ "out_of_scoped", bpfinishpy_get_outofscoped, bpfinishpy_set_outof=
scoped,
>
> 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)
>> =A0{
>> + =A0frame_object *frame_obj =3D (frame_object *) obj;
>> =A0 struct frame_info *frame;
>>
>> =A0 frame =3D frame_find_by_id (frame_obj->frame_id);
>> @@ -103,7 +104,7 @@ frapy_is_valid (PyObject *self, PyObject *args)
>> =A0{
>> =A0 struct frame_info *frame;
>>
>> - =A0frame =3D frame_object_to_frame_info ((frame_object *) self);
>> + =A0frame =3D frame_object_to_frame_info (self);
>> =A0 if (frame =3D=3D NULL)
>> =A0 =A0 Py_RETURN_FALSE;
>>
>> @@ -124,7 +125,7 @@ frapy_name (PyObject *self, PyObject *args)
>>
>> =A0 TRY_CATCH (except, RETURN_MASK_ALL)
>> =A0 =A0 {
>> - =A0 =A0 =A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
>> + =A0 =A0 =A0FRAPY_REQUIRE_VALID (self, frame);
>>
>> =A0 =A0 =A0 find_frame_funname (frame, &name, &lang, NULL);
>> =A0 =A0 }
>> @@ -153,7 +154,7 @@ frapy_type (PyObject *self, PyObject *args)
>>
>> =A0 TRY_CATCH (except, RETURN_MASK_ALL)
>> =A0 =A0 {
>> - =A0 =A0 =A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
>> + =A0 =A0 =A0FRAPY_REQUIRE_VALID (self, frame);
>>
>> =A0 =A0 =A0 type =3D get_frame_type (frame);
>> =A0 =A0 }
>> @@ -174,7 +175,7 @@ frapy_unwind_stop_reason (PyObject *self, PyObject *=
args)
>>
>> =A0 TRY_CATCH (except, RETURN_MASK_ALL)
>> =A0 =A0 {
>> - =A0 =A0 =A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
>> + =A0 =A0 =A0FRAPY_REQUIRE_VALID (self, frame);
>> =A0 =A0 }
>> =A0 GDB_PY_HANDLE_EXCEPTION (except);
>>
>> @@ -195,7 +196,7 @@ frapy_pc (PyObject *self, PyObject *args)
>>
>> =A0 TRY_CATCH (except, RETURN_MASK_ALL)
>> =A0 =A0 {
>> - =A0 =A0 =A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
>> + =A0 =A0 =A0FRAPY_REQUIRE_VALID (self, frame);
>>
>> =A0 =A0 =A0 pc =3D get_frame_pc (frame);
>> =A0 =A0 }
>> @@ -216,7 +217,7 @@ frapy_block (PyObject *self, PyObject *args)
>>
>> =A0 TRY_CATCH (except, RETURN_MASK_ALL)
>> =A0 =A0 {
>> - =A0 =A0 =A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
>> + =A0 =A0 =A0FRAPY_REQUIRE_VALID (self, frame);
>> =A0 =A0 =A0 block =3D get_frame_block (frame, NULL);
>> =A0 =A0 }
>> =A0 GDB_PY_HANDLE_EXCEPTION (except);
>> @@ -257,7 +258,7 @@ frapy_function (PyObject *self, PyObject *args)
>>
>> =A0 TRY_CATCH (except, RETURN_MASK_ALL)
>> =A0 =A0 {
>> - =A0 =A0 =A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
>> + =A0 =A0 =A0FRAPY_REQUIRE_VALID (self, frame);
>>
>> =A0 =A0 =A0 sym =3D find_pc_function (get_frame_address_in_block (frame)=
);
>> =A0 =A0 }
>> @@ -319,7 +320,7 @@ frapy_older (PyObject *self, PyObject *args)
>>
>> =A0 TRY_CATCH (except, RETURN_MASK_ALL)
>> =A0 =A0 {
>> - =A0 =A0 =A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
>> + =A0 =A0 =A0FRAPY_REQUIRE_VALID (self, frame);
>>
>> =A0 =A0 =A0 prev =3D get_prev_frame (frame);
>> =A0 =A0 =A0 if (prev)
>> @@ -348,7 +349,7 @@ frapy_newer (PyObject *self, PyObject *args)
>>
>> =A0 TRY_CATCH (except, RETURN_MASK_ALL)
>> =A0 =A0 {
>> - =A0 =A0 =A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
>> + =A0 =A0 =A0FRAPY_REQUIRE_VALID (self, frame);
>>
>> =A0 =A0 =A0 next =3D get_next_frame (frame);
>> =A0 =A0 =A0 if (next)
>> @@ -377,7 +378,7 @@ frapy_find_sal (PyObject *self, PyObject *args)
>>
>> =A0 TRY_CATCH (except, RETURN_MASK_ALL)
>> =A0 =A0 {
>> - =A0 =A0 =A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
>> + =A0 =A0 =A0FRAPY_REQUIRE_VALID (self, frame);
>>
>> =A0 =A0 =A0 find_frame_sal (frame, &sal);
>> =A0 =A0 =A0 sal_obj =3D symtab_and_line_to_sal_object (sal);
>> @@ -433,7 +434,7 @@ frapy_read_var (PyObject *self, PyObject *args)
>>
>> =A0 =A0 =A0 TRY_CATCH (except, RETURN_MASK_ALL)
>> =A0 =A0 =A0 =A0{
>> - =A0 =A0 =A0 =A0 FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
>> + =A0 =A0 =A0 =A0 FRAPY_REQUIRE_VALID (self, frame);
>>
>> =A0 =A0 =A0 =A0 =A0if (!block)
>> =A0 =A0 =A0 =A0 =A0 =A0block =3D get_frame_block (frame, NULL);
>> @@ -461,7 +462,7 @@ frapy_read_var (PyObject *self, PyObject *args)
>>
>> =A0 TRY_CATCH (except, RETURN_MASK_ALL)
>> =A0 =A0 {
>> - =A0 =A0 =A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
>> + =A0 =A0 =A0FRAPY_REQUIRE_VALID (self, frame);
>>
>> =A0 =A0 =A0 val =3D read_var_value (var, frame);
>> =A0 =A0 }
>> @@ -484,12 +485,11 @@ static PyObject *
>> =A0frapy_select (PyObject *self, PyObject *args)
>> =A0{
>> =A0 struct frame_info *fi;
>> - =A0frame_object *frame =3D (frame_object *) self;
>> =A0 volatile struct gdb_exception except;
>>
>> =A0 TRY_CATCH (except, RETURN_MASK_ALL)
>> =A0 =A0 {
>> - =A0 =A0 =A0FRAPY_REQUIRE_VALID (frame, fi);
>> + =A0 =A0 =A0FRAPY_REQUIRE_VALID (self, fi);
>>
>> =A0 =A0 =A0 select_frame (fi);
>> =A0 =A0 }
>
> I'm not sure the above is needed for the patch? =A0If 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.)
>
>> -
>> +
>> =A0 return 0; /* Break at end. */
>> =A0}
>
> Spurious.
>
> Overall I like the idea, but I am unsure of the implementation. =A0I 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