[RFA 4/4] Change Python code to use new_reference

[RFA 4/4] Change Python code to use new_reference

[Tom Tromey]

This changes a few spots in the Python code to use new_reference
rather than the manual incref+constructor that was previously being
done.

2018-04-29  Tom Tromey  <tom@tromey.com>

	* varobj.c (varobj_set_visualizer): Use new_reference.
	* python/python.c (gdbpy_decode_line): Use new_reference.
	* python/py-cmd.c (cmdpy_function, cmdpy_completer_helper): Use
	new_reference.
---
 gdb/ChangeLog       |  7 +++++++
 gdb/python/py-cmd.c |  7 +++----
 gdb/python/python.c | 10 ++--------
 gdb/varobj.c        |  5 ++---
 4 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
index bff445f62f..27c4689413 100644
--- a/gdb/python/py-cmd.c
+++ b/gdb/python/py-cmd.c
@@ -138,8 +138,8 @@ cmdpy_function (struct cmd_list_element *command,
       error (_("Could not convert arguments to Python string."));
     }
 
-  gdbpy_ref<> ttyobj (from_tty ? Py_True : Py_False);
-  Py_INCREF (ttyobj.get ());
+  gdbpy_ref<> ttyobj
+    = gdbpy_ref<>::new_reference (from_tty ? Py_True : Py_False);
   gdbpy_ref<> result (PyObject_CallMethodObjArgs ((PyObject *) obj, invoke_cst,
 						  argobj.get (), ttyobj.get (),
 						  NULL));
@@ -246,8 +246,7 @@ cmdpy_completer_helper (struct cmd_list_element *command,
   if (word == NULL)
     {
       /* "brkchars" phase.  */
-      wordobj.reset (Py_None);
-      Py_INCREF (Py_None);
+      wordobj = gdbpy_ref<>::new_reference (Py_None);
     }
   else
     {
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 9eae8a1aef..0dd7d6a6ad 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -876,10 +876,7 @@ gdbpy_decode_line (PyObject *self, PyObject *args)
 	}
     }
   else
-    {
-      result.reset (Py_None);
-      Py_INCREF (Py_None);
-    }
+    result = gdbpy_ref<>::new_reference (Py_None);
 
   gdbpy_ref<> return_result (PyTuple_New (2));
   if (return_result == NULL)
@@ -892,10 +889,7 @@ gdbpy_decode_line (PyObject *self, PyObject *args)
 	return NULL;
     }
   else
-    {
-      unparsed.reset (Py_None);
-      Py_INCREF (Py_None);
-    }
+    unparsed = gdbpy_ref<>::new_reference (Py_None);
 
   PyTuple_SetItem (return_result.get (), 0, unparsed.release ());
   PyTuple_SetItem (return_result.get (), 1, result.release ());
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 4656bfa6b9..a0df485ae9 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -1455,9 +1455,8 @@ varobj_set_visualizer (struct varobj *var, const char *visualizer)
   gdbpy_enter_varobj enter_py (var);
 
   mainmod = PyImport_AddModule ("__main__");
-  gdbpy_ref<> globals (PyModule_GetDict (mainmod));
-  Py_INCREF (globals.get ());
-
+  gdbpy_ref<> globals
+    = gdbpy_ref<>::new_reference (PyModule_GetDict (mainmod));
   gdbpy_ref<> constructor (PyRun_String (visualizer, Py_eval_input,
 					 globals.get (), globals.get ()));
 
-- 
2.13.6

[RFA 1/4] Introduce ref_ptr::new_reference

[Tom Tromey]

I noticed a common pattern with gdb::ref_ptr, where callers would
"incref" and then create a new wrapper object, like:

    Py_INCREF (obj);
    gdbpy_ref<> ref (obj);

The ref_ptr constructor intentionally does not acquire a new
reference, but it seemed to me that it would be reasonable to add a
static member function that does so.

In this patch I chose to call the function "new_reference".  I
considered "acquire_reference" as well, but "new" seemed less
ambiguous than "acquire" to me.

ChangeLog
2018-04-29  Tom Tromey  <tom@tromey.com>

	* common/gdb_ref_ptr.h (ref_ptr::new_reference): New static
	method.
---
 gdb/ChangeLog            | 5 +++++
 gdb/common/gdb_ref_ptr.h | 7 +++++++
 2 files changed, 12 insertions(+)

diff --git a/gdb/common/gdb_ref_ptr.h b/gdb/common/gdb_ref_ptr.h
index 57d1db96e6..768c813692 100644
--- a/gdb/common/gdb_ref_ptr.h
+++ b/gdb/common/gdb_ref_ptr.h
@@ -149,6 +149,13 @@ class ref_ptr
     return m_obj;
   }
 
