[RFA 00/11] third series of C++ use in Python layer

[RFA 00/11] third series of C++ use in Python layer

[Tom Tromey]

This series builds on my previous two in order to convert more of the
Python layer to gdbpy_enter and gdbpy_ref.

I built and tested gdb.python/*.exp locally; but I'm also sending this
through the buildbot.

Most of the remaining cleanups in the Python code refer to gdb objects
that haven't been converted to use destructors.  As more things are
converted I will work on cleaning this up.

Tom

[RFA 09/11] Use gdbpy_ref in py_print_frame

[Tom Tromey]

This changes py_print_frame to use gdbpy_ref in a few spots.

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

	* python/py-framefilter.c (py_print_frame): Use gdbpy_ref.
---
 gdb/ChangeLog               |  4 ++++
 gdb/python/py-framefilter.c | 41 ++++++++++++-----------------------------
 2 files changed, 16 insertions(+), 29 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 760bb9c..010190f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,9 @@
 2016-11-12  Tom Tromey  <tom@tromey.com>
 
+	* python/py-framefilter.c (py_print_frame): Use gdbpy_ref.
+
+2016-11-12  Tom Tromey  <tom@tromey.com>
+
 	* python/python.c (ensure_python_env, restore_python_env):
 	Remove.
 	* python/python-internal.h (ensure_python_env): Don't declare.
diff --git a/gdb/python/py-framefilter.c b/gdb/python/py-framefilter.c
index 090686c..c0c7cde 100644
--- a/gdb/python/py-framefilter.c
+++ b/gdb/python/py-framefilter.c
@@ -1171,8 +1171,7 @@ py_print_frame (PyObject *filter, int flags,
       /* Print frame function name.  */
       if (PyObject_HasAttrString (filter, "function"))
 	{
-	  PyObject *py_func = PyObject_CallMethod (filter, "function", NULL);
-	  struct cleanup *py_func_cleanup;
+	  gdbpy_ref py_func (PyObject_CallMethod (filter, "function", NULL));
 	  const char *function = NULL;
 
 	  if (py_func == NULL)
@@ -1180,11 +1179,10 @@ py_print_frame (PyObject *filter, int flags,
 	      do_cleanups (cleanup_stack);
 	      return EXT_LANG_BT_ERROR;
 	    }
-	  py_func_cleanup = make_cleanup_py_decref (py_func);
 
-	  if (gdbpy_is_string (py_func))
+	  if (gdbpy_is_string (py_func.get ()))
 	    {
-	      function_to_free = python_string_to_host_string (py_func);
+	      function_to_free = python_string_to_host_string (py_func.get ());
 
 	      if (function_to_free == NULL)
 		{
@@ -1194,12 +1192,12 @@ py_print_frame (PyObject *filter, int flags,
 
 	      function = function_to_free.get ();
 	    }
-	  else if (PyLong_Check (py_func))
+	  else if (PyLong_Check (py_func.get ()))
 	    {
 	      CORE_ADDR addr;
 	      struct bound_minimal_symbol msymbol;
 
-	      if (get_addr_from_python (py_func, &addr) < 0)
+	      if (get_addr_from_python (py_func.get (), &addr) < 0)
 		{
 		  do_cleanups (cleanup_stack);
 		  return EXT_LANG_BT_ERROR;
@@ -1233,8 +1231,6 @@ py_print_frame (PyObject *filter, int flags,
 	      return EXT_LANG_BT_ERROR;
 	    }
 	  END_CATCH
-
-	  do_cleanups (py_func_cleanup);
 	}
     }
 
@@ -1267,20 +1263,18 @@ py_print_frame (PyObject *filter, int flags,
 
       if (PyObject_HasAttrString (filter, "filename"))
 	{
-	  PyObject *py_fn = PyObject_CallMethod (filter, "filename", NULL);
-	  struct cleanup *py_fn_cleanup;
+	  gdbpy_ref py_fn (PyObject_CallMethod (filter, "filename", NULL));
 
 	  if (py_fn == NULL)
 	    {
 	      do_cleanups (cleanup_stack);
 	      return EXT_LANG_BT_ERROR;
 	    }
-	  py_fn_cleanup = make_cleanup_py_decref (py_fn);
 
 	  if (py_fn != Py_None)
 	    {
 	      gdb::unique_xmalloc_ptr<char>
-		filename (python_string_to_host_string (py_fn));
+		filename (python_string_to_host_string (py_fn.get ()));
 
 	      if (filename == NULL)
 		{
@@ -1304,13 +1298,11 @@ py_print_frame (PyObject *filter, int flags,
 		}
 	      END_CATCH
 	    }
-	  do_cleanups (py_fn_cleanup);
 	}
 
       if (PyObject_HasAttrString (filter, "line"))
 	{
-	  PyObject *py_line = PyObject_CallMethod (filter, "line", NULL);
-	  struct cleanup *py_line_cleanup;
+	  gdbpy_ref py_line (PyObject_CallMethod (filter, "line", NULL));
 	  int line;
 
 	  if (py_line == NULL)
@@ -1318,11 +1310,10 @@ py_print_frame (PyObject *filter, int flags,
 	      do_cleanups (cleanup_stack);
 	      return EXT_LANG_BT_ERROR;
 	    }
-	  py_line_cleanup = make_cleanup_py_decref (py_line);
 
 	  if (py_line != Py_None)
 	    {
-	      line = PyLong_AsLong (py_line);
+	      line = PyLong_AsLong (py_line.get ());
 	      if (PyErr_Occurred ())
 		{
 		  do_cleanups (cleanup_stack);
@@ -1343,7 +1334,6 @@ py_print_frame (PyObject *filter, int flags,
 		}
 	      END_CATCH
 	    }
-	  do_cleanups (py_line_cleanup);
 	}
     }
 
@@ -1376,17 +1366,13 @@ py_print_frame (PyObject *filter, int flags,
     }
 
   {
-    PyObject *elided;
-    struct cleanup *elided_cleanup;
-
     /* Finally recursively print elided frames, if any.  */
-    elided = get_py_iter_from_func (filter, "elided");
+    gdbpy_ref elided (get_py_iter_from_func (filter, "elided"));
     if (elided == NULL)
       {
 	do_cleanups (cleanup_stack);
 	return EXT_LANG_BT_ERROR;
       }
-    elided_cleanup = make_cleanup_py_decref (elided);
 
     if (elided != Py_None)
       {
@@ -1397,17 +1383,15 @@ py_print_frame (PyObject *filter, int flags,
 	if (! ui_out_is_mi_like_p (out))
 	  indent++;
 
-	while ((item = PyIter_Next (elided)))
+	while ((item = PyIter_Next (elided.get ())))
 	  {
-	    struct cleanup *item_cleanup = make_cleanup_py_decref (item);
+	    gdbpy_ref item_ref (item);
 
 	    enum ext_lang_bt_status success = py_print_frame (item, flags,
 							      args_type, out,
 							      indent,
 							      levels_printed);
 
-	    do_cleanups (item_cleanup);
-
 	    if (success == EXT_LANG_BT_ERROR)
 	      {
 		do_cleanups (cleanup_stack);
@@ -1420,7 +1404,6 @@ py_print_frame (PyObject *filter, int flags,
 	    return EXT_LANG_BT_ERROR;
 	  }
       }
-    do_cleanups (elided_cleanup);
   }
 
   do_cleanups (cleanup_stack);
-- 
2.7.4

[RFA 07/11] Use gdbpy_enter_varobj in varobj_value_get_print_value

[Tom Tromey]

This changes the last function in varobj.c to use gdbpy_enter_varobj.

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

	* varobj.c (varobj_value_get_print_value): Use
	gdbpy_enter_varobj.
---
 gdb/ChangeLog |  5 +++++
 gdb/varobj.c  | 21 +++++++++------------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index db5e029..be7ce38 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2016-11-12  Tom Tromey  <tom@tromey.com>
 
+	* varobj.c (varobj_value_get_print_value): Use
+	gdbpy_enter_varobj.
+
+2016-11-12  Tom Tromey  <tom@tromey.com>
+
 	* python/py-prettyprint.c (print_string_repr, print_children):
 	Update.
 	* python/py-lazy-string.c (gdbpy_extract_lazy_string): Change type
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 25c5019..deebe08 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -2431,7 +2431,7 @@ varobj_value_get_print_value (struct value *value,
     {
       PyObject *value_formatter =  var->dynamic->pretty_printer;
 
-      varobj_ensure_python_env (var);
+      gdbpy_enter_varobj enter_py (var);
 
       if (value_formatter)
 	{
@@ -2446,24 +2446,21 @@ varobj_value_get_print_value (struct value *value,
 	  if (PyObject_HasAttr (value_formatter, gdbpy_to_string_cst))
 	    {
 	      struct value *replacement;
-	      PyObject *output = NULL;
 
-	      output = apply_varobj_pretty_printer (value_formatter,
-						    &replacement,
-						    stb);
+	      gdbpy_ref output (apply_varobj_pretty_printer (value_formatter,
+							     &replacement,
+							     stb));
 
 	      /* If we have string like output ...  */
-	      if (output)
+	      if (output != NULL)
 		{
-		  make_cleanup_py_decref (output);
-
 		  /* If this is a lazy string, extract it.  For lazy
 		     strings we always print as a string, so set
 		     string_print.  */
-		  if (gdbpy_is_lazy_string (output))
+		  if (gdbpy_is_lazy_string (output.get ()))
 		    {
-		      gdbpy_extract_lazy_string (output, &str_addr, &type,
-						 &len, &encoding);
+		      gdbpy_extract_lazy_string (output.get (), &str_addr,
+						 &type, &len, &encoding);
 		      string_print = 1;
 		    }
 		  else
@@ -2475,7 +2472,7 @@ varobj_value_get_print_value (struct value *value,
 			 string as a value.  */
 
 		      gdb::unique_xmalloc_ptr<char> s
-			= python_string_to_target_string (output);
+			= python_string_to_target_string (output.get ());
 
 		      if (s)
 			{
-- 
2.7.4

[RFA 04/11] Use gdbpy_enter in python.c

[Tom Tromey]

This changes the last functions in python.c to use gdbpy_enter.  I
split gdbpy_finish_initialization into two functions in order to avoid
some "goto"s.

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

	* python/python.c (python_command): Use gdbpy_enter, gdbpy_ref.
	(do_finish_initialization): New function.  Use gdbpy_ref.
	(gdbpy_finish_initialization): Use gdbpy_enter.  Call
	do_finish_initialization.
---
 gdb/ChangeLog       |  7 +++++
 gdb/python/python.c | 82 ++++++++++++++++++++++++-----------------------------
 2 files changed, 44 insertions(+), 45 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index fe7da49..d335c10 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
 2016-11-12  Tom Tromey  <tom@tromey.com>
 
+	* python/python.c (python_command): Use gdbpy_enter, gdbpy_ref.
+	(do_finish_initialization): New function.  Use gdbpy_ref.
+	(gdbpy_finish_initialization): Use gdbpy_enter.  Call
+	do_finish_initialization.
+
+2016-11-12  Tom Tromey  <tom@tromey.com>
+
 	* python/py-param.c (get_set_value, get_show_value): Use
 	gdbpy_enter, gdbpy_ref.
 
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 03509e4..59fa9e1 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -452,9 +452,7 @@ gdbpy_eval_from_control_command (const struct extension_language_defn *extlang,
 static void
 python_command (char *arg, int from_tty)
 {
-  struct cleanup *cleanup;
-
-  cleanup = ensure_python_env (get_current_arch (), current_language);
+  gdbpy_enter enter_py (get_current_arch (), current_language);
 
   scoped_restore save_async = make_scoped_restore (&current_ui->async, 0);
 
@@ -467,12 +465,11 @@ python_command (char *arg, int from_tty)
   else
     {
       struct command_line *l = get_command_line (python_control, "");
+      struct cleanup *cleanup = make_cleanup_free_command_lines (&l);
 
-      make_cleanup_free_command_lines (&l);
       execute_control_command_untraced (l);
+      do_cleanups (cleanup);
     }
-
-  do_cleanups (cleanup);
 }
 
 \f
@@ -1819,26 +1816,20 @@ message == an error message without a stack will be printed."),
 
 #ifdef HAVE_PYTHON
 
-/* Perform the remaining python initializations.
-   These must be done after GDB is at least mostly initialized.
-   E.g., The "info pretty-printer" command needs the "info" prefix
-   command installed.
-   This is the extension_language_ops.finish_initialization "method".  */
+/* Helper function for gdbpy_finish_initialization.  This does the
+   work and then returns false if an error has occurred and must be
+   displayed, or true on success.  */
 
-static void
-gdbpy_finish_initialization (const struct extension_language_defn *extlang)
+static bool
+do_finish_initialization (const struct extension_language_defn *extlang)
 {
   PyObject *m;
-  char *gdb_pythondir;
   PyObject *sys_path;
-  struct cleanup *cleanup;
-
-  cleanup = ensure_python_env (get_current_arch (), current_language);
 
   /* Add the initial data-directory to sys.path.  */
 
-  gdb_pythondir = concat (gdb_datadir, SLASH_STRING, "python", (char *) NULL);
-  make_cleanup (xfree, gdb_pythondir);
+  std::string gdb_pythondir = (std::string (gdb_datadir) + SLASH_STRING
+			       + "python");
 
   sys_path = PySys_GetObject ("path");
 
@@ -1854,27 +1845,21 @@ gdbpy_finish_initialization (const struct extension_language_defn *extlang)
     }
   if (sys_path && PyList_Check (sys_path))
     {
-      PyObject *pythondir;
-      int err;
-
-      pythondir = PyString_FromString (gdb_pythondir);
-      if (pythondir == NULL)
-	goto fail;
-
-      err = PyList_Insert (sys_path, 0, pythondir);
-      Py_DECREF (pythondir);
-      if (err)
-	goto fail;
+      gdbpy_ref pythondir (PyString_FromString (gdb_pythondir.c_str ()));
+      if (pythondir == NULL || PyList_Insert (sys_path, 0, pythondir.get ()))
+	return false;
     }
   else
-    goto fail;
+    return false;
 
   /* Import the gdb module to finish the initialization, and
      add it to __main__ for convenience.  */
   m = PyImport_AddModule ("__main__");
   if (m == NULL)
-    goto fail;
+    return false;
 
+  /* Keep the reference to gdb_python_module since it is in a global
+     variable.  */
   gdb_python_module = PyImport_ImportModule ("gdb");
   if (gdb_python_module == NULL)
     {
@@ -1885,24 +1870,31 @@ gdbpy_finish_initialization (const struct extension_language_defn *extlang)
 		 "Could not load the Python gdb module from `%s'.\n"
 		 "Limited Python support is available from the _gdb module.\n"
 		 "Suggest passing --data-directory=/path/to/gdb/data-directory.\n"),
-		 gdb_pythondir);
-      do_cleanups (cleanup);
-      return;
+	       gdb_pythondir.c_str ());
+      /* We return "success" here as we've already emitted the
+	 warning.  */
+      return true;
     }
 
-  if (gdb_pymodule_addobject (m, "gdb", gdb_python_module) < 0)
-    goto fail;
+  return gdb_pymodule_addobject (m, "gdb", gdb_python_module) >= 0;
+}
 
-  /* Keep the reference to gdb_python_module since it is in a global
-     variable.  */
+/* Perform the remaining python initializations.
+   These must be done after GDB is at least mostly initialized.
+   E.g., The "info pretty-printer" command needs the "info" prefix
+   command installed.
+   This is the extension_language_ops.finish_initialization "method".  */
 
-  do_cleanups (cleanup);
-  return;
+static void
+gdbpy_finish_initialization (const struct extension_language_defn *extlang)
+{
+  gdbpy_enter enter_py (get_current_arch (), current_language);
 
- fail:
-  gdbpy_print_stack ();
-  warning (_("internal error: Unhandled Python exception"));
-  do_cleanups (cleanup);
+  if (! do_finish_initialization (extlang))
+    {
+      gdbpy_print_stack ();
+      warning (_("internal error: Unhandled Python exception"));
+    }
 }
 
 /* Return non-zero if Python has successfully initialized.
-- 
2.7.4

Re: [RFA 04/11] Use gdbpy_enter in python.c

[Pedro Alves]

LGTM.  Formatting nits below.

On 11/12/2016 09:28 PM, Tom Tromey wrote:
> @@ -1885,24 +1870,31 @@ gdbpy_finish_initialization (const struct extension_language_defn *extlang)
>  		 "Could not load the Python gdb module from `%s'.\n"
>  		 "Limited Python support is available from the _gdb module.\n"
>  		 "Suggest passing --data-directory=/path/to/gdb/data-directory.\n"),
> -		 gdb_pythondir);
> -      do_cleanups (cleanup);
> -      return;
> +	       gdb_pythondir.c_str ());

tabs/spaces here?

> +      /* We return "success" here as we've already emitted the
> +	 warning.  */
> +      return true;
>      }


> +  if (! do_finish_initialization (extlang))

No space after '!'.

Thanks,
Pedro Alves

Re: [RFA 04/11] Use gdbpy_enter in python.c

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> LGTM.  Formatting nits below.
Pedro> On 11/12/2016 09:28 PM, Tom Tromey wrote:
>> @@ -1885,24 +1870,31 @@ gdbpy_finish_initialization (const struct
>> extension_language_defn *extlang)
>> "Could not load the Python gdb module from `%s'.\n"
>> "Limited Python support is available from the _gdb module.\n"
>> "Suggest passing
>> --data-directory=/path/to/gdb/data-directory.\n"),
>> -		 gdb_pythondir);
>> -      do_cleanups (cleanup);
>> -      return;
>> +	       gdb_pythondir.c_str ());

Pedro> tabs/spaces here?

This one looks a bit funny but it is correct.
The new line replaces the "gdb_pythondir" line, which is indented
incorrectly in the current source.

>> +  if (! do_finish_initialization (extlang))

Pedro> No space after '!'.

I've fixed this one.

Tom

[RFA 11/11] Change python_run_simple_file to use gdbpy_ref

[Tom Tromey]

This changes python_run_simple_file to use gdbpy_ref and
unique_xmalloc_ptr.  Thi fixes a latent bug in this function, where
the error path previously ran the cleanups and then referred to one of
the objects just freed.

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

	* python/python.c (python_run_simple_file): Use
	unique_xmalloc_ptr, gdbpy_ref.
---
 gdb/ChangeLog       |  5 +++++
 gdb/python/python.c | 18 +++++-------------
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0f94b2e..b890293 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2016-11-12  Tom Tromey  <tom@tromey.com>
 
+	* python/python.c (python_run_simple_file): Use
+	unique_xmalloc_ptr, gdbpy_ref.
+
+2016-11-12  Tom Tromey  <tom@tromey.com>
+
 	* python/py-prettyprint.c (print_stack_unless_memory_error)
 	(print_string_repr, print_children): Use gdbpy_ref.
 
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 2bfe67f..ea1e647 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -349,25 +349,17 @@ python_run_simple_file (FILE *file, const char *filename)
 
 #else /* _WIN32 */
 
-  char *full_path;
-  PyObject *python_file;
-  struct cleanup *cleanup;
-
   /* Because we have a string for a filename, and are using Python to
      open the file, we need to expand any tilde in the path first.  */
-  full_path = tilde_expand (filename);
-  cleanup = make_cleanup (xfree, full_path);
-  python_file = PyFile_FromString (full_path, "r");
-  if (! python_file)
+  gdb::unique_xmalloc_ptr full_path = tilde_expand (filename);
+  gdbpy_run_events python_file (PyFile_FromString (full_path.get (), "r"));
+  if (python_file == NULL)
     {
-      do_cleanups (cleanup);
       gdbpy_print_stack ();
-      error (_("Error while opening file: %s"), full_path);
+      error (_("Error while opening file: %s"), full_path.get ());
     }
 
-  make_cleanup_py_decref (python_file);
-  PyRun_SimpleFile (PyFile_AsFile (python_file), filename);
-  do_cleanups (cleanup);
+  PyRun_SimpleFile (PyFile_AsFile (python_file.get ()), filename);
 
 #endif /* _WIN32 */
 }
-- 
2.7.4

Re: [RFA 11/11] Change python_run_simple_file to use gdbpy_ref

[Tom Tromey]

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

Tom> +  gdb::unique_xmalloc_ptr full_path = tilde_expand (filename);

This is missing a <char>.
I think if there isn't a win32 buildbot instance, I will just drop this
patch, as I have no way to test it.

Tom

Re: [RFA 11/11] Change python_run_simple_file to use gdbpy_ref

[Pedro Alves]

On 11/12/2016 09:39 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> 
> Tom> +  gdb::unique_xmalloc_ptr full_path = tilde_expand (filename);
> 
> This is missing a <char>.
> I think if there isn't a win32 buildbot instance, I will just drop this
> patch, as I have no way to test it.

I'd rather not drop it, since we'll need it in order to get rid of cleanups.
I think better would be to push this to a branch, to make it easy
for others to try it, and call out for (at least build-) testing help.

Actually, you don't really need that -- that WIN32 code should work
on Unix as well, it's just not as efficient as the other branch.
So you can just hack/invert the #ifndef _WIN32 locally for testing.

Thanks,
Pedro Alves

Re: [RFA 11/11] Change python_run_simple_file to use gdbpy_ref

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Actually, you don't really need that -- that WIN32 code should work
Pedro> on Unix as well, it's just not as efficient as the other branch.
Pedro> So you can just hack/invert the #ifndef _WIN32 locally for testing.

What if I just remove the non-win32 code here?
It might be slightly less efficient, but only slightly; with the upside
being that we'd have just a single code path to test.

Tom

Re: [RFA 11/11] Change python_run_simple_file to use gdbpy_ref

[Pedro Alves]

On 11/16/2016 06:22 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Actually, you don't really need that -- that WIN32 code should work
> Pedro> on Unix as well, it's just not as efficient as the other branch.
> Pedro> So you can just hack/invert the #ifndef _WIN32 locally for testing.
> 
> What if I just remove the non-win32 code here?
> It might be slightly less efficient, but only slightly; with the upside
> being that we'd have just a single code path to test.

That's fine with me.

Thanks,
Pedro Alves

Re: [RFA 11/11] Change python_run_simple_file to use gdbpy_ref

[Pedro Alves]

On 11/16/2016 08:10 PM, Pedro Alves wrote:
> On 11/16/2016 06:22 PM, Tom Tromey wrote:
>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>>
>> Pedro> Actually, you don't really need that -- that WIN32 code should work
>> Pedro> on Unix as well, it's just not as efficient as the other branch.
>> Pedro> So you can just hack/invert the #ifndef _WIN32 locally for testing.
>>
>> What if I just remove the non-win32 code here?
>> It might be slightly less efficient, but only slightly; with the upside
>> being that we'd have just a single code path to test.
> 
> That's fine with me.
> 

BTW, for completeness, a middle ground solution would be to change
the #ifndef to an if ().  


#ifdef _WIN32
# define GDB_PYTHON_OPENS_FILE 1
#else
# define GDB_PYTHON_OPENS_FILE 0
#endif

static void
python_run_simple_file (FILE *file, const char *filename)
{
  if (!GDB_PYTHON_OPENS_FILE)
    {
      PyRun_SimpleFile (file, filename);
    }
  else
    {
      ... longer path here ...
    }
}

That'd get us compile-time testing of both branches everywhere,
without run time penalty (the compiler optimizes out the dead branch).

But as said, making the longer branch the only branch is also
fine with me.

Thanks,
Pedro Alves

Re: [RFA 11/11] Change python_run_simple_file to use gdbpy_ref

[Tom Tromey]

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

Pedro> Actually, you don't really need that -- that WIN32 code should work
Pedro> on Unix as well, it's just not as efficient as the other branch.
Pedro> So you can just hack/invert the #ifndef _WIN32 locally for testing.

Tom> What if I just remove the non-win32 code here?
Tom> It might be slightly less efficient, but only slightly; with the upside
Tom> being that we'd have just a single code path to test.

I looked into this some more, and I am going to drop the patch after
all.

The main issue is that PyFile_AsFile doesn't exist in Python 3.  In
Python 3 it seems that the implementation will have to be more
complicated: it seems necessary to either evaluate Python code (perhaps
a helper in the gdb module's __init__.py); or to read the file in
python_run_simple_file, call the Python parser, and then evaluate the
result.

One of these seems necessary, but it's not really related to this series
and I'm not sure my interest extends to that.

Tom

Re: [RFA 11/11] Change python_run_simple_file to use gdbpy_ref

[Pedro Alves]

On 11/18/2016 11:22 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> 
> Pedro> Actually, you don't really need that -- that WIN32 code should work
> Pedro> on Unix as well, it's just not as efficient as the other branch.
> Pedro> So you can just hack/invert the #ifndef _WIN32 locally for testing.
> 
> Tom> What if I just remove the non-win32 code here?
> Tom> It might be slightly less efficient, but only slightly; with the upside
> Tom> being that we'd have just a single code path to test.
> 
> I looked into this some more, and I am going to drop the patch after
> all.
> 
> The main issue is that PyFile_AsFile doesn't exist in Python 3.  In
> Python 3 it seems that the implementation will have to be more
> complicated: it seems necessary to either evaluate Python code (perhaps
> a helper in the gdb module's __init__.py); or to read the file in
> python_run_simple_file, call the Python parser, and then evaluate the
> result.
> 
> One of these seems necessary, but it's not really related to this series
> and I'm not sure my interest extends to that.

Curious, that means that nobody is building mingw gdb against
Python 3 then.

I did git blame on that function now, and that point at git commit
4c63965b8, which shows that we used to do the longer path everywhere,
and from the git log it sounds like we don't want to go back.

So looks like we're back to the original suggestion of hacking the
WIN32 condition locally and test against Python 2.  If that works,
push it in.

Thanks,
Pedro Alves

Re: [RFA 11/11] Change python_run_simple_file to use gdbpy_ref

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Curious, that means that nobody is building mingw gdb against
Pedro> Python 3 then.

Yeah.

Pedro> I did git blame on that function now, and that point at git commit
Pedro> 4c63965b8, which shows that we used to do the longer path everywhere,
Pedro> and from the git log it sounds like we don't want to go back.

Yes, definitely.  There's an FAQ in the Python documentation about this
exact scenario.

Pedro> So looks like we're back to the original suggestion of hacking the
Pedro> WIN32 condition locally and test against Python 2.  If that works,
Pedro> push it in.

Ok.  Here is the updated patch that worked for me after temporarily
removing the WIN32 condition.

Tom

commit 1f6ef3298d54963cd6c0f84a901f29d91cdd1263
Author: Tom Tromey <tom@tromey.com>
Date:   Sat Nov 12 12:08:17 2016 -0700

    Change python_run_simple_file to use gdbpy_ref
    
    This changes python_run_simple_file to use gdbpy_ref and
    unique_xmalloc_ptr.  Thi fixes a latent bug in this function, where
    the error path previously ran the cleanups and then referred to one of
    the objects just freed.
    
    2016-11-12  Tom Tromey  <tom@tromey.com>
    
    	* python/python.c (python_run_simple_file): Use
    	unique_xmalloc_ptr, gdbpy_ref.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2525ac7..49c226b 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2016-11-12  Tom Tromey  <tom@tromey.com>
 
+	* python/python.c (python_run_simple_file): Use
+	unique_xmalloc_ptr, gdbpy_ref.
+
+2016-11-12  Tom Tromey  <tom@tromey.com>
+
 	* python/py-prettyprint.c (print_stack_unless_memory_error)
 	(print_string_repr, print_children): Use gdbpy_ref.
 	(dummy_python_frame): New class.
diff --git a/gdb/python/python.c b/gdb/python/python.c
index b1d086c..7a38ca2 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -349,25 +349,17 @@ python_run_simple_file (FILE *file, const char *filename)
 
 #else /* _WIN32 */
 
-  char *full_path;
-  PyObject *python_file;
-  struct cleanup *cleanup;
-
   /* Because we have a string for a filename, and are using Python to
      open the file, we need to expand any tilde in the path first.  */
-  full_path = tilde_expand (filename);
-  cleanup = make_cleanup (xfree, full_path);
-  python_file = PyFile_FromString (full_path, "r");
-  if (! python_file)
+  gdb::unique_xmalloc_ptr<char> full_path (tilde_expand (filename));
+  gdbpy_ref python_file (PyFile_FromString (full_path.get (), "r"));
+  if (python_file == NULL)
     {
-      do_cleanups (cleanup);
       gdbpy_print_stack ();
-      error (_("Error while opening file: %s"), full_path);
+      error (_("Error while opening file: %s"), full_path.get ());
     }
 
-  make_cleanup_py_decref (python_file);
-  PyRun_SimpleFile (PyFile_AsFile (python_file), filename);
-  do_cleanups (cleanup);
+  PyRun_SimpleFile (PyFile_AsFile (python_file.get ()), filename);
 
 #endif /* _WIN32 */
 }

Re: [RFA 11/11] Change python_run_simple_file to use gdbpy_ref

[Pedro Alves]

On 11/19/2016 05:03 PM, Tom Tromey wrote:

> Pedro> I did git blame on that function now, and that point at git commit
> Pedro> 4c63965b8, which shows that we used to do the longer path everywhere,
> Pedro> and from the git log it sounds like we don't want to go back.
> 
> Yes, definitely.  There's an FAQ in the Python documentation about this
> exact scenario.
> 
> Pedro> So looks like we're back to the original suggestion of hacking the
> Pedro> WIN32 condition locally and test against Python 2.  If that works,
> Pedro> push it in.
> 
> Ok.  Here is the updated patch that worked for me after temporarily
> removing the WIN32 condition.

OK.

Thanks,
Pedro Alves

[RFA 05/11] Use gdbpy_enter_varobj in more of varobj.c

[Tom Tromey]

This converts most of the remaining functions in varobj.c to use
gdbpy_enter_varobj.

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

	* varobj.c (varobj_get_display_hint)
	(dynamic_varobj_has_child_method, install_new_value_visualizer)
	(varobj_set_visualizer, free_variable): Use
	gdbpy_enter_varobj.
---
 gdb/ChangeLog |  7 +++++++
 gdb/varobj.c  | 44 ++++++++++++++------------------------------
 2 files changed, 21 insertions(+), 30 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d335c10..778b267 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
 2016-11-12  Tom Tromey  <tom@tromey.com>
 
+	* varobj.c (varobj_get_display_hint)
+	(dynamic_varobj_has_child_method, install_new_value_visualizer)
+	(varobj_set_visualizer, free_variable): Use
+	gdbpy_enter_varobj.
+
+2016-11-12  Tom Tromey  <tom@tromey.com>
+
 	* python/python.c (python_command): Use gdbpy_enter, gdbpy_ref.
 	(do_finish_initialization): New function.  Use gdbpy_ref.
 	(gdbpy_finish_initialization): Use gdbpy_enter.  Call
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 8abda59..dbb80a8 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -34,6 +34,7 @@
 #if HAVE_PYTHON
 #include "python/python.h"
 #include "python/python-internal.h"
+#include "python/py-ref.h"
 #else
 typedef int PyObject;
 #endif
@@ -570,17 +571,13 @@ varobj_get_display_hint (const struct varobj *var)
   gdb::unique_xmalloc_ptr<char> result;
 
 #if HAVE_PYTHON
-  struct cleanup *back_to;
-
   if (!gdb_python_initialized)
     return NULL;
 
-  back_to = varobj_ensure_python_env (var);
+  gdbpy_enter_varobj enter_py (var);
 
   if (var->dynamic->pretty_printer != NULL)
     result = gdbpy_get_display_hint (var->dynamic->pretty_printer);
-
-  do_cleanups (back_to);
 #endif
 
   return result;
@@ -702,17 +699,13 @@ install_dynamic_child (struct varobj *var,
 static int
 dynamic_varobj_has_child_method (const struct varobj *var)
 {
-  struct cleanup *back_to;
   PyObject *printer = var->dynamic->pretty_printer;
-  int result;
 
   if (!gdb_python_initialized)
     return 0;
 
-  back_to = varobj_ensure_python_env (var);
-  result = PyObject_HasAttr (printer, gdbpy_children_cst);
-  do_cleanups (back_to);
-  return result;
+  gdbpy_enter_varobj enter_py (var);
+  return PyObject_HasAttr (printer, gdbpy_children_cst);
 }
 #endif
 
@@ -1220,16 +1213,12 @@ install_new_value_visualizer (struct varobj *var)
 
   if (var->dynamic->constructor != Py_None && var->value != NULL)
     {
-      struct cleanup *cleanup;
-
-      cleanup = varobj_ensure_python_env (var);
+      gdbpy_enter_varobj enter_py (var);
 
       if (var->dynamic->constructor == NULL)
 	install_default_visualizer (var);
       else
 	construct_visualizer (var, var->dynamic->constructor);
-
-      do_cleanups (cleanup);
     }
 #else
   /* Do nothing.  */
@@ -1489,35 +1478,31 @@ void
 varobj_set_visualizer (struct varobj *var, const char *visualizer)
 {
 #if HAVE_PYTHON
-  PyObject *mainmod, *globals, *constructor;
-  struct cleanup *back_to;
+  PyObject *mainmod;
 
   if (!gdb_python_initialized)
     return;
 
-  back_to = varobj_ensure_python_env (var);
+  gdbpy_enter_varobj enter_py (var);
 
   mainmod = PyImport_AddModule ("__main__");
-  globals = PyModule_GetDict (mainmod);
-  Py_INCREF (globals);
-  make_cleanup_py_decref (globals);
+  gdbpy_ref globals (PyModule_GetDict (mainmod));
+  Py_INCREF (globals.get ());
 
-  constructor = PyRun_String (visualizer, Py_eval_input, globals, globals);
+  gdbpy_ref constructor (PyRun_String (visualizer, Py_eval_input,
+				       globals.get (), globals.get ()));
 
-  if (! constructor)
+  if (constructor == NULL)
     {
       gdbpy_print_stack ();
       error (_("Could not evaluate visualizer expression: %s"), visualizer);
     }
 
-  construct_visualizer (var, constructor);
-  Py_XDECREF (constructor);
+  construct_visualizer (var, constructor.get ());
 
   /* If there are any children now, wipe them.  */
   varobj_delete (var, 1 /* children only */);
   var->num_children = -1;
-
-  do_cleanups (back_to);
 #else
   error (_("Python support required"));
 #endif
@@ -2106,11 +2091,10 @@ free_variable (struct varobj *var)
 #if HAVE_PYTHON
   if (var->dynamic->pretty_printer != NULL)
     {
-      struct cleanup *cleanup = varobj_ensure_python_env (var);
+      gdbpy_enter_varobj enter_py (var);
 
       Py_XDECREF (var->dynamic->constructor);
       Py_XDECREF (var->dynamic->pretty_printer);
-      do_cleanups (cleanup);
     }
 #endif
 
-- 
2.7.4

[RFA 02/11] Use gdbpy_enter in fnpy_call

[Tom Tromey]

This changes fnpy_call to use gdbpy_enter and gdbpy_ref.

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

	* python/py-function.c (fnpy_call): Use gdbpy_enter, gdbpy_ref.
---
 gdb/ChangeLog            |  4 ++++
 gdb/python/py-function.c | 32 +++++++++++---------------------
 2 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 9c0e5d2..1a7e8e2 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,9 @@
 2016-11-12  Tom Tromey  <tom@tromey.com>
 
+	* python/py-function.c (fnpy_call): Use gdbpy_enter, gdbpy_ref.
+
+2016-11-12  Tom Tromey  <tom@tromey.com>
+
 	* python/py-cmd.c (cmdpy_function): Use gdbpy_enter, gdbpy_ref.
 
 2016-11-10  Tom Tromey  <tom@tromey.com>
diff --git a/gdb/python/py-function.c b/gdb/python/py-function.c
index 2bdab7e..00f0848 100644
--- a/gdb/python/py-function.c
+++ b/gdb/python/py-function.c
@@ -62,34 +62,28 @@ fnpy_call (struct gdbarch *gdbarch, const struct language_defn *language,
   struct value *value = NULL;
   /* 'result' must be set to NULL, this initially indicates whether
      the function was called, or not.  */
-  PyObject *result = NULL;
-  PyObject *callable, *args;
-  struct cleanup *cleanup;
+  gdbpy_ref result;
 
-  cleanup = ensure_python_env (gdbarch, language);
+  gdbpy_enter enter_py (gdbarch, language);
 
-  args = convert_values_to_python (argc, argv);
+  gdbpy_ref args (convert_values_to_python (argc, argv));
   /* convert_values_to_python can return NULL on error.  If we
      encounter this, do not call the function, but allow the Python ->
      error code conversion below to deal with the Python exception.
      Note, that this is different if the function simply does not
      have arguments.  */
 
-  if (args)
+  if (args != NULL)
     {
-      callable = PyObject_GetAttrString ((PyObject *) cookie, "invoke");
-      if (! callable)
-	{
-	  Py_DECREF (args);
-	  error (_("No method named 'invoke' in object."));
-	}
+      gdbpy_ref callable (PyObject_GetAttrString ((PyObject *) cookie,
+						  "invoke"));
+      if (callable == NULL)
+	error (_("No method named 'invoke' in object."));
 
-      result = PyObject_Call (callable, args, NULL);
-      Py_DECREF (callable);
-      Py_DECREF (args);
+      result.reset (PyObject_Call (callable.get (), args.get (), NULL));
     }
 
-  if (!result)
+  if (result == NULL)
     {
       PyObject *ptype, *pvalue, *ptraceback;
 
@@ -141,17 +135,13 @@ fnpy_call (struct gdbarch *gdbarch, const struct language_defn *language,
 	}
     }
 
-  value = convert_value_from_python (result);
+  value = convert_value_from_python (result.get ());
   if (value == NULL)
     {
-      Py_DECREF (result);
       gdbpy_print_stack ();
       error (_("Error while executing Python code."));
     }
 
-  Py_DECREF (result);
-  do_cleanups (cleanup);
-
   return value;
 }
 
-- 
2.7.4

[RFA 03/11] Use gdbpy_enter in py-param.c

[Tom Tromey]

This converts the remaining functions in py-param.c to use
gdbpy_enter.

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

	* python/py-param.c (get_set_value, get_show_value): Use
	gdbpy_enter, gdbpy_ref.
---
 gdb/ChangeLog         |  5 ++++
 gdb/python/py-param.c | 75 ++++++++++++++++++++++++---------------------------
 2 files changed, 40 insertions(+), 40 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1a7e8e2..fe7da49 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2016-11-12  Tom Tromey  <tom@tromey.com>
 
+	* python/py-param.c (get_set_value, get_show_value): Use
+	gdbpy_enter, gdbpy_ref.
+
+2016-11-12  Tom Tromey  <tom@tromey.com>
+
 	* python/py-function.c (fnpy_call): Use gdbpy_enter, gdbpy_ref.
 
 2016-11-12  Tom Tromey  <tom@tromey.com>
diff --git a/gdb/python/py-param.c b/gdb/python/py-param.c
index 026fa67..d402928 100644
--- a/gdb/python/py-param.c
+++ b/gdb/python/py-param.c
@@ -363,18 +363,24 @@ get_set_value (char *args, int from_tty,
 {
   PyObject *obj = (PyObject *) get_cmd_context (c);
   gdb::unique_xmalloc_ptr<char> set_doc_string;
-  struct cleanup *cleanup = ensure_python_env (get_current_arch (),
-					       current_language);
-  PyObject *set_doc_func = PyString_FromString ("get_set_string");
 
-  if (! set_doc_func)
-    goto error;
+  gdbpy_enter enter_py (get_current_arch (), current_language);
+  gdbpy_ref set_doc_func (PyString_FromString ("get_set_string"));
 
-  if (PyObject_HasAttr (obj, set_doc_func))
+  if (set_doc_func == NULL)
     {
-      set_doc_string = call_doc_function (obj, set_doc_func, NULL);
+      gdbpy_print_stack ();
+      return;
+    }
+
+  if (PyObject_HasAttr (obj, set_doc_func.get ()))
+    {
+      set_doc_string = call_doc_function (obj, set_doc_func.get (), NULL);
       if (! set_doc_string)
-	goto error;
+	{
+	  gdbpy_print_stack ();
+	  return;
+	}
     }
   else
     {
@@ -385,16 +391,6 @@ get_set_value (char *args, int from_tty,
     }
 
   fprintf_filtered (gdb_stdout, "%s\n", set_doc_string.get ());
-
-  Py_XDECREF (set_doc_func);
-  do_cleanups (cleanup);
-  return;
-
- error:
-  Py_XDECREF (set_doc_func);
-  gdbpy_print_stack ();
-  do_cleanups (cleanup);
-  return;
 }
 
 /* A callback function that is registered against the respective
@@ -410,24 +406,33 @@ get_show_value (struct ui_file *file, int from_tty,
 {
   PyObject *obj = (PyObject *) get_cmd_context (c);
   gdb::unique_xmalloc_ptr<char> show_doc_string;
-  struct cleanup *cleanup = ensure_python_env (get_current_arch (),
-					       current_language);
-  PyObject *show_doc_func = PyString_FromString ("get_show_string");
 
-  if (! show_doc_func)
-    goto error;
+  gdbpy_enter enter_py (get_current_arch (), current_language);
+  gdbpy_ref show_doc_func (PyString_FromString ("get_show_string"));
+
+  if (show_doc_func == NULL)
+    {
+      gdbpy_print_stack ();
+      return;
+    }
 
-  if (PyObject_HasAttr (obj, show_doc_func))
+  if (PyObject_HasAttr (obj, show_doc_func.get ()))
     {
-      PyObject *val_obj = PyString_FromString (value);
+      gdbpy_ref val_obj (PyString_FromString (value));
 
-      if (! val_obj)
-	goto error;
+      if (val_obj == NULL)
+	{
+	  gdbpy_print_stack ();
+	  return;
+	}
 
-      show_doc_string = call_doc_function (obj, show_doc_func, val_obj);
-      Py_DECREF (val_obj);
+      show_doc_string = call_doc_function (obj, show_doc_func.get (),
+					   val_obj.get ());
       if (! show_doc_string)
-	goto error;
+	{
+	  gdbpy_print_stack ();
+	  return;
+	}
 
       fprintf_filtered (file, "%s\n", show_doc_string.get ());
     }
@@ -439,16 +444,6 @@ get_show_value (struct ui_file *file, int from_tty,
       show_doc_string  = get_doc_string (obj, show_doc_cst);
       fprintf_filtered (file, "%s %s\n", show_doc_string.get (), value);
     }
-
-  Py_XDECREF (show_doc_func);
-  do_cleanups (cleanup);
-  return;
-
- error:
-  Py_XDECREF (show_doc_func);
-  gdbpy_print_stack ();
-  do_cleanups (cleanup);
-  return;
 }
 \f
 
-- 
2.7.4

[RFA 08/11] Remove ensure_python_env

[Tom Tromey]

All of gdb has been converted away from ensure_python_env and
varobj_ensure_python_env now; so remove them.

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

	* python/python.c (ensure_python_env, restore_python_env):
	Remove.
	* python/python-internal.h (ensure_python_env): Don't declare.
	* varobj.h (varobj_ensure_python_env): Don't declare.
	* varobj.c (varobj_ensure_python_env): Remove.
---
 gdb/ChangeLog                |  8 ++++++++
 gdb/python/python-internal.h |  3 ---
 gdb/python/python.c          | 23 -----------------------
 gdb/varobj.c                 |  8 --------
 gdb/varobj.h                 |  2 --
 5 files changed, 8 insertions(+), 36 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index be7ce38..760bb9c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,13 @@
 2016-11-12  Tom Tromey  <tom@tromey.com>
 
+	* python/python.c (ensure_python_env, restore_python_env):
+	Remove.
+	* python/python-internal.h (ensure_python_env): Don't declare.
+	* varobj.h (varobj_ensure_python_env): Don't declare.
+	* varobj.c (varobj_ensure_python_env): Remove.
+
+2016-11-12  Tom Tromey  <tom@tromey.com>
+
 	* varobj.c (varobj_value_get_print_value): Use
 	gdbpy_enter_varobj.
 
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index d167251..fdbff30 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -535,9 +535,6 @@ class gdbpy_enter_varobj : public gdbpy_enter
 
 };
 
-struct cleanup *ensure_python_env (struct gdbarch *gdbarch,
-				   const struct language_defn *language);
-
 extern struct gdbarch *python_gdbarch;
 extern const struct language_defn *python_language;
 
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 59fa9e1..2bfe67f 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -241,29 +241,6 @@ gdbpy_enter::~gdbpy_enter ()
   restore_active_ext_lang (m_previous_active);
 }
 
-static void
-restore_python_env (void *p)
-{
-  gdbpy_enter *env = (gdbpy_enter *) p;
-
-  delete env;
-}
-
-/* Called before entering the Python interpreter to install the
-   current language and architecture to be used for Python values.
-   Also set the active extension language for GDB so that SIGINT's
-   are directed our way, and if necessary install the right SIGINT
-   handler.  */
-
-struct cleanup *
-ensure_python_env (struct gdbarch *gdbarch,
-                   const struct language_defn *language)
-{
-  gdbpy_enter *env = new gdbpy_enter (gdbarch, language);
-
-  return make_cleanup (restore_python_env, env);
-}
-
 /* Set the quit flag.  */
 
 static void
diff --git a/gdb/varobj.c b/gdb/varobj.c
index deebe08..4580b87 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -226,14 +226,6 @@ is_root_p (const struct varobj *var)
 }
 
 #ifdef HAVE_PYTHON
-/* Helper function to install a Python environment suitable for
-   use during operations on VAR.  */
-struct cleanup *
-varobj_ensure_python_env (const struct varobj *var)
-{
-  return ensure_python_env (var->root->exp->gdbarch,
-			    var->root->exp->language_defn);
-}
 
 /* See python-internal.h.  */
 gdbpy_enter_varobj::gdbpy_enter_varobj (const struct varobj *var)
diff --git a/gdb/varobj.h b/gdb/varobj.h
index df5c6cb..3ecf071 100644
--- a/gdb/varobj.h
+++ b/gdb/varobj.h
@@ -320,8 +320,6 @@ extern int varobj_has_more (const struct varobj *var, int to);
 
 extern int varobj_is_dynamic_p (const struct varobj *var);
 
-extern struct cleanup *varobj_ensure_python_env (const struct varobj *var);
-
 extern int varobj_default_value_is_changeable_p (const struct varobj *var);
 extern int varobj_value_is_changeable_p (const struct varobj *var);
 
-- 
2.7.4

Re: [RFA 08/11] Remove ensure_python_env

[Pedro Alves]

LGTM.  A comment on a comment, below.

On 11/12/2016 09:28 PM, Tom Tromey wrote:
> -/* Called before entering the Python interpreter to install the
> -   current language and architecture to be used for Python values.
> -   Also set the active extension language for GDB so that SIGINT's
> -   are directed our way, and if necessary install the right SIGINT
> -   handler.  */

Does gdbpy_enter's description have an equivalent comment, or would we
lose some detail that would better be moved?  I haven't gone back and
checked your original patch.

> -struct cleanup *
> -ensure_python_env (struct gdbarch *gdbarch,
> -                   const struct language_defn *language)

Thanks,
Pedro Alves

Re: [RFA 08/11] Remove ensure_python_env

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Does gdbpy_enter's description have an equivalent comment, or would we
Pedro> lose some detail that would better be moved?  I haven't gone back and
Pedro> checked your original patch.

We would lose some detail.  I've gone back and simply reused this
comment in the patch that introduces gdbpy_enter.

Tom

[RFA 06/11] Change type of encoding argument to gdbpy_extract_lazy_string

[Tom Tromey]

This changes gdbpy_extract_lazy_string's "encoding" argument to be a
unique_xmalloc_ptr.  I chose this rather than std::string because it
can sometimes be NULL.

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

	* python/py-prettyprint.c (print_string_repr, print_children):
	Update.
	* python/py-lazy-string.c (gdbpy_extract_lazy_string): Change type
	of "encoding".
	* varobj.c (varobj_value_get_print_value): Update.
	* python/python-internal.h (gdbpy_extract_lazy_string): Update.
---
 gdb/ChangeLog                |  9 +++++++++
 gdb/python/py-lazy-string.c  |  9 ++++-----
 gdb/python/py-prettyprint.c  | 10 ++++------
 gdb/python/python-internal.h |  3 ++-
 gdb/varobj.c                 |  7 +++----
 5 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 778b267..db5e029 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,14 @@
 2016-11-12  Tom Tromey  <tom@tromey.com>
 
+	* python/py-prettyprint.c (print_string_repr, print_children):
+	Update.
+	* python/py-lazy-string.c (gdbpy_extract_lazy_string): Change type
+	of "encoding".
+	* varobj.c (varobj_value_get_print_value): Update.
+	* python/python-internal.h (gdbpy_extract_lazy_string): Update.
+
+2016-11-12  Tom Tromey  <tom@tromey.com>
+
 	* varobj.c (varobj_get_display_hint)
 	(dynamic_varobj_has_child_method, install_new_value_visualizer)
 	(varobj_set_visualizer, free_variable): Use
diff --git a/gdb/python/py-lazy-string.c b/gdb/python/py-lazy-string.c
index d4b40df..74515ae 100644
--- a/gdb/python/py-lazy-string.c
+++ b/gdb/python/py-lazy-string.c
@@ -180,14 +180,13 @@ gdbpy_is_lazy_string (PyObject *result)
 }
 
 /* Extract the parameters from the lazy string object STRING.
-   ENCODING will either be set to NULL, or will be allocated with
-   xmalloc, in which case the callers is responsible for freeing
-   it.  */
+   ENCODING may be set to NULL, if no encoding is found.  */
 
 void
 gdbpy_extract_lazy_string (PyObject *string, CORE_ADDR *addr,
 			   struct type **str_type,
-			   long *length, char **encoding)
+			   long *length,
+			   gdb::unique_xmalloc_ptr<char> *encoding)
 {
   lazy_string_object *lazy;
 
@@ -198,7 +197,7 @@ gdbpy_extract_lazy_string (PyObject *string, CORE_ADDR *addr,
   *addr = lazy->address;
   *str_type = lazy->type;
   *length = lazy->length;
-  *encoding = lazy->encoding ? xstrdup (lazy->encoding) : NULL;
+  encoding->reset (lazy->encoding ? xstrdup (lazy->encoding) : NULL);
 }
 
 \f
diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c
index 5f11397..e7a3e76 100644
--- a/gdb/python/py-prettyprint.c
+++ b/gdb/python/py-prettyprint.c
@@ -301,15 +301,14 @@ print_string_repr (PyObject *printer, const char *hint,
 	  CORE_ADDR addr;
 	  long length;
 	  struct type *type;
-	  char *encoding = NULL;
+	  gdb::unique_xmalloc_ptr<char> encoding;
 	  struct value_print_options local_opts = *options;
 
-	  make_cleanup (free_current_contents, &encoding);
 	  gdbpy_extract_lazy_string (py_str, &addr, &type,
 				     &length, &encoding);
 
 	  local_opts.addressprint = 0;
-	  val_print_string (type, encoding, addr, (int) length,
+	  val_print_string (type, encoding.get (), addr, (int) length,
 			    stream, &local_opts);
 	}
       else
@@ -610,14 +609,13 @@ print_children (PyObject *printer, const char *hint,
 	  CORE_ADDR addr;
 	  struct type *type;
 	  long length;
-	  char *encoding = NULL;
+	  gdb::unique_xmalloc_ptr<char> encoding;
 	  struct value_print_options local_opts = *options;
 
-	  make_cleanup (free_current_contents, &encoding);
 	  gdbpy_extract_lazy_string (py_v, &addr, &type, &length, &encoding);
 
 	  local_opts.addressprint = 0;
-	  val_print_string (type, encoding, addr, (int) length, stream,
+	  val_print_string (type, encoding.get (), addr, (int) length, stream,
 			    &local_opts);
 	}
       else if (gdbpy_is_string (py_v))
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index caab6af..d167251 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -580,7 +580,8 @@ gdb::unique_xmalloc_ptr<char> gdbpy_exception_to_string (PyObject *ptype,
 int gdbpy_is_lazy_string (PyObject *result);
 void gdbpy_extract_lazy_string (PyObject *string, CORE_ADDR *addr,
 				struct type **str_type,
-				long *length, char **encoding);
+				long *length,
+				gdb::unique_xmalloc_ptr<char> *encoding);
 
 int gdbpy_is_value_object (PyObject *obj);
 
diff --git a/gdb/varobj.c b/gdb/varobj.c
index dbb80a8..25c5019 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -2413,7 +2413,7 @@ varobj_value_get_print_value (struct value *value,
   struct value_print_options opts;
   struct type *type = NULL;
   long len = 0;
-  char *encoding = NULL;
+  gdb::unique_xmalloc_ptr<char> encoding;
   /* Initialize it just to avoid a GCC false warning.  */
   CORE_ADDR str_addr = 0;
   int string_print = 0;
@@ -2464,7 +2464,6 @@ varobj_value_get_print_value (struct value *value,
 		    {
 		      gdbpy_extract_lazy_string (output, &str_addr, &type,
 						 &len, &encoding);
-		      make_cleanup (free_current_contents, &encoding);
 		      string_print = 1;
 		    }
 		  else
@@ -2520,11 +2519,11 @@ varobj_value_get_print_value (struct value *value,
   /* If the THEVALUE has contents, it is a regular string.  */
   if (!thevalue.empty ())
     LA_PRINT_STRING (stb, type, (gdb_byte *) thevalue.c_str (),
-		     len, encoding, 0, &opts);
+		     len, encoding.get (), 0, &opts);
   else if (string_print)
     /* Otherwise, if string_print is set, and it is not a regular
        string, it is a lazy string.  */
-    val_print_string (type, encoding, str_addr, len, stb, &opts);
+    val_print_string (type, encoding.get (), str_addr, len, stb, &opts);
   else
     /* All other cases.  */
     common_val_print (value, stb, 0, &opts, current_language);
-- 
2.7.4

Re: [RFA 06/11] Change type of encoding argument to gdbpy_extract_lazy_string

[Pedro Alves]

On 11/12/2016 09:28 PM, Tom Tromey wrote:
> This changes gdbpy_extract_lazy_string's "encoding" argument to be a
> unique_xmalloc_ptr.  I chose this rather than std::string because it
> can sometimes be NULL.

[OOC, the other day I backported GCC trunk's C++17's std::optional
to C++11, just to see how it'd look.  Surprisingly, it compiles
almost OOTB, with no loss in functionality.  Mostly, it requires 
backporting a few trait types that are missing in C++11.  Anyway, I
wouldn't really propose adding that unless there was a great
advantage.]

> 
> 2016-11-12  Tom Tromey  <tom@tromey.com>
> 
> 	* python/py-prettyprint.c (print_string_repr, print_children):
> 	Update.
> 	* python/py-lazy-string.c (gdbpy_extract_lazy_string): Change type
> 	of "encoding".
> 	* varobj.c (varobj_value_get_print_value): Update.
> 	* python/python-internal.h (gdbpy_extract_lazy_string): Update.

LGTM.

Thanks,
Pedro Alves

[RFA 10/11] Use gdbpy_ref in py-prettyprint.c

[Tom Tromey]

This changes some spots in py-prettyprint.c to use gdbpy_ref.

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

	* python/py-prettyprint.c (print_stack_unless_memory_error)
	(print_string_repr, print_children): Use gdbpy_ref.
---
 gdb/ChangeLog               |  5 +++
 gdb/python/py-prettyprint.c | 84 ++++++++++++++++-----------------------------
 2 files changed, 34 insertions(+), 55 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 010190f..0f94b2e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2016-11-12  Tom Tromey  <tom@tromey.com>
 
+	* python/py-prettyprint.c (print_stack_unless_memory_error)
+	(print_string_repr, print_children): Use gdbpy_ref.
+
+2016-11-12  Tom Tromey  <tom@tromey.com>
+
 	* python/py-framefilter.c (py_print_frame): Use gdbpy_ref.
 
 2016-11-12  Tom Tromey  <tom@tromey.com>
diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c
index e7a3e76..d7c9145 100644
--- a/gdb/python/py-prettyprint.c
+++ b/gdb/python/py-prettyprint.c
@@ -252,13 +252,13 @@ print_stack_unless_memory_error (struct ui_file *stream)
 {
   if (PyErr_ExceptionMatches (gdbpy_gdb_memory_error))
     {
-      struct cleanup *cleanup;
       PyObject *type, *value, *trace;
 
       PyErr_Fetch (&type, &value, &trace);
-      cleanup = make_cleanup_py_decref (type);
-      make_cleanup_py_decref (value);
-      make_cleanup_py_decref (trace);
+
+      gdbpy_ref type_ref (type);
+      gdbpy_ref value_ref (value);
+      gdbpy_ref trace_ref (trace);
 
       gdb::unique_xmalloc_ptr<char>
 	msg (gdbpy_exception_to_string (type, value));
@@ -268,8 +268,6 @@ print_stack_unless_memory_error (struct ui_file *stream)
       else
 	fprintf_filtered (stream, _("<error reading variable: %s>"),
 			  msg.get ());
-
-      do_cleanups (cleanup);
     }
   else
     gdbpy_print_stack ();
@@ -286,17 +284,14 @@ print_string_repr (PyObject *printer, const char *hint,
 		   struct gdbarch *gdbarch)
 {
   struct value *replacement = NULL;
-  PyObject *py_str = NULL;
   enum string_repr_result result = string_repr_ok;
 
-  py_str = pretty_print_one_value (printer, &replacement);
-  if (py_str)
+  gdbpy_ref py_str (pretty_print_one_value (printer, &replacement));
+  if (py_str != NULL)
     {
-      struct cleanup *cleanup = make_cleanup_py_decref (py_str);
-
       if (py_str == Py_None)
 	result = string_repr_none;
-      else if (gdbpy_is_lazy_string (py_str))
+      else if (gdbpy_is_lazy_string (py_str.get ()))
 	{
 	  CORE_ADDR addr;
 	  long length;
@@ -304,7 +299,7 @@ print_string_repr (PyObject *printer, const char *hint,
 	  gdb::unique_xmalloc_ptr<char> encoding;
 	  struct value_print_options local_opts = *options;
 
-	  gdbpy_extract_lazy_string (py_str, &addr, &type,
+	  gdbpy_extract_lazy_string (py_str.get (), &addr, &type,
 				     &length, &encoding);
 
 	  local_opts.addressprint = 0;
@@ -313,22 +308,20 @@ print_string_repr (PyObject *printer, const char *hint,
 	}
       else
 	{
-	  PyObject *string;
-
-	  string = python_string_to_target_python_string (py_str);
-	  if (string)
+	  gdbpy_ref string
+	    (python_string_to_target_python_string (py_str.get ()));
+	  if (string != NULL)
 	    {
 	      char *output;
 	      long length;
 	      struct type *type;
 
-	      make_cleanup_py_decref (string);
 #ifdef IS_PY3K
-	      output = PyBytes_AS_STRING (string);
-	      length = PyBytes_GET_SIZE (string);
+	      output = PyBytes_AS_STRING (string.get ());
+	      length = PyBytes_GET_SIZE (string.get ());
 #else
-	      output = PyString_AsString (string);
-	      length = PyString_Size (string);
+	      output = PyString_AsString (string.get ());
+	      length = PyString_Size (string.get ());
 #endif
 	      type = builtin_type (gdbarch)->builtin_char;
 
@@ -344,8 +337,6 @@ print_string_repr (PyObject *printer, const char *hint,
 	      print_stack_unless_memory_error (stream);
 	    }
 	}
-
-      do_cleanups (cleanup);
     }
   else if (replacement)
     {
@@ -453,11 +444,9 @@ print_children (PyObject *printer, const char *hint,
 {
   int is_map, is_array, done_flag, pretty;
   unsigned int i;
-  PyObject *children, *iter;
 #ifndef IS_PY3K
   PyObject *frame;
 #endif
-  struct cleanup *cleanups;
 
   if (! PyObject_HasAttr (printer, gdbpy_children_cst))
     return;
@@ -467,23 +456,20 @@ print_children (PyObject *printer, const char *hint,
   is_map = hint && ! strcmp (hint, "map");
   is_array = hint && ! strcmp (hint, "array");
 
-  children = PyObject_CallMethodObjArgs (printer, gdbpy_children_cst,
-					 NULL);
-  if (! children)
+  gdbpy_ref children (PyObject_CallMethodObjArgs (printer, gdbpy_children_cst,
+						  NULL));
+  if (children == NULL)
     {
       print_stack_unless_memory_error (stream);
       return;
     }
 
-  cleanups = make_cleanup_py_decref (children);
-
-  iter = PyObject_GetIter (children);
-  if (!iter)
+  gdbpy_ref iter (PyObject_GetIter (children.get ()));
+  if (iter == NULL)
     {
       print_stack_unless_memory_error (stream);
-      goto done;
+      return;
     }
-  make_cleanup_py_decref (iter);
 
   /* Use the prettyformat_arrays option if we are printing an array,
      and the pretty option otherwise.  */
@@ -501,23 +487,19 @@ print_children (PyObject *printer, const char *hint,
      where it insists on having a non-NULL tstate->frame when
      a generator is called.  */
 #ifndef IS_PY3K
-  frame = push_dummy_python_frame ();
-  if (!frame)
-    {
-      gdbpy_print_stack ();
-      goto done;
-    }
-  make_cleanup_py_decref (frame);
+  gdbpy_ref frame (push_dummy_python_frame ());
+  if (frame == NULL)
+    return;
 #endif
 
   done_flag = 0;
   for (i = 0; i < options->print_max; ++i)
     {
-      PyObject *py_v, *item = PyIter_Next (iter);
+      PyObject *py_v;
       const char *name;
-      struct cleanup *inner_cleanup;
 
-      if (! item)
+      gdbpy_ref item (PyIter_Next (iter.get ()));
+      if (item == NULL)
 	{
 	  if (PyErr_Occurred ())
 	    print_stack_unless_memory_error (stream);
@@ -528,16 +510,15 @@ print_children (PyObject *printer, const char *hint,
 	  break;
 	}
 
-      if (! PyTuple_Check (item) || PyTuple_Size (item) != 2)
+      if (! PyTuple_Check (item.get ()) || PyTuple_Size (item.get ()) != 2)
 	{
 	  PyErr_SetString (PyExc_TypeError,
 			   _("Result of children iterator not a tuple"
 			     " of two elements."));
 	  gdbpy_print_stack ();
-	  Py_DECREF (item);
 	  continue;
 	}
-      if (! PyArg_ParseTuple (item, "sO", &name, &py_v))
+      if (! PyArg_ParseTuple (item.get (), "sO", &name, &py_v))
 	{
 	  /* The user won't necessarily get a stack trace here, so provide
 	     more context.  */
@@ -545,10 +526,8 @@ print_children (PyObject *printer, const char *hint,
 	    fprintf_unfiltered (gdb_stderr,
 				_("Bad result from children iterator.\n"));
 	  gdbpy_print_stack ();
-	  Py_DECREF (item);
 	  continue;
 	}
-      inner_cleanup = make_cleanup_py_decref (item);
 
       /* Print initial "{".  For other elements, there are three
 	 cases:
@@ -643,8 +622,6 @@ print_children (PyObject *printer, const char *hint,
 
       if (is_map && i % 2 == 0)
 	fputs_filtered ("] = ", stream);
-
-      do_cleanups (inner_cleanup);
     }
 
   if (i)
@@ -665,9 +642,6 @@ print_children (PyObject *printer, const char *hint,
 	}
       fputs_filtered ("}", stream);
     }
-
- done:
-  do_cleanups (cleanups);
 }
 
 enum ext_lang_rc
-- 
2.7.4

Re: [RFA 10/11] Use gdbpy_ref in py-prettyprint.c

[Tom Tromey]

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

Tom> This changes some spots in py-prettyprint.c to use gdbpy_ref.
Tom> 2016-11-12  Tom Tromey  <tom@tromey.com>
Tom> 	* python/py-prettyprint.c (print_stack_unless_memory_error)
Tom> 	(print_string_repr, print_children): Use gdbpy_ref.

The buildbot revealed that this patch had some problems.  I had failed
to notice that push_dummy_python_frame, a subroutine of print_children,
created a cleanup that had to be run by the caller.  This code path is
only used by Python 2, which is why I didn't see it in my local testing.
(I usually build only against Python 3, but curiously the buildbot only
seems to build against Python 2.)

This updated patch rewrites push_dummy_python_frame to be a class that
handles the necessary cleanup in its destructor.

I've tested this on x86-64 Fedora 24 using both Python 2 and Python 3.

I think the patch has changed enough to require re-review.

Tom

commit 5d7057c317feab7ed4788d1531190ac4fbc8b160
Author: Tom Tromey <tom@tromey.com>
Date:   Sat Nov 12 12:07:16 2016 -0700

    Use gdbpy_ref in py-prettyprint.c
    
    This changes some spots in py-prettyprint.c to use gdbpy_ref.  It also
    changes push_dummy_python_frame to be a class, rather than having it
    create a cleanup.
    
    2016-11-12  Tom Tromey  <tom@tromey.com>
    
    	* python/py-prettyprint.c (print_stack_unless_memory_error)
    	(print_string_repr, print_children): Use gdbpy_ref.
    	(dummy_python_frame): New class.
    	(dummy_python_frame::dummy_python_frame): Rename from
    	push_dummy_python_frame.
    	(py_restore_tstate): Remove.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5947b50..2525ac7 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,14 @@
 2016-11-12  Tom Tromey  <tom@tromey.com>
 
+	* python/py-prettyprint.c (print_stack_unless_memory_error)
+	(print_string_repr, print_children): Use gdbpy_ref.
+	(dummy_python_frame): New class.
+	(dummy_python_frame::dummy_python_frame): Rename from
+	push_dummy_python_frame.
+	(py_restore_tstate): Remove.
+
+2016-11-12  Tom Tromey  <tom@tromey.com>
+
 	* python/py-framefilter.c (py_print_frame): Use gdbpy_ref.
 
 2016-11-12  Tom Tromey  <tom@tromey.com>
diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c
index e7a3e76..8c3d24a 100644
--- a/gdb/python/py-prettyprint.c
+++ b/gdb/python/py-prettyprint.c
@@ -252,13 +252,13 @@ print_stack_unless_memory_error (struct ui_file *stream)
 {
   if (PyErr_ExceptionMatches (gdbpy_gdb_memory_error))
     {
-      struct cleanup *cleanup;
       PyObject *type, *value, *trace;
 
       PyErr_Fetch (&type, &value, &trace);
-      cleanup = make_cleanup_py_decref (type);
-      make_cleanup_py_decref (value);
-      make_cleanup_py_decref (trace);
+
+      gdbpy_ref type_ref (type);
+      gdbpy_ref value_ref (value);
+      gdbpy_ref trace_ref (trace);
 
       gdb::unique_xmalloc_ptr<char>
 	msg (gdbpy_exception_to_string (type, value));
@@ -268,8 +268,6 @@ print_stack_unless_memory_error (struct ui_file *stream)
       else
 	fprintf_filtered (stream, _("<error reading variable: %s>"),
 			  msg.get ());
-
-      do_cleanups (cleanup);
     }
   else
     gdbpy_print_stack ();
@@ -286,17 +284,14 @@ print_string_repr (PyObject *printer, const char *hint,
 		   struct gdbarch *gdbarch)
 {
   struct value *replacement = NULL;
-  PyObject *py_str = NULL;
   enum string_repr_result result = string_repr_ok;
 
-  py_str = pretty_print_one_value (printer, &replacement);
-  if (py_str)
+  gdbpy_ref py_str (pretty_print_one_value (printer, &replacement));
+  if (py_str != NULL)
     {
-      struct cleanup *cleanup = make_cleanup_py_decref (py_str);
-
       if (py_str == Py_None)
 	result = string_repr_none;
-      else if (gdbpy_is_lazy_string (py_str))
+      else if (gdbpy_is_lazy_string (py_str.get ()))
 	{
 	  CORE_ADDR addr;
 	  long length;
@@ -304,7 +299,7 @@ print_string_repr (PyObject *printer, const char *hint,
 	  gdb::unique_xmalloc_ptr<char> encoding;
 	  struct value_print_options local_opts = *options;
 
-	  gdbpy_extract_lazy_string (py_str, &addr, &type,
+	  gdbpy_extract_lazy_string (py_str.get (), &addr, &type,
 				     &length, &encoding);
 
 	  local_opts.addressprint = 0;
@@ -313,22 +308,20 @@ print_string_repr (PyObject *printer, const char *hint,
 	}
       else
 	{
-	  PyObject *string;
-
-	  string = python_string_to_target_python_string (py_str);
-	  if (string)
+	  gdbpy_ref string
+	    (python_string_to_target_python_string (py_str.get ()));
+	  if (string != NULL)
 	    {
 	      char *output;
 	      long length;
 	      struct type *type;
 
-	      make_cleanup_py_decref (string);
 #ifdef IS_PY3K
-	      output = PyBytes_AS_STRING (string);
-	      length = PyBytes_GET_SIZE (string);
+	      output = PyBytes_AS_STRING (string.get ());
+	      length = PyBytes_GET_SIZE (string.get ());
 #else
-	      output = PyString_AsString (string);
-	      length = PyString_Size (string);
+	      output = PyString_AsString (string.get ());
+	      length = PyString_Size (string.get ());
 #endif
 	      type = builtin_type (gdbarch)->builtin_char;
 
@@ -344,8 +337,6 @@ print_string_repr (PyObject *printer, const char *hint,
 	      print_stack_unless_memory_error (stream);
 	    }
 	}
-
-      do_cleanups (cleanup);
     }
   else if (replacement)
     {
@@ -364,80 +355,84 @@ print_string_repr (PyObject *printer, const char *hint,
 }
 
 #ifndef IS_PY3K
-static void
-py_restore_tstate (void *p)
-{
-  PyFrameObject *frame = (PyFrameObject *) p;
-  PyThreadState *tstate = PyThreadState_GET ();
-
-  tstate->frame = frame;
-}
 
 /* Create a dummy PyFrameObject, needed to work around
    a Python-2.4 bug with generators.  */
-static PyObject *
-push_dummy_python_frame (void)
+class dummy_python_frame
 {
-  PyObject *empty_string, *null_tuple, *globals;
-  PyCodeObject *code;
-  PyFrameObject *frame;
-  PyThreadState *tstate;
+ public:
 
-  empty_string = PyString_FromString ("");
-  if (!empty_string)
-    return NULL;
+  dummy_python_frame ();
 
-  null_tuple = PyTuple_New (0);
-  if (!null_tuple)
-    {
-      Py_DECREF (empty_string);
-      return NULL;
-    }
+  ~dummy_python_frame ()
+  {
+    if (m_valid)
+      m_tstate->frame = m_saved_frame;
+  }
 
-  code = PyCode_New (0,			/* argcount */
-		     0,			/* nlocals */
-		     0,			/* stacksize */
-		     0,			/* flags */
-		     empty_string,	/* code */
-		     null_tuple,	/* consts */
-		     null_tuple,	/* names */
-		     null_tuple,	/* varnames */
-#if PYTHON_API_VERSION >= 1010
-		     null_tuple,	/* freevars */
-		     null_tuple,	/* cellvars */
-#endif
-		     empty_string,	/* filename */
-		     empty_string,	/* name */
-		     1,			/* firstlineno */
-		     empty_string	/* lnotab */
-		    );
+  bool failed () const
+  {
+    return !m_valid;
+  }
 
-  Py_DECREF (empty_string);
-  Py_DECREF (null_tuple);
+ private:
 
-  if (!code)
-    return NULL;
+  bool m_valid;
+  PyFrameObject *m_saved_frame;
+  gdbpy_ref m_frame;
+  PyThreadState *m_tstate;
+};
 
-  globals = PyDict_New ();
-  if (!globals)
-    {
-      Py_DECREF (code);
-      return NULL;
-    }
+dummy_python_frame::dummy_python_frame ()
+: m_valid (false),
+  m_saved_frame (NULL),
+  m_tstate (NULL)
+{
+  PyCodeObject *code;
+  PyFrameObject *frame;
 
-  tstate = PyThreadState_GET ();
+  gdbpy_ref empty_string (PyString_FromString (""));
+  if (empty_string == NULL)
+    return;
 
-  frame = PyFrame_New (tstate, code, globals, NULL);
+  gdbpy_ref null_tuple (PyTuple_New (0));
+  if (null_tuple == NULL)
+    return;
 
-  Py_DECREF (globals);
-  Py_DECREF (code);
+  code = PyCode_New (0,			  /* argcount */
+		     0,			  /* locals */
+		     0,			  /* stacksize */
+		     0,			  /* flags */
+		     empty_string.get (), /* code */
+		     null_tuple.get (),	  /* consts */
+		     null_tuple.get (),	  /* names */
+		     null_tuple.get (),	  /* varnames */
+#if PYTHON_API_VERSION >= 1010
+		     null_tuple.get (),	  /* freevars */
+		     null_tuple.get (),	  /* cellvars */
+#endif
+		     empty_string.get (), /* filename */
+		     empty_string.get (), /* name */
+		     1,			  /* firstlineno */
+		     empty_string.get ()  /* lnotab */
+		     );
+  if (code == NULL)
+    return;
+  gdbpy_ref code_holder ((PyObject *) code);
 
-  if (!frame)
-    return NULL;
+  gdbpy_ref globals (PyDict_New ());
+  if (globals == NULL)
+    return;
+
+  m_tstate = PyThreadState_GET ();
+  frame = PyFrame_New (m_tstate, code, globals.get (), NULL);
+  if (frame == NULL)
+    return;
 
-  tstate->frame = frame;
-  make_cleanup (py_restore_tstate, frame->f_back);
-  return (PyObject *) frame;
+  m_frame.reset ((PyObject *) frame);
+  m_tstate->frame = frame;
+  m_saved_frame = frame->f_back;
+  m_valid = true;
 }
 #endif
 
@@ -453,11 +448,6 @@ print_children (PyObject *printer, const char *hint,
 {
   int is_map, is_array, done_flag, pretty;
   unsigned int i;
-  PyObject *children, *iter;
-#ifndef IS_PY3K
-  PyObject *frame;
-#endif
-  struct cleanup *cleanups;
 
   if (! PyObject_HasAttr (printer, gdbpy_children_cst))
     return;
@@ -467,23 +457,20 @@ print_children (PyObject *printer, const char *hint,
   is_map = hint && ! strcmp (hint, "map");
   is_array = hint && ! strcmp (hint, "array");
 
-  children = PyObject_CallMethodObjArgs (printer, gdbpy_children_cst,
-					 NULL);
-  if (! children)
+  gdbpy_ref children (PyObject_CallMethodObjArgs (printer, gdbpy_children_cst,
+						  NULL));
+  if (children == NULL)
     {
       print_stack_unless_memory_error (stream);
       return;
     }
 
-  cleanups = make_cleanup_py_decref (children);
-
-  iter = PyObject_GetIter (children);
-  if (!iter)
+  gdbpy_ref iter (PyObject_GetIter (children.get ()));
+  if (iter == NULL)
     {
       print_stack_unless_memory_error (stream);
-      goto done;
+      return;
     }
-  make_cleanup_py_decref (iter);
 
   /* Use the prettyformat_arrays option if we are printing an array,
      and the pretty option otherwise.  */
@@ -501,23 +488,22 @@ print_children (PyObject *printer, const char *hint,
      where it insists on having a non-NULL tstate->frame when
      a generator is called.  */
 #ifndef IS_PY3K
-  frame = push_dummy_python_frame ();
-  if (!frame)
+  dummy_python_frame frame;
+  if (frame.failed ())
     {
       gdbpy_print_stack ();
-      goto done;
+      return;
     }
-  make_cleanup_py_decref (frame);
 #endif
 
   done_flag = 0;
   for (i = 0; i < options->print_max; ++i)
     {
-      PyObject *py_v, *item = PyIter_Next (iter);
+      PyObject *py_v;
       const char *name;
-      struct cleanup *inner_cleanup;
 
-      if (! item)
+      gdbpy_ref item (PyIter_Next (iter.get ()));
+      if (item == NULL)
 	{
 	  if (PyErr_Occurred ())
 	    print_stack_unless_memory_error (stream);
@@ -528,16 +514,15 @@ print_children (PyObject *printer, const char *hint,
 	  break;
 	}
 
-      if (! PyTuple_Check (item) || PyTuple_Size (item) != 2)
+      if (! PyTuple_Check (item.get ()) || PyTuple_Size (item.get ()) != 2)
 	{
 	  PyErr_SetString (PyExc_TypeError,
 			   _("Result of children iterator not a tuple"
 			     " of two elements."));
 	  gdbpy_print_stack ();
-	  Py_DECREF (item);
 	  continue;
 	}
-      if (! PyArg_ParseTuple (item, "sO", &name, &py_v))
+      if (! PyArg_ParseTuple (item.get (), "sO", &name, &py_v))
 	{
 	  /* The user won't necessarily get a stack trace here, so provide
 	     more context.  */
@@ -545,10 +530,8 @@ print_children (PyObject *printer, const char *hint,
 	    fprintf_unfiltered (gdb_stderr,
 				_("Bad result from children iterator.\n"));
 	  gdbpy_print_stack ();
-	  Py_DECREF (item);
 	  continue;
 	}
-      inner_cleanup = make_cleanup_py_decref (item);
 
       /* Print initial "{".  For other elements, there are three
 	 cases:
@@ -643,8 +626,6 @@ print_children (PyObject *printer, const char *hint,
 
       if (is_map && i % 2 == 0)
 	fputs_filtered ("] = ", stream);
-
-      do_cleanups (inner_cleanup);
     }
 
   if (i)
@@ -665,9 +646,6 @@ print_children (PyObject *printer, const char *hint,
 	}
       fputs_filtered ("}", stream);
     }
-
- done:
-  do_cleanups (cleanups);
 }
 
 enum ext_lang_rc

Re: [RFA 10/11] Use gdbpy_ref in py-prettyprint.c

[Pedro Alves]

On 11/19/2016 05:08 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> 
> Tom> This changes some spots in py-prettyprint.c to use gdbpy_ref.
> Tom> 2016-11-12  Tom Tromey  <tom@tromey.com>
> Tom> 	* python/py-prettyprint.c (print_stack_unless_memory_error)
> Tom> 	(print_string_repr, print_children): Use gdbpy_ref.
> 
> The buildbot revealed that this patch had some problems.  I had failed
> to notice that push_dummy_python_frame, a subroutine of print_children,
> created a cleanup that had to be run by the caller.  This code path is
> only used by Python 2, which is why I didn't see it in my local testing.
> (I usually build only against Python 3, but curiously the buildbot only
> seems to build against Python 2.)
> 
> This updated patch rewrites push_dummy_python_frame to be a class that
> handles the necessary cleanup in its destructor.
> 
> I've tested this on x86-64 Fedora 24 using both Python 2 and Python 3.
> 
> I think the patch has changed enough to require re-review.

LGTM.

Thanks,
Pedro Alves

[RFA 01/11] Use gdbpy_enter in cmdpy_function

[Tom Tromey]

This changes cmdpy_function to use gdbpy_enter and gdbpy_ref.

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

	* python/py-cmd.c (cmdpy_function): Use gdbpy_enter, gdbpy_ref.
---
 gdb/ChangeLog       |  4 ++++
 gdb/python/py-cmd.c | 26 ++++++++++----------------
 2 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 10cb67f..9c0e5d2 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2016-11-12  Tom Tromey  <tom@tromey.com>
+
+	* python/py-cmd.c (cmdpy_function): Use gdbpy_enter, gdbpy_ref.
+
 2016-11-10  Tom Tromey  <tom@tromey.com>
 
 	* python/py-varobj.c (py_varobj_iter_dtor, py_varobj_iter_next):
diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
index 9e59f7c..17daaee 100644
--- a/gdb/python/py-cmd.c
+++ b/gdb/python/py-cmd.c
@@ -117,10 +117,8 @@ static void
 cmdpy_function (struct cmd_list_element *command, char *args, int from_tty)
 {
   cmdpy_object *obj = (cmdpy_object *) get_cmd_context (command);
-  PyObject *argobj, *ttyobj, *result;
-  struct cleanup *cleanup;
 
-  cleanup = ensure_python_env (get_current_arch (), current_language);
+  gdbpy_enter enter_py (get_current_arch (), current_language);
 
   if (! obj)
     error (_("Invalid invocation of Python command object."));
@@ -129,7 +127,6 @@ cmdpy_function (struct cmd_list_element *command, char *args, int from_tty)
       if (obj->command->prefixname)
 	{
 	  /* A prefix command does not need an invoke method.  */
-	  do_cleanups (cleanup);
 	  return;
 	}
       error (_("Python command object missing 'invoke' method."));
@@ -137,21 +134,21 @@ cmdpy_function (struct cmd_list_element *command, char *args, int from_tty)
 
   if (! args)
     args = "";
-  argobj = PyUnicode_Decode (args, strlen (args), host_charset (), NULL);
-  if (! argobj)
+  gdbpy_ref argobj (PyUnicode_Decode (args, strlen (args), host_charset (),
+				      NULL));
+  if (argobj == NULL)
     {
       gdbpy_print_stack ();
       error (_("Could not convert arguments to Python string."));
     }
 
-  ttyobj = from_tty ? Py_True : Py_False;
-  Py_INCREF (ttyobj);
-  result = PyObject_CallMethodObjArgs ((PyObject *) obj, invoke_cst, argobj,
-				       ttyobj, NULL);
-  Py_DECREF (argobj);
-  Py_DECREF (ttyobj);
+  gdbpy_ref ttyobj (from_tty ? Py_True : Py_False);
+  Py_INCREF (ttyobj.get ());
+  gdbpy_ref result (PyObject_CallMethodObjArgs ((PyObject *) obj, invoke_cst,
+						argobj.get (), ttyobj.get (),
+						NULL));
 
-  if (! result)
+  if (result == NULL)
     {
       PyObject *ptype, *pvalue, *ptraceback;
 
@@ -199,9 +196,6 @@ cmdpy_function (struct cmd_list_element *command, char *args, int from_tty)
 	  error ("%s", msg.get ());
 	}
     }
-
-  Py_DECREF (result);
-  do_cleanups (cleanup);
 }
 
 /* Helper function for the Python command completers (both "pure"
-- 
2.7.4

Re: [RFA 00/11] third series of C++ use in Python layer

[Pedro Alves]

On 11/12/2016 09:28 PM, Tom Tromey wrote:
> This series builds on my previous two in order to convert more of the
> Python layer to gdbpy_enter and gdbpy_ref.
> 
> I built and tested gdb.python/*.exp locally; but I'm also sending this
> through the buildbot.
> 
> Most of the remaining cleanups in the Python code refer to gdb objects
> that haven't been converted to use destructors.  As more things are
> converted I will work on cleaning this up.

I sent a few nits here and there, but mostly all LGTM.
All patches that I didn't reply to are OK as is.

Thanks,
Pedro Alves