[PATCH 0/4] Disallow the return of borrowed references

[PATCH 0/4] Disallow the return of borrowed references

[Tom Tromey]

While working on a different Python patch, I misread
pspace_to_pspace_object and thought it was not handling reference
counts properly.

It is -- but I found it difficult to reason about because it returns a
borrowed reference, but uses gdbpy_ref as well.

This series changes gdb so that none of the Python functions will
return a borrowed reference.  These functions are modified instead to
return a new reference using gdbpy_ref<>, which not only makes the
intent clear, but also makes it more difficult to have a buggy caller.

This could go further and make all functions return gdbpy_ref<>.
(Even Python-facing ones could be done via template fuction wrappers.)
However I have not done this.

I think now gdb should disallow Python functions returning borrowed
references.  Accepting a borrowed reference is still ok, and I think
should continue to be.

I think this series found at least one bug:
infpy_thread_from_thread_handle could return None without incref'ing
it.

Let me know what you think.  I tested this on x86-64 Fedora 28.

Tom

[PATCH 1/4] Change pspace_to_pspace_object to return a new reference

[Tom Tromey]

This changes pspace_to_pspace_object to return a new reference and
fixes up all the callers.

gdb/ChangeLog
2018-09-12  Tom Tromey  <tom@tromey.com>

	* python/python-internal.h (pspace_to_pspace_object): Change
	return type.
	* python/py-newobjfileevent.c
	(create_clear_objfiles_event_object): Update.
	* python/py-xmethods.c (gdbpy_get_matching_xmethod_workers):
	Update.
	* python/python.c (gdbpy_get_current_progspace): Update.
	(gdbpy_progspaces): Update.
	* python/py-progspace.c (pspace_to_pspace_object): Return a new
	reference.
	* python/py-objfile.c (objfpy_get_progspace): Update.
	* python/py-prettyprint.c (find_pretty_printer_from_progspace):
	Update.
---
 gdb/ChangeLog                   | 16 ++++++++++++++++
 gdb/python/py-newobjfileevent.c | 10 ++++------
 gdb/python/py-objfile.c         |  6 ++----
 gdb/python/py-prettyprint.c     |  6 +++---
 gdb/python/py-progspace.c       | 31 ++++++++++++++++---------------
 gdb/python/py-xmethods.c        |  6 +++---
 gdb/python/python-internal.h    |  3 +--
 gdb/python/python.c             | 11 +++--------
 8 files changed, 48 insertions(+), 41 deletions(-)

diff --git a/gdb/python/py-newobjfileevent.c b/gdb/python/py-newobjfileevent.c
index a9341a3be18..aa31fb4849b 100644
--- a/gdb/python/py-newobjfileevent.c
+++ b/gdb/python/py-newobjfileevent.c
@@ -66,12 +66,10 @@ create_clear_objfiles_event_object (void)
   if (objfile_event == NULL)
     return NULL;
 
-  /* Note that pspace_to_pspace_object returns a borrowed reference,
-     so we don't need a decref here.  */
-  PyObject *py_progspace = pspace_to_pspace_object (current_program_space);
-  if (!py_progspace || evpy_add_attribute (objfile_event.get (),
-					   "progspace",
-					   py_progspace) < 0)
+  gdbpy_ref<> py_progspace = pspace_to_pspace_object (current_program_space);
+  if (py_progspace == NULL || evpy_add_attribute (objfile_event.get (),
+						  "progspace",
+						  py_progspace.get ()) < 0)
     return NULL;
 
   return objfile_event;
diff --git a/gdb/python/py-objfile.c b/gdb/python/py-objfile.c
index c2b40ff5352..8fd160c7bf8 100644
--- a/gdb/python/py-objfile.c
+++ b/gdb/python/py-objfile.c
@@ -167,10 +167,8 @@ objfpy_get_progspace (PyObject *self, void *closure)
 
   if (obj->objfile)
     {
-      PyObject *pspace =  pspace_to_pspace_object (obj->objfile->pspace);
-
-      Py_XINCREF (pspace);
-      return pspace;
+      gdbpy_ref<> pspace = pspace_to_pspace_object (obj->objfile->pspace);
+      return pspace.get ();
     }
 
   Py_RETURN_NONE;
diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c
index 7da0f2d9af2..9aadd3b27c9 100644
--- a/gdb/python/py-prettyprint.c
+++ b/gdb/python/py-prettyprint.c
@@ -128,11 +128,11 @@ find_pretty_printer_from_objfiles (PyObject *value)
 static PyObject *
 find_pretty_printer_from_progspace (PyObject *value)
 {
-  PyObject *obj = pspace_to_pspace_object (current_program_space);
+  gdbpy_ref<> obj = pspace_to_pspace_object (current_program_space);
 
-  if (!obj)
+  if (obj == NULL)
     return NULL;
-  gdbpy_ref<> pp_list (pspy_get_printers (obj, NULL));
+  gdbpy_ref<> pp_list (pspy_get_printers (obj.get (), NULL));
   return search_pp_list (pp_list.get (), value);
 }
 
diff --git a/gdb/python/py-progspace.c b/gdb/python/py-progspace.c
index 3eaa466666d..2aeeca92eb2 100644
--- a/gdb/python/py-progspace.c
+++ b/gdb/python/py-progspace.c
@@ -337,30 +337,31 @@ py_free_pspace (struct program_space *pspace, void *datum)
   object->pspace = NULL;
 }
 
-/* Return a borrowed reference to the Python object of type Pspace
+/* Return a new reference to the Python object of type Pspace
    representing PSPACE.  If the object has already been created,
    return it.  Otherwise, create it.  Return NULL and set the Python
    error on failure.  */
 
-PyObject *
+gdbpy_ref<>
 pspace_to_pspace_object (struct program_space *pspace)
 {
-  gdbpy_ref<pspace_object> object
-    ((pspace_object *) program_space_data (pspace, pspy_pspace_data_key));
-  if (object == NULL)
+  PyObject *result
+    ((PyObject *) program_space_data (pspace, pspy_pspace_data_key));
+  if (result == NULL)
     {
-      object.reset (PyObject_New (pspace_object, &pspace_object_type));
-      if (object != NULL)
-	{
-	  if (!pspy_initialize (object.get ()))
-	    return NULL;
-
-	  object->pspace = pspace;
-	  set_program_space_data (pspace, pspy_pspace_data_key, object.get ());
-	}
+      gdbpy_ref<pspace_object> object
+	((pspace_object *) PyObject_New (pspace_object, &pspace_object_type));
+      if (object == NULL)
+	return NULL;
+      if (!pspy_initialize (object.get ()))
+	return NULL;
+
+      object->pspace = pspace;
+      set_program_space_data (pspace, pspy_pspace_data_key, object.get ());
+      result = (PyObject *) object.release ();
     }
 
-  return (PyObject *) object.release ();
+  return gdbpy_ref<>::new_reference (result);
 }
 
 int
