From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gdb-patches-return-80968-listarch-gdb-patches=sources.redhat.com@sourceware.org>
Received: (qmail 5836 invoked by alias); 11 May 2011 10:31:57 -0000
Received: (qmail 5814 invoked by uid 22791); 11 May 2011 10:31:53 -0000
X-SWARE-Spam-Status: No, hits=-6.0 required=5.0	tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_EG,TW_WB,TW_YB,T_FILL_THIS_FORM_SHORT,T_RP_MATCHES_RCVD
X-Spam-Check-By: sourceware.org
Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28)    by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 11 May 2011 10:31:35 +0000
Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])	by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p4BAVVle018871	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK);	Wed, 11 May 2011 06:31:31 -0400
Received: from localhost.localdomain (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1])	by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p4BAVTLI012147;	Wed, 11 May 2011 06:31:30 -0400
From: Phil Muldoon <pmuldoon@redhat.com>
To: Kevin Pouget <kevin.pouget@gmail.com>
Cc: gdb@sourceware.org, gdb-patches@sourceware.org
Subject: Re: [RFC] Python Finish Breakpoints
References: <BANLkTim+YH64zkWvdz2_kVUUj=AJ7v4LKw@mail.gmail.com>	<BANLkTi=gtHzWzXOJzB+a19=jkfk1hGwQBg@mail.gmail.com>	<BANLkTikVdqbMqjguTV8ct0TWiBDhHGYtLg@mail.gmail.com>	<BANLkTik6ct3VG=qVmPrqgOPnFvFQuoPp7A@mail.gmail.com>
Reply-to: pmuldoon@redhat.com
X-URL: http://www.redhat.com
Date: Wed, 11 May 2011 10:31:00 -0000
In-Reply-To: <BANLkTik6ct3VG=qVmPrqgOPnFvFQuoPp7A@mail.gmail.com> (Kevin	Pouget's message of "Wed, 11 May 2011 03:43:41 -0400")
Message-ID: <m3pqnpil4u.fsf@redhat.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
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/msg00279.txt.bz2

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)
>> {
>> =C2=A0 *a +=3D 5;
>> =C2=A0 sleep(a);
>> =C2=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)
>> =C2=A0 /* Save the function value return registers, if we care.
>> =C2=A0 =C2=A0 =C2=A0We might be about to restore their previous contents=
. =C2=A0*/
>> - =C2=A0if (inferior_thread ()->control.proceed_to_finish)
>> + =C2=A0/*if (inferior_thread ()->control.proceed_to_finish)*/
>> =C2=A0 =C2=A0...
>> =C2=A0 =C2=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).  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 =3D \
> =C2=A0 =C2=A0 =C2=A0 =C2=A0py-block.o \
> =C2=A0 =C2=A0 =C2=A0 =C2=A0py-bpevent.o \
> =C2=A0 =C2=A0 =C2=A0 =C2=A0py-breakpoint.o \
> + =C2=A0 =C2=A0 =C2=A0 py-finishbreakpoint.o \
> =C2=A0 =C2=A0 =C2=A0 =C2=A0py-cmd.o \
> =C2=A0 =C2=A0 =C2=A0 =C2=A0py-continueevent.o \
> =C2=A0 =C2=A0 =C2=A0 =C2=A0py-event.o \

This is a nit I have personally, but you can put the .o file in the
correct alphabetical order?=20=20

> @@ -309,6 +310,7 @@ SUBDIR_PYTHON_SRCS =3D \
> =C2=A0 =C2=A0 =C2=A0 =C2=A0python/py-block.c \
> =C2=A0 =C2=A0 =C2=A0 =C2=A0python/py-bpevent.c \
> =C2=A0 =C2=A0 =C2=A0 =C2=A0python/py-breakpoint.c \
> + =C2=A0 =C2=A0 =C2=A0 python/py-finishbreakpoint.c \
> =C2=A0 =C2=A0 =C2=A0 =C2=A0python/py-cmd.c \
> =C2=A0 =C2=A0 =C2=A0 =C2=A0python/py-continueevent.c \
> =C2=A0 =C2=A0 =C2=A0 =C2=A0python/py-event.c \

