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

[Kevin Pouget <kevin.pouget@gmail.com>]

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