RFC: reference counting for value

[Tom Tromey <tromey at redhat dot com>]

This patch implements reference counting for struct value.

This fixes a segv that can be seen using the Python Value code.
The new test case is included.  I chose this particular approach
because it seems more efficient than sprinkling value_copy calls
around.

This patch does introduce a couple of python-mi regressions.  It turns
out that the python varobj code has some bad problems; I'm going to be
fixing this area soon.

The implementation is slightly different from classic reference
counting because it co-exists with the all_values chain.  In
particular, the chain counts as an implicit reference, and values are
born with a 0 count.

The "increment reference" function is called "release_value" for
historical reasons (I can change this if anybody cares).  Most code
does not require any change, as one always had to pair release_value
and value_free calls anyhow.

There may be a few calls to value_copy in the sources that are now
redundant.

Let me know what you think.  I'll wait a few days for comments before
putting this in.

Tom

2009-06-23  Tom Tromey  <tromey@redhat.com>

       * varobj.c (instantiate_pretty_printer): Don't call value_copy.
       (update_dynamic_varobj_children): Don't call value_copy.
       * value.h (preserve_one_value): Declare.
       * value.c (struct value) <refcount>: New field.
       (value_free): Handle reference count.
       (release_value): Likewise.
       (preserve_one_value): No longer static.
       (preserve_values): Call preserve_python_values.  Don't use
       values_in_python.
       * python/python.h (values_in_python): Don't declare.
       (preserve_python_values): Declare.
       * python/python.c (pretty_print_one_value): Don't use value_copy.
       (gdbpy_get_varobj_pretty_printer): Likewise.
       * python/python-value.c (values_in_python): Change type.  Now
       static.
       (struct value_object): Give struct a tag.  Add 'next' field.
       (valpy_dealloc): Update for changes to value_object.
       (valpy_new): Likewise.
       (value_to_value_object): Likewise.
       (valpy_positive): Don't use value_copy.
       (value_object_to_value): Document reference counting behavior.
       (convert_value_from_python): Likewise.
       (preserve_python_values): New function.

2009-06-23  Tom Tromey  <tromey@redhat.com>

       * gdb.python/python-value.exp (test_cast_regression): New proc.

