[RFA] fix ref counting of inferior_to_inferior_object

[RFA] fix ref counting of inferior_to_inferior_object

[Doug Evans]

Hi.
While adding an inferior attribute to the newobjfile event,
I noticed inferior_to_inferior_object sometimes returns (AFAICT)
a new reference and sometimes returns a borrowed reference.

This patch makes it consistently return a borrowed reference
(for consistency with obfile_to_objfile_object), and then
updates the rest of the code.

Regression tested on amd64-linux.
Ok to check in?

2013-09-12  Doug Evans  <dje@google.com>

	* python/py-exitedevent.c (create_exited_event_object): Add comment.
	* python/py-inferior.c (inferior_to_inferior_object): Always return
	a borrowed reference.
	(find_inferior_object): Return a new reference to the inferior object.
	(build_inferior_list): Update, inferior_to_inferior_object returns
	a borrowed reference.
	(gdbpy_selected_inferior): Ditto.

Index: python/py-exitedevent.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-exitedevent.c,v
retrieving revision 1.8
diff -u -p -r1.8 py-exitedevent.c
--- python/py-exitedevent.c	20 May 2013 20:09:01 -0000	1.8
+++ python/py-exitedevent.c	12 Sep 2013 22:19:49 -0000
@@ -49,6 +49,8 @@ create_exited_event_object (const LONGES
 	goto fail;
     }
 
