RFC: reference counting for value

RFC: reference counting for value

[Tom Tromey]

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.  */
 

Re: RFC: reference counting for value

[Daniel Jacobowitz]

On Tue, Jun 23, 2009 at 02:26:31PM -0600, Tom Tromey wrote:
> 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.

IMO it would be nice to rename it (separately).

I am trying to resurrect an old patch of Vladimir's, which gives
bitfield values a parent pointer.  We fetch the parent once, instead
of once per bitfield.  That raised an interesting question:

> +  /* 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;
>  

If we release_value the parent every time we create a child, and
value_free it every time we collect a child, the parent will be freed
as soon as its last child is.  This is a change in the value behavior,
because otherwise it would hang around until value_free_to_mark or
free_all_values.

Is this going to bite us?  We could, instead, record release_value
references separately from parent references and leave the value on
the chain.  But if it doesn't matter, I'd rather not.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: reference counting for value

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Daniel> I am trying to resurrect an old patch of Vladimir's, which gives
Daniel> bitfield values a parent pointer.  We fetch the parent once, instead
Daniel> of once per bitfield.  That raised an interesting question:

Tom> +  /* The reference count.  A value that is still on the `all_values'
Tom> +     list will have a reference count of 0.  A call to `release_value'
Tom> +     will increment the reference count (and remove the value from the
Tom> +     list, the first time).  A call to `value_free' will decrement the
Tom> +     reference count, and will free the value when there are no more
Tom> +     references.  */
Tom> +  int refcount;
Tom> +
Tom> /* Register number if the value is from a register.  */
Tom> short regnum;
Tom> 

Daniel> If we release_value the parent every time we create a child, and
Daniel> value_free it every time we collect a child, the parent will be freed
Daniel> as soon as its last child is.  This is a change in the value behavior,
Daniel> because otherwise it would hang around until value_free_to_mark or
Daniel> free_all_values.

Daniel> Is this going to bite us?

Yes, I think so.

Daniel> We could, instead, record release_value
Daniel> references separately from parent references and leave the value on
Daniel> the chain.  But if it doesn't matter, I'd rather not.

Another idea I've been kicking around a bit is to also reference count
the contents.  This would solve this particular problem without
needing a bitfield->parent reference, as the two would just share some
structure.

My reasons for considering this change are, first, it would be more
memory-efficient in some value_copy cases; and, second, I think it
would let us merge val_print and value_print.

I was also thinking that it would help with properly implementing
unavailable pieces via DW_OP_piece (I thought: removing val_print
would make it simpler to handle all this via the value API, and
structure sharing seemed necessary for removing val_print), but now
I'm not as sure.  It is probably just as easy to pass a "valid" bitmap
through the val_print hierarchy.

BTW, I have not checked in the value reference counting patch.  I plan
to it until I've dealt with the python/varojb regression it
introduces.

Tom

Re: RFC: reference counting for value

[Daniel Jacobowitz]

On Tue, Jul 07, 2009 at 08:25:44AM -0600, Tom Tromey wrote:
> Another idea I've been kicking around a bit is to also reference count
> the contents.  This would solve this particular problem without
> needing a bitfield->parent reference, as the two would just share some
> structure.

Would it?  We need to be able to fetch the contents in response to a
request from the bitfield, so everything needed to unlazy would
have to be in the shared structure; I guess that's address and length,
here.  Also lval type.  Would we have to duplicate these in the value
and the shared contents?

If two values have a shared contents structure, I'm not sure what
point there is having them different value structures in the first place.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: reference counting for value

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Tom> Another idea I've been kicking around a bit is to also reference count
Tom> the contents.  This would solve this particular problem without
Tom> needing a bitfield->parent reference, as the two would just share some
Tom> structure.

Daniel> Would it?  We need to be able to fetch the contents in response to a
Daniel> request from the bitfield, so everything needed to unlazy would
Daniel> have to be in the shared structure; I guess that's address and length,
Daniel> here.  Also lval type.  Would we have to duplicate these in the value
Daniel> and the shared contents?

I hadn't been thinking about the lazy case, but after thinking about
it a bit, it still doesn't seem so bad to me.

Yeah, you'd have to push this stuff into the contents (lval type?  I
dunno).

Something like

struct value_contents
{
  enum lval_type lval : 8;
  unsigned int lazy : 1;
  unsigned int refcount : 23;  // or whatever
  CORE_ADDR address;
  gdb_byte contents[1];  // should perhaps use the aligner stuff from before
};

struct value
{
...
  struct value_contents *contents;
}

value::offset would continue to be an offset into the contents; when
doing something like a field reference, we'd make a new reference to
'contents' and set the offset in the new value appropriately.

For unavailable pieces, we'd also stick the validity bitmap (or
something) into value_contents.

Daniel> If two values have a shared contents structure, I'm not sure
Daniel> what point there is having them different value structures in
Daniel> the first place.

To act as a content cache, so we can avoid duplicating large contents
when copying (and slicing, etc) values and avoid (as in your case)
redundant fetches.

Some other caching approach may work just as well.  That's how I
understand the parent-pointer idea as well -- a specialized cache.

I'm not really wedded to this.  I'm not even sure I want to get rid of
val_print -- I find it to be a strange wart, but it would probably be
a lot of work for not much benefit (unless it really does simplify the
unavailable piece thing).

Tom