[patch] [python] gdb.Inferior reference count fixes

[Phil Muldoon <pmuldoon at redhat dot com>]

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  <pmuldoon@redhat.com>

	* 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;
 }