From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19874 invoked by alias); 30 Jun 2011 12:41:29 -0000 Received: (qmail 19864 invoked by uid 22791); 30 Jun 2011 12:41:27 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,TW_EG,T_TO_NO_BRKTS_FREEMAIL X-Spam-Check-By: sourceware.org Received: from mail-vw0-f41.google.com (HELO mail-vw0-f41.google.com) (209.85.212.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 30 Jun 2011 12:41:11 +0000 Received: by vws4 with SMTP id 4so2035967vws.0 for ; Thu, 30 Jun 2011 05:41:10 -0700 (PDT) Received: by 10.220.5.210 with SMTP id 18mr758895vcw.83.1309437670116; Thu, 30 Jun 2011 05:41:10 -0700 (PDT) MIME-Version: 1.0 Received: by 10.220.188.75 with HTTP; Thu, 30 Jun 2011 05:40:50 -0700 (PDT) In-Reply-To: References: From: Kevin Pouget Date: Thu, 30 Jun 2011 12:41:00 -0000 Message-ID: Subject: Re: [patch] [python] gdb.Inferior reference count fixes To: pmuldoon@redhat.com Cc: 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: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-06/txt/msg00487.txt.bz2 Hi Phil, thanks for your offer to split the patch this way, I hope that the FSF paperwork won't take too long so that we can apply these patches just one thing: + thread_object *found =3D NULL; ... + if (found) + return found; return NULL; the last lines are equivalent to: return found; is it on purpose (ie, for readability) ? cordially, Kevin On Thu, Jun 30, 2011 at 1:42 PM, Phil Muldoon wrote: > > Here is the inferior reference patch fix I promised to break out from > the current_inferior patch. =A0The previous patch is here: > > http://sourceware.org/ml/gdb-patches/2011-06/msg00392.html > > Kevin will check in the selected_inferior patch when the FSF paperwork > is processed. > > I have not included tests for this patch as there is not a way to test > the leak without directly acquiring a gdb.Inferior. The gdb.inferiors() > method creates the gdb.Inferior if needed, and then adds it to a list, > so the ref_count =3D 2. =A0This bug appears when a gdb.Inferior only has a > ref_count of 1. > > I will attempt to walk you through the scenario. > > Right now, we create gdb.Inferior objects on-demand in Python. =A0The > function inferior_to_inferior_object is called in several places in the > Python code. =A0In the current version, it can return a borrowed or a new > reference. > > How does it do this? > > When a gdb.Inferior object is requested via inferior_to_inferior_object > we check to see if we previously created one with: > > --> inf_obj =3D inferior_data (inferior, infpy_inf_data_key); > > If a gdb.Inferior has already been created for that GDB inferior, we > just return the reference, but do not increment the reference count > (borrowed). =A0If there has not been a gdb.Inferior previously created, we > create it, initialize it, and stash it inside the GDB inferior space > with: > > --> set_inferior_data (inferior, infpy_inf_data_key, inf_obj); > > This returns a new reference. > > The problem is that consumers of inferior_to_inferior_object do not know > whether they have a borrowed or a new reference, and are unable to > determine if they should increment the reference count or not. > > Given this scenario: > > foo =3D gdb.current_inferior() > bar =3D gdb.current_inferior() > > bar =3D foo (foo's reference count is 1) > > Then, if you do this: > > foo =3D None > > Then 'foo' is garbage collected. =A0The 'bar' object, however, still poin= ts to > the address 'foo' was at (because it has a borrowed reference). =A0'bar' > is now invalid, and will eventually cause a sigsegv when the user > operates it. > > This patch simply makes inferior_to_inferior_object always return a new > reference, and brings clarity to the responsibility of object reference > counting to the API. =A0It also clears out stale gdb.Inferior references > from the GDB Inferior with a new deconstructor dealloc(). =A0This is > necessary for something like > > foo =3D gdb.current_inferior() > foo =3D None > bar =3D gdb.current_inferior() > > Before this patch we were not clearing the GDB inferior data, and so it > was returning a bogus pointer to 'bar' (the now garbage collected 'foo'). > > The rest of the patch is simply adjusting existing callers to work with > this new behaviour. > > Cheers, > > Phil > -- > > 2011-06-30 =A0Phil Muldoon =A0 > > =A0 =A0 =A0 =A0* python/py-inferior.c (infpy_dealloc): New function. > =A0 =A0 =A0 =A0(inferior_to_inferior_object): Return a new object, or a > =A0 =A0 =A0 =A0new reference to the existing object. > =A0 =A0 =A0 =A0(find_thread_object): Cleanup references to inferior. > =A0 =A0 =A0 =A0(delete_thread_object): Ditto. > =A0 =A0 =A0 =A0* python/py-infthread.c (create_thread_object): Do not inc= rement > =A0 =A0 =A0 =A0inferior reference count. > > > > -- > diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c > index f226501..92b28b7 100644 > --- a/gdb/python/py-inferior.c > +++ b/gdb/python/py-inferior.c > @@ -125,9 +125,10 @@ python_inferior_exit (struct inferior *inf) > =A0 do_cleanups (cleanup); > =A0} > > -/* Return a borrowed reference to the Python object of type Inferior > +/* Return a reference to the Python object of type Inferior > =A0 =A0representing INFERIOR. =A0If the object has already been created, > - =A0 return it, =A0otherwise, create it. =A0Return NULL on failure. =A0*/ > + =A0 return it and increment the reference count, =A0otherwise, create i= t. > + =A0 Return NULL on failure. =A0*/ > =A0PyObject * > =A0inferior_to_inferior_object (struct inferior *inferior) > =A0{ > @@ -154,13 +155,14 @@ inferior_to_inferior_object (struct inferior *infer= ior) > > =A0 =A0 =A0 do_cleanups (cleanup); > =A0 =A0 } > + =A0else > + =A0 =A0Py_INCREF ((PyObject *)inf_obj); > > =A0 return (PyObject *) inf_obj; > =A0} > > =A0/* Finds the Python Inferior object for the given PID. =A0Returns a > - =A0 borrowed reference, or NULL if PID does not match any inferior > - =A0 object. =A0*/ > + =A0 reference, or NULL if PID does not match any inferior object. */ > > =A0PyObject * > =A0find_inferior_object (int pid) > @@ -180,6 +182,7 @@ find_thread_object (ptid_t ptid) > =A0 int pid; > =A0 struct threadlist_entry *thread; > =A0 PyObject *inf_obj; > + =A0thread_object *found =3D NULL; > > =A0 pid =3D PIDGET (ptid); > =A0 if (pid =3D=3D 0) > @@ -187,11 +190,21 @@ find_thread_object (ptid_t ptid) > > =A0 inf_obj =3D find_inferior_object (pid); > > - =A0if (inf_obj) > - =A0 =A0for (thread =3D ((inferior_object *)inf_obj)->threads; thread; > - =A0 =A0 =A0 =A0thread =3D thread->next) > - =A0 =A0 =A0if (ptid_equal (thread->thread_obj->thread->ptid, ptid)) > - =A0 =A0 =A0 return thread->thread_obj; > + =A0if (! inf_obj) > + =A0 =A0return NULL; > + > + =A0for (thread =3D ((inferior_object *)inf_obj)->threads; thread; > + =A0 =A0 =A0 thread =3D thread->next) > + =A0 =A0if (ptid_equal (thread->thread_obj->thread->ptid, ptid)) > + =A0 =A0 =A0{ > + =A0 =A0 =A0 found =3D thread->thread_obj; > + =A0 =A0 =A0 break; > + =A0 =A0 =A0} > + > + =A0Py_DECREF (inf_obj); > + > + =A0if (found) > + =A0 =A0return found; > > =A0 return NULL; > =A0} > @@ -245,7 +258,10 @@ delete_thread_object (struct thread_info *tp, int ig= nore) > =A0 =A0 =A0 break; > > =A0 if (!*entry) > - =A0 =A0return; > + =A0 =A0{ > + =A0 =A0 =A0Py_DECREF (inf_obj); > + =A0 =A0 =A0return; > + =A0 =A0} > > =A0 cleanup =3D ensure_python_env (python_gdbarch, python_language); > > @@ -256,6 +272,7 @@ delete_thread_object (struct thread_info *tp, int ign= ore) > =A0 inf_obj->nthreads--; > > =A0 Py_DECREF (tmp->thread_obj); > + =A0Py_DECREF (inf_obj); > =A0 xfree (tmp); > > =A0 do_cleanups (cleanup); > @@ -321,8 +338,15 @@ build_inferior_list (struct inferior *inf, void *arg) > =A0{ > =A0 PyObject *list =3D arg; > =A0 PyObject *inferior =3D inferior_to_inferior_object (inf); > + =A0int success =3D 0; > > - =A0if (PyList_Append (list, inferior)) > + =A0if (! inferior) > + =A0 =A0return 0; > + > + =A0success =3D PyList_Append (list, inferior); > + =A0Py_DECREF (inferior); > + > + =A0if (success) > =A0 =A0 return 1; > > =A0 return 0; > @@ -617,6 +657,17 @@ infpy_is_valid (PyObject *self, PyObject *args) > =A0 Py_RETURN_TRUE; > =A0} > > +static void > +infpy_dealloc (PyObject *obj) > +{ > + =A0inferior_object *inf_obj =3D (inferior_object *) obj; > + =A0struct inferior *inf =3D inf_obj->inferior; > + > + =A0if (! inf) > + =A0 =A0return; > + > + =A0set_inferior_data (inf, infpy_inf_data_key, NULL); > +} > > =A0/* Clear the INFERIOR pointer in an Inferior object and clear the > =A0 =A0thread list. =A0*/ > @@ -714,7 +765,7 @@ static PyTypeObject inferior_object_type =3D > =A0 "gdb.Inferior", =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* tp_name */ > =A0 sizeof (inferior_object), =A0 =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 */ > + =A0infpy_dealloc, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* tp_dealloc */ > =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 */ > diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c > index b37c53c..cb714c7 100644 > --- a/gdb/python/py-infthread.c > +++ b/gdb/python/py-infthread.c > @@ -49,7 +49,6 @@ create_thread_object (struct thread_info *tp) > > =A0 thread_obj->thread =3D tp; > =A0 thread_obj->inf_obj =3D find_inferior_object (PIDGET (tp->ptid)); > - =A0Py_INCREF (thread_obj->inf_obj); > > =A0 return thread_obj; > =A0} >