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

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

[Phil Muldoon]

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

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

[Kevin Pouget]

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 = 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 <pmuldoon@redhat.com> wrote:
>
> 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;
>  }
>

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

[Phil Muldoon]

Kevin Pouget <kevin.pouget@gmail.com> writes:

> 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

Hope so too! This patch does not have a dependency on your patch so
(pending approval) it could go in now.


> just one thing:
>
> + thread_object *found = NULL;
> ...
> +  if (found)
> +    return found;
>   return NULL;
>
> the last lines are equivalent to: return found; is it on purpose (ie,
> for readability) ?

I think this was a rewrite  and I missed that hunk.  It is correct, if
redundant.  Thanks for spotting it.  I'll alter it locally.

Cheers

Phil

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

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> 2011-06-30  Phil Muldoon  <pmuldoon@redhat.com>
Phil> 	* python/py-inferior.c (infpy_dealloc): New function.
Phil>         (inferior_to_inferior_object): Return a new object, or a
Phil> 	new reference to the existing object.
Phil>         (find_thread_object): Cleanup references to inferior.
Phil>         (delete_thread_object): Ditto.
Phil>         * python/py-infthread.c (create_thread_object): Do not increment
Phil> 	inferior reference count.

Ok.

Tom

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

[Phil Muldoon]

Tom Tromey <tromey@redhat.com> writes:

>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
>
> Phil> 2011-06-30  Phil Muldoon  <pmuldoon@redhat.com>
> Phil> 	* python/py-inferior.c (infpy_dealloc): New function.
> Phil>         (inferior_to_inferior_object): Return a new object, or a
> Phil> 	new reference to the existing object.
> Phil>         (find_thread_object): Cleanup references to inferior.
> Phil>         (delete_thread_object): Ditto.
> Phil>         * python/py-infthread.c (create_thread_object): Do not increment
> Phil> 	inferior reference count.
>
> Ok.
>

Committed, thanks.

Cheers

Phil