Ditto, see above.

> @@ -2038,6 +2040,10 @@ py-breakpoint.o: $(srcdir)/python/py-breakpoint.c
> =C2=A0 =C2=A0 =C2=A0 =C2=A0$(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/p=
y-breakpoint.c
> =C2=A0 =C2=A0 =C2=A0 =C2=A0$(POSTCOMPILE)
>
> +py-finishbreakpoint.o: $(srcdir)/python/py-finishbreakpoint.c
> + =C2=A0 =C2=A0 =C2=A0 $(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-fi=
nishbreakpoint.c
> + =C2=A0 =C2=A0 =C2=A0 $(POSTCOMPILE)
> +

Ditto.

> =C2=A0py-cmd.o: $(srcdir)/python/py-cmd.c
> =C2=A0 =C2=A0 =C2=A0 =C2=A0$(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/p=
y-cmd.c
> =C2=A0 =C2=A0 =C2=A0 =C2=A0$(POSTCOMPILE)


> +void
> =C2=A0create_breakpoint_sal (struct gdbarch *gdbarch,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 struct symtabs_and_lines sals, char *addr_string,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=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,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int enabled,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int internal);
>
> +extern void create_breakpoint_sal (struct gdbarch *gdbarch,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct symtabs_and_lines s=
als,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 char *addr_string,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 char *cond_string,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 enum bptype type, enum bpd=
isp disposition,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 int thread, int task, int =
ignore_count,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct breakpoint_ops *ops=
, int from_tty,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 int enabled, int internal,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 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,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct type =
*value_type);
> +
> =C2=A0/* Address at which inferior stopped. =C2=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.

> - =C2=A0if (inferior_thread ()->control.proceed_to_finish)
> + =C2=A0/*if (inferior_thread ()->control.proceed_to_finish)*/
> =C2=A0 =C2=A0 {
> =C2=A0 =C2=A0 =C2=A0 /* This should not be necessary. =C2=A0*/
> =C2=A0 =C2=A0 =C2=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 @@
> =C2=A0 =C2=A0You should have received a copy of the GNU General Public Li=
cense
> =C2=A0 =C2=A0along with this program. =C2=A0If not, see <http://www.gnu.o=
rg/licenses/>. =C2=A0*/
>
> +
> +

Spurious newlines.

> =C2=A0/* This is used to initialize various gdb.bp_* constants. =C2=A0*/
> =C2=A0struct pybp_code
> =C2=A0{
> @@ -806,21 +773,25 @@ gdbpy_breakpoint_created (struct breakpoint *bp)
> =C2=A0 =C2=A0 }
> =C2=A0 else
> =C2=A0 =C2=A0 newbp =3D PyObject_New (breakpoint_object, &breakpoint_obje=
ct_type);
> - =C2=A0if (newbp)
> - =C2=A0 =C2=A0{
> - =C2=A0 =C2=A0 =C2=A0newbp->number =3D bp->number;
> - =C2=A0 =C2=A0 =C2=A0newbp->bp =3D bp;
> - =C2=A0 =C2=A0 =C2=A0newbp->bp->py_bp_object =3D newbp;
> - =C2=A0 =C2=A0 =C2=A0Py_INCREF (newbp);
> - =C2=A0 =C2=A0 =C2=A0++bppy_live;
> - =C2=A0 =C2=A0}
> - =C2=A0else
> - =C2=A0 =C2=A0{
> - =C2=A0 =C2=A0 =C2=A0PyErr_SetString (PyExc_RuntimeError,
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0_("Error while creating breakpoint from GDB."));
> - =C2=A0 =C2=A0 =C2=A0gdbpy_print_stack ();
> - =C2=A0 =C2=A0}
> +
> + =C2=A0if (!newbp)
> + =C2=A0 =C2=A0goto fail;
> +
> + =C2=A0newbp->number =3D bp->number;
> + =C2=A0newbp->bp =3D bp;
> + =C2=A0newbp->bp->py_bp_object =3D newbp;
> +
> + =C2=A0Py_INCREF (newbp);
> + =C2=A0++bppy_live;
> +
> + =C2=A0goto success;
> +
> +fail:
> + =C2=A0PyErr_SetString (PyExc_RuntimeError,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 _("Error=
 while creating breakpoint from GDB."));
> + =C2=A0gdbpy_print_stack ();
>
> +success:
> =C2=A0 PyGILState_Release (state);
> =C2=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?  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 =3D
> +PyTypeObject breakpoint_object_type =3D
> =C2=A0{
> =C2=A0 PyObject_HEAD_INIT (NULL)
> =C2=A0 0, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /*ob_size*/
> =C2=A0 "gdb.Breakpoint", =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
/*tp_name*/
> =C2=A0 sizeof (breakpoint_object), =C2=A0 =C2=A0/*tp_basicsize*/
> =C2=A0 0, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /*tp_itemsize*/
> - =C2=A00, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /*tp_dealloc*/
> + =C2=A00, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/*tp_dealloc*/

Spurious change.

> =C2=A0 0, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /*tp_print*/
> =C2=A0 0, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /*tp_getattr*/
> =C2=A0 0, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /*tp_setattr*/
> @@ -1008,7 +979,7 @@ static PyTypeObject breakpoint_object_type =3D
> =C2=A0 0, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* tp_dict */
> =C2=A0 0, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* tp_descr_get */
> =C2=A0 0, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* tp_descr_set */
> - =C2=A00, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* tp_dictoffset */
> + =C2=A00, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* 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
> +
> + =C2=A0 Copyright (C) 2008, 2009, 2010, 2011 Free Software Foundation, I=
nc.
> +
> + =C2=A0 This file is part of GDB.
> +
> + =C2=A0 This program is free software; you can redistribute it and/or mo=
dify
> + =C2=A0 it under the terms of the GNU General Public License as publishe=
d by
> + =C2=A0 the Free Software Foundation; either version 3 of the License, or
> + =C2=A0 (at your option) any later version.
> +
> + =C2=A0 This program is distributed in the hope that it will be useful,
> + =C2=A0 but WITHOUT ANY WARRANTY; without even the implied warranty of
> + =C2=A0 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. =C2=A0See t=
he
> + =C2=A0 GNU General Public License for more details.
> +
> + =C2=A0 You should have received a copy of the GNU General Public License
> + =C2=A0 along with this program. =C2=A0If not, see <http://www.gnu.org/l=
icenses/>. =C2=A0*/
> +
> +#ifndef GDB_PY_BREAKPOINT_H
> +#define GDB_PY_BREAKPOINT_H
> +
> +/* Require that BREAKPOINT be a valid breakpoint ID; throw a Python
> + =C2=A0 exception if it is invalid. =C2=A0*/
> +#define BPPY_REQUIRE_VALID(Breakpoint) =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0\
> + =C2=A0 =C2=A0do { =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0\
> + =C2=A0 =C2=A0 =C2=A0if ((Breakpoint)->bp =3D=3D NULL) =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 \
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0return PyErr_Format (PyExc_RuntimeError, =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0\
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 _("Breakpoint %d is invalid."), =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0\
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 (Breakpoint)->number); =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 \
> + =C2=A0 =C2=A0} while (0)
> +
> +/* Require that BREAKPOINT be a valid breakpoint ID; throw a Python
> + =C2=A0 exception if it is invalid. =C2=A0This macro is for use in sette=
r functions. =C2=A0*/
> +#define BPPY_SET_REQUIRE_VALID(Breakpoint) =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0\
> + =C2=A0 =C2=A0do { =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0\
> + =C2=A0 =C2=A0 =C2=A0if ((Breakpoint)->bp =3D=3D NULL) =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 \
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0{ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 \
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0PyErr_Format (PyExc_RuntimeError, _("=
Breakpoint %d is invalid."), \
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0(Breakpoint)->number); =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0\
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -1; =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0\
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0} =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 \
> + =C2=A0 =C2=A0} while (0)
> +
> +struct breakpoint_object
> +{
> + =C2=A0PyObject_HEAD
> +
> + =C2=A0/* The breakpoint number according to gdb. =C2=A0*/
> + =C2=A0int number;
> +
> + =C2=A0/* The gdb breakpoint object, or NULL if the breakpoint has been
> + =C2=A0 =C2=A0 deleted. =C2=A0*/
> + =C2=A0struct breakpoint *bp;
> +};
> +
> +/* Variables used to pass information between the Breakpoint
> + =C2=A0 constructor and the breakpoint-created hook function. =C2=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.  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
> + =C2=A0 the current callstack. If BP_OBJ has the attribute OUT_OF_SCOPED=
 and
> + =C2=A0 its value is FALSE, trigger the method OUT_OF_SCOPE and set the =
flag to
> + =C2=A0 TRUE. =C2=A0*/

Two spaces after . in the comment.

> +bpfinishpy_detect_out_scope_cb (struct breakpoint *b, void *args)
> +{
> + =C2=A0struct breakpoint *bp_stopped =3D (struct breakpoint *) args;
> + =C2=A0PyObject *py_bp =3D (PyObject *) b->py_bp_object;
> + =C2=A0PyGILState_STATE state;
> +
> + =C2=A0/* Prevent python SEGFAULT because of missing thread state. =C2=
=A0*/
> + =C2=A0state =3D PyGILState_Ensure();

There is a specialized cleanup function that does this for you:

For example:

  cleanup =3D 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.

> + =C2=A0PyGILState_Release (state);

do_cleanups (cleanup).  Also make sure any local failure goto branches
do this too.

> + =C2=A0 =C2=A0 =C2=A0return;
> +
> + =C2=A0Py_INCREF (&finish_breakpoint_object_type);
> + =C2=A0PyModule_AddObject (gdb_module, "FinishBreakpoint",
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0(PyObject *) &finish_breakpoint_object_type);
> +
> + =C2=A0observer_attach_normal_stop (bpfinishpy_handle_stop);
> +}
> +
> +static PyGetSetDef finish_breakpoint_object_getset[] =3D {
> + =C2=A0{ "out_of_scoped", bpfinishpy_get_outofscoped, bpfinishpy_set_out=
ofscoped,

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)
> =C2=A0{
> + =C2=A0frame_object *frame_obj =3D (frame_object *) obj;
> =C2=A0 struct frame_info *frame;
>
> =C2=A0 frame =3D frame_find_by_id (frame_obj->frame_id);
> @@ -103,7 +104,7 @@ frapy_is_valid (PyObject *self, PyObject *args)
> =C2=A0{
> =C2=A0 struct frame_info *frame;
>
> - =C2=A0frame =3D frame_object_to_frame_info ((frame_object *) self);
> + =C2=A0frame =3D frame_object_to_frame_info (self);
> =C2=A0 if (frame =3D=3D NULL)
> =C2=A0 =C2=A0 Py_RETURN_FALSE;
>
> @@ -124,7 +125,7 @@ frapy_name (PyObject *self, PyObject *args)
>
> =C2=A0 TRY_CATCH (except, RETURN_MASK_ALL)
> =C2=A0 =C2=A0 {
> - =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID (self, frame);
>
> =C2=A0 =C2=A0 =C2=A0 find_frame_funname (frame, &name, &lang, NULL);
> =C2=A0 =C2=A0 }
> @@ -153,7 +154,7 @@ frapy_type (PyObject *self, PyObject *args)
>
> =C2=A0 TRY_CATCH (except, RETURN_MASK_ALL)
> =C2=A0 =C2=A0 {
> - =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID (self, frame);
>
> =C2=A0 =C2=A0 =C2=A0 type =3D get_frame_type (frame);
> =C2=A0 =C2=A0 }
> @@ -174,7 +175,7 @@ frapy_unwind_stop_reason (PyObject *self, PyObject *a=
rgs)
>
> =C2=A0 TRY_CATCH (except, RETURN_MASK_ALL)
> =C2=A0 =C2=A0 {
> - =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID (self, frame);
> =C2=A0 =C2=A0 }
> =C2=A0 GDB_PY_HANDLE_EXCEPTION (except);
>
> @@ -195,7 +196,7 @@ frapy_pc (PyObject *self, PyObject *args)
>
> =C2=A0 TRY_CATCH (except, RETURN_MASK_ALL)
> =C2=A0 =C2=A0 {
> - =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID (self, frame);
>
> =C2=A0 =C2=A0 =C2=A0 pc =3D get_frame_pc (frame);
> =C2=A0 =C2=A0 }
> @@ -216,7 +217,7 @@ frapy_block (PyObject *self, PyObject *args)
>
> =C2=A0 TRY_CATCH (except, RETURN_MASK_ALL)
> =C2=A0 =C2=A0 {
> - =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID (self, frame);
> =C2=A0 =C2=A0 =C2=A0 block =3D get_frame_block (frame, NULL);
> =C2=A0 =C2=A0 }
> =C2=A0 GDB_PY_HANDLE_EXCEPTION (except);
> @@ -257,7 +258,7 @@ frapy_function (PyObject *self, PyObject *args)
>
> =C2=A0 TRY_CATCH (except, RETURN_MASK_ALL)
> =C2=A0 =C2=A0 {
> - =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID (self, frame);
>
> =C2=A0 =C2=A0 =C2=A0 sym =3D find_pc_function (get_frame_address_in_block=
 (frame));
> =C2=A0 =C2=A0 }
> @@ -319,7 +320,7 @@ frapy_older (PyObject *self, PyObject *args)
>
> =C2=A0 TRY_CATCH (except, RETURN_MASK_ALL)
> =C2=A0 =C2=A0 {
> - =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID (self, frame);
>
> =C2=A0 =C2=A0 =C2=A0 prev =3D get_prev_frame (frame);
> =C2=A0 =C2=A0 =C2=A0 if (prev)
> @@ -348,7 +349,7 @@ frapy_newer (PyObject *self, PyObject *args)
>
> =C2=A0 TRY_CATCH (except, RETURN_MASK_ALL)
> =C2=A0 =C2=A0 {
> - =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID (self, frame);
>
> =C2=A0 =C2=A0 =C2=A0 next =3D get_next_frame (frame);
> =C2=A0 =C2=A0 =C2=A0 if (next)
> @@ -377,7 +378,7 @@ frapy_find_sal (PyObject *self, PyObject *args)
>
> =C2=A0 TRY_CATCH (except, RETURN_MASK_ALL)
> =C2=A0 =C2=A0 {
> - =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID (self, frame);
>
> =C2=A0 =C2=A0 =C2=A0 find_frame_sal (frame, &sal);
> =C2=A0 =C2=A0 =C2=A0 sal_obj =3D symtab_and_line_to_sal_object (sal);
> @@ -433,7 +434,7 @@ frapy_read_var (PyObject *self, PyObject *args)
>
> =C2=A0 =C2=A0 =C2=A0 TRY_CATCH (except, RETURN_MASK_ALL)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0{
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 FRAPY_REQUIRE_VALID ((frame_object *) self,=
 frame);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 FRAPY_REQUIRE_VALID (self, frame);
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!block)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0block =3D get_frame_block (frame=
, NULL);
> @@ -461,7 +462,7 @@ frapy_read_var (PyObject *self, PyObject *args)
>
> =C2=A0 TRY_CATCH (except, RETURN_MASK_ALL)
> =C2=A0 =C2=A0 {
> - =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
> + =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID (self, frame);
>
> =C2=A0 =C2=A0 =C2=A0 val =3D read_var_value (var, frame);
> =C2=A0 =C2=A0 }
> @@ -484,12 +485,11 @@ static PyObject *
> =C2=A0frapy_select (PyObject *self, PyObject *args)
> =C2=A0{
> =C2=A0 struct frame_info *fi;
> - =C2=A0frame_object *frame =3D (frame_object *) self;
> =C2=A0 volatile struct gdb_exception except;
>
> =C2=A0 TRY_CATCH (except, RETURN_MASK_ALL)
> =C2=A0 =C2=A0 {
> - =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID (frame, fi);
> + =C2=A0 =C2=A0 =C2=A0FRAPY_REQUIRE_VALID (self, fi);
>
> =C2=A0 =C2=A0 =C2=A0 select_frame (fi);
> =C2=A0 =C2=A0 }

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.


> -
> +
> =C2=A0 return 0; /* Break at end. */
> =C2=A0}

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