[patch] fixing leaking struct value* in py-value.c

[patch] fixing leaking struct value* in py-value.c

[Aman Gupta]

[-- Attachment #1: Type: text/plain, Size: 6128 bytes --]

Bug: http://sourceware.org/bugzilla/show_bug.cgi?id=10896

Given:

$ cat simple.c
#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>

struct blah {
  char test[2];
  int i;
  float none;
} *list_of_blahs;

int main(){
  list_of_blahs = malloc(sizeof(struct blah) * 2000000);
  printf("pid: %d\n", getpid());
  while(1){
    sleep(1);
  }
}


$ cat simple.py
gdb.execute('p list_of_blahs')
list = gdb.history(0)
end = list + 1000000

while list < end:
  list['i']
  list += 1


$ ./simple &
[1] 16572
$ gdb -ex 'py execfile("./simple.py")' ./simple 16572

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.

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)
     }

   value_obj->value = value;
-  value_incref (value);
+  release_value_or_incref (value);
   value_obj->address = NULL;
   value_obj->type = NULL;
   note_value (value_obj);
@@ -267,6 +267,7 @@ valpy_cast (PyObject *self, PyObject *args)
 {
   PyObject *type_obj;
   struct type *type;
+  PyObject *res_obj = NULL;
   struct value *res_val = NULL;	  /* Initialize to appease gcc warning.  */
   volatile struct gdb_exception except;

@@ -305,8 +306,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))
     {
@@ -344,9 +347,13 @@ valpy_getitem (PyObject *self, PyObject *key)
     }

   xfree (field);
-  GDB_PY_HANDLE_EXCEPTION (except);
+  GDB_PY_HANDLE_EXCEPTION_AND_CLEANUP (except, mark);
+
+  if (res_val)
+    res_obj = value_to_value_object (res_val);

-  return res_val ? value_to_value_object (res_val) : NULL;
+  value_free_to_mark (mark);
+  return res_obj;
 }

 static int
@@ -427,8 +434,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)
     {
@@ -519,9 +528,13 @@ valpy_binop (enum valpy_opcode opcode, PyObject
*self, PyObject *other)
 	  break;
 	}
     }
-  GDB_PY_HANDLE_EXCEPTION (except);
+  GDB_PY_HANDLE_EXCEPTION_AND_CLEANUP (except, mark);
+
+  if (res_val)
+    res_obj = value_to_value_object (res_val);

-  return res_val ? value_to_value_object (res_val) : NULL;
+  value_free_to_mark (mark);
+  return res_obj;
 }

 static PyObject *
@@ -684,6 +697,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
@@ -742,6 +756,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.  */
@@ -851,7 +866,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;
       note_value (val_obj);
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 47662d9..309ee5b 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -149,11 +149,17 @@ PyObject *gdbpy_parameter_value (enum var_types, void *);
 /* Use this after a TRY_EXCEPT to throw the appropriate Python
    exception.  */
 #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)

 /* Use this after a TRY_EXCEPT to throw the appropriate Python
diff --git a/gdb/value.c b/gdb/value.c
index 2f31185..9187a7f 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -691,7 +691,7 @@ free_all_values (void)
 /* Remove VAL from the chain all_values
    so it will not be freed automatically.  */

-void
+int
 release_value (struct value *val)
 {
   struct value *v;
@@ -699,7 +699,7 @@ release_value (struct value *val)
   if (all_values == val)
     {
       all_values = val->next;
-      return;
+      return 1;
     }

   for (v = all_values; v; v = v->next)
@@ -707,9 +707,21 @@ release_value (struct value *val)
       if (v->next == val)
 	{
 	  v->next = val->next;
-	  break;
+	  return 1;
 	}
     }
+
+  return 0;
+}
+
+/* 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  */
diff --git a/gdb/value.h b/gdb/value.h
index 0da7031..5705210 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -595,7 +595,9 @@ extern void value_free (struct value *val);

 extern void free_all_values (void);