diff --git a/gdb/python/py-xmethods.c b/gdb/python/py-xmethods.c
index f7e3c37210c..c568295a165 100644
--- a/gdb/python/py-xmethods.c
+++ b/gdb/python/py-xmethods.c
@@ -122,7 +122,6 @@ gdbpy_get_matching_xmethod_workers
    std::vector<xmethod_worker_up> *dm_vec)
 {
   struct objfile *objfile;
-  PyObject *py_progspace;
 
   gdb_assert (obj_type != NULL && method_name != NULL);
 
@@ -170,10 +169,11 @@ gdbpy_get_matching_xmethod_workers
 
   /* Gather debug methods matchers registered with the current program
      space.  */
-  py_progspace = pspace_to_pspace_object (current_program_space);
+  gdbpy_ref<> py_progspace = pspace_to_pspace_object (current_program_space);
   if (py_progspace != NULL)
     {
-      gdbpy_ref<> pspace_matchers (pspy_get_xmethods (py_progspace, NULL));
+      gdbpy_ref<> pspace_matchers (pspy_get_xmethods (py_progspace.get (),
+						      NULL));
 
       gdbpy_ref<> temp (PySequence_Concat (py_xmethod_matcher_list.get (),
 					   pspace_matchers.get ()));
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 3874fdc5e79..a6dc5f090b5 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -517,8 +517,7 @@ PyObject *value_to_value_object (struct value *v);
 PyObject *type_to_type_object (struct type *);
 PyObject *frame_info_to_frame_object (struct frame_info *frame);
 PyObject *symtab_to_linetable_object (PyObject *symtab);
-PyObject *pspace_to_pspace_object (struct program_space *)
-    CPYCHECKER_RETURNS_BORROWED_REF;
+gdbpy_ref<> pspace_to_pspace_object (struct program_space *);
 PyObject *pspy_get_printers (PyObject *, void *);
 PyObject *pspy_get_frame_filters (PyObject *, void *);
 PyObject *pspy_get_frame_unwinders (PyObject *, void *);
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 6f798a0e461..93d6d1156d8 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1339,12 +1339,7 @@ gdbpy_print_stack (void)
 static PyObject *
 gdbpy_get_current_progspace (PyObject *unused1, PyObject *unused2)
 {
-  PyObject *result;
-
-  result = pspace_to_pspace_object (current_program_space);
-  if (result)
-    Py_INCREF (result);
-  return result;
+  return pspace_to_pspace_object (current_program_space).release ();
 }
 
 /* Return a sequence holding all the Progspaces.  */
@@ -1360,9 +1355,9 @@ gdbpy_progspaces (PyObject *unused1, PyObject *unused2)
 
   ALL_PSPACES (ps)
   {
-    PyObject *item = pspace_to_pspace_object (ps);
+    gdbpy_ref<> item = pspace_to_pspace_object (ps);
 
-    if (!item || PyList_Append (list.get (), item) == -1)
+    if (item == NULL || PyList_Append (list.get (), item.get ()) == -1)
       return NULL;
   }
 
-- 
2.17.1

Re: [PATCH 1/4] Change pspace_to_pspace_object to return a new reference

[Simon Marchi]

On 2018-09-13 1:30 a.m., Tom Tromey wrote:
> diff --git a/gdb/python/py-objfile.c b/gdb/python/py-objfile.c
> index c2b40ff5352..8fd160c7bf8 100644
> --- a/gdb/python/py-objfile.c
> +++ b/gdb/python/py-objfile.c
> @@ -167,10 +167,8 @@ objfpy_get_progspace (PyObject *self, void *closure)
>  
>    if (obj->objfile)
>      {
> -      PyObject *pspace =  pspace_to_pspace_object (obj->objfile->pspace);
> -
> -      Py_XINCREF (pspace);
> -      return pspace;
> +      gdbpy_ref<> pspace = pspace_to_pspace_object (obj->objfile->pspace);
> +      return pspace.get ();
>      }

Can you double check this, shouldn't it be .release()?  Otherwise, LGTM.

Simon

Re: [PATCH 1/4] Change pspace_to_pspace_object to return a new reference

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> +      gdbpy_ref<> pspace = pspace_to_pspace_object (obj->objfile->pspace);
>> +      return pspace.get ();
>> }

Simon> Can you double check this, shouldn't it be .release()?  Otherwise, LGTM.

Yes it should.  Whoops :(
I made this change.

Tom

Re: [PATCH 1/4] Change pspace_to_pspace_object to return a new reference

[Simon Marchi]

On 2018-09-13 1:30 a.m., Tom Tromey wrote:
> This changes pspace_to_pspace_object to return a new reference and
> fixes up all the callers.

Actually, I think the patch is missing a hunk or something, I get this error:

/home/simark/src/binutils-gdb/gdb/python/py-inferior.c:474:13: error: no viable conversion from 'gdbpy_ref<>' (aka 'gdb::ref_ptr<_object, gdbpy_ref_policy<_object> >') to 'PyObject *' (aka '_object *')
  PyObject *py_pspace = pspace_to_pspace_object (pspace);
            ^           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.


Simon

Re: [PATCH 1/4] Change pspace_to_pspace_object to return a new reference

[Simon Marchi]

On 2018-09-15 9:19 p.m., Simon Marchi wrote:
> On 2018-09-13 1:30 a.m., Tom Tromey wrote:
>> This changes pspace_to_pspace_object to return a new reference and
>> fixes up all the callers.
> 
> Actually, I think the patch is missing a hunk or something, I get this error:
> 
> /home/simark/src/binutils-gdb/gdb/python/py-inferior.c:474:13: error: no viable conversion from 'gdbpy_ref<>' (aka 'gdb::ref_ptr<_object, gdbpy_ref_policy<_object> >') to 'PyObject *' (aka '_object *')
>   PyObject *py_pspace = pspace_to_pspace_object (pspace);
>             ^           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 error generated.

Ah ok, this is the code I added recently, that's why.  I think changing it to

  return pspace_to_pspace_object (pspace).release ();

should work.

Simon

Re: [PATCH 1/4] Change pspace_to_pspace_object to return a new reference

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Ah ok, this is the code I added recently, that's why.  I think changing it to
Simon>   return pspace_to_pspace_object (pspace).release ();
Simon> should work.

Yep, I got this on rebasing and did what you suggest here.

Tom

[PATCH 4/4] Remove CPYCHECKER_RETURNS_BORROWED_REF

[Tom Tromey]

CPYCHECKER_RETURNS_BORROWED_REF is not used, and I think should never
be used.  This patch removes it.

gdb/ChangeLog
2018-09-12  Tom Tromey  <tom@tromey.com>

	* python/python-internal.h (CPYCHECKER_RETURNS_BORROWED_REF):
	Remove.
---
 gdb/ChangeLog                |  5 +++++
 gdb/python/python-internal.h | 11 +++--------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index dc8d7cdd32a..59e20238856 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -27,14 +27,9 @@
    comes with the Python plugin for GCC.  See:
    https://gcc-python-plugin.readthedocs.org/en/latest/cpychecker.html
    The checker defines a WITH_ macro for each attribute it
-   exposes.  */
-
-#ifdef WITH_CPYCHECKER_RETURNS_BORROWED_REF_ATTRIBUTE
-#define CPYCHECKER_RETURNS_BORROWED_REF			\
-  __attribute__ ((cpychecker_returns_borrowed_ref))
-#else
-#define CPYCHECKER_RETURNS_BORROWED_REF
-#endif
+   exposes.  Note that we intentionally do not use
+   'cpychecker_returns_borrowed_ref' -- that idiom is forbidden in
+   gdb.  */
 
 #ifdef WITH_CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF_ATTRIBUTE
 #define CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF(ARG)		\
-- 
2.17.1

[PATCH 2/4] Change objfile_to_objfile_object to return a new reference

[Tom Tromey]

This changes objfile_to_objfile_object to return a new references and
fixes up all the uses.

2018-09-12  Tom Tromey  <tom@tromey.com>

	* python/python-internal.h (objfile_to_objfile_object): Change
	return type.
	* python/py-newobjfileevent.c (create_new_objfile_event_object):
	Update.
	* python/py-xmethods.c (gdbpy_get_matching_xmethod_workers):
	Update.
	* python/python.c (gdbpy_get_current_objfile): Update.
	(gdbpy_objfiles): Update.
	* python/py-objfile.c (objfpy_get_owner, gdbpy_lookup_objfile):
	Update.
	(objfile_to_objfile_object): Return a new reference.
	* python/py-symtab.c (stpy_get_objfile): Update.
	* python/py-prettyprint.c (find_pretty_printer_from_objfiles):
	Update.
---
 gdb/ChangeLog                   | 17 +++++++++++++
 gdb/python/py-newobjfileevent.c | 10 +++-----
 gdb/python/py-objfile.c         | 43 +++++++++++++--------------------
 gdb/python/py-prettyprint.c     |  6 ++---
 gdb/python/py-symtab.c          |  5 +---
 gdb/python/py-xmethods.c        |  5 ++--
 gdb/python/python-internal.h    |  3 +--
 gdb/python/python.c             | 11 +++------
 8 files changed, 49 insertions(+), 51 deletions(-)

diff --git a/gdb/python/py-newobjfileevent.c b/gdb/python/py-newobjfileevent.c
index aa31fb4849b..55e884dcaf5 100644
--- a/gdb/python/py-newobjfileevent.c
+++ b/gdb/python/py-newobjfileevent.c
@@ -28,12 +28,10 @@ create_new_objfile_event_object (struct objfile *objfile)
   if (objfile_event == NULL)
     return NULL;
 
-  /* Note that objfile_to_objfile_object returns a borrowed reference,
-     so we don't need a decref here.  */
-  PyObject *py_objfile = objfile_to_objfile_object (objfile);
-  if (!py_objfile || evpy_add_attribute (objfile_event.get (),
-                                         "new_objfile",
-                                         py_objfile) < 0)
+  gdbpy_ref<> py_objfile = objfile_to_objfile_object (objfile);
+  if (py_objfile == NULL || evpy_add_attribute (objfile_event.get (),
+						"new_objfile",
+						py_objfile.get ()) < 0)
     return NULL;
 
   return objfile_event;
diff --git a/gdb/python/py-objfile.c b/gdb/python/py-objfile.c
index 8fd160c7bf8..cef34f1bcc2 100644
--- a/gdb/python/py-objfile.c
+++ b/gdb/python/py-objfile.c
@@ -115,12 +115,7 @@ objfpy_get_owner (PyObject *self, void *closure)
 
   owner = objfile->separate_debug_objfile_backlink;
   if (owner != NULL)
-    {
-      PyObject *result = objfile_to_objfile_object (owner);
-
-      Py_XINCREF (result);
-      return result;
-    }
+    return objfile_to_objfile_object (owner).release ();
   Py_RETURN_NONE;
 }
 
@@ -590,12 +585,7 @@ gdbpy_lookup_objfile (PyObject *self, PyObject *args, PyObject *kw)
     objfile = objfpy_lookup_objfile_by_name (name);
 
   if (objfile != NULL)
-    {
-      PyObject *result = objfile_to_objfile_object (objfile);
-
-      Py_XINCREF (result);
-      return result;
-    }
+    return objfile_to_objfile_object (objfile).release ();
 
   PyErr_SetString (PyExc_ValueError, _("Objfile not found."));
   return NULL;
@@ -613,30 +603,31 @@ py_free_objfile (struct objfile *objfile, void *datum)
   object->objfile = NULL;
 }
 
