This is the mail archive of the archer@sourceware.org mailing list for the Archer project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[python] Implement value reference counting


I'm checking this in on the python branch.

This implements reference counting for 'struct value'.
This fixes a crash that was reported here.  A regression test is
included.

The reference counting is a bit odd -- the "increment reference"
operation is called "release_value", for historical reasons.
I may change this when I submit it upstream.

This patch causes some python MI regressions.  This area needs an
overhaul anyhow, which I intend to do soon.

Tom

gdb
	* varobj.c (instantiate_pretty_printer): Don't call value_copy.
	(update_dynamic_varobj_children): Don't call value_copy.
	(value_get_print_value): Clean up the returned value.
	* 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.  Don't use value_copy.
	(preserve_python_values): New function.
gdb/testsuite
	* gdb.python/python-value.exp (test_cast_regression): New proc.

diff --git a/gdb/python/python-value.c b/gdb/python/python-value.c
index 743e6a6..8c4361e 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"
@@ -59,20 +50,33 @@ struct value *values_in_python = NULL;
 #define builtin_type_pybool \
   language_bool_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);
 
@@ -123,11 +127,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)
@@ -547,9 +563,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 *
@@ -806,13 +820,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)
 {
@@ -824,7 +840,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 struct value.  */
 
 struct value *
 convert_value_from_python (PyObject *obj)
@@ -906,7 +923,7 @@ convert_value_from_python (PyObject *obj)
 	    }
 	}
       else if (PyObject_TypeCheck (obj, &value_object_type))
-	value = value_copy (((value_object *) obj)->value);
+	value = ((value_object *) obj)->value;
       else
 	PyErr_Format (PyExc_TypeError, _("Could not convert Python object: %s"),
 		      PyString_AsString (PyObject_Str (obj)));
@@ -1057,4 +1074,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.c b/gdb/python/python.c
index e97bef8..2f82a74 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -829,14 +829,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.  */
+   an owned reference to the value, 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;
@@ -846,16 +847,14 @@ pretty_print_one_value (PyObject *printer, struct value **out_value)
 	{
 	  if (gdbpy_is_string (result))
 	    output = python_string_to_host_string (result);
-	  else if (PyObject_TypeCheck (result, &value_object_type))
+	  else
 	    {
-	      /* 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.  */
-	      struct value *v = value_object_to_value (result);
-	      *out_value = value_copy (v);
+	      *out_value = convert_value_from_python (result);
+	      /* We must increment the value's refcount, because we
+		 are about to decref RESULT, and this may result in
+		 the value being destroyed.  */
+	      release_value (*out_value);
 	    }
-	  else
-	    *out_value = convert_value_from_python (result);
 	  Py_DECREF (result);
 	}
     }
@@ -908,21 +907,26 @@ 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, (gdb_byte *) output, strlen (output),
 			 1, 0, options);
       else
 	fputs_filtered (output, stream);
-      xfree (output);
     }
   else if (replacement)
-    common_val_print (replacement, stream, recurse, options, language);
+    {
+      make_cleanup (value_free_cleanup, replacement);
+      common_val_print (replacement, stream, recurse, options, language);
+    }
   else
     gdbpy_print_stack ();
+  do_cleanups (cleanups);
 }
 
 static void
@@ -1234,12 +1238,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)
@@ -1265,14 +1270,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.h b/gdb/python/python.h
index 767af86..0d52ae9 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 *);
 
 void source_python_script (FILE *stream, char *file);
@@ -36,4 +34,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 2d6d3e4..f58630a 100644
--- a/gdb/testsuite/gdb.python/python-value.exp
+++ b/gdb/testsuite/gdb.python/python-value.exp
@@ -245,6 +245,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.
 
@@ -282,3 +291,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 fd05f07..c63f11c 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -155,6 +155,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;
 
@@ -555,13 +563,27 @@ value_free (struct value *val)
 {
   if (val)
     {
-      type_decref (val->type);
-      type_decref (val->enclosing_type);
-      xfree (val->contents);
-      xfree (val);
+      /* 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)
+	{
+	  type_decref (val->type);
+	  type_decref (val->enclosing_type);
+	  xfree (val->contents);
+	  xfree (val);
+	}
+      else
+	--val->refcount;
     }
 }
 
+void
+value_free_cleanup (void *arg)
+{
+  value_free (arg);
+}
+
 /* Free all values allocated since MARK was obtained by value_mark
    (except for those released).  */
 void
@@ -602,22 +624,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  */
@@ -1070,7 +1096,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)
 {
@@ -1120,8 +1146,7 @@ preserve_values (struct objfile *objfile)
   for (var = internalvars; var; var = var->next)
     preserve_one_value (var->value, objfile, copied_types);
 
-  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 fd0d2c9..dde61fd 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -519,6 +519,9 @@ extern int destructor_name_p (const char *name, const struct type *type);
 
 extern void value_free (struct value *val);
 
+/* The same as value_free, but suitable for use as a cleanup.  */
+extern void value_free_cleanup (void *val);
+
 extern void free_all_values (void);
 
 extern void release_value (struct value *val);
@@ -589,6 +592,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 49b4a43..c47c837 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -715,15 +715,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;
 
@@ -884,16 +877,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.  */
 
@@ -2249,7 +2233,7 @@ value_get_print_value (struct value *value, enum varobj_display_formats format,
 {
   long dummy;
   struct ui_file *stb;
-  struct cleanup *old_chain;
+  struct cleanup *old_chain = make_cleanup (null_cleanup, 0);
   char *thevalue = NULL;
   struct value_print_options opts;
 
@@ -2282,14 +2266,19 @@ value_get_print_value (struct value *value, enum varobj_display_formats format,
 	    return thevalue;
 	  }
 	if (replacement)
-	  value = replacement;
+	  {
+	    value = replacement;
+	    /* We have a reference to the value, so we must arrange to
+	       free it later.  */
+	    make_cleanup (value_free_cleanup, value);
+	  }
       }
     PyGILState_Release (state);
   }
 #endif
 
   stb = mem_fileopen ();
-  old_chain = make_cleanup_ui_file_delete (stb);
+  make_cleanup_ui_file_delete (stb);
 
   get_formatted_print_options (&opts, format_code[(int) format]);
   opts.deref_ref = 0;


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]