diff --git a/gdb/python/python-prettyprint.c b/gdb/python/python-prettyprint.c
index 117a5e4..7bd339e 100644
--- a/gdb/python/python-prettyprint.c
+++ b/gdb/python/python-prettyprint.c
@@ -125,14 +125,15 @@ find_pretty_printer (PyObject *value)
 /* Pretty-print a single value, via the printer object PRINTER.  If
    the function returns a string, an xmalloc()d copy is returned.
    Otherwise, if the function returns a value, a *OUT_VALUE is set to
-   the value, and NULL is returned.  On error, *OUT_VALUE is set to
-   NULL and NULL is returned.  */
+   the value (still on the all_values chain), and NULL is returned.
+   On error, *OUT_VALUE is set to NULL and NULL is returned.  */
 static char *
 pretty_print_one_value (PyObject *printer, struct value **out_value)
 {
   char *output = NULL;
   volatile struct gdb_exception except;
 
+  *out_value = NULL;
   TRY_CATCH (except, RETURN_MASK_ALL)
     {
       PyObject *result;
@@ -185,22 +186,24 @@ print_string_repr (PyObject *printer, const char *hint,
 {
   char *output;
   struct value *replacement = NULL;
+  struct cleanup *cleanups = make_cleanup (null_cleanup, 0);
 
   output = pretty_print_one_value (printer, &replacement);
   if (output)
     {
+      make_cleanup (xfree, output);
       if (hint && !strcmp (hint, "string"))
 	LA_PRINT_STRING (stream, builtin_type (current_gdbarch)->builtin_char,
 			 (gdb_byte *) output, strlen (output),
 			 0, options);
       else
 	fputs_filtered (output, stream);
-      xfree (output);
     }
   else if (replacement)
     common_val_print (replacement, stream, recurse, options, language);
   else
     gdbpy_print_stack ();
+  do_cleanups (cleanups);
 }
 
 static void
@@ -512,12 +515,13 @@ apply_val_pretty_printer (struct type *type, const gdb_byte *valaddr,
 
 /* Apply a pretty-printer for the varobj code.  PRINTER_OBJ is the
    print object.  It must have a 'to_string' method (but this is
-   checked by varobj, not here) which takes no arguments and
-   returns a string.  This function returns an xmalloc()d string if
-   the printer returns a string.  The printer may return a replacement
-   value instead; in this case *REPLACEMENT is set to the replacement
-   value, and this function returns NULL.  On error, *REPLACEMENT is
-   set to NULL and this function also returns NULL.  */
+   checked by varobj, not here) which takes no arguments and returns a
+   string.  This function returns an xmalloc()d string if the printer
+   returns a string.  The printer may return a replacement value
+   instead; in this case *REPLACEMENT is set to a new reference to the
+   replacement value, and this function returns NULL.  On error,
+   *REPLACEMENT is set to NULL and this function also returns
+   NULL.  */
 char *
 apply_varobj_pretty_printer (PyObject *printer_obj,
 			     struct value **replacement)
@@ -543,14 +547,7 @@ gdbpy_get_varobj_pretty_printer (struct value *value)
 {
   PyObject *val_obj;
   PyObject *pretty_printer = NULL;
-  volatile struct gdb_exception except;
 
-  TRY_CATCH (except, RETURN_MASK_ALL)
-    {
-      value = value_copy (value);
-    }
-  GDB_PY_HANDLE_EXCEPTION (except);
-  
   val_obj = value_to_value_object (value);
   if (! val_obj)
     return NULL;
diff --git a/gdb/python/python-value.c b/gdb/python/python-value.c
index 1b34f47..3d14dd2 100644
--- a/gdb/python/python-value.c
+++ b/gdb/python/python-value.c
@@ -26,15 +26,6 @@
 #include "dfp.h"
 #include "valprint.h"
 
-/* List of all values which are currently exposed to Python. It is
-   maintained so that when an objfile is discarded, preserve_values
-   can copy the values' types if needed.  This is declared
-   unconditionally to reduce the number of uses of HAVE_PYTHON in the
-   generic code.  */
-/* This variable is unnecessarily initialized to NULL in order to 
-   work around a linker bug on MacOS.  */
-struct value *values_in_python = NULL;
-
 #ifdef HAVE_PYTHON
 
 #include "python-internal.h"
@@ -58,20 +49,33 @@ struct value *values_in_python = NULL;
 #define builtin_type_pychar \
   language_string_char_type (current_language, current_gdbarch)
 
-typedef struct {
+typedef struct value_object {
   PyObject_HEAD
+  struct value_object *next;
   struct value *value;
   PyObject *address;
   PyObject *type;
 } value_object;
 
+/* List of all values which are currently exposed to Python. It is
+   maintained so that when an objfile is discarded, preserve_values
+   can copy the values' types if needed.  */
+/* This variable is unnecessarily initialized to NULL in order to
+   work around a linker bug on MacOS.  */
+static value_object *values_in_python = NULL;
+
 /* Called by the Python interpreter when deallocating a value object.  */
 static void
 valpy_dealloc (PyObject *obj)
 {
   value_object *self = (value_object *) obj;
+  value_object **iter;
 
-  value_remove_from_list (&values_in_python, self->value);
+  /* Remove OBJ from the global list.  */
+  iter = &values_in_python;
+  while (*iter != self)
+    iter = &(*iter)->next;
+  *iter = (*iter)->next;
 
   value_free (self->value);
 
@@ -122,11 +126,23 @@ valpy_new (PyTypeObject *subtype, PyObject *args, PyObject *keywords)
   value_obj->address = NULL;
   value_obj->type = NULL;
   release_value (value);
-  value_prepend_to_list (&values_in_python, value);
+  value_obj->next = values_in_python;
+  values_in_python = value_obj;
 
   return (PyObject *) value_obj;
 }
 
+/* Iterate over all the Value objects, calling preserve_one_value on
+   each.  */
+void
+preserve_python_values (struct objfile *objfile, htab_t copied_types)
+{
+  value_object *iter;
+
+  for (iter = values_in_python; iter; iter = iter->next)
+    preserve_one_value (iter->value, objfile, copied_types);
+}
+
 /* Given a value of a pointer type, apply the C unary * operator to it.  */
 static PyObject *
 valpy_dereference (PyObject *self, PyObject *args)
@@ -543,9 +559,7 @@ valpy_negative (PyObject *self)
 static PyObject *
 valpy_positive (PyObject *self)
 {
-  struct value *copy = value_copy (((value_object *) self)->value);
-
-  return value_to_value_object (copy);
+  return value_to_value_object (((value_object *) self)->value);
 }
 
 static PyObject *
@@ -802,13 +816,15 @@ value_to_value_object (struct value *val)
       val_obj->address = NULL;
       val_obj->type = NULL;
       release_value (val);
-      value_prepend_to_list (&values_in_python, val);
+      val_obj->next = values_in_python;
+      values_in_python = val_obj;
     }
 
   return (PyObject *) val_obj;
 }
 
-/* Returns value structure corresponding to the given value object.  */
+/* Returns a borrowed reference to the struct value corresponding to
+   the given value object.  */
 struct value *
 value_object_to_value (PyObject *self)
 {
@@ -820,7 +836,8 @@ value_object_to_value (PyObject *self)
 }
 
 /* Try to convert a Python value to a gdb value.  If the value cannot
-   be converted, set a Python exception and return NULL.  */
+   be converted, set a Python exception and return NULL.  Returns a
+   borrowed reference to the resulting value.  */
 
 struct value *
 convert_value_from_python (PyObject *obj)
@@ -875,7 +892,12 @@ convert_value_from_python (PyObject *obj)
 	    }
 	}
       else if (PyObject_TypeCheck (obj, &value_object_type))
-	value = value_copy (((value_object *) obj)->value);
+	{
+	  /* This lets callers freely decref the Value wrapper object
+	     and not worry about whether or not the value will
+	     disappear.  */
+	  value = value_copy (((value_object *) obj)->value);
+	}
       else
 	PyErr_Format (PyExc_TypeError, _("Could not convert Python object: %s"),
 		      PyString_AsString (PyObject_Str (obj)));
@@ -1018,4 +1040,12 @@ PyTypeObject value_object_type = {
   valpy_new			  /* tp_new */
 };
 
+#else
+
+void
+preserve_python_values (struct objfile *objfile, htab_t copied_types)
+{
+  /* Nothing.  */
+}
+
 #endif /* HAVE_PYTHON */
diff --git a/gdb/python/python.h b/gdb/python/python.h
index 33b0437..8577252 100644
--- a/gdb/python/python.h
+++ b/gdb/python/python.h
@@ -22,8 +22,6 @@
 
 #include "value.h"
 
-extern struct value *values_in_python;
-
 void eval_python_from_control_command (struct command_line *);
 
 int apply_val_pretty_printer (struct type *type, const gdb_byte *valaddr,
@@ -32,4 +30,7 @@ int apply_val_pretty_printer (struct type *type, const gdb_byte *valaddr,
 			      const struct value_print_options *options,
 			      const struct language_defn *language);
 
+void preserve_python_values (struct objfile *objfile, htab_t copied_types);
+
+
 #endif /* GDB_PYTHON_H */
diff --git a/gdb/testsuite/gdb.python/python-value.exp b/gdb/testsuite/gdb.python/python-value.exp
index 20333f6..82199b0 100644
--- a/gdb/testsuite/gdb.python/python-value.exp
+++ b/gdb/testsuite/gdb.python/python-value.exp
@@ -273,6 +273,15 @@ proc test_value_after_death {} {
     "print value's type"
 }
 
+# Regression test for a cast failure.  The bug was that if we cast a
+# value to its own type, gdb could crash.  This happened because we
+# could end up double-freeing a struct value.
+proc test_cast_regression {} {
+  gdb_test "python v = gdb.Value(5)" "" "create value for cast test"
+  gdb_test "python v = v.cast(v.type)" "" "cast value for cast test"
+  gdb_test "python print v" "5" "print value for cast test"
+}
+
 # Start with a fresh gdb.
 
 gdb_exit
@@ -303,3 +312,4 @@ if ![runto_main] then {
 
 test_value_in_inferior
 test_value_after_death
+test_cast_regression
diff --git a/gdb/value.c b/gdb/value.c
index 7566921..326bb8f 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -164,6 +164,14 @@ struct value
      taken off this list.  */
   struct value *next;
 
+  /* The reference count.  A value that is still on the `all_values'
+     list will have a reference count of 0.  A call to `release_value'
+     will increment the reference count (and remove the value from the
+     list, the first time).  A call to `value_free' will decrement the
+     reference count, and will free the value when there are no more
+     references.  */
+  int refcount;
+
   /* Register number if the value is from a register.  */
   short regnum;
 
@@ -594,17 +602,25 @@ value_free (struct value *val)
 {
   if (val)
     {
-      if (VALUE_LVAL (val) == lval_computed)
+      /* If the count was already 0, then the value was on the
+	 all_values list, and we must be freeing back to some
+	 point.  */
+      if (val->refcount <= 1)
 	{
-	  struct lval_funcs *funcs = val->location.computed.funcs;
+	  if (VALUE_LVAL (val) == lval_computed)
+	    {
+	      struct lval_funcs *funcs = val->location.computed.funcs;
 
-	  if (funcs->free_closure)
-	    funcs->free_closure (val);
-	}
+	      if (funcs->free_closure)
+		funcs->free_closure (val);
+	    }
 
-      xfree (val->contents);
+	  xfree (val->contents);
+	  xfree (val);
+	}
+      else
+	--val->refcount;
     }
-  xfree (val);
 }
 
 /* Free all values allocated since MARK was obtained by value_mark
@@ -647,22 +663,26 @@ free_all_values (void)
 void
 release_value (struct value *val)
 {
-  struct value *v;
-
-  if (all_values == val)
+  /* If the reference count is nonzero, then we have already removed
+     the item from the list, so there is no reason to do it again.  */
+  if (val->refcount == 0)
     {
-      all_values = val->next;
-      return;
-    }
-
-  for (v = all_values; v; v = v->next)
-    {
-      if (v->next == val)
+      if (all_values == val)
+	all_values = val->next;
+      else
 	{
-	  v->next = val->next;
-	  break;
+	  struct value *v;
+	  for (v = all_values; v; v = v->next)
+	    {
+	      if (v->next == val)
+		{
+		  v->next = val->next;
+		  break;
+		}
+	    }
 	}
     }
+  ++val->refcount;
 }
 
 /* Release all values up to mark  */
@@ -1298,7 +1318,7 @@ add_internal_function (const char *name, const char *doc,
 /* Update VALUE before discarding OBJFILE.  COPIED_TYPES is used to
    prevent cycles / duplicates.  */
 
-static void
+void
 preserve_one_value (struct value *value, struct objfile *objfile,
 		    htab_t copied_types)
 {
@@ -1354,8 +1374,7 @@ preserve_values (struct objfile *objfile)
 	}
     }
 
-  for (val = values_in_python; val; val = val->next)
-    preserve_one_value (val, objfile, copied_types);
+  preserve_python_values (objfile, copied_types);
 
   htab_delete (copied_types);
 }
