This is the mail archive of the
archer@sourceware.org
mailing list for the Archer project.
[python] Implement value reference counting
- From: Tom Tromey <tromey at redhat dot com>
- To: Project Archer <archer at sourceware dot org>
- Date: Mon, 22 Jun 2009 13:15:20 -0600
- Subject: [python] Implement value reference counting
- Reply-to: Tom Tromey <tromey at redhat dot com>
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;