From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3450 invoked by alias); 23 Jun 2009 20:26:49 -0000 Received: (qmail 3440 invoked by uid 22791); 23 Jun 2009 20:26:48 -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 Received: from mx2.redhat.com (HELO mx2.redhat.com) (66.187.237.31) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 23 Jun 2009 20:26:38 +0000 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n5NKQajJ021371 for ; Tue, 23 Jun 2009 16:26:36 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx2.corp.redhat.com (8.13.1/8.13.1) with ESMTP id n5NKQZBa020577; Tue, 23 Jun 2009 16:26:35 -0400 Received: from opsy.redhat.com (vpn-12-196.rdu.redhat.com [10.11.12.196]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id n5NKQYEj028715; Tue, 23 Jun 2009 16:26:34 -0400 Received: by opsy.redhat.com (Postfix, from userid 500) id 47C693785E4; Tue, 23 Jun 2009 14:26:33 -0600 (MDT) To: gdb-patches@sourceware.org Subject: RFC: reference counting for value From: Tom Tromey Reply-To: tromey@redhat.com Date: Tue, 23 Jun 2009 20:26:00 -0000 Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-06/txt/msg00636.txt.bz2 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 * 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) : 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 * 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. */