Thanks for the feedback. Here is an updated patch against the latest archer-tromey-python with the fixes you mentioned. I ran `make check` before and after the patch on a "Linux 2.6.32.3 #3 SMP Thu Feb 4 23:11:14 UTC 2010 x86_64 GNU/Linux" and there were no changes to the gdb.sum file (other than 3 lines with newer timestamps). diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 7968e93..62030a5 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,13 @@ +2010-11-04 Aman Gupta + * gdb/value.c (release_value): Return 0 on success. + (release_value_or_incref): New function, incref value if already + released. + * gdb/python/py-value.c (valpy_new, value_to_value_object): Use + release_value_or_incref() instead of value_incref() to prevent leaks. + (valpy_richcompare, valpy_binop, valpy_getitem): Call value_mark() + before calling into gdb, and make sure to value_free_to_mark() before + returning or raising any exceptions. + 2010-10-04 Doug Evans * cc-with-index.sh: New file. diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c index 0aeea7c..4cf3c5b 100644 --- a/gdb/python/py-value.c +++ b/gdb/python/py-value.c @@ -149,7 +149,7 @@ valpy_new (PyTypeObject *subtype, PyObject *args, PyObject *keywords) } value_obj->value = value; - value_incref (value); + release_value_or_incref (value); value_obj->address = NULL; value_obj->type = NULL; value_obj->dynamic_type = NULL; @@ -444,8 +444,10 @@ valpy_getitem (PyObject *self, PyObject *key) { value_object *self_value = (value_object *) self; char *field = NULL; + PyObject *res_obj = NULL; struct value *res_val = NULL; volatile struct gdb_exception except; + struct value *mark = value_mark (); if (gdbpy_is_string (key)) { @@ -485,9 +487,15 @@ valpy_getitem (PyObject *self, PyObject *key) } xfree (field); + if (except.reason < 0) + value_free_to_mark (mark); GDB_PY_HANDLE_EXCEPTION (except); - return res_val ? value_to_value_object (res_val) : NULL; + if (res_val) + res_obj = value_to_value_object (res_val); + + value_free_to_mark (mark); + return res_obj; } static int @@ -623,8 +631,10 @@ enum valpy_opcode static PyObject * valpy_binop (enum valpy_opcode opcode, PyObject *self, PyObject *other) { + PyObject *res_obj = NULL; struct value *res_val = NULL; /* Initialize to appease gcc warning. */ volatile struct gdb_exception except; + struct value *mark = value_mark (); TRY_CATCH (except, RETURN_MASK_ALL) { @@ -715,9 +725,16 @@ valpy_binop (enum valpy_opcode opcode, PyObject *self, PyObject *other) break; } } + + if (except.reason < 0) + value_free_to_mark (mark); GDB_PY_HANDLE_EXCEPTION (except); - return res_val ? value_to_value_object (res_val) : NULL; + if (res_val) + res_obj = value_to_value_object (res_val); + + value_free_to_mark (mark); + return res_obj; } static PyObject * @@ -878,6 +895,7 @@ valpy_richcompare (PyObject *self, PyObject *other, int op) int result = 0; struct value *value_other; volatile struct gdb_exception except; + struct value *mark = value_mark (); if (other == Py_None) /* Comparing with None is special. From what I can tell, in Python @@ -936,6 +954,7 @@ valpy_richcompare (PyObject *self, PyObject *other, int op) break; } } + value_free_to_mark (mark); GDB_PY_HANDLE_EXCEPTION (except); /* In this case, the Python exception has already been set. */ @@ -1059,7 +1078,7 @@ value_to_value_object (struct value *val) if (val_obj != NULL) { val_obj->value = val; - value_incref (val); + release_value_or_incref (val); val_obj->address = NULL; val_obj->type = NULL; val_obj->dynamic_type = NULL; diff --git a/gdb/value.c b/gdb/value.c index 55fcd38..0d98ba8 100644 --- a/gdb/value.c +++ b/gdb/value.c @@ -737,9 +737,10 @@ free_value_chain (struct value *v) } /* Remove VAL from the chain all_values - so it will not be freed automatically. */ + so it will not be freed automatically. + Result of 0 indicates the value was found and released. */ -void +int release_value (struct value *val) { struct value *v; @@ -748,7 +749,7 @@ release_value (struct value *val) { all_values = val->next; val->next = NULL; - return; + return 0; } for (v = all_values; v; v = v->next) @@ -757,12 +758,25 @@ release_value (struct value *val) { v->next = val->next; val->next = NULL; - break; + return 0; } } + + return -1; +} + +/* Release VAL or increment its reference count if + it was released already */ + +void +release_value_or_incref (struct value *val) +{ + if (release_value (val) != 0) + value_incref (val); } /* Release all values up to mark */ + struct value * value_release_to_mark (struct value *mark) { diff --git a/gdb/value.h b/gdb/value.h index 12cbc0a..fc5a8b3 100644 --- a/gdb/value.h +++ b/gdb/value.h @@ -641,7 +641,9 @@ extern void free_all_values (void); extern void free_value_chain (struct value *v); -extern void release_value (struct value *val); +extern int release_value (struct value *val); + +extern void release_value_or_incref (struct value *val); extern int record_latest_value (struct value *val); On Tue, Jan 12, 2010 at 3:21 AM, Phil Muldoon wrote: > > On 12/10/2009 11:15 PM, Aman Gupta wrote: > > The gdb process will leak a large amount of memory (>512mb RSS) while > > executing the python loop. Some (but not all) of this memory is > > returned after the loop completes and free_all_values() is called for > > the next command. The following patch against archer-tromey-python > > keeps the memory usage constant during the loop. > > > On my tests, the consumption noted is far in excess of that, so good > find. > > > > > diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c > > index 14efd79..f305b01 100644 > > --- a/gdb/python/py-value.c > > +++ b/gdb/python/py-value.c > > @@ -143,7 +143,7 @@ valpy_new (PyTypeObject *subtype, PyObject *args, > > PyObject *keywords) > > Your mailer appears to be wrapping patches.  So this means the patch > command will complain of a malformed patch.  In this case it was > easily fixed in an editor, but a heads up in any case. > > > >  #define GDB_PY_HANDLE_EXCEPTION(Exception)                           \ > > +    GDB_PY_HANDLE_EXCEPTION_AND_CLEANUP(Exception, NULL) > > + > > +#define GDB_PY_HANDLE_EXCEPTION_AND_CLEANUP(Exception, mark)                         \ > >      do {                                                             \ > > -      if (Exception.reason < 0)                                              \ > > +      if (Exception.reason < 0) {                                    \ > > +        if (mark)                                            \ > > +          value_free_to_mark (mark);                                         \ > >       return PyErr_Format (Exception.reason == RETURN_QUIT            \ > >                            ? PyExc_KeyboardInterrupt : PyExc_RuntimeError, \ > >                            "%s", Exception.message);                  \ > > +      }                                              \ > >      } while (0) > > When I try to compile against archer-tromey-python with: > > ../archer/configure && make CFLAGS="-O0 -g3" > > I get a compile error: > > cc1: warnings being treated as errors > ../../archer/gdb/python/py-symbol.c: In function ‘gdbpy_lookup_symbol’: > ../../archer/gdb/python/py-symbol.c:200: error: implicit declaration of function ‘value_free_to_mark’ > make[2]: *** [py-symbol.o] Error 1 > > This is because GDB_PY_HANDLE_EXCEPTION is used in many sources files, > and value_free_to_mark is defined in value.{h|c}. And as you've > taken over the definition of GDB_PY_HANDLE_EXCEPTION the substitution > has some side-effects. So this needs to be fixed first. > > Did you spot or note any regressions from a full test run by comparing > the gdb.sum files before and after the patch?  A note indicating the > platform/architecture of testing, and any regressions spotted would be > great, thanks. > > Also, this patch needs a ChangeLog. > > Cheers, > > Phil