[4/5] Make Python inferior-related internal functions return a gdbpy_inf_ref

  From: Simon Marchi <simon.marchi@polymtl.ca>

The functions inferior_to_inferior_object and find_inferior_object
return a new reference to an inferior_object.  This means that the
caller owns that reference and is responsible for decrementing it when
it's done.  To avoid the possibility of the caller forgetting to DECREF
when it's done with the reference, make those functions return a
gdbpy_inf_ref instead of a plain pointer.

If the caller doesn't need the reference after it has used it,
gdbpy_inf_ref will take care of removing that reference.  If the
reference needs to outlive the gdbpy_inf_ref object (e.g. because we are
return the value to Python, which will take ownership of the reference),
the caller will have to release the pointer.  At least it will be
explicit and it won't be ambiguous.

I added comments in inferior_to_inferior_object for the poor souls who
will have to deal with this again in the future.

A couple of things I am not sure about:

  * I am not sure whether the behaviour is right with the assignment
  operator in delete_thread_object, so if somebody could take a look at
  that in particular it would be appreciated:

    gdbpy_inf_ref inf_obj_ref = find_inferior_object (ptid_get_pid (tp->ptid));

  I suppose it's the operator= version which moves the reference that is
  invoked?

  * I used .release() on the reference in create_thread_object with a
  comment explaining why, but I would need some more pairs of eyes on
  that to see if it's right.

gdb/ChangeLog:

	* python/py-inferior.c (inferior_to_inferior_object): Return a
	gdbpy_inf_ref.
	(find_inferior_object): Return a gdbpy_inf_ref.
	(delete_thread_object): Use gdbpy_inf_ref.
	(gdbpy_selected_inferior): Likewise.
	* python/py-infthread.c (create_thread_object): Adapt to
	gdbpy_inf_ref.
	* python/python-internal.h: Include py-ref.h.
	(inferior_to_inferior_object): Return a gdbpy_inf_ref.
	(find_inferior_object): Likewise.
---
 gdb/python/py-inferior.c     | 41 +++++++++++++++++++----------------------
 gdb/python/py-infthread.c    |  6 +++++-
 gdb/python/py-ref.h          |  2 ++
 gdb/python/python-internal.h |  6 ++++--
 4 files changed, 30 insertions(+), 25 deletions(-)
  

On 2017-01-23 17:40, Simon Marchi wrote:
>   * I used .release() on the reference in create_thread_object with a
>   comment explaining why, but I would need some more pairs of eyes on
>   that to see if it's right.

Ok, so I've looked at this a bit more, and I think that my comment is 
wrong, but the code is right (meaning that the comment didn't match the 
code in the first place).

> diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c
> index c7553310c3..79fb5d12d1 100644
> --- a/gdb/python/py-infthread.c
> +++ b/gdb/python/py-infthread.c
> @@ -46,7 +46,11 @@ create_thread_object (struct thread_info *tp)
>      return NULL;
> 
>    thread_obj->thread = tp;
> -  thread_obj->inf_obj = find_inferior_object (ptid_get_pid 
> (tp->ptid));
> +
> +  /* The thread holds a weak reference to its inferior to avoid 
> creating a
> +     reference loop between the inferior and its threads.  */
> +  gdbpy_inf_ref inf_obj_ref = find_inferior_object (ptid_get_pid 
> (tp->ptid));
> +  thread_obj->inf_obj = inf_obj_ref.release ();

The Thread objects do hold strong references to their Inferior object, 
and it's not really a problem.  The only way for an Inferior object to 
be deallocated is if the corresponding gdb inferior object is removed.  
A prerequisite of this happening is that all its threads have exited 
(either by finishing themselves or by being killed by gdb).  When a 
thread exits, we remove the reference from the Inferior to the Thread.  
If no other reference to the Thread exist (e.g. a Python variable), the 
Thread will be deallocated immediately, removing the reference it had to 
the Inferior.

The reference cycle should only exist while the threads are running, 
which is not a state in which we can nor want to deallocate the Inferior 
object.  As soon as the threads exit, the cycle is broken.

So the .release() is right, the corresponding DECREF is in thpy_dealloc.
  

On 01/23/2017 10:40 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> The functions inferior_to_inferior_object and find_inferior_object
> return a new reference to an inferior_object.  This means that the
> caller owns that reference and is responsible for decrementing it when
> it's done.  To avoid the possibility of the caller forgetting to DECREF
> when it's done with the reference, make those functions return a
> gdbpy_inf_ref instead of a plain pointer.

I like this style of API.  I've argued for it before too.

> If the caller doesn't need the reference after it has used it,
> gdbpy_inf_ref will take care of removing that reference.  If the
> reference needs to outlive the gdbpy_inf_ref object (e.g. because we are
> return the value to Python, which will take ownership of the reference),
> the caller will have to release the pointer.  At least it will be
> explicit and it won't be ambiguous.
> 
> I added comments in inferior_to_inferior_object for the poor souls who
> will have to deal with this again in the future.
> 
> A couple of things I am not sure about:
> 
>   * I am not sure whether the behaviour is right with the assignment
>   operator in delete_thread_object, so if somebody could take a look at
>   that in particular it would be appreciated:
> 
>     gdbpy_inf_ref inf_obj_ref = find_inferior_object (ptid_get_pid (tp->ptid));
> 
>   I suppose it's the operator= version which moves the reference that is
>   invoked?

Since this is initialization, op= is not called.  This either
calls the copy constructor, or find_inferior_object constructs the
object that it returns directly on top of &inf_obj_ref
(i.e., no copy at all) [RVO/NRVO].

http://stackoverflow.com/questions/2847787/constructor-or-assignment-operator
https://en.wikipedia.org/wiki/Return_value_optimization

