From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6981 invoked by alias); 30 Jun 2011 11:43:10 -0000 Received: (qmail 6971 invoked by uid 22791); 30 Jun 2011 11:43:09 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_EG,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; Thu, 30 Jun 2011 11:42:51 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p5UBgomO030181 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 30 Jun 2011 07:42:51 -0400 Received: from localhost.localdomain (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p5UBgnmx009856 for ; Thu, 30 Jun 2011 07:42:50 -0400 From: Phil Muldoon To: gdb-patches@sourceware.org Subject: [patch] [python] gdb.Inferior reference count fixes Reply-to: pmuldoon@redhat.com X-URL: http://www.redhat.com Date: Thu, 30 Jun 2011 11:43:00 -0000 Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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/msg00486.txt.bz2 Here is the inferior reference patch fix I promised to break out from the current_inferior patch. The 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 = 2. This 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. The function inferior_to_inferior_object is called in several places in the Python code. In 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 = 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). If 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 = gdb.current_inferior() bar = gdb.current_inferior() bar = foo (foo's reference count is 1) Then, if you do this: foo = None Then 'foo' is garbage collected. The 'bar' object, however, still points to the address 'foo' was at (because it has a borrowed reference). '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. It also clears out stale gdb.Inferior references from the GDB Inferior with a new deconstructor dealloc(). This is necessary for something like foo = gdb.current_inferior() foo = None bar = 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 Phil Muldoon * python/py-inferior.c (infpy_dealloc): New function. (inferior_to_inferior_object): Return a new object, or a new reference to the existing object. (find_thread_object): Cleanup references to inferior. (delete_thread_object): Ditto. * python/py-infthread.c (create_thread_object): Do not increment inferior 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) do_cleanups (cleanup); } -/* Return a borrowed reference to the Python object of type Inferior +/* Return a reference to the Python object of type Inferior representing INFERIOR. If the object has already been created, - return it, otherwise, create it. Return NULL on failure. */ + return it and increment the reference count, otherwise, create it. + Return NULL on failure. */ PyObject * inferior_to_inferior_object (struct inferior *inferior) { @@ -154,13 +155,14 @@ inferior_to_inferior_object (struct inferior *inferior) do_cleanups (cleanup); } + else + Py_INCREF ((PyObject *)inf_obj); return (PyObject *) inf_obj; } /* Finds the Python Inferior object for the given PID. Returns a - borrowed reference, or NULL if PID does not match any inferior - object. */ + reference, or NULL if PID does not match any inferior object. */ PyObject * find_inferior_object (int pid) @@ -180,6 +182,7 @@ find_thread_object (ptid_t ptid) int pid; struct threadlist_entry *thread; PyObject *inf_obj; + thread_object *found = NULL; pid = PIDGET (ptid); if (pid == 0) @@ -187,11 +190,21 @@ find_thread_object (ptid_t ptid) inf_obj = find_inferior_object (pid); - if (inf_obj) - for (thread = ((inferior_object *)inf_obj)->threads; thread; - thread = thread->next) - if (ptid_equal (thread->thread_obj->thread->ptid, ptid)) - return thread->thread_obj; + if (! inf_obj) + return NULL; + + for (thread = ((inferior_object *)inf_obj)->threads; thread; + thread = thread->next) + if (ptid_equal (thread->thread_obj->thread->ptid, ptid)) + { + found = thread->thread_obj; + break; + } + + Py_DECREF (inf_obj); + + if (found) + return found; return NULL; } @@ -245,7 +258,10 @@ delete_thread_object (struct thread_info *tp, int ignore) break; if (!*entry) - return; + { + Py_DECREF (inf_obj); + return; + } cleanup = ensure_python_env (python_gdbarch, python_language); @@ -256,6 +272,7 @@ delete_thread_object (struct thread_info *tp, int ignore) inf_obj->nthreads--; Py_DECREF (tmp->thread_obj); + Py_DECREF (inf_obj); xfree (tmp); do_cleanups (cleanup); @@ -321,8 +338,15 @@ build_inferior_list (struct inferior *inf, void *arg) { PyObject *list = arg; PyObject *inferior = inferior_to_inferior_object (inf); + int success = 0; - if (PyList_Append (list, inferior)) + if (! inferior) + return 0; + + success = PyList_Append (list, inferior); + Py_DECREF (inferior); + + if (success) return 1; return 0; @@ -617,6 +657,17 @@ infpy_is_valid (PyObject *self, PyObject *args) Py_RETURN_TRUE; } +static void +infpy_dealloc (PyObject *obj) +{ + inferior_object *inf_obj = (inferior_object *) obj; + struct inferior *inf = inf_obj->inferior; + + if (! inf) + return; + + set_inferior_data (inf, infpy_inf_data_key, NULL); +} /* Clear the INFERIOR pointer in an Inferior object and clear the thread list. */ @@ -714,7 +765,7 @@ static PyTypeObject inferior_object_type = "gdb.Inferior", /* tp_name */ sizeof (inferior_object), /* tp_basicsize */ 0, /* tp_itemsize */ - 0, /* tp_dealloc */ + infpy_dealloc, /* tp_dealloc */ 0, /* tp_print */ 0, /* tp_getattr */ 0, /* tp_setattr */ 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) thread_obj->thread = tp; thread_obj->inf_obj = find_inferior_object (PIDGET (tp->ptid)); - Py_INCREF (thread_obj->inf_obj); return thread_obj; }