[patch] [python] Implement Inferior.current_inferior

[patch] [python] Implement Inferior.current_inferior

[Phil Muldoon]

This patch implements the current_inferior function for the Python
Inferior class.  It also fixes a bug that I found along the way.  I
did not break out the bug into a separate patch as it is related to
the current inferior functionality.

The bug, in brief:

In the case of: python print gdb.selected_inferior()

When GDB does not actually have an inferior loaded into GDB, this causes
a bug where we do not clear the inferior cleanup data.  The above
statement creates a very short-lived reference to a Python object.  In
doing so, as with all inferiors, we register py_cleanup_inferior with
set_inferior_data.  However as this object is deallocated soon after,
but the empty GDB inferior is still around, that old registration still
exists and points to a now deallocated object.  In this case, I simply
added a Python deconstructor that clears that data.

A point I am not sure on is the call to set_inferior_data in the
destructor.  Should I call clear_inferior_data there?  I looked at that
function and it appears to clear out all of the inferior data, which is
something we do not want.  I instead opted to set the inferior data
matched to the key to NULL.  I am not sure if this is correct.

I've also had to change the behaviour of inferior_to_inferior_object to
always return a new reference, or NULL.  Previously that function could
return a new reference or a borrowed reference and the caller would
have to incref/decref as needed.  The problem with this approach is that
the caller does not know if the reference is new (do not increment
the reference), or borrowed (increment the reference).  This in
particular was a problem for gdb.current_inferior.  

Cheers,

Phil

-- 

2011-06-27  Phil Muldoon  <pmuldoon@redhat.com>

        PR/Python 12692

	* python/py-inferior.c (gdbpy_current_inferior): New function.
	(infpy_dealloc): Ditto.
        (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.
        * python/python.c (inferior_object_type): Register current
	inferior function.
        * python/python-internal.h: Define gdbpy_current_inferior.

2011-06-27  Phil Muldoon <pmuldoon@redhat.com>

	* gdb.texinfo (Inferiors In Python): Document current_inferior
	function.

2011-06-27  Phil Muldoon  <pmuldoon@redhat.com>

	* gdb.python/py-inferior.exp: Add current_inferior tests.

--

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 3a705c2..343fd7c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -21988,6 +21988,11 @@ module:
 Return a tuple containing all inferior objects.
 @end defun
 
+@defun current_inferior
+Return a @code{gdb.Inferior} object containing the current inferior
+selected in @value{GDBN}.
+@end defun
+
 A @code{gdb.Inferior} object has the following attributes:
 
 @table @code
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;
@@ -350,6 +374,22 @@ gdbpy_inferiors (PyObject *unused, PyObject *unused2)
   return PyList_AsTuple (list);
 }
 
