diff mbox

[RFA,16/20] Use gdbpy_enter in gdbpy_get_matching_xmethod_workers

Message ID 1478816387-27064-17-git-send-email-tom@tromey.com
State New
Headers show

Commit Message

Tom Tromey Nov. 10, 2016, 10:19 p.m. UTC
Change gdbpy_get_matching_xmethod_workers to use gdbpy_enter and
gdbpy_reference.

2016-11-10  Tom Tromey  <tom@tromey.com>

	* python/py-xmethods.c (gdbpy_get_matching_xmethod_workers): use
	gdbpy_enter, gdbpy_reference.
---
 gdb/ChangeLog            |   5 ++
 gdb/python/py-xmethods.c | 153 ++++++++++++++++++-----------------------------
 2 files changed, 63 insertions(+), 95 deletions(-)

Comments

Pedro Alves Nov. 10, 2016, 11:19 p.m. UTC | #1
On 11/10/2016 10:19 PM, Tom Tromey wrote:
> @@ -221,26 +210,21 @@ gdbpy_get_matching_xmethod_workers
>    py_progspace = pspace_to_pspace_object (current_program_space);
>    if (py_progspace != NULL)
>      {
> -      PyObject *temp = py_xmethod_matcher_list;
> -      PyObject *pspace_matchers = pspy_get_xmethods (py_progspace, NULL);
> +      gdbpy_reference pspace_matchers (pspy_get_xmethods (py_progspace, NULL));
>  
> -      py_xmethod_matcher_list = PySequence_Concat (temp, pspace_matchers);
> -      Py_DECREF (temp);
> -      Py_DECREF (pspace_matchers);

I'm a little confused here.  Don't we still need to account for
these two Py_DECREFs?

> -      if (py_xmethod_matcher_list == NULL)
> +      gdbpy_reference temp (PySequence_Concat (py_xmethod_matcher_list.get (),
> +					       pspace_matchers.get ()));
> +      if (temp == NULL)
>  	{
>  	  gdbpy_print_stack ();
> -	  do_cleanups (cleanups);
> -
>  	  return EXT_LANG_RC_ERROR;
Tom Tromey Nov. 11, 2016, 3:17 a.m. UTC | #2
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> -      Py_DECREF (temp);
>> -      Py_DECREF (pspace_matchers);

Pedro> I'm a little confused here.  Don't we still need to account for
Pedro> these two Py_DECREFs?

The patch reads a little messily.  The new code is:

      gdbpy_reference pspace_matchers (pspy_get_xmethods (py_progspace, NULL));

      gdbpy_reference temp (PySequence_Concat (py_xmethod_matcher_list.get (),
					       pspace_matchers.get ()));
      if (temp == NULL)
	{
	  gdbpy_print_stack ();
	  return EXT_LANG_RC_ERROR;
	}

      py_xmethod_matcher_list = temp;

So the decrefs are accounted for by the destructors; and that assignment
at the end encodes an incref via the copy constructor.  Moving here
would have been a valid choice as well, like:

     py_xmethod_matcher_list = std::move (temp);

... though I deleted the move constructor since it wasn't used in the
earlier series and Jan pointed out that it had a bug.

Tom
Pedro Alves Nov. 11, 2016, 3:24 a.m. UTC | #3
On 11/11/2016 03:17 AM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
>>> -      Py_DECREF (temp);
>>> -      Py_DECREF (pspace_matchers);
> 
> Pedro> I'm a little confused here.  Don't we still need to account for
> Pedro> these two Py_DECREFs?
> 
> The patch reads a little messily.  The new code is:
> 
>       gdbpy_reference pspace_matchers (pspy_get_xmethods (py_progspace, NULL));
> 
>       gdbpy_reference temp (PySequence_Concat (py_xmethod_matcher_list.get (),
> 					       pspace_matchers.get ()));
>       if (temp == NULL)
> 	{
> 	  gdbpy_print_stack ();
> 	  return EXT_LANG_RC_ERROR;
> 	}
> 
>       py_xmethod_matcher_list = temp;
> 
> So the decrefs are accounted for by the destructors; and that assignment
> at the end encodes an incref via the copy constructor.  Moving here
> would have been a valid choice as well, like:

Ah, I see.

> 
>      py_xmethod_matcher_list = std::move (temp);
> 
> ... though I deleted the move constructor since it wasn't used in the
> earlier series and Jan pointed out that it had a bug.

As per my comment on the other patch, I think that's a mistake though.

Thanks,
Pedro Alves
Tom Tromey Nov. 12, 2016, 5:08 p.m. UTC | #4
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> py_xmethod_matcher_list = std::move (temp);
>> 
>> ... though I deleted the move constructor since it wasn't used in the
>> earlier series and Jan pointed out that it had a bug.

Pedro> As per my comment on the other patch, I think that's a mistake though.

I've restored the rvalue constructor and assignment operator, and I
changed this patch to use std::move, saving an incref/decref pair.

Tom
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1a415fb..fef6a71 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@ 
 2016-11-10  Tom Tromey  <tom@tromey.com>
 
+	* python/py-xmethods.c (gdbpy_get_matching_xmethod_workers): use
+	gdbpy_enter, gdbpy_reference.
+
+2016-11-10  Tom Tromey  <tom@tromey.com>
+
 	* python/python.c (python_interactive_command): Use gdbpy_enter.
 
 2016-11-10  Tom Tromey  <tom@tromey.com>
diff --git a/gdb/python/py-xmethods.c b/gdb/python/py-xmethods.c
index 2c86478..ca83b0b 100644
--- a/gdb/python/py-xmethods.c
+++ b/gdb/python/py-xmethods.c
@@ -26,6 +26,7 @@ 
 
 #include "python.h"
 #include "python-internal.h"
+#include "py-ref.h"
 
 static const char enabled_field_name[] = "enabled";
 static const char match_method_name[] = "match";
@@ -156,33 +157,26 @@  gdbpy_get_matching_xmethod_workers
    struct type *obj_type, const char *method_name,
    xmethod_worker_vec **dm_vec)
 {
-  struct cleanup *cleanups;
   struct objfile *objfile;
   VEC (xmethod_worker_ptr) *worker_vec = NULL;
-  PyObject *py_type, *py_progspace;
-  PyObject *py_xmethod_matcher_list = NULL, *list_iter, *matcher;
+  PyObject *py_progspace;
 
   gdb_assert (obj_type != NULL && method_name != NULL);
 
-  cleanups = ensure_python_env (get_current_arch (), current_language);
+  gdbpy_enter enter_py (get_current_arch (), current_language);
 
-  py_type = type_to_type_object (obj_type);
+  gdbpy_reference py_type (type_to_type_object (obj_type));
   if (py_type == NULL)
     {
       gdbpy_print_stack ();
-      do_cleanups (cleanups);
-
       return EXT_LANG_RC_ERROR;
     }
-  make_cleanup_py_decref (py_type);
 
   /* Create an empty list of debug methods.  */
-  py_xmethod_matcher_list = PyList_New (0);
+  gdbpy_reference py_xmethod_matcher_list (PyList_New (0));
   if (py_xmethod_matcher_list == NULL)
     {
       gdbpy_print_stack ();
-      do_cleanups (cleanups);
-
       return EXT_LANG_RC_ERROR;
     }
 
@@ -192,28 +186,23 @@  gdbpy_get_matching_xmethod_workers
   ALL_OBJFILES (objfile)
     {
       PyObject *py_objfile = objfile_to_objfile_object (objfile);
-      PyObject *objfile_matchers, *temp = py_xmethod_matcher_list;
 
       if (py_objfile == NULL)
 	{
 	  gdbpy_print_stack ();
-	  Py_DECREF (py_xmethod_matcher_list);
-	  do_cleanups (cleanups);
-
 	  return EXT_LANG_RC_ERROR;
 	}
 
-      objfile_matchers = objfpy_get_xmethods (py_objfile, NULL);
-      py_xmethod_matcher_list = PySequence_Concat (temp, objfile_matchers);
-      Py_DECREF (temp);
-      Py_DECREF (objfile_matchers);
-      if (py_xmethod_matcher_list == NULL)
+      gdbpy_reference objfile_matchers (objfpy_get_xmethods (py_objfile, NULL));
+      gdbpy_reference temp (PySequence_Concat (py_xmethod_matcher_list.get (),
+					       objfile_matchers.get ()));
+      if (temp == NULL)
 	{
 	  gdbpy_print_stack ();
-	  do_cleanups (cleanups);
-
 	  return EXT_LANG_RC_ERROR;
 	}
+
+      py_xmethod_matcher_list = temp;
     }
 
   /* Gather debug methods matchers registered with the current program
@@ -221,26 +210,21 @@  gdbpy_get_matching_xmethod_workers
   py_progspace = pspace_to_pspace_object (current_program_space);
   if (py_progspace != NULL)
     {
-      PyObject *temp = py_xmethod_matcher_list;
-      PyObject *pspace_matchers = pspy_get_xmethods (py_progspace, NULL);
+      gdbpy_reference pspace_matchers (pspy_get_xmethods (py_progspace, NULL));
 
-      py_xmethod_matcher_list = PySequence_Concat (temp, pspace_matchers);
-      Py_DECREF (temp);
-      Py_DECREF (pspace_matchers);
-      if (py_xmethod_matcher_list == NULL)
+      gdbpy_reference temp (PySequence_Concat (py_xmethod_matcher_list.get (),
+					       pspace_matchers.get ()));
+      if (temp == NULL)
 	{
 	  gdbpy_print_stack ();
-	  do_cleanups (cleanups);
-
 	  return EXT_LANG_RC_ERROR;
 	}
+
+      py_xmethod_matcher_list = temp;
     }
   else
     {
       gdbpy_print_stack ();
-      Py_DECREF (py_xmethod_matcher_list);
-      do_cleanups (cleanups);
-
       return EXT_LANG_RC_ERROR;
     }
 
@@ -248,117 +232,96 @@  gdbpy_get_matching_xmethod_workers
   if (gdb_python_module != NULL
       && PyObject_HasAttrString (gdb_python_module, matchers_attr_str))
     {
-      PyObject *gdb_matchers;
-      PyObject *temp = py_xmethod_matcher_list;
-
-      gdb_matchers = PyObject_GetAttrString (gdb_python_module,
-					     matchers_attr_str);
+      gdbpy_reference gdb_matchers (PyObject_GetAttrString (gdb_python_module,
+							    matchers_attr_str));
       if (gdb_matchers != NULL)
 	{
-	  py_xmethod_matcher_list = PySequence_Concat (temp, gdb_matchers);
-	  Py_DECREF (temp);
-	  Py_DECREF (gdb_matchers);
-	  if (py_xmethod_matcher_list == NULL)
+	  gdbpy_reference temp
+	    (PySequence_Concat (py_xmethod_matcher_list.get (),
+				gdb_matchers.get ()));
+	  if (temp == NULL)
 	    {
 	      gdbpy_print_stack ();
-	      do_cleanups (cleanups);
-
 	      return EXT_LANG_RC_ERROR;
 	    }
+
+	  py_xmethod_matcher_list = temp;
 	}
       else
 	{
 	  gdbpy_print_stack ();
-	  Py_DECREF (py_xmethod_matcher_list);
-	  do_cleanups (cleanups);
-
 	  return EXT_LANG_RC_ERROR;
 	}
     }
 
-  /* Safe to make a cleanup for py_xmethod_matcher_list now as it
-     will not change any more.  */
-  make_cleanup_py_decref (py_xmethod_matcher_list);
-
-  list_iter = PyObject_GetIter (py_xmethod_matcher_list);
+  gdbpy_reference list_iter (PyObject_GetIter (py_xmethod_matcher_list.get ()));
   if (list_iter == NULL)
     {
       gdbpy_print_stack ();
-      do_cleanups (cleanups);
-
       return EXT_LANG_RC_ERROR;
     }
-  while ((matcher = PyIter_Next (list_iter)) != NULL)
+  while (true)
     {
-      PyObject *match_result = invoke_match_method (matcher, py_type,
-						    method_name);
+      gdbpy_reference matcher (PyIter_Next (list_iter.get ()));
+      if (matcher == NULL)
+	{
+	  if (PyErr_Occurred ())
+	    {
+	      gdbpy_print_stack ();
+	      return EXT_LANG_RC_ERROR;
+	    }
+	  break;
+	}
+
+      gdbpy_reference match_result
+	(invoke_match_method (matcher.get (), py_type.get (), method_name));
 
       if (match_result == NULL)
 	{
 	  gdbpy_print_stack ();
-	  Py_DECREF (matcher);
-	  do_cleanups (cleanups);
-
 	  return EXT_LANG_RC_ERROR;
 	}
       if (match_result == Py_None)
 	; /* This means there was no match.  */
-      else if (PySequence_Check (match_result))
+      else if (PySequence_Check (match_result.get ()))
 	{
-	  PyObject *iter = PyObject_GetIter (match_result);
-	  PyObject *py_worker;
+	  gdbpy_reference iter (PyObject_GetIter (match_result.get ()));
 
 	  if (iter == NULL)
 	    {
 	      gdbpy_print_stack ();
-	      Py_DECREF (matcher);
-	      Py_DECREF (match_result);
-	      do_cleanups (cleanups);
-
 	      return EXT_LANG_RC_ERROR;
 	    }
-	  while ((py_worker = PyIter_Next (iter)) != NULL)
+	  while (true)
 	    {
 	      struct xmethod_worker *worker;
 
-	      worker = new_python_xmethod_worker (py_worker, py_type);
+	      gdbpy_reference py_worker (PyIter_Next (iter.get ()));
+	      if (py_worker == NULL)
+		{
+		  if (PyErr_Occurred ())
+		    {
+		      gdbpy_print_stack ();
+		      return EXT_LANG_RC_ERROR;
+		    }
+		  break;
+		}
+
+	      worker = new_python_xmethod_worker (py_worker.get (),
+						  py_type.get ());
 	      VEC_safe_push (xmethod_worker_ptr, worker_vec, worker);
-	      Py_DECREF (py_worker);
-	    }
-	  Py_DECREF (iter);
-	  /* Report any error that could have occurred while iterating.  */
-	  if (PyErr_Occurred ())
-	    {
-	      gdbpy_print_stack ();
-	      Py_DECREF (matcher);
-	      Py_DECREF (match_result);
-	      do_cleanups (cleanups);
-
-	      return EXT_LANG_RC_ERROR;
 	    }
 	}
       else
 	{
 	  struct xmethod_worker *worker;
 
-	  worker = new_python_xmethod_worker (match_result, py_type);
+	  worker = new_python_xmethod_worker (match_result.get (),
+					      py_type.get ());
 	  VEC_safe_push (xmethod_worker_ptr, worker_vec, worker);
 	}
-
-      Py_DECREF (match_result);
-      Py_DECREF (matcher);
-    }
-  Py_DECREF (list_iter);
-  /* Report any error that could have occurred while iterating.  */
-  if (PyErr_Occurred ())
-    {
-      gdbpy_print_stack ();
-      do_cleanups (cleanups);
-
-      return EXT_LANG_RC_ERROR;
     }
 
-  do_cleanups (cleanups);
   *dm_vec = worker_vec;
 
   return EXT_LANG_RC_OK;