-/* Return a borrowed reference to the Python object of type Objfile
+/* Return a new reference to the Python object of type Objfile
    representing OBJFILE.  If the object has already been created,
    return it.  Otherwise, create it.  Return NULL and set the Python
    error on failure.  */
 
-PyObject *
+gdbpy_ref<>
 objfile_to_objfile_object (struct objfile *objfile)
 {
-  gdbpy_ref<objfile_object> object
-    ((objfile_object *) objfile_data (objfile, objfpy_objfile_data_key));
-  if (object == NULL)
+  PyObject *result
+    = ((PyObject *) objfile_data (objfile, objfpy_objfile_data_key));
+  if (result == NULL)
     {
-      object.reset (PyObject_New (objfile_object, &objfile_object_type));
-      if (object != NULL)
-	{
-	  if (!objfpy_initialize (object.get ()))
-	    return NULL;
+      gdbpy_ref<objfile_object> object
+	((objfile_object *) PyObject_New (objfile_object, &objfile_object_type));
+      if (object == NULL)
+	return NULL;
+      if (!objfpy_initialize (object.get ()))
+	return NULL;
 
-	  object->objfile = objfile;
-	  set_objfile_data (objfile, objfpy_objfile_data_key, object.get ());
-	}
+      object->objfile = objfile;
+      set_objfile_data (objfile, objfpy_objfile_data_key, object.get ());
+      result = (PyObject *) object.release ();
     }
 
-  return (PyObject *) object.release ();
+  return gdbpy_ref<>::new_reference (result);
 }
 
 int
diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c
index 9aadd3b27c9..a7062225816 100644
--- a/gdb/python/py-prettyprint.c
+++ b/gdb/python/py-prettyprint.c
@@ -97,15 +97,15 @@ find_pretty_printer_from_objfiles (PyObject *value)
 
   ALL_OBJFILES (obj)
   {
-    PyObject *objf = objfile_to_objfile_object (obj);
-    if (!objf)
+    gdbpy_ref<> objf = objfile_to_objfile_object (obj);
+    if (objf == NULL)
       {
 	/* Ignore the error and continue.  */
 	PyErr_Clear ();
 	continue;
       }
 
-    gdbpy_ref<> pp_list (objfpy_get_printers (objf, NULL));
+    gdbpy_ref<> pp_list (objfpy_get_printers (objf.get (), NULL));
     gdbpy_ref<> function (search_pp_list (pp_list.get (), value));
 
     /* If there is an error in any objfile list, abort the search and exit.  */
diff --git a/gdb/python/py-symtab.c b/gdb/python/py-symtab.c
index be7fb3ec471..9bb20dab31f 100644
--- a/gdb/python/py-symtab.c
+++ b/gdb/python/py-symtab.c
@@ -117,13 +117,10 @@ static PyObject *
 stpy_get_objfile (PyObject *self, void *closure)
 {
   struct symtab *symtab = NULL;
-  PyObject *result;
 
   STPY_REQUIRE_VALID (self, symtab);
 
-  result = objfile_to_objfile_object (SYMTAB_OBJFILE (symtab));
-  Py_XINCREF (result);
-  return result;
+  return objfile_to_objfile_object (SYMTAB_OBJFILE (symtab)).release ();
 }
 
 /* Getter function for symtab.producer.  */
diff --git a/gdb/python/py-xmethods.c b/gdb/python/py-xmethods.c
index c568295a165..8e616cd4e2d 100644
--- a/gdb/python/py-xmethods.c
+++ b/gdb/python/py-xmethods.c
@@ -147,7 +147,7 @@ gdbpy_get_matching_xmethod_workers
      list individually, but there's no data yet to show it's needed.  */
   ALL_OBJFILES (objfile)
     {
-      PyObject *py_objfile = objfile_to_objfile_object (objfile);
+      gdbpy_ref<> py_objfile = objfile_to_objfile_object (objfile);
 
       if (py_objfile == NULL)
 	{
@@ -155,7 +155,8 @@ gdbpy_get_matching_xmethod_workers
 	  return EXT_LANG_RC_ERROR;
 	}
 
-      gdbpy_ref<> objfile_matchers (objfpy_get_xmethods (py_objfile, NULL));
+      gdbpy_ref<> objfile_matchers (objfpy_get_xmethods (py_objfile.get (),
+							 NULL));
       gdbpy_ref<> temp (PySequence_Concat (py_xmethod_matcher_list.get (),
 					   objfile_matchers.get ()));
       if (temp == NULL)
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index a6dc5f090b5..cbba3ff8ef6 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -523,8 +523,7 @@ PyObject *pspy_get_frame_filters (PyObject *, void *);
 PyObject *pspy_get_frame_unwinders (PyObject *, void *);
 PyObject *pspy_get_xmethods (PyObject *, void *);
 
-PyObject *objfile_to_objfile_object (struct objfile *)
-    CPYCHECKER_RETURNS_BORROWED_REF;
+gdbpy_ref<> objfile_to_objfile_object (struct objfile *);
 PyObject *objfpy_get_printers (PyObject *, void *);
 PyObject *objfpy_get_frame_filters (PyObject *, void *);
 PyObject *objfpy_get_frame_unwinders (PyObject *, void *);
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 93d6d1156d8..778ddb9f342 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1421,15 +1421,10 @@ gdbpy_execute_objfile_script (const struct extension_language_defn *extlang,
 static PyObject *
 gdbpy_get_current_objfile (PyObject *unused1, PyObject *unused2)
 {
-  PyObject *result;
-
   if (! gdbpy_current_objfile)
     Py_RETURN_NONE;
 
-  result = objfile_to_objfile_object (gdbpy_current_objfile);
-  if (result)
-    Py_INCREF (result);
-  return result;
+  return objfile_to_objfile_object (gdbpy_current_objfile).release ();
 }
 
 /* Return a sequence holding all the Objfiles.  */
@@ -1445,9 +1440,9 @@ gdbpy_objfiles (PyObject *unused1, PyObject *unused2)
 
   ALL_OBJFILES (objf)
   {
-    PyObject *item = objfile_to_objfile_object (objf);
+    gdbpy_ref<> item = objfile_to_objfile_object (objf);
 
-    if (!item || PyList_Append (list.get (), item) == -1)
+    if (item == NULL || PyList_Append (list.get (), item.get ()) == -1)
       return NULL;
   }
 
-- 
2.17.1

Re: [PATCH 2/4] Change objfile_to_objfile_object to return a new reference

[Simon Marchi]

On 2018-09-13 1:30 a.m., Tom Tromey wrote:
> This changes objfile_to_objfile_object to return a new references and
> fixes up all the uses.
> 
> 2018-09-12  Tom Tromey  <tom@tromey.com>
> 
> 	* python/python-internal.h (objfile_to_objfile_object): Change
> 	return type.
> 	* python/py-newobjfileevent.c (create_new_objfile_event_object):
> 	Update.
> 	* python/py-xmethods.c (gdbpy_get_matching_xmethod_workers):
> 	Update.
> 	* python/python.c (gdbpy_get_current_objfile): Update.
> 	(gdbpy_objfiles): Update.
> 	* python/py-objfile.c (objfpy_get_owner, gdbpy_lookup_objfile):
> 	Update.
> 	(objfile_to_objfile_object): Return a new reference.
> 	* python/py-symtab.c (stpy_get_objfile): Update.
> 	* python/py-prettyprint.c (find_pretty_printer_from_objfiles):
> 	Update.

I'm also getting some failures with this patch;

  CXX    python/python.o
/home/simark/src/binutils-gdb/gdb/python/python.c:1432:12: error: assigning to 'PyObject *' (aka '_object *') from incompatible type 'gdbpy_ref<>' (aka 'gdb::ref_ptr<_object, gdbpy_ref_policy<_object> >')
  result = objfile_to_objfile_object (gdbpy_current_objfile);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/simark/src/binutils-gdb/gdb/python/python.c:1451:17: error: no viable conversion from 'gdbpy_ref<>' (aka 'gdb::ref_ptr<_object, gdbpy_ref_policy<_object> >') to 'PyObject *' (aka '_object *')
      PyObject *item = objfile_to_objfile_object (objf);
                ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The rest of the changes looks good to me.

Simon

Re: [PATCH 2/4] Change objfile_to_objfile_object to return a new reference

[Simon Marchi]

On 2018-09-15 9:28 p.m., Simon Marchi wrote:
> On 2018-09-13 1:30 a.m., Tom Tromey wrote:
>> This changes objfile_to_objfile_object to return a new references and
>> fixes up all the uses.
>>
>> 2018-09-12  Tom Tromey  <tom@tromey.com>
>>
>> 	* python/python-internal.h (objfile_to_objfile_object): Change
>> 	return type.
>> 	* python/py-newobjfileevent.c (create_new_objfile_event_object):
>> 	Update.
>> 	* python/py-xmethods.c (gdbpy_get_matching_xmethod_workers):
>> 	Update.
>> 	* python/python.c (gdbpy_get_current_objfile): Update.
>> 	(gdbpy_objfiles): Update.
>> 	* python/py-objfile.c (objfpy_get_owner, gdbpy_lookup_objfile):
>> 	Update.
>> 	(objfile_to_objfile_object): Return a new reference.
>> 	* python/py-symtab.c (stpy_get_objfile): Update.
>> 	* python/py-prettyprint.c (find_pretty_printer_from_objfiles):
>> 	Update.
> 
> I'm also getting some failures with this patch;
> 
>   CXX    python/python.o
> /home/simark/src/binutils-gdb/gdb/python/python.c:1432:12: error: assigning to 'PyObject *' (aka '_object *') from incompatible type 'gdbpy_ref<>' (aka 'gdb::ref_ptr<_object, gdbpy_ref_policy<_object> >')
>   result = objfile_to_objfile_object (gdbpy_current_objfile);
>            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /home/simark/src/binutils-gdb/gdb/python/python.c:1451:17: error: no viable conversion from 'gdbpy_ref<>' (aka 'gdb::ref_ptr<_object, gdbpy_ref_policy<_object> >') to 'PyObject *' (aka '_object *')
>       PyObject *item = objfile_to_objfile_object (objf);
>                 ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Forget about this failure.  I had trouble applying the patches (finding
the right base commit) and managed to get rid of the changes in that file
when dealing with the conflicts.  Everything looks fine here.

Simon

[PATCH 3/4] Change thread_to_thread_object to return a new reference

[Tom Tromey]

This changes thread_to_thread_object to return a new reference and
fixes up all the callers.

2018-09-12  Tom Tromey  <tom@tromey.com>

	* python/python-internal.h (thread_to_thread_object): Change
	return type.
	* python/py-inferior.c (thread_to_thread_object): Return a new
	reference.
	(infpy_thread_from_thread_handle): Update.
	* python/py-infthread.c (gdbpy_selected_thread): Update.
	* python/py-stopevent.c (create_stop_event_object): Update.
	* python/py-threadevent.c (py_get_event_thread): Return a new
	reference.
	(py_get_event_thread): Update.
	* python/py-event.h (py_get_event_thread): Change return type.
	* python/py-continueevent.c (create_continue_event_object):
	Update.
---
 gdb/ChangeLog                 | 16 ++++++++++++
 gdb/python/py-continueevent.c |  5 ++--
 gdb/python/py-event.h         |  5 ++--
 gdb/python/py-inferior.c      | 46 ++++++++++++++++-------------------
 gdb/python/py-infthread.c     | 10 +-------
 gdb/python/py-stopevent.c     |  4 +--
 gdb/python/py-threadevent.c   |  8 +++---
 gdb/python/python-internal.h  |  3 +--
 8 files changed, 50 insertions(+), 47 deletions(-)

diff --git a/gdb/python/py-continueevent.c b/gdb/python/py-continueevent.c
index 759b4831366..9708c0d4059 100644
--- a/gdb/python/py-continueevent.c
+++ b/gdb/python/py-continueevent.c
@@ -32,12 +32,13 @@
 static gdbpy_ref<>
 create_continue_event_object (ptid_t ptid)
 {
-  PyObject *py_thr = py_get_event_thread (ptid);
+  gdbpy_ref<> py_thr = py_get_event_thread (ptid);
 
   if (py_thr == nullptr)
     return nullptr;
 
-  return create_thread_event_object (&continue_event_object_type, py_thr);
+  return create_thread_event_object (&continue_event_object_type,
+				     py_thr.get ());
 }
 
 /* Callback function which notifies observers when a continue event occurs.
diff --git a/gdb/python/py-event.h b/gdb/python/py-event.h
index 56003e8785f..96be83b44c5 100644
--- a/gdb/python/py-event.h
+++ b/gdb/python/py-event.h
@@ -68,9 +68,8 @@ extern gdbpy_ref<> create_event_object (PyTypeObject *py_type);
    running in non-stop mode then the event is thread specific, otherwise
    it is process wide.
    This function returns the currently stopped thread in non-stop mode and
-   Py_None otherwise.  In each case it returns a borrowed reference.  */
-extern PyObject *py_get_event_thread (ptid_t ptid)
-  CPYCHECKER_RETURNS_BORROWED_REF;
+   Py_None otherwise.  */
+extern gdbpy_ref<> py_get_event_thread (ptid_t ptid);
 
 extern gdbpy_ref<> create_thread_event_object (PyTypeObject *py_type,
 					       PyObject *thread);
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index 1cf37296973..bf8ac75a316 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -306,7 +306,7 @@ find_inferior_object (int pid)
   return NULL;
 }
 
-thread_object *
+gdbpy_ref<>
 thread_to_thread_object (thread_info *thr)
 {
   gdbpy_ref<inferior_object> inf_obj (inferior_to_inferior_object (thr->inf));
@@ -317,7 +317,7 @@ thread_to_thread_object (thread_info *thr)
        thread != NULL;
        thread = thread->next)
     if (thread->thread_obj->thread == thr)
-      return thread->thread_obj;
+      return gdbpy_ref<>::new_reference ((PyObject *) thread->thread_obj);
 
   return NULL;
 }
@@ -817,7 +817,7 @@ infpy_is_valid (PyObject *self, PyObject *args)
 PyObject *
 infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw)
 {
-  PyObject *handle_obj, *result;
+  PyObject *handle_obj;
   inferior_object *inf_obj = (inferior_object *) self;
   static const char *keywords[] = { "thread_handle", NULL };
 
@@ -826,8 +826,6 @@ infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw)
   if (! gdb_PyArg_ParseTupleAndKeywords (args, kw, "O", keywords, &handle_obj))
     return NULL;
 