+  /* Acquire a new reference and return a ref_ptr that owns it.  */
+  static ref_ptr<T, Policy> new_reference (T *obj)
+  {
+    Policy::incref (obj);
+    return ref_ptr<T, Policy> (obj);
+  }
+
  private:
 
   T *m_obj;
-- 
2.13.6

[RFA 3/4] Use new_reference for struct value

[Tom Tromey]

value_incref returned its argument just as a convenience, which in the
end turned out to only be used in precisely the cases where
new_reference helps.  So, this patch changes value_incref to return
void and changes some value-using code to use new_reference.

I also noticed that the comments for value_incref and value_decref
were swapped, so this patch fixes those.

2018-04-29  Tom Tromey  <tom@tromey.com>

	* varobj.c (install_new_value): Use new_reference.
	* value.h (value_incref): Return void.  Swap intro comment with
	value_decref.
	* value.c (set_value_parent): Use new_reference.
	(value_incref): Return void.  Update intro comment.
	(release_value): Use new_reference.
	* dwarf2loc.c (dwarf2_evaluate_loc_desc_full): Use new_reference.
---
 gdb/ChangeLog   | 10 ++++++++++
 gdb/dwarf2loc.c |  2 +-
 gdb/value.c     | 10 ++++------
 gdb/value.h     |  8 ++++----
 gdb/varobj.c    |  2 +-
 5 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 243e047b9a..78d46688bc 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -2482,7 +2482,7 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
 	    /* Preserve VALUE because we are going to free values back
 	       to the mark, but we still need the value contents
 	       below.  */
-	    value_ref_ptr value_holder (value_incref (value));
+	    value_ref_ptr value_holder = value_ref_ptr::new_reference (value);
 	    free_values.free_to_mark ();
 
 	    retval = allocate_value (subobj_type);
diff --git a/gdb/value.c b/gdb/value.c
index 12aa2b8bb4..eefaaaab49 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1133,7 +1133,7 @@ value_parent (const struct value *value)
 void
 set_value_parent (struct value *value, struct value *parent)
 {
-  value->parent = value_ref_ptr (value_incref (parent));
+  value->parent = value_ref_ptr::new_reference (parent);
 }
 
 gdb_byte *
@@ -1572,14 +1572,12 @@ value_mark (void)
   return all_values.back ().get ();
 }
 
-/* Take a reference to VAL.  VAL will not be deallocated until all
-   references are released.  */
+/* See value.h.  */
 
-struct value *
+void
 value_incref (struct value *val)
 {
   val->reference_count++;
-  return val;
 }
 
 /* Release a reference to VAL, which was acquired with value_incref.
@@ -1635,7 +1633,7 @@ release_value (struct value *val)
   /* We must always return an owned reference.  Normally this happens
      because we transfer the reference from the value chain, but in
      this case the value was not on the chain.  */
-  return value_ref_ptr (value_incref (val));
+  return value_ref_ptr::new_reference (val);
 }
 
 /* See value.h.  */
diff --git a/gdb/value.h b/gdb/value.h
index b58f78998a..060cef5639 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -88,12 +88,12 @@ struct value_print_options;
 
 struct value;
 
-/* Decrease VAL's reference count.  When the reference count drops to
-   0, VAL will be freed.  */
+/* Increase VAL's reference count.  VAL is returned.  */
 
-extern struct value *value_incref (struct value *val);
+extern void value_incref (struct value *val);
 
-/* Increate VAL's reference count.  VAL is returned.  */
+/* Decrease VAL's reference count.  When the reference count drops to
+   0, VAL will be freed.  */
 
 extern void value_decref (struct value *val);
 
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 2d00964317..4656bfa6b9 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -1329,7 +1329,7 @@ install_new_value (struct varobj *var, struct value *value, bool initial)
      code that might release it.  */
   value_ref_ptr value_holder;
   if (value != NULL)