+/* Returns the current inferior in GDB as a Gdb.Inferior object.  */
+PyObject *
+gdbpy_current_inferior (PyObject *self, PyObject *args)
+{
+  struct inferior *inf = current_inferior ();
+  PyObject *inferior = NULL;
+
+  /* There should always be an inferior entry, even when there are no
+     inferiors loaded into GDB.  */
+  gdb_assert (inf != NULL);
+
+  inferior = inferior_to_inferior_object (inf);
+
+  return inferior;
+}
+
 /* Membuf and memory manipulation.  */
 
 /* Implementation of gdb.read_memory (address, length).
@@ -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;
 }
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index b65109d..ab83a47 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -147,6 +147,7 @@ PyObject *gdbpy_create_lazy_string_object (CORE_ADDR address, long length,
 					   const char *encoding,
 					   struct type *type);
 PyObject *gdbpy_inferiors (PyObject *unused, PyObject *unused2);
+PyObject *gdbpy_current_inferior (PyObject *self, PyObject *args);
 PyObject *gdbpy_selected_thread (PyObject *self, PyObject *args);
 PyObject *gdbpy_string_to_argv (PyObject *self, PyObject *args);
 PyObject *gdbpy_parameter (PyObject *self, PyObject *args);
diff --git a/gdb/python/python.c b/gdb/python/python.c
index ddfe9ba..787b0ff 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1283,6 +1283,10 @@ Return the selected thread object." },
   { "inferiors", gdbpy_inferiors, METH_NOARGS,
     "inferiors () -> (gdb.Inferior, ...).\n\
 Return a tuple containing all inferiors." },
+  { "current_inferior", gdbpy_current_inferior, METH_NOARGS,
+    "current_inferior () -> gdb.Inferior.\n\
+Return the current GDB inferior object." },
+
   {NULL, NULL, 0, NULL}
 };
 
diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp
index b853c79..51b31c9 100644
--- a/gdb/testsuite/gdb.python/py-inferior.exp
+++ b/gdb/testsuite/gdb.python/py-inferior.exp
@@ -211,3 +211,46 @@ gdb_test "python print inf_list\[0\].is_valid()" "False" \
          "Check inferior validity"
 gdb_test "python print inf_list\[1\].is_valid()" "True" \
          "Check inferior validity"
+
+# Test current_inferior () functionality.
+# Start with a fresh gdb.
+clean_restart ${testfile}
+if ![runto_main] then {
+    fail "Can't run to main"
+    return 0
+}
+
+gdb_test_no_output "python i = gdb.current_inferior()" \
+    "Check the current inferior"
+
+gdb_test "python print i" \
+    "<gdb.Inferior object at 0x\[\[:xdigit:\]\]+>" \
+    "Verify inferior from current_inferior"
+gdb_test "python print 'result =', i.num" " = \[0-9\]+" \
+    "Test current_inferior Inferior.num"
+gdb_test "python print 'result =', i.pid" " = \[0-9\]+" \
+    "Test current_inferior Inferior.pid"
+gdb_test_no_output "python t = i.threads()" \
+    "Allocate threads from the current_inferior"
+gdb_test "python print len(t)" "1" \
+    "There should only be one thread from current_inferior"
+gdb_test "python print t\[0\].name" ".*py-inferior.*" \
+    "The name of the current_inferior thread"
+
+clean_restart ${testfile}
+
+# Test empty inferior
+gdb_test_no_output "python ie = gdb.current_inferior()" \
+    "Check the current empty inferior"
+
+gdb_test "python print ie" \
+    "<gdb.Inferior object at 0x\[\[:xdigit:\]\]+>" \
+    "Verify empty inferior from current_inferior"
+gdb_test "python print 'result =', ie.num" " = 1" \
+    "Test current_inferior Inferior.num"
+gdb_test "python print 'result =', ie.pid" " = 0" \
+    "Test current_inferior Inferior.pid"
+gdb_test_no_output "python te = ie.threads()" \
+    "Allocate threads from the current_inferior"
+gdb_test "python print len(te)" "0" \
+    "There should not be any threads from empty inferior"

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

[Kevin Pouget]

Hi,

I actually proposed and got approved a patch to solve this bug two months ago,

http://sources.redhat.com/ml/gdb-patches/2011-04/msg00389.html

but I'm still waiting for these crazy copyright assignment papers to
cross the ocean, it looks like that they won't never reach the old
continent, too bad ...

cheers,

Kevin

On Mon, Jun 27, 2011 at 4:10 PM, Phil Muldoon <pmuldoon@redhat.com> wrote:
>
> This patch implements the current_inferior function for the Python
> Inferior class.  It also fixes a bug that I found along the way.  I
> did not break out the bug into a separate patch as it is related to
> the current inferior functionality.
>
> The bug, in brief:
>
> In the case of: python print gdb.selected_inferior()
>
> When GDB does not actually have an inferior loaded into GDB, this causes
> a bug where we do not clear the inferior cleanup data.  The above
> statement creates a very short-lived reference to a Python object.  In
> doing so, as with all inferiors, we register py_cleanup_inferior with
> set_inferior_data.  However as this object is deallocated soon after,
> but the empty GDB inferior is still around, that old registration still
> exists and points to a now deallocated object.  In this case, I simply
> added a Python deconstructor that clears that data.
>
> A point I am not sure on is the call to set_inferior_data in the
> destructor.  Should I call clear_inferior_data there?  I looked at that
> function and it appears to clear out all of the inferior data, which is
> something we do not want.  I instead opted to set the inferior data
> matched to the key to NULL.  I am not sure if this is correct.
>
> I've also had to change the behaviour of inferior_to_inferior_object to
> always return a new reference, or NULL.  Previously that function could
> return a new reference or a borrowed reference and the caller would
> have to incref/decref as needed.  The problem with this approach is that
> the caller does not know if the reference is new (do not increment
> the reference), or borrowed (increment the reference).  This in
> particular was a problem for gdb.current_inferior.
>
> Cheers,
>
> Phil
>
> --
>
> 2011-06-27  Phil Muldoon  <pmuldoon@redhat.com>
>
>        PR/Python 12692
>
>        * python/py-inferior.c (gdbpy_current_inferior): New function.
>        (infpy_dealloc): Ditto.
>        (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.
>        * python/python.c (inferior_object_type): Register current
>        inferior function.
>        * python/python-internal.h: Define gdbpy_current_inferior.
>
> 2011-06-27  Phil Muldoon <pmuldoon@redhat.com>
>
>        * gdb.texinfo (Inferiors In Python): Document current_inferior
>        function.
>
> 2011-06-27  Phil Muldoon  <pmuldoon@redhat.com>
>
>        * gdb.python/py-inferior.exp: Add current_inferior tests.
>
> --
>
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 3a705c2..343fd7c 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -21988,6 +21988,11 @@ module:
>  Return a tuple containing all inferior objects.
>  @end defun
>
> +@defun current_inferior
> +Return a @code{gdb.Inferior} object containing the current inferior
> +selected in @value{GDBN}.
> +@end defun
> +
>  A @code{gdb.Inferior} object has the following attributes:
>
>  @table @code
> 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;
> @@ -350,6 +374,22 @@ gdbpy_inferiors (PyObject *unused, PyObject *unused2)
>   return PyList_AsTuple (list);
>  }
>
> +/* Returns the current inferior in GDB as a Gdb.Inferior object.  */
> +PyObject *
> +gdbpy_current_inferior (PyObject *self, PyObject *args)
> +{
> +  struct inferior *inf = current_inferior ();
> +  PyObject *inferior = NULL;
> +
> +  /* There should always be an inferior entry, even when there are no
> +     inferiors loaded into GDB.  */
> +  gdb_assert (inf != NULL);
> +
> +  inferior = inferior_to_inferior_object (inf);
> +
> +  return inferior;
> +}
> +
>  /* Membuf and memory manipulation.  */
>
>  /* Implementation of gdb.read_memory (address, length).
> @@ -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;
>  }
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index b65109d..ab83a47 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -147,6 +147,7 @@ PyObject *gdbpy_create_lazy_string_object (CORE_ADDR address, long length,
>                                           const char *encoding,
>                                           struct type *type);
>  PyObject *gdbpy_inferiors (PyObject *unused, PyObject *unused2);
> +PyObject *gdbpy_current_inferior (PyObject *self, PyObject *args);
>  PyObject *gdbpy_selected_thread (PyObject *self, PyObject *args);
>  PyObject *gdbpy_string_to_argv (PyObject *self, PyObject *args);
>  PyObject *gdbpy_parameter (PyObject *self, PyObject *args);
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index ddfe9ba..787b0ff 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -1283,6 +1283,10 @@ Return the selected thread object." },
>   { "inferiors", gdbpy_inferiors, METH_NOARGS,
>     "inferiors () -> (gdb.Inferior, ...).\n\
>  Return a tuple containing all inferiors." },
> +  { "current_inferior", gdbpy_current_inferior, METH_NOARGS,
> +    "current_inferior () -> gdb.Inferior.\n\
> +Return the current GDB inferior object." },
> +
>   {NULL, NULL, 0, NULL}
>  };
>
> diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp
> index b853c79..51b31c9 100644
> --- a/gdb/testsuite/gdb.python/py-inferior.exp
> +++ b/gdb/testsuite/gdb.python/py-inferior.exp
> @@ -211,3 +211,46 @@ gdb_test "python print inf_list\[0\].is_valid()" "False" \
>          "Check inferior validity"
>  gdb_test "python print inf_list\[1\].is_valid()" "True" \
>          "Check inferior validity"
> +
> +# Test current_inferior () functionality.
> +# Start with a fresh gdb.
> +clean_restart ${testfile}
> +if ![runto_main] then {
> +    fail "Can't run to main"
> +    return 0
> +}
> +
> +gdb_test_no_output "python i = gdb.current_inferior()" \
> +    "Check the current inferior"
> +
> +gdb_test "python print i" \
> +    "<gdb.Inferior object at 0x\[\[:xdigit:\]\]+>" \
> +    "Verify inferior from current_inferior"
> +gdb_test "python print 'result =', i.num" " = \[0-9\]+" \
> +    "Test current_inferior Inferior.num"
> +gdb_test "python print 'result =', i.pid" " = \[0-9\]+" \
> +    "Test current_inferior Inferior.pid"
> +gdb_test_no_output "python t = i.threads()" \
> +    "Allocate threads from the current_inferior"
> +gdb_test "python print len(t)" "1" \
> +    "There should only be one thread from current_inferior"
> +gdb_test "python print t\[0\].name" ".*py-inferior.*" \
> +    "The name of the current_inferior thread"
> +
> +clean_restart ${testfile}
> +
> +# Test empty inferior
> +gdb_test_no_output "python ie = gdb.current_inferior()" \
> +    "Check the current empty inferior"
> +
> +gdb_test "python print ie" \
> +    "<gdb.Inferior object at 0x\[\[:xdigit:\]\]+>" \
> +    "Verify empty inferior from current_inferior"
> +gdb_test "python print 'result =', ie.num" " = 1" \
> +    "Test current_inferior Inferior.num"
> +gdb_test "python print 'result =', ie.pid" " = 0" \
> +    "Test current_inferior Inferior.pid"
> +gdb_test_no_output "python te = ie.threads()" \
> +    "Allocate threads from the current_inferior"
> +gdb_test "python print len(te)" "0" \
> +    "There should not be any threads from empty inferior"
>

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

[Phil Muldoon]

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

> Hi,
>
> I actually proposed and got approved a patch to solve this bug two months ago,
>
> http://sources.redhat.com/ml/gdb-patches/2011-04/msg00389.html
>
> but I'm still waiting for these crazy copyright assignment papers to
> cross the ocean, it looks like that they won't never reach the old
> continent, too bad ...

Ping the maintainers and see if they can rattle some FSF folks into
action.

I do not mind which patch fixes the issues.  But please assign a bug to
yourself if you are working on it, so we don't duplicate effort.  (On
that note, I did not assign it to myself yet either so touche).

Whether your patch, or mine makes it in I don't mind.  The patches looks
similar, but mine has a few extra tests and accounts for the reference
leaking/sigsegv.  I would be happy to add your name to the ChangeLog as
co-implementer if the maintainers think this is ok (and they approve the
patch).

Cheers,

Phil

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

[Joel Brobecker]

> Whether your patch, or mine makes it in I don't mind.  The patches looks
> similar, but mine has a few extra tests and accounts for the reference
> leaking/sigsegv.  I would be happy to add your name to the ChangeLog as
> co-implementer if the maintainers think this is ok (and they approve the
> patch).

It could be a merge of the two patches, with both names on the CL,
I think.

-- 
Joel

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

[Phil Muldoon]

Joel Brobecker <brobecker@adacore.com> writes:

>> Whether your patch, or mine makes it in I don't mind.  The patches looks
>> similar, but mine has a few extra tests and accounts for the reference
>> leaking/sigsegv.  I would be happy to add your name to the ChangeLog as
>> co-implementer if the maintainers think this is ok (and they approve the
>> patch).
>
> It could be a merge of the two patches, with both names on the CL,
> I think.

As our patches are basically the same, the merge would not really be
needed.  Apart from the reference counting hunks, they are basically the
same.  I would take my patch which deals with the reference counting
issue and add Kevin's name to the ChangeLog. (At least to me that is the
cleanest way to do it other than trying to merge two very similar
patches.)

My real question was: As Kevin is lost in FSF paperwork with substantial
delays, if it would be okay under the current FSF assignment rules to
co-credit the ChangeLog with Kevin as the author.  It probably isn't - I
know some projects do this, but I have no experience with this scenario
in GDB.  As Kevin has a previous implementation that is pending FSF
paperwork (I am not sure if his patch was reviewed), I think he should
have the first shot/discussion on that patch.  I guess we have to just
wait?



Cheers,

Phil

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

[Kevin Pouget]

Hi,

sorry for the troubles, I'll assign the patches to myself next times!

> I would be happy to add your name to the ChangeLog as
> co-implementer if the maintainers think this is ok

thanks for this offer, I think it would be fair to do it this way if
it's possible

>  As Kevin has a previous implementation that is pending FSF
> paperwork (I am not sure if his patch was reviewed)

the patch was reviewed and accepted,

> http://sources.redhat.com/ml/gdb-patches/2011-04/msg00453.html
> From: Tom Tromey
> Space before open paren.
> This is ok with this changed, pending doc review.
> Tom
and
> http://sources.redhat.com/ml/gdb-patches/2011-05/msg00677.html
> From: Eli Zaretskii
> The patch for the manual is okay.

(I bounced the the copyright clerk, don't know why it's so long ...)


thanks,

Kevin

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

[Phil Muldoon]

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

> Hi,
>
> sorry for the troubles, I'll assign the patches to myself next times!

That's ok, like I mentioned I did not do it for this bug either!  But
assignments do avoid issues like these.  As it happens I spent most of
the time with the patch fixing the reference counting buglets.  So go
ahead and assign yourself whatever bugs you are working on now.  When
you get your FSF paperwork sorted generally, and maintainer approval for
each patch, make sure you put the bug number in the ChangeLog (look for
other examples of this), and close the bug with the target milestone set
for the expected release of this patch (e.g. 7.4).

> the patch was reviewed and accepted,

Ok, then lets do this.  You wrote the patch, it was reviewed and
accepted so your patch should go in. This functionality is targeted
for 7.4 and so that is quite a long time away. You will (hopefully) have
FSF assignment sorted by then!  I'll break out the reference counting
fixes and submit it as a separate patch.  This is important as
selected_inferior will crash on short lived anonymous references.  The
two patches should come together nicely, then.


Does this sound okay to you?

Thanks again for all your work, Kevin.

Cheers,

Phil

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

[Tom Tromey]

>>>>> "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.

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.

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.

Tom

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

[Tom Tromey]

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

Kevin> I actually proposed and got approved a patch to solve this bug
Kevin> two months ago,
Kevin> http://sources.redhat.com/ml/gdb-patches/2011-04/msg00389.html
Kevin> but I'm still waiting for these crazy copyright assignment papers to
Kevin> cross the ocean, it looks like that they won't never reach the old
Kevin> continent, too bad ...

I don't know what is up, but the FSF seems to be going very slowly with
assignments again.  I think all you can do is keep pinging them.

Tom

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

[Phil Muldoon]

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