This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] MI: Free values when updating


On Tuesday 23 January 2007 11:55, Nick Roberts wrote:
>  > I don't understand this change at all. It means that if you have 
>  > a reference variable then:
>  > 
>  >         1. For initial value, dereferenced value will be saved
>  >         2. For second and subsequent values, the value of reference itself will
>  >         be saved. 
>  > 
>  > This is exactly what the code block in question tries to prevent --
>  > it tries to make sure we always store dereferenced value.
>  >
>  > When I try to run tests with this patch applied, I get:
>  > 
>  > Running ../.././gdb/testsuite/gdb.mi/mi-var-cmd.exp ...
>  > ERROR: Couldn't send -var-create i  * i to GDB.
>  > ERROR: Couldn't send -var-create l * l to GDB.
>  > ERROR: Couldn't send -var-create linteger * linteger to GDB.
>  > 
>  > and many more errors like this. Did you run the testsuite after change?
> 
> OK, it's not the right patch but I do think that release_values shouldn't be
> called when updating; 

Why? If we're about to store a value in varobj->value, we must call release_value
on that value,  otherwise the value is going to be deleted at a random point in time,
and our varobj will be rather useless.

> it's just that this patch stops calling it at other times 
> when it's needed.  Without any change, do enable timings (if you have that
> patch), create a variable object of a large array and all its children then
> repeatedly do "-var-update *".  It should take longer and longer to execute.

Why? Is it because the memory consumption of gdb grows, or because the list
of released values grows without ever being cleared, or for some other reason?

> Then try the patch below which does pass all the tests (just intended for
> illustration not approval).

Could you perhaps try the attached one, which I think is ready for checkin.
The install_new_value function documents that the incoming value should not be
released yet, in one place that requirement was not met. No regressions on x86.

>  > Can you explain where the memory leak comes from. At the end of the function I see:
>  > 
>  >     if (var->value != NULL)
>  >        value_free (var->value);
>  > 
>  > and the value passed to function itself does not have release_value called on it,
>  > so should be freed automatically.
> 
> I've not said it's a memory leak but it's computationally wasteful to
> call release_value for each variable object when updating.

As I've said, we need to do it to avoid the value to be deleted at random point in time.
So there must be something else? Does you free_values patch makes any difference here?

- Volodya
Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.79
diff -u -p -r1.79 varobj.c
--- varobj.c	16 Jan 2007 02:12:49 -0000	1.79
+++ varobj.c	23 Jan 2007 09:12:07 -0000
@@ -1987,11 +1987,7 @@ c_value_of_root (struct varobj **var_han
       /* We need to catch errors here, because if evaluate
          expression fails we just want to make val->error = 1 and
          go on */
-      if (gdb_evaluate_expression (var->root->exp, &new_val))
-	{
-	  release_value (new_val);
-	}
-
+      gdb_evaluate_expression (var->root->exp, &new_val);
       return new_val;
     }
 

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]