-  result = Py_None;
-
   if (!gdbpy_is_value_object (handle_obj))
     {
       PyErr_SetString (PyExc_TypeError,
@@ -835,29 +833,27 @@ infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw)
 
       return NULL;
     }
-  else
+
+  gdbpy_ref<> result;
+  TRY
+    {
+      struct thread_info *thread_info;
+      struct value *val = value_object_to_value (handle_obj);
+
+      thread_info = find_thread_by_handle (val, inf_obj->inferior);
+      if (thread_info != NULL)
+	result = thread_to_thread_object (thread_info);
+    }
+  CATCH (except, RETURN_MASK_ALL)
     {
-      TRY
-	{
-	  struct thread_info *thread_info;
-	  struct value *val = value_object_to_value (handle_obj);
-
-	  thread_info = find_thread_by_handle (val, inf_obj->inferior);
-	  if (thread_info != NULL)
-	    {
-	      result = (PyObject *) thread_to_thread_object (thread_info);
-	      if (result != NULL)
-		Py_INCREF (result);
-	    }
-	}
-      CATCH (except, RETURN_MASK_ALL)
-	{
-	  GDB_PY_HANDLE_EXCEPTION (except);
-	}
-      END_CATCH
+      GDB_PY_HANDLE_EXCEPTION (except);
     }