-    value_holder.reset (value_incref (value));
+    value_holder = value_ref_ptr::new_reference (value);
 
   /* Below, we'll be comparing string rendering of old and new
      values.  Don't get string rendering if the value is
-- 
2.13.6

[RFA 2/4] Remove new_bfd_ref

[Tom Tromey]

For gdb_bfd_ref_ptr, gdb already had a convenience function like the
new gdb_ref_ptr::new_reference -- called new_bfd_ref.  This patch
removes it in favor of the new common function.

While doing this I also noticed that the comment for gdb_bfd_open was
incorrect (in a way related to reference counting), so this patch
updates the comment as well.

ChangeLog
2018-04-29  Tom Tromey  <tom@tromey.com>

	* symfile-mem.c (symbol_file_add_from_memory): Use new_reference.
	* gdb_bfd.h (new_bfd_ref): Remove.
	(gdb_bfd_open): Update comment.
	* gdb_bfd.c (gdb_bfd_open, gdb_bfd_fopen, gdb_bfd_openr)
	(gdb_bfd_openw, gdb_bfd_openr_iovec, gdb_bfd_record_inclusion)
	(gdb_bfd_fdopenr): Use new_reference.
	* exec.c (exec_file_attach): Use new_reference.
---
 gdb/ChangeLog     | 10 ++++++++++
 gdb/exec.c        |  2 +-
 gdb/gdb_bfd.c     | 16 ++++++++--------
 gdb/gdb_bfd.h     | 25 +++++++------------------
 gdb/symfile-mem.c |  2 +-
 5 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/gdb/exec.c b/gdb/exec.c
index f94c15f1f5..b529b7a0d3 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -246,7 +246,7 @@ exec_file_attach (const char *filename, int from_tty)
   /* First, acquire a reference to the current exec_bfd.  We release
      this at the end of the function; but acquiring it now lets the
      BFD cache return it if this call refers to the same file.  */
-  gdb_bfd_ref_ptr exec_bfd_holder = new_bfd_ref (exec_bfd);
+  gdb_bfd_ref_ptr exec_bfd_holder = gdb_bfd_ref_ptr::new_reference (exec_bfd);
 
   /* Remove any previous exec file.  */
   exec_close ();
diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 02b1375b8a..8fedeb438d 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -450,7 +450,7 @@ gdb_bfd_open (const char *name, const char *target, int fd)
 			    host_address_to_string (abfd),
 			    bfd_get_filename (abfd));
       close (fd);
-      return new_bfd_ref (abfd);
+      return gdb_bfd_ref_ptr::new_reference (abfd);
     }
 
   abfd = bfd_fopen (name, target, FOPEN_RB, fd);
@@ -470,7 +470,7 @@ gdb_bfd_open (const char *name, const char *target, int fd)
       *slot = abfd;
     }
 
-  return new_bfd_ref (abfd);
+  return gdb_bfd_ref_ptr::new_reference (abfd);
 }
 
 /* A helper function that releases any section data attached to the
@@ -781,7 +781,7 @@ gdb_bfd_fopen (const char *filename, const char *target, const char *mode,
 {
   bfd *result = bfd_fopen (filename, target, mode, fd);
 
-  return new_bfd_ref (result);
+  return gdb_bfd_ref_ptr::new_reference (result);
 }
 
 /* See gdb_bfd.h.  */
@@ -791,7 +791,7 @@ gdb_bfd_openr (const char *filename, const char *target)
 {
   bfd *result = bfd_openr (filename, target);
 
-  return new_bfd_ref (result);
+  return gdb_bfd_ref_ptr::new_reference (result);
 }
 
 /* See gdb_bfd.h.  */
@@ -801,7 +801,7 @@ gdb_bfd_openw (const char *filename, const char *target)
 {
   bfd *result = bfd_openw (filename, target);
 
-  return new_bfd_ref (result);
+  return gdb_bfd_ref_ptr::new_reference (result);
 }
 
 /* See gdb_bfd.h.  */
@@ -826,7 +826,7 @@ gdb_bfd_openr_iovec (const char *filename, const char *target,
 				 open_func, open_closure,
 				 pread_func, close_func, stat_func);
 
-  return new_bfd_ref (result);
+  return gdb_bfd_ref_ptr::new_reference (result);
 }
 
 /* See gdb_bfd.h.  */
@@ -871,7 +871,7 @@ gdb_bfd_record_inclusion (bfd *includer, bfd *includee)
   struct gdb_bfd_data *gdata;
 
   gdata = (struct gdb_bfd_data *) bfd_usrdata (includer);
-  gdata->included_bfds.push_back (new_bfd_ref (includee));
+  gdata->included_bfds.push_back (gdb_bfd_ref_ptr::new_reference (includee));
 }
 
 /* See gdb_bfd.h.  */
@@ -881,7 +881,7 @@ gdb_bfd_fdopenr (const char *filename, const char *target, int fd)
 {
   bfd *result = bfd_fdopenr (filename, target, fd);
 
-  return new_bfd_ref (result);
+  return gdb_bfd_ref_ptr::new_reference (result);
 }
 
 \f
diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h
index 2e4ac2c409..85300b91f6 100644
--- a/gdb/gdb_bfd.h
+++ b/gdb/gdb_bfd.h
@@ -68,27 +68,16 @@ struct gdb_bfd_ref_policy
 /* A gdb::ref_ptr that has been specialized for BFD objects.  */
 typedef gdb::ref_ptr<struct bfd, gdb_bfd_ref_policy> gdb_bfd_ref_ptr;
 
-/* A helper function that calls gdb_bfd_ref and returns a
-   gdb_bfd_ref_ptr.  */
-
-static inline gdb_bfd_ref_ptr
-new_bfd_ref (struct bfd *abfd)
-{
-  gdb_bfd_ref (abfd);
-  return gdb_bfd_ref_ptr (abfd);
-}
-
 /* Open a read-only (FOPEN_RB) BFD given arguments like bfd_fopen.
    If NAME starts with TARGET_SYSROOT_PREFIX then the BFD will be
    opened using target fileio operations if necessary.  Returns NULL
-   on error.  On success, returns a new reference to the BFD, which
-   must be freed with gdb_bfd_unref.  BFDs returned by this call are
-   shared among all callers opening the same file.  If FD is not -1,
-   then after this call it is owned by BFD.  If the BFD was not
-   accessed using target fileio operations then the filename
-   associated with the BFD and accessible with bfd_get_filename will
-   not be exactly NAME but rather NAME with TARGET_SYSROOT_PREFIX
-   stripped.  */
+   on error.  On success, returns a new reference to the BFD.  BFDs
+   returned by this call are shared among all callers opening the same
+   file.  If FD is not -1, then after this call it is owned by BFD.
+   If the BFD was not accessed using target fileio operations then the
+   filename associated with the BFD and accessible with
+   bfd_get_filename will not be exactly NAME but rather NAME with
+   TARGET_SYSROOT_PREFIX stripped.  */
 
 gdb_bfd_ref_ptr gdb_bfd_open (const char *name, const char *target, int fd);
 
diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c
index c060711772..2e82beb7a4 100644
--- a/gdb/symfile-mem.c
+++ b/gdb/symfile-mem.c
@@ -99,7 +99,7 @@ symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr,
     error (_("Failed to read a valid object file image from memory."));
 
   /* Manage the new reference for the duration of this function.  */
-  gdb_bfd_ref_ptr nbfd_holder = new_bfd_ref (nbfd);
+  gdb_bfd_ref_ptr nbfd_holder = gdb_bfd_ref_ptr::new_reference (nbfd);
 
   xfree (bfd_get_filename (nbfd));
   if (name == NULL)
-- 
2.13.6

[RFA 0/4] add ref_ptr::new_reference

[Tom Tromey]

This patch introduces a new ref_ptr::new_reference convenience
function and then changes various places in gdb to use it.  This
consolidates some existing boilerplate into a simpler idiom.

Regression tested by the buildbot.

Tom

Re: [RFA 4/4] Change Python code to use new_reference

[Phil Muldoon]

On 30/04/18 05:21, Tom Tromey wrote:
> his changes a few spots in the Python code to use new_reference
> rather than the manual incref+constructor that was previously being
> done.
> 
> 2018-04-29  Tom Tromey  <tom@tromey.com>
> 
> 	* varobj.c (varobj_set_visualizer): Use new_reference.
> 	* python/python.c (gdbpy_decode_line): Use new_reference.
> 	* python/py-cmd.c (cmdpy_function, cmdpy_completer_helper): Use
> 	new_reference.
> ---

The Python related changes are largely mechanical in nature but just wanted to note LGTM.

Cheers

Phil

Re: [RFA 3/4] Use new_reference for struct value

[Pedro Alves]

Hi Tom,

The series looks fine to me.  Tiny comment below.

On 04/30/2018 05:21 AM, Tom Tromey wrote:

> diff --git a/gdb/value.h b/gdb/value.h
> index b58f78998a..060cef5639 100644
> --- a/gdb/value.h
> +++ b/gdb/value.h
> @@ -88,12 +88,12 @@ struct value_print_options;
>  
>  struct value;
>  
> -/* Decrease VAL's reference count.  When the reference count drops to
> -   0, VAL will be freed.  */
> +/* Increase VAL's reference count.  VAL is returned.  */

"VAL is returned" is stale here.

Thanks,
Pedro Alves

Re: [RFA 3/4] Use new_reference for struct value

[Tom Tromey]

>> +/* Increase VAL's reference count.  VAL is returned.  */

Pedro> "VAL is returned" is stale here.

Thanks, I'll fix this and then push this series.

Tom