From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18888 invoked by alias); 22 Jun 2009 19:15:51 -0000 Mailing-List: contact archer-help@sourceware.org; run by ezmlm Sender: Precedence: bulk List-Post: List-Help: List-Subscribe: List-Id: Received: (qmail 18831 invoked by uid 22791); 22 Jun 2009 19:15:37 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org To: Project Archer Subject: [python] Implement value reference counting From: Tom Tromey Reply-To: Tom Tromey Date: Mon, 22 Jun 2009 19:15:00 -0000 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2009-q2/txt/msg00186.txt.bz2 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) : 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;