+  END_CATCH
 
-  return result;
+  if (result == NULL)
+    result = gdbpy_ref<>::new_reference (Py_None);
+
+  return result.release ();
 }
 
 
diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c
index 36ae71b6358..4b2705a0af6 100644
--- a/gdb/python/py-infthread.c
+++ b/gdb/python/py-infthread.c
@@ -284,15 +284,7 @@ PyObject *
 gdbpy_selected_thread (PyObject *self, PyObject *args)
 {
   if (inferior_ptid != null_ptid)
-    {
-      PyObject *thread_obj
-	= (PyObject *) thread_to_thread_object (inferior_thread ());
-      if (thread_obj != NULL)
-	{
-	  Py_INCREF (thread_obj);
-	  return thread_obj;
-	}
-    }
+    return thread_to_thread_object (inferior_thread ()).release ();
 
   Py_RETURN_NONE;
 }
diff --git a/gdb/python/py-stopevent.c b/gdb/python/py-stopevent.c
index f6bd207f3b8..cbdf7b7e7e0 100644
--- a/gdb/python/py-stopevent.c
+++ b/gdb/python/py-stopevent.c
@@ -23,8 +23,8 @@
 gdbpy_ref<>
 create_stop_event_object (PyTypeObject *py_type)
 {
-  return create_thread_event_object (py_type,
-				     py_get_event_thread (inferior_ptid));
+  gdbpy_ref<> thread = py_get_event_thread (inferior_ptid);
+  return create_thread_event_object (py_type, thread.get ());
 }
 
 /* Callback observers when a stop event occurs.  This function will create a
diff --git a/gdb/python/py-threadevent.c b/gdb/python/py-threadevent.c
index 4f822b4ae09..13af1c840ba 100644
--- a/gdb/python/py-threadevent.c
+++ b/gdb/python/py-threadevent.c
@@ -22,19 +22,19 @@
 
 /* See py-event.h.  */
 
-PyObject *
+gdbpy_ref<>
 py_get_event_thread (ptid_t ptid)
 {
-  PyObject *pythread = nullptr;
+  gdbpy_ref<> pythread;
 
   if (non_stop)
     {
       thread_info *thread = find_thread_ptid (ptid);
       if (thread != nullptr)
-	pythread = (PyObject *) thread_to_thread_object (thread);
+	pythread = thread_to_thread_object (thread);
     }
   else
-    pythread = Py_None;
+    pythread = gdbpy_ref<>::new_reference (Py_None);
 
   if (pythread == nullptr)
     {
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index cbba3ff8ef6..dc8d7cdd32a 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -533,8 +533,7 @@ PyObject *gdbpy_lookup_objfile (PyObject *self, PyObject *args, PyObject *kw);
 PyObject *gdbarch_to_arch_object (struct gdbarch *gdbarch);
 
 thread_object *create_thread_object (struct thread_info *tp);
-thread_object *thread_to_thread_object (thread_info *thr)
-  CPYCHECKER_RETURNS_BORROWED_REF;
+gdbpy_ref<> thread_to_thread_object (thread_info *thr);;
 inferior_object *inferior_to_inferior_object (inferior *inf);
 
 const struct block *block_object_to_block (PyObject *obj);
-- 
2.17.1

Re: [PATCH 3/4] Change thread_to_thread_object to return a new reference

[Simon Marchi]

I have some comments about style, correctness-wise is looks good to me.

On 2018-09-13 1:30 a.m., Tom Tromey wrote:
> diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
> index 1cf37296973..bf8ac75a316 100644
> --- a/gdb/python/py-inferior.c
> +++ b/gdb/python/py-inferior.c
> @@ -306,7 +306,7 @@ find_inferior_object (int pid)
>    return NULL;
>  }
>  
> -thread_object *
> +gdbpy_ref<>
>  thread_to_thread_object (thread_info *thr)
>  {
>    gdbpy_ref<inferior_object> inf_obj (inferior_to_inferior_object (thr->inf));
> @@ -317,7 +317,7 @@ thread_to_thread_object (thread_info *thr)
>         thread != NULL;
>         thread = thread->next)
>      if (thread->thread_obj->thread == thr)
> -      return thread->thread_obj;
> +      return gdbpy_ref<>::new_reference ((PyObject *) thread->thread_obj);
>  
>    return NULL;
>  }
> @@ -817,7 +817,7 @@ infpy_is_valid (PyObject *self, PyObject *args)
>  PyObject *
>  infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw)
>  {
> -  PyObject *handle_obj, *result;
> +  PyObject *handle_obj;
>    inferior_object *inf_obj = (inferior_object *) self;
>    static const char *keywords[] = { "thread_handle", NULL };
>  
> @@ -826,8 +826,6 @@ infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw)
>    if (! gdb_PyArg_ParseTupleAndKeywords (args, kw, "O", keywords, &handle_obj))
>      return NULL;
>  
> -  result = Py_None;
> -
>    if (!gdbpy_is_value_object (handle_obj))
>      {
>        PyErr_SetString (PyExc_TypeError,
> @@ -835,29 +833,27 @@ infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw)
>  
>        return NULL;
>      }
> -  else
> +
> +  gdbpy_ref<> result;
> +  TRY
> +    {
> +      struct thread_info *thread_info;
> +      struct value *val = value_object_to_value (handle_obj);
> +
> +      thread_info = find_thread_by_handle (val, inf_obj->inferior);
> +      if (thread_info != NULL)
> +	result = thread_to_thread_object (thread_info);
> +    }
> +  CATCH (except, RETURN_MASK_ALL)
>      {
> -      TRY
> -	{
> -	  struct thread_info *thread_info;
> -	  struct value *val = value_object_to_value (handle_obj);
> -
> -	  thread_info = find_thread_by_handle (val, inf_obj->inferior);
> -	  if (thread_info != NULL)
> -	    {
> -	      result = (PyObject *) thread_to_thread_object (thread_info);
> -	      if (result != NULL)
> -		Py_INCREF (result);
> -	    }
> -	}
> -      CATCH (except, RETURN_MASK_ALL)
> -	{
> -	  GDB_PY_HANDLE_EXCEPTION (except);
> -	}
> -      END_CATCH
> +      GDB_PY_HANDLE_EXCEPTION (except);
>      }
> +  END_CATCH
>  
> -  return result;
> +  if (result == NULL)
> +    result = gdbpy_ref<>::new_reference (Py_None);

