Re: [patch] [python] Implement Inferior.current_inferior

[Phil Muldoon <pmuldoon@redhat.com>]

Tom Tromey <tromey@redhat.com> writes:

>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
>
> Phil> -/* Return a borrowed reference to the Python object of type Inferior
> Phil> +/* Return a reference to the Python object of type Inferior
> Phil>     representing INFERIOR.  If the object has already been created,
> Phil> -   return it,  otherwise, create it.  Return NULL on failure.  */
> Phil> +   return it and increment the reference count,  otherwise, create it.
> Phil> +   Return NULL on failure.  */
> Phil>  PyObject *
> Phil>  inferior_to_inferior_object (struct inferior *inferior)
> Phil>  {
> Phil> @@ -154,13 +155,14 @@ inferior_to_inferior_object (struct inferior *inferior)
>  
> Phil>        do_cleanups (cleanup);
> Phil>      }
> Phil> +  else
> Phil> +    Py_INCREF ((PyObject *)inf_obj);
>
> I don't really follow this.

Well it took me a bit too.  inferior_to_inferior_object is used in a
multitude of scenarios.  This patchlet addresses a new Python object
created to follow an internal GDB inferior.

Take this example:

foo = gdb.current_inferior()
bar = gdb.current_inferior()

So the first time with foo, a new Python inferior object is created. And
that reference is stashed in the GDB inferior object with
set_inferior_data.  The second time, inferior_to_inferior_object queries
the inferior, finds the reference stashed in the GDB inferior and
returns that stashed Python inferior object.  But before this patch, bar
would not have had the reference count incremented.  This bug was not
really exposed before the current_inferior patch.  This patch fixes
that. Why? Because, if you do:

foo = None

Then the reference count = 0, and bar points to garbage after Python
GC's the foo object.

 
> I think the best model would be that the 'struct inferior' owns a
> reference to the gdb.Inferior object, and then
> inferior_to_inferior_object is consistent: it either always returns a
> new reference or a borrowed reference.

In the context of this patch, I decided against a wholesale rewriting of
how we store inferiors.  Now that his patch is split, it might be time
to implement your suggestion.

> For a borrowed reference, I think you can leave this function as-is.
>
> For a new reference, I think you need to remove the 'else' from your
> patch, and update infpy_dealloc to decref.

Functions that return either a borrowed or new references really are
bogus IMO.  The consumer does not know what they are getting.
inferior_to_inferior_object does this.  This patch uniformly returns a
new reference, and the callers are responsible for the life-cycle of that
reference.  While that means a little more busy-work for the callers,
they can be consistent in dealing with the life-cycle of that object.

That being said, I kind of like your idea above. 

Cheers,

Phil