> @@ -207,39 +207,38 @@ python_new_objfile (struct objfile *objfile)
>     representing INFERIOR.  If the object has already been created,
>     return it and increment the reference count,  otherwise, create it.
>     Return NULL on failure.  */
> -inferior_object *
> +gdbpy_inf_ref
>  inferior_to_inferior_object (struct inferior *inferior)
>  {
...
> -      if (!inf_obj)
> -	  return NULL;
> +      if (inf_obj == NULL)
> +	return gdbpy_inf_ref ();

You shouldn't need changes like this one.  gdbpy_ref has an
implicit ctor that takes nullptr_t exactly to allow implicit
construction from null.
>  

> @@ -304,39 +303,34 @@ add_thread_object (struct thread_info *tp)
>  static void
>  delete_thread_object (struct thread_info *tp, int ignore)
>  {
> -  inferior_object *inf_obj;
>    struct threadlist_entry **entry, *tmp;
>  
>    if (!gdb_python_initialized)
>      return;
>  
>    gdbpy_enter enter_py (python_gdbarch, python_language);
> +  gdbpy_inf_ref inf_obj_ref = find_inferior_object (ptid_get_pid (tp->ptid));
>  
> -  inf_obj
> -    = (inferior_object *) find_inferior_object (ptid_get_pid (tp->ptid));
> -  if (!inf_obj)
> +  if (inf_obj_ref == NULL)
>      return;
>  
>    /* Find thread entry in its inferior's thread_list.  */
> -  for (entry = &inf_obj->threads; *entry != NULL; entry =
> -	 &(*entry)->next)
> +  for (entry = &inf_obj_ref.get ()->threads;

Hmm, changes like these are odd.  gdbpy_ref has an operator->
implementation, so inf_obj->threads should do the right thing?

> @@ -815,7 +809,10 @@ py_free_inferior (struct inferior *inf, void *datum)
>  PyObject *
>  gdbpy_selected_inferior (PyObject *self, PyObject *args)
>  {
> -  return (PyObject *) inferior_to_inferior_object (current_inferior ());
> +  gdbpy_inf_ref inf_obj_ref (inferior_to_inferior_object (current_inferior ()));

If the function returns gdbpy_inf_ref already, I much prefer
using = initialization over (), like:

  gdbpy_inf_ref inf_obj_ref
     = inferior_to_inferior_object (current_inferior ());

The reason is that this makes it more obvious what is going on.
The ctor taking a PyObject* is explicit so inferior_to_inferior_object
must be returning a gdbpy_inf_ref.

With:

  gdbpy_inf_ref inf_obj_ref (inferior_to_inferior_object (current_inferior ()));

one has to wonder what constructor is being called, and whether there's
some kind of explicit conversion going on.

So the = version is more to the point and thus makes it
for a clearer read because there's less to reason about.

> +
> +  /* Release the reference, it will now be managed by Python.  */
> +  return (PyObject *) inf_obj_ref.release ();
>  }

Thanks,
Pedro Alves
  

Thanks for the comments.  I'll update my branch, but I'll wait until 
Tom's series is pushed and see what's still relevant in mine.

On 2017-02-09 07:30, Pedro Alves wrote:
>> @@ -207,39 +207,38 @@ python_new_objfile (struct objfile *objfile)
>>     representing INFERIOR.  If the object has already been created,
>>     return it and increment the reference count,  otherwise, create 
>> it.
>>     Return NULL on failure.  */
>> -inferior_object *
>> +gdbpy_inf_ref
>>  inferior_to_inferior_object (struct inferior *inferior)
>>  {
> ...
>> -      if (!inf_obj)
>> -	  return NULL;
>> +      if (inf_obj == NULL)
>> +	return gdbpy_inf_ref ();
> 
> You shouldn't need changes like this one.  gdbpy_ref has an
> implicit ctor that takes nullptr_t exactly to allow implicit
> construction from null.

Ok.  This required adding the corresponding constructor in 
gdbpy_ref_base:

     gdbpy_ref_base (const std::nullptr_t)
     : gdb::ref_ptr<T, gdbpy_ref_policy<T>> (nullptr)
     {
     }

>>    /* Find thread entry in its inferior's thread_list.  */
>> -  for (entry = &inf_obj->threads; *entry != NULL; entry =
>> -	 &(*entry)->next)
>> +  for (entry = &inf_obj_ref.get ()->threads;
> 
> Hmm, changes like these are odd.  gdbpy_ref has an operator->
> implementation, so inf_obj->threads should do the right thing?

Hmm you're right, not sure why I added those.

>> @@ -815,7 +809,10 @@ py_free_inferior (struct inferior *inf, void 
>> *datum)
>>  PyObject *
>>  gdbpy_selected_inferior (PyObject *self, PyObject *args)
>>  {
>> -  return (PyObject *) inferior_to_inferior_object (current_inferior 
>> ());
>> +  gdbpy_inf_ref inf_obj_ref (inferior_to_inferior_object 
>> (current_inferior ()));
> 
> If the function returns gdbpy_inf_ref already, I much prefer
> using = initialization over (), like:
> 
>   gdbpy_inf_ref inf_obj_ref
>      = inferior_to_inferior_object (current_inferior ());
> 
> The reason is that this makes it more obvious what is going on.
> The ctor taking a PyObject* is explicit so inferior_to_inferior_object
> must be returning a gdbpy_inf_ref.
> 
> With:
> 
>   gdbpy_inf_ref inf_obj_ref (inferior_to_inferior_object 
> (current_inferior ()));
> 
> one has to wonder what constructor is being called, and whether there's
> some kind of explicit conversion going on.
> 
> So the = version is more to the point and thus makes it
> for a clearer read because there's less to reason about.

Right, it's more obvious.

Thanks,

Simon