I would suggest using Py_RETURN_NONE, which we already use at many places.

Instead of setting that result variable, is there something preventing you
from returning directly above, like this?

      if (thread_info != NULL)
	return thread_to_thread_object (thread_info).release ();

The bottom line could just be unconditionally Py_RETURN_NONE.

> +
> +  return result.release ();
>  }
>  
>  
> diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c
> index 36ae71b6358..4b2705a0af6 100644
> --- a/gdb/python/py-infthread.c
> +++ b/gdb/python/py-infthread.c
> @@ -284,15 +284,7 @@ PyObject *
>  gdbpy_selected_thread (PyObject *self, PyObject *args)
>  {
>    if (inferior_ptid != null_ptid)
> -    {
> -      PyObject *thread_obj
> -	= (PyObject *) thread_to_thread_object (inferior_thread ());
> -      if (thread_obj != NULL)
> -	{
> -	  Py_INCREF (thread_obj);
> -	  return thread_obj;
> -	}
> -    }
> +    return thread_to_thread_object (inferior_thread ()).release ();

I know it's logically not supposed to happen, but here it's possible for
thread_to_thread_object to return NULL.  Returning NULL to Python is the
way to have it throw an exception, I believe, is that what you want to do
here?  The previous code would return None in that case.

>  
>    Py_RETURN_NONE;
>  }
> diff --git a/gdb/python/py-stopevent.c b/gdb/python/py-stopevent.c
> index f6bd207f3b8..cbdf7b7e7e0 100644
> --- a/gdb/python/py-stopevent.c
> +++ b/gdb/python/py-stopevent.c
> @@ -23,8 +23,8 @@
>  gdbpy_ref<>
>  create_stop_event_object (PyTypeObject *py_type)
>  {
> -  return create_thread_event_object (py_type,
> -				     py_get_event_thread (inferior_ptid));
> +  gdbpy_ref<> thread = py_get_event_thread (inferior_ptid);
> +  return create_thread_event_object (py_type, thread.get ());
>  }
>  
>  /* Callback observers when a stop event occurs.  This function will create a
> diff --git a/gdb/python/py-threadevent.c b/gdb/python/py-threadevent.c
> index 4f822b4ae09..13af1c840ba 100644
> --- a/gdb/python/py-threadevent.c
> +++ b/gdb/python/py-threadevent.c
> @@ -22,19 +22,19 @@
>  
>  /* See py-event.h.  */
>  
> -PyObject *
> +gdbpy_ref<>
>  py_get_event_thread (ptid_t ptid)
>  {
> -  PyObject *pythread = nullptr;
> +  gdbpy_ref<> pythread;
>  
>    if (non_stop)
>      {
>        thread_info *thread = find_thread_ptid (ptid);
>        if (thread != nullptr)
> -	pythread = (PyObject *) thread_to_thread_object (thread);
> +	pythread = thread_to_thread_object (thread);
>      }
>    else
> -    pythread = Py_None;
> +    pythread = gdbpy_ref<>::new_reference (Py_None);

Again, I would suggest Py_RETURN_NONE.

I would also propose moving the code below in the if, to show that it only
really applies to the non-stop case (if I understand the code correctly).

>  
>    if (pythread == nullptr)
>      {

Simon

Re: [PATCH 3/4] Change thread_to_thread_object to return a new reference

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> I have some comments about style, correctness-wise is looks good to me.

Oops, I missed this note and pushed the patch in.

I will write a new patch to address the comments a bit later.
Sorry about that.

Tom

Re: [PATCH 3/4] Change thread_to_thread_object to return a new reference

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> +  if (result == NULL)
>> +    result = gdbpy_ref<>::new_reference (Py_None);

Simon> I would suggest using Py_RETURN_NONE, which we already use at many places.

Simon> Instead of setting that result variable, is there something preventing you
Simon> from returning directly above, like this?

Simon>       if (thread_info != NULL)
Simon> 	return thread_to_thread_object (thread_info).release ();

Simon> The bottom line could just be unconditionally Py_RETURN_NONE.

I made this change.

In the past you couldn't return from inside a try/catch but I think it
is ok now.


I couldn't use Py_RETURN_NONE in py_get_event_thread because it returns
a gdbpy_ref<> -- and I think the latter is more important.


I re-read all of this and I think thread_to_thread_object has a latent
bug.  It can return NULL early due to an error:

  gdbpy_ref<inferior_object> inf_obj (inferior_to_inferior_object (thr->inf));
  if (inf_obj == NULL)
    return NULL;

or it can return NULL at the end meaning "thread not found".

I think it is best to have a single style - returning NULL should also
set the Python exception.

It seems to me that the second NULL return should just set an
exception.  It's not a normal case.

Let me know what you think of this.

My apologies again for missing this review and pushing too eagerly.

Tom

commit 0a58a05fb6574c0e9c84c5160e99f5c922a6a0eb
Author: Tom Tromey <tom@tromey.com>
Date:   Sun Sep 16 08:02:22 2018 -0600

    Simplify uses of thread_to_thread_object
    
    An review by Simon of an earlier showed a few spots related to
    thread_to_thread_object that could be simplified.  This also detected
    a latent bug, where thread_to_thread_object was inconsistent about
    setting the Python exception before a NULL return.
    
    Tested on x86-64 Fedora 28.
    
    gdb/ChangeLog
    2018-09-16  Tom Tromey  <tom@tromey.com>
    
            * python/py-threadevent.c (py_get_event_thread): Simplify.
            * python/py-inferior.c (infpy_thread_from_thread_handle):
            Return immediately after calling thread_to_thread_object.  Use
            Py_RETURN_NONE.
            (thread_to_thread_object): Set the exception on a NULL return.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d36f6cde426..492fcad4120 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2018-09-16  Tom Tromey  <tom@tromey.com>
+
+	* python/py-threadevent.c (py_get_event_thread): Simplify.
+	* python/py-inferior.c (infpy_thread_from_thread_handle):
+	Return immediately after calling thread_to_thread_object.  Use
+	Py_RETURN_NONE.
+	(thread_to_thread_object): Set the exception on a NULL return.
+
 2018-09-16  Tom Tromey  <tom@tromey.com>
 
 	* python/python-internal.h (CPYCHECKER_RETURNS_BORROWED_REF):
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index 68c4c9d411e..145f53d14af 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -319,6 +319,8 @@ thread_to_thread_object (thread_info *thr)
     if (thread->thread_obj->thread == thr)
       return gdbpy_ref<>::new_reference ((PyObject *) thread->thread_obj);
 
+  PyErr_SetString (PyExc_SystemError,
+		   _("could not find gdb thread object"));
   return NULL;
 }
 
@@ -849,7 +851,6 @@ infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw)
       return NULL;
     }
 