diff --git a/gdb/value.h b/gdb/value.h
index db4dcc1..a2a6d42 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -646,6 +646,8 @@ extern void preserve_values (struct objfile *);
 
 extern struct value *value_copy (struct value *);
 
+extern void preserve_one_value (struct value *, struct objfile *, htab_t);
+
 /* From valops.c */
 
 extern struct value *varying_to_slice (struct value *);
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 089fc53..6b5ffdf 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -721,15 +721,8 @@ instantiate_pretty_printer (PyObject *constructor, struct value *value)
 #if HAVE_PYTHON
   PyObject *val_obj = NULL; 
   PyObject *printer;
-  volatile struct gdb_exception except;
 
-  TRY_CATCH (except, RETURN_MASK_ALL)
-    {
-      value = value_copy (value);
-    }
-  GDB_PY_HANDLE_EXCEPTION (except);
   val_obj = value_to_value_object (value);
-
   if (! val_obj)
     return NULL;
 
@@ -890,16 +883,7 @@ update_dynamic_varobj_children (struct varobj *var,
       if (!PyArg_ParseTuple (item, "sO", &name, &py_v))
 	error (_("Invalid item from the child list"));
       
-      if (PyObject_TypeCheck (py_v, &value_object_type))
-	{
-	  /* If we just call convert_value_from_python for this type,
-	     we won't know who owns the result.  For this one case we
-	     need to copy the resulting value.  */
-	  v = value_object_to_value (py_v);
-	  v = value_copy (v);
-	}
-      else
-	v = convert_value_from_python (py_v);
+      v = convert_value_from_python (py_v);
 
       /* TODO: This assume the name of the i-th child never changes.  */