-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);

[-- Attachment #2: gdb-python-leak.patch --]
[-- Type: application/octet-stream, Size: 5266 bytes --]

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)
     }
 
   value_obj->value = value;
-  value_incref (value);
+  release_value_or_incref (value);
   value_obj->address = NULL;
   value_obj->type = NULL;
   note_value (value_obj);
@@ -267,6 +267,7 @@ valpy_cast (PyObject *self, PyObject *args)
 {
   PyObject *type_obj;
   struct type *type;
+  PyObject *res_obj = NULL;
   struct value *res_val = NULL;	  /* Initialize to appease gcc warning.  */
   volatile struct gdb_exception except;
 
@@ -305,8 +306,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))
     {  
@@ -344,9 +347,13 @@ valpy_getitem (PyObject *self, PyObject *key)
     }
 
   xfree (field);
-  GDB_PY_HANDLE_EXCEPTION (except);
+  GDB_PY_HANDLE_EXCEPTION_AND_CLEANUP (except, mark);
+
+  if (res_val)
+    res_obj = value_to_value_object (res_val);
 
-  return res_val ? value_to_value_object (res_val) : NULL;
+  value_free_to_mark (mark);
+  return res_obj;
 }
 
 static int
@@ -427,8 +434,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)
     {
@@ -519,9 +528,13 @@ valpy_binop (enum valpy_opcode opcode, PyObject *self, PyObject *other)
 	  break;
 	}
     }
-  GDB_PY_HANDLE_EXCEPTION (except);
+  GDB_PY_HANDLE_EXCEPTION_AND_CLEANUP (except, mark);
+
+  if (res_val)
+    res_obj = value_to_value_object (res_val);
 
-  return res_val ? value_to_value_object (res_val) : NULL;
+  value_free_to_mark (mark);
+  return res_obj;
 }
 
 static PyObject *
@@ -684,6 +697,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
@@ -742,6 +756,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.  */
@@ -851,7 +866,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;
       note_value (val_obj);
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 47662d9..309ee5b 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -149,11 +149,17 @@ PyObject *gdbpy_parameter_value (enum var_types, void *);
 /* Use this after a TRY_EXCEPT to throw the appropriate Python
    exception.  */
 #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)
 
 /* Use this after a TRY_EXCEPT to throw the appropriate Python
diff --git a/gdb/value.c b/gdb/value.c
index 2f31185..9187a7f 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -691,7 +691,7 @@ free_all_values (void)
 /* Remove VAL from the chain all_values
    so it will not be freed automatically.  */
 
-void
+int
 release_value (struct value *val)
 {
   struct value *v;
@@ -699,7 +699,7 @@ release_value (struct value *val)
   if (all_values == val)
     {
       all_values = val->next;
-      return;
+      return 1;
     }
 
   for (v = all_values; v; v = v->next)
@@ -707,9 +707,21 @@ release_value (struct value *val)
       if (v->next == val)
 	{
 	  v->next = val->next;
-	  break;
+	  return 1;
 	}
     }
+
+  return 0;
+}
+
+/* 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  */
diff --git a/gdb/value.h b/gdb/value.h
index 0da7031..5705210 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -595,7 +595,9 @@ extern void value_free (struct value *val);
 
 extern void free_all_values (void);
 
-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);
 

Re: [patch] fixing leaking struct value* in py-value.c

[Phil Muldoon]

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

Re: [patch] fixing leaking struct value* in py-value.c

[Aman Gupta]

[-- Attachment #1: Type: text/plain, Size: 8467 bytes --]

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  <aman@tmm1.net>
+	* 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  <dje@google.com>

 	* 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 <pmuldoon@redhat.com> 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

[-- Attachment #2: python-value-leak.patch --]
[-- Type: application/octet-stream, Size: 4982 bytes --]

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  <aman@tmm1.net>
+	* 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  <dje@google.com>
 
 	* 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);