-  gdbpy_ref<> result;
   TRY
     {
       struct thread_info *thread_info;
@@ -857,7 +858,7 @@ infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw)
 
       thread_info = find_thread_by_handle (val, inf_obj->inferior);
       if (thread_info != NULL)
-	result = thread_to_thread_object (thread_info);
+	return thread_to_thread_object (thread_info).release ();
     }
   CATCH (except, RETURN_MASK_ALL)
     {
@@ -865,10 +866,7 @@ infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw)
     }
   END_CATCH
 
-  if (result == NULL)
-    result = gdbpy_ref<>::new_reference (Py_None);
-
-  return result.release ();
+  Py_RETURN_NONE;
 }
 
 /* Implement repr() for gdb.Inferior.  */
diff --git a/gdb/python/py-threadevent.c b/gdb/python/py-threadevent.c
index 13af1c840ba..ea62540ff84 100644
--- a/gdb/python/py-threadevent.c
+++ b/gdb/python/py-threadevent.c
@@ -25,24 +25,15 @@
 gdbpy_ref<>
 py_get_event_thread (ptid_t ptid)
 {
-  gdbpy_ref<> pythread;
-
   if (non_stop)
     {
       thread_info *thread = find_thread_ptid (ptid);
       if (thread != nullptr)
-	pythread = thread_to_thread_object (thread);
-    }
-  else
-    pythread = gdbpy_ref<>::new_reference (Py_None);
-
-  if (pythread == nullptr)
-    {
+	return thread_to_thread_object (thread);
       PyErr_SetString (PyExc_RuntimeError, "Could not find event thread");
       return NULL;
     }
-
-  return pythread;
+  return gdbpy_ref<>::new_reference (Py_None);
 }
 
 gdbpy_ref<>

Re: [PATCH 3/4] Change thread_to_thread_object to return a new reference

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> I think it is best to have a single style - returning NULL should also
Tom> set the Python exception.

Maybe the gdbpy_ref (1-argument) constructor and release methods could
assert that the Python exception is set if the underlying pointer is
NULL.  That would not get full checking but maybe it would catch some
problems.  And maybe we should simply use gdbpy_ref in many more places
in the Python layer -- ideally, reserve raw pointers solely for
parameters which are borrowed references.

Tom

Re: [PATCH 3/4] Change thread_to_thread_object to return a new reference

[Simon Marchi]

On 2018-09-16 10:05, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
>>> +  if (result == NULL)
>>> +    result = gdbpy_ref<>::new_reference (Py_None);
> 
> Simon> I would suggest using Py_RETURN_NONE, which we already use at
> many places.
> 
> Simon> Instead of setting that result variable, is there something
> preventing you
> Simon> from returning directly above, like this?
> 
> Simon>       if (thread_info != NULL)
> Simon> 	return thread_to_thread_object (thread_info).release ();
> 
> Simon> The bottom line could just be unconditionally Py_RETURN_NONE.
> 
> I made this change.
> 
> In the past you couldn't return from inside a try/catch but I think it
> is ok now.
> 
> 
> I couldn't use Py_RETURN_NONE in py_get_event_thread because it returns
> a gdbpy_ref<> -- and I think the latter is more important.

Oh right.  While working on this code I though adding a 
GDB_Py_REF_RETURN_NONE macro, which would be like Py_RETURN_NONE but 
returning a gdbpy_ref.  Not sure it's really that useful, since what you 
wrote is explicit and clear.

> I re-read all of this and I think thread_to_thread_object has a latent
> bug.  It can return NULL early due to an error:
> 
>   gdbpy_ref<inferior_object> inf_obj (inferior_to_inferior_object 
> (thr->inf));
>   if (inf_obj == NULL)
>     return NULL;
> 
> or it can return NULL at the end meaning "thread not found".

I am not sure how these two mode of failure differ.  Don't we expect the 
passed thread_info object to always be valid, and therefore both cases 
returning NULL would be an internal GDB logic error?

> I think it is best to have a single style - returning NULL should also
> set the Python exception.

That makes sense, so the caller can just check for NULL and return NULL 
right away (as the snippet above does).  We should expect 
inferior_to_inferior_object to set the exception on failure.

> It seems to me that the second NULL return should just set an
> exception.  It's not a normal case.

Right.

Responding directly to your next message:

> Maybe the gdbpy_ref (1-argument) constructor and release methods could
> assert that the Python exception is set if the underlying pointer is
> NULL.  That would not get full checking but maybe it would catch some
> problems.  And maybe we should simply use gdbpy_ref in many more places
> in the Python layer -- ideally, reserve raw pointers solely for
> parameters which are borrowed references.

Makes sense.

> Let me know what you think of this.
> 
> My apologies again for missing this review and pushing too eagerly.

No problem, LGTM.

Simon

Re: [PATCH 3/4] Change thread_to_thread_object to return a new reference

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> On 2018-09-16 10:05, Tom Tromey wrote:
>> I re-read all of this and I think thread_to_thread_object has a latent
>> bug.  It can return NULL early due to an error:
>> 
>> gdbpy_ref<inferior_object> inf_obj (inferior_to_inferior_object
>> (thr->inf));
>> if (inf_obj == NULL)
>> return NULL;
>> 
>> or it can return NULL at the end meaning "thread not found".

Simon> I am not sure how these two mode of failure differ.  Don't we expect
Simon> the passed thread_info object to always be valid, and therefore both
Simon> cases returning NULL would be an internal GDB logic error?

I just meant that the difference here is whether or not the Python
exception is set -- that this function isn't following the normal,
simple convention here, but that I think it should.

Tom

Re: [PATCH 0/4] Disallow the return of borrowed references

[Simon Marchi]

On 2018-09-13 1:30 a.m., Tom Tromey wrote:
> While working on a different Python patch, I misread
> pspace_to_pspace_object and thought it was not handling reference
> counts properly.
> 
> It is -- but I found it difficult to reason about because it returns a
> borrowed reference, but uses gdbpy_ref as well.
> 
> This series changes gdb so that none of the Python functions will
> return a borrowed reference.  These functions are modified instead to
> return a new reference using gdbpy_ref<>, which not only makes the
> intent clear, but also makes it more difficult to have a buggy caller.
> 
> This could go further and make all functions return gdbpy_ref<>.
> (Even Python-facing ones could be done via template fuction wrappers.)
> However I have not done this.
> 
> I think now gdb should disallow Python functions returning borrowed
> references.  Accepting a borrowed reference is still ok, and I think
> should continue to be.
> 
> I think this series found at least one bug:
> infpy_thread_from_thread_handle could return None without incref'ing
> it.
> 
> Let me know what you think.  I tested this on x86-64 Fedora 28.

I think it makes sense and simplifies the way to think about the code.  The
only difference is a little bit more increfing/decrefing, but it's probably
not significant.

Simon