+  /* Note that inferior_to_inferior_object returns a borrowed reference,
+     so we don't need a decref here.  */
   inf_obj = inferior_to_inferior_object (inf);
   if (!inf_obj || evpy_add_attribute (exited_event,
                                       "inferior",
Index: python/py-inferior.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-inferior.c,v
retrieving revision 1.36
diff -u -p -r1.36 py-inferior.c
--- python/py-inferior.c	18 Jun 2013 18:43:27 -0000	1.36
+++ python/py-inferior.c	12 Sep 2013 22:19:50 -0000
@@ -161,8 +161,9 @@ python_new_objfile (struct objfile *objf
 
 /* Return a reference to the Python object of type Inferior
    representing INFERIOR.  If the object has already been created,
-   return it and increment the reference count,  otherwise, create it.
+   return it,  otherwise create it.  The result is a borrowed reference.
    Return NULL on failure.  */
+
 PyObject *
 inferior_to_inferior_object (struct inferior *inferior)
 {
@@ -180,15 +181,12 @@ inferior_to_inferior_object (struct infe
       inf_obj->nthreads = 0;
 
       set_inferior_data (inferior, infpy_inf_data_key, inf_obj);
-
     }
-  else
-    Py_INCREF ((PyObject *)inf_obj);
 
   return (PyObject *) inf_obj;
 }
 
-/* Finds the Python Inferior object for the given PID.  Returns a
+/* Finds the Python Inferior object for the given PID.  Returns a new
    reference, or NULL if PID does not match any inferior object. */
 
 PyObject *
@@ -197,7 +195,12 @@ find_inferior_object (int pid)
   struct inferior *inf = find_inferior_pid (pid);
 
   if (inf)
-    return inferior_to_inferior_object (inf);
+    {
+      PyObject *inf_obj = inferior_to_inferior_object (inf);
+
+      Py_XINCREF (inf_obj);
+      return inf_obj;
+    }
 
   return NULL;
 }
@@ -384,7 +387,6 @@ build_inferior_list (struct inferior *in
     return 0;
 
   success = PyList_Append (list, inferior);
-  Py_DECREF (inferior);
 
   if (success)
     return 1;
@@ -768,12 +770,16 @@ py_free_inferior (struct inferior *inf, 
 }
 
 /* Implementation of gdb.selected_inferior() -> gdb.Inferior.
-   Returns the current inferior object.  */
+   Returns a new reference to the current inferior object.  */
 
 PyObject *
 gdbpy_selected_inferior (PyObject *self, PyObject *args)
 {
-  return inferior_to_inferior_object (current_inferior ());
+  PyObject *inf_obj;
+
+  inf_obj = inferior_to_inferior_object (current_inferior ());
+  Py_XINCREF (inf_obj);
+  return inf_obj;
 }
 
 int

Re: [RFA] fix ref counting of inferior_to_inferior_object

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> While adding an inferior attribute to the newobjfile event,
Doug> I noticed inferior_to_inferior_object sometimes returns (AFAICT)
Doug> a new reference and sometimes returns a borrowed reference.

The current code is maybe weird but I think it isn't incorrect.  That
said it is fine to change it as well.

The current model is that the Python object mirroring the inferior
clears the inferior->Python mapping when it is finally destroyed.
If the Python code then requests the Python object for that inferior
again, a new object is created.  This is "ok" because the Inferior
object doesn't carry any user state.

Doug>  /* Return a reference to the Python object of type Inferior
Doug>     representing INFERIOR.  If the object has already been created,
Doug> -   return it and increment the reference count,  otherwise, create it.
Doug> +   return it,  otherwise create it.  The result is a borrowed reference.
Doug>     Return NULL on failure.  */

This change requires a CPYCHECKER_RETURNS_BORROWED_REF annotation on the
declaration of inferior_to_inferior_object.

Tom

Re: [RFA] fix ref counting of inferior_to_inferior_object

[Doug Evans]

On Mon, Sep 23, 2013 at 1:41 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Doug" == Doug Evans <dje@google.com> writes:
>
> Doug> While adding an inferior attribute to the newobjfile event,
> Doug> I noticed inferior_to_inferior_object sometimes returns (AFAICT)
> Doug> a new reference and sometimes returns a borrowed reference.
>
> The current code is maybe weird but I think it isn't incorrect.  That
> said it is fine to change it as well.
>
> The current model is that the Python object mirroring the inferior
> clears the inferior->Python mapping when it is finally destroyed.
> If the Python code then requests the Python object for that inferior
> again, a new object is created.  This is "ok" because the Inferior
> object doesn't carry any user state.

Doesn't the caller always need to know whether s/he is getting a new
reference or a borrowed reference?
How will s/he keep the reference count correct?
[maybe I'm misunderstanding the terms used here]

Re: [RFA] fix ref counting of inferior_to_inferior_object

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

>> The current model is that the Python object mirroring the inferior
>> clears the inferior->Python mapping when it is finally destroyed.
>> If the Python code then requests the Python object for that inferior
>> again, a new object is created.  This is "ok" because the Inferior
>> object doesn't carry any user state.

Doug> Doesn't the caller always need to know whether s/he is getting a new
Doug> reference or a borrowed reference?
Doug> How will s/he keep the reference count correct?
Doug> [maybe I'm misunderstanding the terms used here]

I believe that in this case, the caller always gets a new reference.
gdb's "struct inferior" does not own a reference here.

There are different ways to tie the lifetimes of gdb objects and their
Python wrappers.  One model is that the gdb object owns a reference.
Another model is that it does not.

Which one we pick depends on a few factors, including whim I suppose.
If the object has user-settable state, though, then the owning model
must be preferred.

In the "does not own" model, then the destruction of the last Python
reference must also clear the link from the gdb object to the Python
object.  In this case that is done in infpy_dealloc.

It's unclear to me whether we've made the best available choices here.
There was some discussion on irc about the difficulty of making weak
references to gdb's wrapper objects.  (This may be just a buglet in the
class definitions; but it calls into question the "is_valid" model.)

Tom

Re: [RFA] fix ref counting of inferior_to_inferior_object

[Doug Evans]

On Wed, Sep 25, 2013 at 11:32 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Doug" == Doug Evans <dje@google.com> writes:
>
>>> The current model is that the Python object mirroring the inferior
>>> clears the inferior->Python mapping when it is finally destroyed.
>>> If the Python code then requests the Python object for that inferior
>>> again, a new object is created.  This is "ok" because the Inferior
>>> object doesn't carry any user state.
>
> Doug> Doesn't the caller always need to know whether s/he is getting a new
> Doug> reference or a borrowed reference?
> Doug> How will s/he keep the reference count correct?
> Doug> [maybe I'm misunderstanding the terms used here]
>
> I believe that in this case, the caller always gets a new reference.
> gdb's "struct inferior" does not own a reference here.

Blech.  Can I ask for a coding convention that prohibits the use of
the word "reference" by itself?  1/2 :-)
Instead, always use "borrowed reference" or "new reference".
Otherwise, I have to dig to make sure the author or user of the code
didn't mistake one for the other (and I'd rather be doing something
else :-)).

> There are different ways to tie the lifetimes of gdb objects and their
> Python wrappers.  One model is that the gdb object owns a reference.
> Another model is that it does not.
>
> Which one we pick depends on a few factors, including whim I suppose.
> If the object has user-settable state, though, then the owning model
> must be preferred.

Not that you disagree or anything, but IWBN to remove whim from the equation.
Consistency Is Good, and all that.

If we're going to store a pointer to the Python object in a gdb
registry, why not have a convention that gdb owns a reference?
[could be missing something of course]

> In the "does not own" model, then the destruction of the last Python
> reference must also clear the link from the gdb object to the Python
> object.  In this case that is done in infpy_dealloc.
>
> It's unclear to me whether we've made the best available choices here.
> There was some discussion on irc about the difficulty of making weak
> references to gdb's wrapper objects.  (This may be just a buglet in the
> class definitions; but it calls into question the "is_valid" model.)
>
> Tom

Re: [RFA] fix ref counting of inferior_to_inferior_object

[Tom Tromey]

Doug> Not that you disagree or anything, but IWBN to remove whim from
Doug> the equation.  Consistency Is Good, and all that.

I'm not so sure.  I think consistency is one good among many, and there
is a cost to enforcing these sorts of rules.

This is particularly true if the code is already not consistent.  Unless
maybe you are volunteering to change it all.

Doug> If we're going to store a pointer to the Python object in a gdb
Doug> registry, why not have a convention that gdb owns a reference?
Doug> [could be missing something of course]

I don't think I have an example offhand.  I suppose it is potentially a
problem in a case where there are many such objects and we don't want to
retain them all.  But those cases are probably rare, maybe non-existent
in the current code.  So maybe it is ok.

Tom

Re: [RFA] fix ref counting of inferior_to_inferior_object

[Doug Evans]

On Wed, Sep 25, 2013 at 1:11 PM, Tom Tromey <tromey@redhat.com> wrote:
> Doug> Not that you disagree or anything, but IWBN to remove whim from
> Doug> the equation.  Consistency Is Good, and all that.
>
> I'm not so sure.  I think consistency is one good among many, and there
> is a cost to enforcing these sorts of rules.

There's (in general) less cost in enforcing fewer rules.

> This is particularly true if the code is already not consistent.  Unless
> maybe you are volunteering to change it all.

I *will* volunteer to set a direction that we can all type in. :-)